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>

Some more comments after seeing more conversions:

> +static bool super_iter_iget(struct inode *inode, int flags)
> +{
> +	bool ret = false;
> +

Here I'd do a lockless icount_read_once(inode) check if INO_ITER_F_REFERENCED
is set to avoid unnecessary i_lock round trip.

> +	spin_lock(&inode->i_lock);
> +	if (inode_state_read(inode) & (I_NEW | I_FREEING | I_WILL_FREE))
> +		goto out_unlock;
> +
> +	if ((flags & INO_ITER_F_REFERENCED) && !icount_read(inode))
> +		goto out_unlock;
> +
> +	__iget(inode);
> +	ret = true;
> +out_unlock:
> +	spin_unlock(&inode->i_lock);
> +	return ret;
> +}
> +
> +static int generic_iter_inodes(struct super_block *sb, ino_iter_fn iter_fn,
> +			       void *private_data, int flags)
> +{
> +	struct inode *inode, *old_inode = NULL;
> +	int ret = INO_ITER_RET_DONE;
> +
> +	spin_lock(&sb->s_inode_list_lock);
> +	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> +		if (!super_iter_iget(inode, flags))
> +			continue;
> +
> +		spin_unlock(&sb->s_inode_list_lock);
> +		iput(old_inode);
> +
> +		ret = iter_fn(inode, private_data);
> +
> +		old_inode = inode;
> +		if (ret == INO_ITER_RET_ABORT) {
> +			ret = INO_ITER_RET_DONE;
> +			goto out_loop;
> +		}
> +		if (ret < 0)
> +			goto out_loop;
> +
> +		cond_resched();
> +		spin_lock(&sb->s_inode_list_lock);
> +	}
> +	spin_unlock(&sb->s_inode_list_lock);
> +out_loop:
> +	iput(old_inode);
> +	return ret;
> +}
> +
> +/*
> + * 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);

After some thought, I'd probably make this function inline in some VFS
internal header. That way the compiler can inline everything and avoid the
extra costs of indirect function calls, etc. These iterators tend to be
rather lightweight so it would avoid performance regressions from the
conversion.

I was considering the same for the generic iterator as well but there I
don't have so strong opinion as the function is already relatively big and
the callbacks are usually more heavy. Might still be worth some more
detailed investigation and performance measurements...

> +		if (ret == INO_ITER_RET_ABORT)
> +			break;
> +	}
> +	spin_unlock(&sb->s_inode_list_lock);
> +	rcu_read_unlock();
> +
> +	return ret;
> +}

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