Re: [PATCH 2/3] vfs: convert inode iterators to the unsafe super_iter_inodes()

[Jan Kara]

On Thu 15-10-26 17:38:53, Huang Shijie wrote:
> Convert VFS internal superblock inode iterators that cannot use
> referenced inodes to super_iter_inodes() with INO_ITER_F_UNSAFE set:
>   - Dquot removal
>   - inode eviction
>   - The nr_blockdev_pages()
>   - generic_shutdown_super()
> 
> Signed-off-by: Huang Shijie <huangsj@xxxxxxxx>

See some comments below.

> diff --git a/fs/inode.c b/fs/inode.c
> index a9d37be390a1..139c56ee9d11 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -853,8 +853,11 @@ static void evict(struct inode *inode)
>   * Dispose-list gets a local list with local inodes in it, so it doesn't
>   * need to worry about list corruption and SMP locks.
>   */
> -static void dispose_list(struct list_head *head)
> +static bool dispose_list(struct list_head *head)
>  {
> +	if (list_empty(head))
> +		return false;
> +
>  	while (!list_empty(head)) {
>  		struct inode *inode;
>  
> @@ -864,6 +867,29 @@ static void dispose_list(struct list_head *head)
>  		evict(inode);
>  		cond_resched();
>  	}
> +	return true;
> +}
> +
> +static int evict_inode_fn(struct inode *inode, void *data)
> +{
> +	struct list_head *dispose = data;
> +

Here we miss the unlocked check
            if (icount_read_once(inode))
                    continue;
so every inode will now go through the i_lock roundtrip.

> +	spin_lock(&inode->i_lock);
> +	if (icount_read(inode) ||
> +	    (inode_state_read(inode) & (I_NEW | I_FREEING | I_WILL_FREE))) {
> +		spin_unlock(&inode->i_lock);
> +		return INO_ITER_RET_DONE;
> +	}
> +
> +	inode_state_set(inode, I_FREEING);
> +	inode_lru_list_del(inode);
> +	spin_unlock(&inode->i_lock);
> +	list_add(&inode->i_lru, dispose);
> +
> +	/* Abort so evict_inodes() can dispose of the queued inodes first. */
> +	if (need_resched())
> +		return INO_ITER_RET_ABORT;
> +	return INO_ITER_RET_DONE;
>  }

The need_resched() handling needs more thought here. The way you've done it
will just abort the iteration, evict gathered inodes and then restart the
whole iteration (which will then have to skip again all the inodes we could
not evict). Which is not what evict_inodes() does today and we really had
to resort to those tricks to avoid softlockups under certain loads.
Frankly, this iteration is so special that I'd be willing to leave it
unconverted for now rather than trying to bend the general iterator for its
needs.

> @@ -877,44 +903,16 @@ static void dispose_list(struct list_head *head)
>   */
>  void evict_inodes(struct super_block *sb)
>  {
> -	struct inode *inode;
>  	LIST_HEAD(dispose);
>  
> -	spin_lock(&sb->s_inode_list_lock);
> -	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> -		if (icount_read_once(inode))
> -			continue;
> -
> -		spin_lock(&inode->i_lock);
> -		if (icount_read(inode)) {
> -			spin_unlock(&inode->i_lock);
> -			continue;
> -		}
> -		if (inode_state_read(inode) & (I_NEW | I_FREEING | I_WILL_FREE)) {
> -			spin_unlock(&inode->i_lock);
> -			continue;
> -		}
> -
> -		inode_state_set(inode, I_FREEING);
> -		inode_lru_list_del(inode);
> -		spin_unlock(&inode->i_lock);
> -
> +	do {
>  		/*
> -		 * Keep this inode out of dispose so it stays on s_inodes while
> -		 * the list lock is dropped. I_FREEING prevents new references
> -		 * and leaves eviction to us, so we can resume the walk from it.
> +		 * Taking references would keep the inodes we are trying to
> +		 * evict alive, so use the unsafe iterator and let
> +		 * evict_inode_fn() do the state checks and queue them.
>  		 */
> -		if (need_resched()) {
> -			spin_unlock(&sb->s_inode_list_lock);
> -			cond_resched();
> -			dispose_list(&dispose);
> -			spin_lock(&sb->s_inode_list_lock);
> -		}
> -		list_add(&inode->i_lru, &dispose);
> -	}
> -	spin_unlock(&sb->s_inode_list_lock);
> -
> -	dispose_list(&dispose);
> +		super_iter_inodes(sb, evict_inode_fn, &dispose, INO_ITER_F_UNSAFE);
> +	} while (dispose_list(&dispose));
>  }
>  EXPORT_SYMBOL_GPL(evict_inodes);

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