Re: [PATCH 1/3] xfs: remove tag parameter from xfs_inode_walk{,_ag}

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Mar 19, 2021 at 06:25:01AM +0000, Christoph Hellwig wrote:
> On Thu, Mar 18, 2021 at 03:33:45PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > It turns out that there is a 1:1 mapping between the execute and tag
> > parameters that are passed to xfs_inode_walk_ag:
> > 
> > 	xfs_dqrele_inode => XFS_ICI_NO_TAG
> > 	xfs_blockgc_scan_inode => XFS_ICI_BLOCKGC_TAG
> > 
> > The radix tree tags are an implementation detail of the inode cache,
> > which means that callers outside of xfs_icache.c have no business
> > passing in radix tree tags.  Since we're about to get rid of the
> > indirect calls in the BLOCKGC case, eliminate the extra argument in
> > favor of computing the ICI tag from the execute argument passed into the
> > function.
> 
> This seems backwards to me.  I'd rather deduce the function from the
> talk, which seems like a more sensible pattern.

Fair enough.

> That being said, the quota inode walk is a little different in that it
> doesn't use any tags, so switching it to a plain list_for_each_entry_safe
> on sb->s_inodes would seems more sensible, something like this untested
> patch:

Hmm, well, I look forward to hearing the results of your testing. :)

I /think/ this will work, since quotaoff doesn't touch inodes that can't
be igrabbed (i.e. their VFS state is gone), so walking sb->s_inodes
/should/ be the same.  The only thing I'm not sure about is that the vfs
removes the inode from the sb list before clear_inode sets I_FREEING
(to prevent further igrab), which /could/ introduce a behavioral change?
Though I think even if quotaoff ends up racing with evict_inodes, the
xfs_fs_destroy_inode call will inactivate the inode and drop the dquots
(before the next patchset) or queue the inode for inactivation and
detach the dquots.

One thing that occurs to me -- do the quota and rt metadata inodes end
up on the sb inode list?  The rt metadata inodes definitely contribute
to the root dquot's inode counts.

--D

> ---
> From 9ae07b6bf8c6b1337a627c8f0ad619c56511b343 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 19 Mar 2021 07:16:31 +0100
> Subject: xfs: use s_inodes in xfs_qm_dqrele_all_inodes
> 
> Using xfs_inode_walk in xfs_qm_dqrele_all_inodes is complete overkill,
> given that function simplify wants to iterate all live inodes known
> to the VFS.  Just iterate over the s_inodes list.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_qm_syscalls.c | 50 +++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 11f1e2fbf22f44..4e33919ed04b56 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -748,41 +748,27 @@ xfs_qm_scall_getquota_next(
>  	return error;
>  }
>  
> -STATIC int
> +static void
>  xfs_dqrele_inode(
>  	struct xfs_inode	*ip,
> -	void			*args)
> +	uint			flags)
>  {
> -	uint			*flags = args;
> -
> -	/* skip quota inodes */
> -	if (ip == ip->i_mount->m_quotainfo->qi_uquotaip ||
> -	    ip == ip->i_mount->m_quotainfo->qi_gquotaip ||
> -	    ip == ip->i_mount->m_quotainfo->qi_pquotaip) {
> -		ASSERT(ip->i_udquot == NULL);
> -		ASSERT(ip->i_gdquot == NULL);
> -		ASSERT(ip->i_pdquot == NULL);
> -		return 0;
> -	}
> -
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	if ((*flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
> +	if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
>  		xfs_qm_dqrele(ip->i_udquot);
>  		ip->i_udquot = NULL;
>  	}
> -	if ((*flags & XFS_GQUOTA_ACCT) && ip->i_gdquot) {
> +	if ((flags & XFS_GQUOTA_ACCT) && ip->i_gdquot) {
>  		xfs_qm_dqrele(ip->i_gdquot);
>  		ip->i_gdquot = NULL;
>  	}
> -	if ((*flags & XFS_PQUOTA_ACCT) && ip->i_pdquot) {
> +	if ((flags & XFS_PQUOTA_ACCT) && ip->i_pdquot) {
>  		xfs_qm_dqrele(ip->i_pdquot);
>  		ip->i_pdquot = NULL;
>  	}
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -	return 0;
>  }
>  
> -
>  /*
>   * Go thru all the inodes in the file system, releasing their dquots.
>   *
> @@ -794,7 +780,29 @@ xfs_qm_dqrele_all_inodes(
>  	struct xfs_mount	*mp,
>  	uint			flags)
>  {
> +	struct super_block	*sb = mp->m_super;
> +	struct inode		*inode, *old_inode = NULL;
> +
>  	ASSERT(mp->m_quotainfo);
> -	xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode,
> -			&flags, XFS_ICI_NO_TAG);
> +
> +	spin_lock(&sb->s_inode_list_lock);
> +	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> +		struct xfs_inode	*ip = XFS_I(inode);
> +
> +		if (xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
> +			continue;
> +		if (!igrab(inode))
> +			continue;
> +		spin_unlock(&sb->s_inode_list_lock);
> +
> +		iput(old_inode);
> +		old_inode = inode;
> +
> +		xfs_dqrele_inode(ip, flags);
> +
> +		spin_lock(&sb->s_inode_list_lock);
> +	}
> +	spin_unlock(&sb->s_inode_list_lock);
> +
> +	iput(old_inode);
>  }
> -- 
> 2.30.1
>