linux-kernel - Re: [PATCH 1/2] fuse: replace fuse_queue_forget with fuse_force

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240726153908.GD3432726@perftesting>
Date: Fri, 26 Jul 2024 11:39:08 -0400
From: Josef Bacik <josef@...icpanda.com>
To: yangyun <yangyun50@...wei.com>
Cc: Miklos Szeredi <miklos@...redi.hu>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] fuse: replace fuse_queue_forget with
 fuse_force_forget if error

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@...wei.com>

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