Re: [PATCH 1/2] fuse: replace fuse_queue_forget with fuse_force_forget if error

[yangyun]

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.