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

[Josef Bacik]

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.

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.

A better approach would be to make the allocation optional based on the
->no_forget flag.  Thanks,

Josef