| Message ID | 20250210143351.31119-1-luis@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v3] fuse: add new function to invalidate cache for all inodes | expand |
On 2/10/25 6:33 AM, 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. > > Signed-off-by: Luis Henriques <luis@igalia.com> > --- > * 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 > > fs/fuse/inode.c | 67 +++++++++++++++++++++++++++++++++++---- > include/uapi/linux/fuse.h | 3 ++ > 2 files changed, 63 insertions(+), 7 deletions(-) > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c > index e9db2cb8c150..45b9fbb54d42 100644 > --- a/fs/fuse/inode.c > +++ b/fs/fuse/inode.c > @@ -547,25 +547,78 @@ struct inode *fuse_ilookup(struct fuse_conn *fc, u64 nodeid, > return NULL; > } > > +static void inval_single_inode(struct inode *inode, struct fuse_conn *fc) > +{ > + struct fuse_inode *fi; > + > + fi = get_fuse_inode(inode); > + spin_lock(&fi->lock); > + fi->attr_version = atomic64_inc_return(&fc->attr_version); > + spin_unlock(&fi->lock); > + fuse_invalidate_attr(inode); > + forget_all_cached_acls(inode); > +} > + > +static int fuse_reverse_inval_all(struct fuse_conn *fc) > +{ > + struct fuse_mount *fm; > + struct super_block *sb; > + struct inode *inode, *old_inode = NULL; > + > + inode = fuse_ilookup(fc, FUSE_ROOT_ID, NULL); > + if (!inode) > + return -ENOENT; > + > + fm = get_fuse_mount(inode); I think if you pass in &fm as the 3rd arg to fuse_ilookup(), it'll pass back the fuse mount and we won't need get_fuse_mount(). > + 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) { > + spin_lock(&inode->i_lock); > + if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) || > + !atomic_read(&inode->i_count)) { Will inode->i_count ever be 0? AFAIU, inode->i_count tracks the inode refcount, so if this is 0, doesn't this mean it wouldn't be on the sb->s_inodes list? > + spin_unlock(&inode->i_lock); > + continue; > + } > + > + __iget(inode); > + spin_unlock(&inode->i_lock); > + spin_unlock(&sb->s_inode_list_lock); Maybe worth adding a comment here since there can be inodes added after the s_inode_list_lock is dropped and before it's acquired again that when inodes get added to the head of sb->s_inodes, it's always for I_NEW inodes. > + iput(old_inode); Maybe a dumb question but why is old_inode needed? Why can't iput()just be called right after inval_single_inode()? > + > + inval_single_inode(inode, fc); > + > + old_inode = inode; > + cond_resched(); Could you explain why a cond_resched() is needed here? > + spin_lock(&sb->s_inode_list_lock); > + } > + spin_unlock(&sb->s_inode_list_lock); > + iput(old_inode); > + > + shrink_dcache_sb(sb); > + > + return 0; > +} > + Thanks, Joanne > int fuse_reverse_inval_inode(struct fuse_conn *fc, u64 nodeid, > loff_t offset, loff_t len) > { > - struct fuse_inode *fi; > struct inode *inode; > pgoff_t pg_start; > pgoff_t pg_end; > > + if (nodeid == FUSE_INVAL_ALL_INODES) > + return fuse_reverse_inval_all(fc); > + > inode = fuse_ilookup(fc, nodeid, NULL); > if (!inode) > return -ENOENT; > > - fi = get_fuse_inode(inode); > - spin_lock(&fi->lock); > - fi->attr_version = atomic64_inc_return(&fc->attr_version); > - spin_unlock(&fi->lock); > + inval_single_inode(inode, fc); > > - fuse_invalidate_attr(inode); > - forget_all_cached_acls(inode); > if (offset >= 0) { > pg_start = offset >> PAGE_SHIFT; > if (len <= 0) > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h > index 5e0eb41d967e..e5852b63f99f 100644 > --- a/include/uapi/linux/fuse.h > +++ b/include/uapi/linux/fuse.h > @@ -669,6 +669,9 @@ enum fuse_notify_code { > FUSE_NOTIFY_CODE_MAX, > }; > > +/* The nodeid to request to invalidate all inodes */ > +#define FUSE_INVAL_ALL_INODES 0 > + > /* The read buffer is required to be at least 8k, but may be much larger */ > #define FUSE_MIN_READ_BUFFER 8192 > >
On 2/10/25 20:33, Joanne Koong wrote: > On 2/10/25 6:33 AM, 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. >> >> Signed-off-by: Luis Henriques <luis@igalia.com> >> --- >> * 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 >> >> fs/fuse/inode.c | 67 +++++++++++++++++++++++++++++++++++---- >> include/uapi/linux/fuse.h | 3 ++ >> 2 files changed, 63 insertions(+), 7 deletions(-) >> >> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c >> index e9db2cb8c150..45b9fbb54d42 100644 >> --- a/fs/fuse/inode.c >> +++ b/fs/fuse/inode.c >> @@ -547,25 +547,78 @@ struct inode *fuse_ilookup(struct fuse_conn *fc, >> u64 nodeid, >> return NULL; >> } >> +static void inval_single_inode(struct inode *inode, struct >> fuse_conn *fc) >> +{ >> + struct fuse_inode *fi; >> + >> + fi = get_fuse_inode(inode); >> + spin_lock(&fi->lock); >> + fi->attr_version = atomic64_inc_return(&fc->attr_version); >> + spin_unlock(&fi->lock); >> + fuse_invalidate_attr(inode); >> + forget_all_cached_acls(inode); >> +} >> + >> +static int fuse_reverse_inval_all(struct fuse_conn *fc) >> +{ >> + struct fuse_mount *fm; >> + struct super_block *sb; >> + struct inode *inode, *old_inode = NULL; >> + >> + inode = fuse_ilookup(fc, FUSE_ROOT_ID, NULL); >> + if (!inode) >> + return -ENOENT; >> + >> + fm = get_fuse_mount(inode); > > I think if you pass in &fm as the 3rd arg to fuse_ilookup(), it'll pass > back the fuse mount and we won't need get_fuse_mount(). > >> + 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) { >> + spin_lock(&inode->i_lock); >> + if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) || >> + !atomic_read(&inode->i_count)) { > > Will inode->i_count ever be 0? AFAIU, inode->i_count tracks the inode > refcount, so if this is 0, doesn't this mean it wouldn't be on the sb- >>s_inodes list? > >> + spin_unlock(&inode->i_lock); >> + continue; >> + } >> + >> + __iget(inode); >> + spin_unlock(&inode->i_lock); >> + spin_unlock(&sb->s_inode_list_lock); > > Maybe worth adding a comment here since there can be inodes added after > the s_inode_list_lock is dropped and before it's acquired again that > when inodes get added to the head of sb->s_inodes, it's always for I_NEW > inodes. > >> + iput(old_inode); > > Maybe a dumb question but why is old_inode needed? Why can't iput()just > be called right after inval_single_inode()? I had wondered the same in v1. Issue is that there is a list iteration that releases the locks - if the put would be done immediately it could not continue on "old_inode" as it might not exist anymore. > >> + >> + inval_single_inode(inode, fc); >> + >> + old_inode = inode; >> + cond_resched(); > > Could you explain why a cond_resched() is needed here? Give other threads a chance to work? The list might be huge? Thanks, Bernd
Hi Joanne Bernd has already added a few comments. I'm just adding a few more on top. On Mon, Feb 10 2025, Bernd Schubert wrote: > On 2/10/25 20:33, Joanne Koong wrote: >> On 2/10/25 6:33 AM, 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. >>> >>> Signed-off-by: Luis Henriques <luis@igalia.com> >>> --- >>> * 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 >>> >>> fs/fuse/inode.c | 67 +++++++++++++++++++++++++++++++++++---- >>> include/uapi/linux/fuse.h | 3 ++ >>> 2 files changed, 63 insertions(+), 7 deletions(-) >>> >>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c >>> index e9db2cb8c150..45b9fbb54d42 100644 >>> --- a/fs/fuse/inode.c >>> +++ b/fs/fuse/inode.c >>> @@ -547,25 +547,78 @@ struct inode *fuse_ilookup(struct fuse_conn *fc, >>> u64 nodeid, >>> return NULL; >>> } >>> +static void inval_single_inode(struct inode *inode, struct >>> fuse_conn *fc) >>> +{ >>> + struct fuse_inode *fi; >>> + >>> + fi = get_fuse_inode(inode); >>> + spin_lock(&fi->lock); >>> + fi->attr_version = atomic64_inc_return(&fc->attr_version); >>> + spin_unlock(&fi->lock); >>> + fuse_invalidate_attr(inode); >>> + forget_all_cached_acls(inode); >>> +} >>> + >>> +static int fuse_reverse_inval_all(struct fuse_conn *fc) >>> +{ >>> + struct fuse_mount *fm; >>> + struct super_block *sb; >>> + struct inode *inode, *old_inode = NULL; >>> + >>> + inode = fuse_ilookup(fc, FUSE_ROOT_ID, NULL); >>> + if (!inode) >>> + return -ENOENT; >>> + >>> + fm = get_fuse_mount(inode); >> >> I think if you pass in &fm as the 3rd arg to fuse_ilookup(), it'll pass >> back the fuse mount and we won't need get_fuse_mount(). Yeah, good catch! That makes sense, I didn't noticed that this third argument could be used that way. I'll make sure next rev simplifies this code. >>> + 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) { >>> + spin_lock(&inode->i_lock); >>> + if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) || >>> + !atomic_read(&inode->i_count)) { >> >> Will inode->i_count ever be 0? AFAIU, inode->i_count tracks the inode >> refcount, so if this is 0, doesn't this mean it wouldn't be on the sb- >>>s_inodes list? My point in having this check is that, while iterating the inodes list, the inode may be iput()'ed before we actually have the chance to lock it. This is simply to skip the (unlikely) case when this happens. Maybe it's not really necessary, maybe the previous checks (the ->i_state) are enough. But I've seen this pattern in other places (for example, in evict_inodes()). >>> + spin_unlock(&inode->i_lock); >>> + continue; >>> + } >>> + >>> + __iget(inode); >>> + spin_unlock(&inode->i_lock); >>> + spin_unlock(&sb->s_inode_list_lock); >> >> Maybe worth adding a comment here since there can be inodes added after >> the s_inode_list_lock is dropped and before it's acquired again that >> when inodes get added to the head of sb->s_inodes, it's always for I_NEW >> inodes. >> >>> + iput(old_inode); >> >> Maybe a dumb question but why is old_inode needed? Why can't iput()just >> be called right after inval_single_inode()? > > I had wondered the same in v1. Issue is that there is a list iteration > that releases the locks - if the put would be done immediately it could > not continue on "old_inode" as it might not exist anymore. Exactly, thanks Bernd. We release the locks and do the cond_resched() below for the cases where we have a huge amount of inodes to invalidate. I'll prepare a new rev with some extra code comments (including those suggested by Bernd before) and remove the call to get_fuse_mount(). Thank you both for your feedback. Much appreciated! Cheers,
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index e9db2cb8c150..45b9fbb54d42 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -547,25 +547,78 @@ struct inode *fuse_ilookup(struct fuse_conn *fc, u64 nodeid, return NULL; } +static void inval_single_inode(struct inode *inode, struct fuse_conn *fc) +{ + struct fuse_inode *fi; + + fi = get_fuse_inode(inode); + spin_lock(&fi->lock); + fi->attr_version = atomic64_inc_return(&fc->attr_version); + spin_unlock(&fi->lock); + fuse_invalidate_attr(inode); + forget_all_cached_acls(inode); +} + +static int fuse_reverse_inval_all(struct fuse_conn *fc) +{ + struct fuse_mount *fm; + struct super_block *sb; + struct inode *inode, *old_inode = NULL; + + 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) { + spin_lock(&inode->i_lock); + if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) || + !atomic_read(&inode->i_count)) { + spin_unlock(&inode->i_lock); + continue; + } + + __iget(inode); + spin_unlock(&inode->i_lock); + spin_unlock(&sb->s_inode_list_lock); + iput(old_inode); + + inval_single_inode(inode, fc); + + old_inode = inode; + cond_resched(); + spin_lock(&sb->s_inode_list_lock); + } + spin_unlock(&sb->s_inode_list_lock); + iput(old_inode); + + shrink_dcache_sb(sb); + + return 0; +} + int fuse_reverse_inval_inode(struct fuse_conn *fc, u64 nodeid, loff_t offset, loff_t len) { - struct fuse_inode *fi; struct inode *inode; pgoff_t pg_start; pgoff_t pg_end; + if (nodeid == FUSE_INVAL_ALL_INODES) + return fuse_reverse_inval_all(fc); + inode = fuse_ilookup(fc, nodeid, NULL); if (!inode) return -ENOENT; - fi = get_fuse_inode(inode); - spin_lock(&fi->lock); - fi->attr_version = atomic64_inc_return(&fc->attr_version); - spin_unlock(&fi->lock); + inval_single_inode(inode, fc); - fuse_invalidate_attr(inode); - forget_all_cached_acls(inode); if (offset >= 0) { pg_start = offset >> PAGE_SHIFT; if (len <= 0) diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h index 5e0eb41d967e..e5852b63f99f 100644 --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -669,6 +669,9 @@ enum fuse_notify_code { FUSE_NOTIFY_CODE_MAX, }; +/* The nodeid to request to invalidate all inodes */ +#define FUSE_INVAL_ALL_INODES 0 + /* The read buffer is required to be at least 8k, but may be much larger */ #define FUSE_MIN_READ_BUFFER 8192
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. Signed-off-by: Luis Henriques <luis@igalia.com> --- * 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 fs/fuse/inode.c | 67 +++++++++++++++++++++++++++++++++++---- include/uapi/linux/fuse.h | 3 ++ 2 files changed, 63 insertions(+), 7 deletions(-)