On Thu, Oct 03, 2024 at 12:12:29AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 02, 2024 at 11:33:19AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > Add a new superblock method for iterating all cached inodes in the
> > inode cache.
>
> The method is added later, this just adds an abstraction.
Ah, I forgot to remove that from the commit message when I split the
patch up....
> > +/**
> > + * super_iter_inodes - iterate all the cached inodes on a superblock
> > + * @sb: superblock to iterate
> > + * @iter_fn: callback to run on every inode found.
> > + *
> > + * This function iterates all cached inodes on a superblock that are not in
> > + * the process of being initialised or torn down. It will run @iter_fn() with
> > + * a valid, referenced inode, so it is safe for the caller to do anything
> > + * it wants with the inode except drop the reference the iterator holds.
> > + *
> > + */
>
> Spurious empty comment line above.
>
> > +void super_iter_inodes_unsafe(struct super_block *sb, ino_iter_fn iter_fn,
> > + void *private_data)
> > +{
> > + struct inode *inode;
> > + int ret;
> > +
> > + rcu_read_lock();
> > + spin_lock(&sb->s_inode_list_lock);
> > + list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> > + ret = iter_fn(inode, private_data);
> > + if (ret == INO_ITER_ABORT)
> > + break;
> > + }
>
> Looking at the entire series, splitting the helpers for the unsafe
> vs safe iteration feels a bit of an odd API design given that the
> INO_ITER_REFERENCED can be passed to super_iter_inodes, but is an
> internal flag pass here to the file system method.
The INO_ITER_REFERENCED flag is a hack to support the whacky
fsnotify and landlock iterators that are run after evict_inodes()
(which you already noticed...). i.e. there should not be any
unreferenced inodes at this point, so if any are found they should
be skipped.
I think it might be better to remove that flag and replace the
iterator implementation with some kind of SB flag and
WARN_ON_ONCE that fires if a referenced inode is found. With that,
the flags field for super_iter_inodes() can go away...
> Not sure what
> the best way to do it, but maybe just make super_iter_inodes
> a wrapper that calls into the method if available, or
> a generic_iter_inodes_unsafe if the unsafe flag is set, else
> a plain generic_iter_inodes?
Perhaps. I'll look into it.
> > +/* Inode iteration callback return values */
> > +#define INO_ITER_DONE 0
> > +#define INO_ITER_ABORT 1
> > +
> > +/* Inode iteration control flags */
> > +#define INO_ITER_REFERENCED (1U << 0)
> > +#define INO_ITER_UNSAFE (1U << 1)
>
> Please adjust the naming a bit to make clear these are different
> namespaces, e.g. INO_ITER_RET_ and INO_ITER_F_.
Will do.
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
