Message-ID: <87mseqcdar.fsf@igalia.com>
Date: Thu, 13 Feb 2025 11:08:28 +0000
From: Luis Henriques <luis@...lia.com>
To: Dave Chinner <david@...morbit.com>
Cc: Miklos Szeredi <miklos@...redi.hu>,  linux-fsdevel@...r.kernel.org,
  linux-kernel@...r.kernel.org,  Matt Harvey <mharvey@...ptrading.com>,
  Bernd Schubert <bschubert@....com>,  Joanne Koong
 <joannelkoong@...il.com>
Subject: Re: [PATCH v4] fuse: add new function to invalidate cache for all
 inodes

On Thu, Feb 13 2025, Dave Chinner wrote:

> On Wed, Feb 12, 2025 at 11:32:40AM +0000, Luis Henriques wrote:
>> On Wed, Feb 12 2025, Dave Chinner wrote:
>> 
>> > [ FWIW: if the commit message directly references someone else's
>> > related (and somewhat relevant) work, please directly CC those
>> > people on the patch(set). I only noticed this by chance, not because
>> > I read every FUSE related patch that goes by me. ]
>> 
>> Point taken -- I should have included you on CC since the initial RFC.
>> 
>> > On Tue, Feb 11, 2025 at 09:26:04AM +0000, Luis Henriques wrote:
>> >> Currently userspace is able to notify the kernel to invalidate the cache
>> >> for an inode.  This means that, if all the inodes in a filesystem need to
>> >> be invalidated, then userspace needs to iterate through all of them and do
>> >> this kernel notification separately.
>> >> 
>> >> This patch adds a new option that allows userspace to invalidate all the
>> >> inodes with a single notification operation.  In addition to invalidate
>> >> all the inodes, it also shrinks the sb dcache.
>> >
>> > That, IMO, seems like a bit naive - we generally don't allow user
>> > controlled denial of service vectors to be added to the kernel. i.e.
>> > this is the equivalent of allowing FUSE fs specific 'echo 1 >
>> > /proc/sys/vm/drop_caches' via some fuse specific UAPI. We only allow
>> > root access to /proc/sys/vm/drop_caches because it can otherwise be
>> > easily abused to cause system wide performance issues.
>> >
>> > It also strikes me as a somewhat dangerous precendent - invalidating
>> > random VFS caches through user APIs hidden deep in random fs
>> > implementations makes for poor visibility and difficult maintenance
>> > of VFS level functionality...
>> 
>> Hmm... OK, I understand the concern and your comment makes perfect sense.
>> But would it be acceptable to move this API upper in the stack and make it
>> visible at the VFS layer?  Something similar to the 'drop_caches' but with
>> a superblock granularity.  I haven't spent any time thinking how could
>> that be done, but it wouldn't be "hidden deep" anymore.
>
> I'm yet to see any justification for why 'user driven entire
> filesystem cache invalidation' is needed. Get agreement on whether
> the functionality should exist first, then worry about how to
> implement it.
>
>> >> Signed-off-by: Luis Henriques <luis@...lia.com>
>> >> ---
>> >> * Changes since v3
>> >> - Added comments to clarify semantic changes in fuse_reverse_inval_inode()
>> >>   when called with FUSE_INVAL_ALL_INODES (suggested by Bernd).
>> >> - Added comments to inodes iteration loop to clarify __iget/iput usage
>> >>   (suggested by Joanne)
>> >> - Dropped get_fuse_mount() call -- fuse_mount can be obtained from
>> >>   fuse_ilookup() directly (suggested by Joanne)
>> >> 
>> >> (Also dropped the RFC from the subject.)
>> >> 
>> >> * Changes since v2
>> >> - Use the new helper from fuse_reverse_inval_inode(), as suggested by Bernd.
>> >> - Also updated patch description as per checkpatch.pl suggestion.
>> >> 
>> >> * Changes since v1
>> >> As suggested by Bernd, this patch v2 simply adds an helper function that
>> >> will make it easier to replace most of it's code by a call to function
>> >> super_iter_inodes() when Dave Chinner's patch[1] eventually gets merged.
>> >> 
>> >> [1] https://lore.kernel.org/r/20241002014017.3801899-3-david@fromorbit.com
>> >
>> > That doesn't make the functionality any more palatable.
>> >
>> > Those iterators are the first step in removing the VFS inode list
>> > and only maintaining it in filesystems that actually need this
>> > functionality. We want this list to go away because maintaining it
>> > is a general VFS cache scalability limitation.
>> >
>> > i.e. if a filesystem has internal functionality that requires
>> > iterating all instantiated inodes, the filesystem itself should
>> > maintain that list in the most efficient manner for the filesystem's
>> > iteration requirements not rely on the VFS to maintain this
>> > information for it.
>> 
>> Right, and in my use-case that's exactly what is currently being done: the
>> FUSE API to invalidate individual inodes is being used.
>>
>> This new
>> functionality just tries to make life easier to userspace when *all* the
>> inodes need to be invalidated. (For reference, the use-case is CVMFS, a
>> read-only FS, where new generations of a filesystem snapshot may become
>> available at some point and the previous one needs to be wiped from the
>> cache.)
>
> But you can't actually "wipe" referenced inodes from cache. That is a
> use case for revoke(), not inode cache invalidation.

I guess the word "wipe" wasn't the best choice.  See below.

>> > I'm left to ponder why the invalidation isn't simply:
>> >
>> > 	/* Remove all possible active references to cached inodes */
>> > 	shrink_dcache_sb();
>> >
>> > 	/* Remove all unreferenced inodes from cache */
>> > 	invalidate_inodes();
>> >
>> > Which will result in far more of the inode cache for the filesystem
>> > being invalidated than the above code....
>> 
>> To be honest, my initial attempt to implement this feature actually used
>> invalidate_inodes().  For some reason that I don't remember anymore why I
>> decided to implement the iterator myself.  I'll go look at that code again
>> and run some tests on this (much!) simplified version of the invalidation
>> function your suggesting.
>
> The above code, while simpler, still doesn't resolve the problem of
> invalidation of inodes that have active references (e.g. open files
> on them). They can't be "invalidated" in this way - they can't be
> removed from cache until all active references go away.

Sure, I understand that and that's *not* what I'm trying to do.  I guess
I'm just failing to describe my goal.  If there's a userspace process that
has a file open for an inode that does not exist anymore, that process
will continue using it -- the user-space filesystem will have to deal with
that.

Right now, fuse allows the user-space filesystem to notify the kernel that
*one* inode is not valid anymore.  This is a per inode operation.  I guess
this is very useful, for example, for network filesystems, where an inode
may have been deleted from somewhere else.

However, when user-space wants to do this for all the filesystem inodes,
it will be slow.  With my patch all I wanted to do is to make it a bit
less painful by moving the inodes iteration into the kernel.

Cheers,
-- 
Luís

> i.e. any operation that is based on the assumption that we can
> remove all references to inodes and dentries by walking across them
> and dropping cache references to them is fundamentally flawed. To do
> this reliably, all active references have to be hunted down and
> released before the inodes can be removed from VFS visibility. i.e.
> the mythical revoke() operation would need to be implemented for
> this to work...
>
> -Dave.
>
> -- 
> Dave Chinner
> david@...morbit.com

Powered by blists - more mailing lists