Re: [PATCH 1/3] vfs: add inode iteration method super_iter_inodes

[Jan Kara]

On Thu 15-10-26 17:38:52, Huang Shijie wrote:
> This will be used to replace the explicit sb->s_inodes iteration,
> with the caller supplying a callback function and a private data
> pointer that is passed to the callback along with each inode that is
> iterated.
> 
> A single iteration function, super_iter_inodes(), is provided, with
> the iteration mode selected by the INO_ITER_F_UNSAFE flag. The
> INO_ITER_F_REFERENCED flag can additionally restrict the iteration to
> inodes that still have a reference.
> 
> Without INO_ITER_F_UNSAFE, the callback is called with a valid,
> unlocked and referenced inode, so any inode operation, including
> blocking operations, is allowed. The iterator is responsible for
> inode lifecycle management, so the callback only needs to implement
> the operation it wants to perform on all inodes.
> 
> With INO_ITER_F_UNSAFE set, the inodes are iterated without taking a
> reference and the callback is run in an atomic context that must not
> block. This mode is for internal VFS use only and runs under a RCU
> read lock so that the VFS inode is not freed from under the callback.
> It is needed for operations that require tight control over the state
> of the inodes they operate on.
> 
> This mechanism allows the existing sb->s_inodes iteration models to
> be maintained, providing a generic implementation for iterating all
> cached inodes on the superblock.
> 
> Signed-off-by: Huang Shijie <huangsj@xxxxxxxx>

This looks as a good base but see my comments for individual conversions.
Some smaller comments below:

> +/*
> + * Unsafe iteration for internal VFS use only: @iter_fn runs in atomic
> + * context with only an inode existence guarantee, so it must not block
> + * and must perform its own inode state checks. Return INO_ITER_RET_ABORT
> + * to stop the iteration early.
> + */
> +static int generic_iter_inodes_unsafe(struct super_block *sb, ino_iter_fn iter_fn,
> +				      void *private_data)
> +{
> +	struct inode *inode;
> +	int ret = INO_ITER_RET_DONE;
> +
> +	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_RET_ABORT)
> +			break;
> +	}
> +	spin_unlock(&sb->s_inode_list_lock);
> +	rcu_read_unlock();
> +
> +	return ret;
> +}
> +
> +/**
> + * super_iter_inodes - iterate all the cached inodes on a superblock
> + * @sb: superblock to iterate
> + * @iter_fn: callback to run on every inode found
> + * @private_data: private data to pass to @iter_fn
> + * @flags: inode iteration control flags
> + *
> + * 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.
> + *
> + * If @flags has %INO_ITER_F_REFERENCED set, inodes with a zero refcount are
> + * skipped as well, so that they are not needlessly bounced through
> + * __iget()/iput() (which would evict them from the inode cache).
> + *
> + * If @flags has %INO_ITER_F_UNSAFE set, the inodes are iterated without
> + * taking a reference and @iter_fn is run in an atomic context that must not
> + * block; %INO_ITER_F_REFERENCED is ignored in this mode. This is for internal
> + * VFS operations only and almost certainly not what the caller wants.
> + *
> + * Return: %INO_ITER_RET_DONE on completion, a negative errno if @iter_fn()
> + * returned an error (safe iteration only), or %INO_ITER_RET_ABORT if the
> + * unsafe iteration was aborted early.
> + */
> +int super_iter_inodes(struct super_block *sb, ino_iter_fn iter_fn,
> +		      void *private_data, int flags)
> +{
> +	if (flags & INO_ITER_F_UNSAFE)
> +		return generic_iter_inodes_unsafe(sb, iter_fn, private_data);

Since unsafe iteration is a separate function anyway and we advertise it as
an "internal VFS" function (which I agree with), I'd just have
super_iter_inodes() for the normal iteration and super_iter_inodes_unsafe()
for the unsafe iteration which won't be exported to modules (so we really
get the message across this should be VFS internal function). 

> +
> +	return generic_iter_inodes(sb, iter_fn, private_data, flags);
> +}
> +EXPORT_SYMBOL_GPL(super_iter_inodes);
> +

...
> +/* Inode iteration callback return values */
> +#define INO_ITER_RET_DONE		0

Maybe INO_ITER_RET_NEXT? DONE looks too much like the loop should finish.

> +#define INO_ITER_RET_ABORT		1
> +
> +/* Inode iteration control flags */
> +/* Iterate without taking a reference; @iter_fn must not block. */
> +#define INO_ITER_F_UNSAFE		(1U << 1)
> +/* Only iterate inodes that still have a reference. */
> +#define INO_ITER_F_REFERENCED		(1U << 2)

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR