Hi Josef, Thanks for the comment. On Fri, Jul 26, 2024 at 11:39:08AM -0400, Josef Bacik wrote: > On Fri, Jul 26, 2024 at 04:37:51PM +0800, yangyun wrote: > > Most usecases for 'fuse_queue_forget' in the code are about reverting > > the lookup count when error happens, except 'fuse_evict_inode' and > > 'fuse_cleanup_submount_lookup'. Even if there are no errors, it > > still needs alloc 'struct fuse_forget_link'. It is useless, which > > contributes to performance degradation and code mess to some extent. > > > > 'fuse_force_forget' does not need allocate 'struct fuse_forget_link'in > > advance, and is only used by readdirplus before this patch for the reason > > that we do not know how many 'fuse_forget_link' structures will be > > allocated when error happens. > > > > Signed-off-by: yangyun <yangyun50@xxxxxxxxxx> > > Forcing file systems to have their forget suddenly be synchronous in a lot of > cases is going to be a perf regression for them. Sorry for that I didn't notice that 'fuse_force_forget' is synchronous. Actually, I'm not on purpose to make forget be synchronous, just want to reuse the already known function 'fuse_force_forget' to avoid useless memory alloc in some cases. And thank you for the reminder regarding the performance impact of synchronization. > > In some of these cases a synchronous forget is probably ok, as you say a lot of > them are error cases. However d_revalidate() isn't. That's us trying to figure > out if what we have in cache matches the file systems view of the inode, and if > it doesn't we're going to do a re-lookup, so we don't necessarily care for a > synchronous forget in this case. Think of an NFS fuse client where the file got > renamed on the backend and now we're telling the kernel this is the inode we > have. Forcing us to do a synchronous response now is going to be much more > performance impacting than it was pre-this patch. Yeah, this is definitely a performance impacting case. Thank for your adivce. > > A better approach would be to make the allocation optional based on the > ->no_forget flag. Thanks, The reason that I don't make the allocataion optional is that even the no_forget flag is disabled, there are still many useless memory allocations if no errors. And The code is also a bit messy because of those allocations. Since forget is not necessarily synchronous (In my opinion, the pre-this patch use of synchronous 'fuse_force_forget' is an error case and also not necessarily synchronous), what about just changing the 'fuse_force_forget' to be asynchronous? By this way, all the forget requests are asynchronous (less impact on performance) and we doesn't need to allocate useless memory in advance. > > Josef Thank you once again for your time and advice.
