Re: [RFC PATCH] fuse: add new function to invalidate cache for all inodes

[Bernd Schubert <bernd@bsbernd.com>]

On 1/15/25 19:07, Luis Henriques wrote:
> Hi Bernd,
> 
> On Wed, Jan 15 2025, Bernd Schubert wrote:
> 
>> On 1/15/25 17:32, 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 superblock dcache.
>>
>> Out of interest, what is the use case?
> 
> This is for a read-only filesystem.  However, the filesystem objects
> (files, directories, ...) may change dramatically in an atomic way, so
> that a totally different set of objects replaces the old one.
> 
> Obviously, this patch would help with the process of getting rid of the
> old generation of the filesystem.
> 
>>>
>>> Signed-off-by: Luis Henriques <luis@igalia.com>
>>> ---
>>> Just an additional note that this patch could eventually be simplified if
>>> Dave Chinner patch to iterate through the superblock inodes[1] is merged.
>>>
>>> [1] https://lore.kernel.org/r/20241002014017.3801899-3-david@fromorbit.com
>>>
>>>  fs/fuse/inode.c           | 53 +++++++++++++++++++++++++++++++++++++++
>>>  include/uapi/linux/fuse.h |  3 +++
>>>  2 files changed, 56 insertions(+)
>>>
>>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>>> index 3ce4f4e81d09..1fd9a5f303da 100644
>>> --- a/fs/fuse/inode.c
>>> +++ b/fs/fuse/inode.c
>>> @@ -546,6 +546,56 @@ struct inode *fuse_ilookup(struct fuse_conn *fc, u64 nodeid,
>>>  	return NULL;
>>>  }
>>>  
>>> +static int fuse_reverse_inval_all(struct fuse_conn *fc)
>>> +{
>>> +	struct fuse_mount *fm;
>>> +	struct super_block *sb;
>>> +	struct inode *inode, *old_inode = NULL;
>>> +	struct fuse_inode *fi;
>>> +
>>> +	inode = fuse_ilookup(fc, FUSE_ROOT_ID, NULL);
>>> +	if (!inode)
>>> +		return -ENOENT;
>>> +
>>> +	fm = get_fuse_mount(inode);
>>> +	iput(inode);
>>> +	if (!fm)
>>> +		return -ENOENT;
>>> +	sb = fm->sb;
>>> +
>>> +	spin_lock(&sb->s_inode_list_lock);
>>> +	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
>>
>> Maybe list_for_each_entry_safe() and then you can iput(inode) before the
>> next iteration?
> 
> I can rework this loop, but are you sure it's safe to use that?  (Genuine
> question!)
> 
> I could only find two places where list_for_each_entry_safe() is being
> used to walk through the sb inodes.  And they both use an auxiliary list
> that holds the inodes to be processed later.  All other places use the
> pattern I'm following here.
> 
> Or did I misunderstood your suggestion?


Actually my mistake, yeah you cannot use list_for_each_entry_safe() 
because you are giving up the list lock and the next entry, which
is already obtained by _safe might not be valid anymore.

Sorry for the noise!


Thanks,
Bernd