Re: [PATCH 05/11] xfs: avoid ilock games in the quota scrubber

[Brian Foster <bfoster@redhat.com>]

On Tue, Apr 17, 2018 at 07:40:07PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Now that we have a new QMOPT_QUOTIP_LOCKED flag, use it to dqget in the
> quota scrubber so that we can ilock the quota ip at the start of scrub
> and keep it locked all the way to the end.  This enables us to remove
> quite a bit of locking-related games and manage the quota ip the same
> way we treat other inodes being scrubbed.  Allocate a transaction so
> that we can evade deadlock issues in bmbt lookups.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/quota.c |   46 ++++++++++++++++++----------------------------
>  fs/xfs/scrub/scrub.c |    4 ++++
>  fs/xfs/scrub/scrub.h |    1 +
>  3 files changed, 23 insertions(+), 28 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
> index 363e318..1068a91 100644
> --- a/fs/xfs/scrub/quota.c
> +++ b/fs/xfs/scrub/quota.c
> @@ -66,12 +66,24 @@ xfs_scrub_setup_quota(
>  	struct xfs_inode		*ip)
>  {
>  	uint				dqtype;
> +	int				error;
> +
> +	if (!XFS_IS_QUOTA_RUNNING(sc->mp) || !XFS_IS_QUOTA_ON(sc->mp))
> +		return -ENOENT;
>  
>  	dqtype = xfs_scrub_quota_to_dqtype(sc);
>  	if (dqtype == 0)
>  		return -EINVAL;
> +	sc->has_quotaofflock = true;
> +	mutex_lock(&sc->mp->m_quotainfo->qi_quotaofflock);
>  	if (!xfs_this_quota_on(sc->mp, dqtype))
>  		return -ENOENT;
> +	error = xfs_scrub_setup_fs(sc, ip);

Looks good:

Reviewed-by: Brian Foster <bfoster@redhat.com>

Not really related, but FYI the ip parameter to xfs_scrub_setup_fs()
appears to be unused.

Brian

> +	if (error)
> +		return error;
> +	sc->ip = xfs_quota_inode(sc->mp, dqtype);
> +	xfs_ilock(sc->ip, XFS_ILOCK_EXCL);
> +	sc->ilock_flags = XFS_ILOCK_EXCL;
>  	return 0;
>  }
>  
> @@ -203,7 +215,6 @@ xfs_scrub_quota(
>  	struct xfs_bmbt_irec		irec = { 0 };
>  	struct xfs_scrub_quota_info	sqi;
>  	struct xfs_mount		*mp = sc->mp;
> -	struct xfs_inode		*ip;
>  	struct xfs_quotainfo		*qi = mp->m_quotainfo;
>  	xfs_fileoff_t			max_dqid_off;
>  	xfs_fileoff_t			off = 0;
> @@ -211,25 +222,12 @@ xfs_scrub_quota(
>  	int				nimaps;
>  	int				error = 0;
>  
> -	if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
> -		return -ENOENT;
> -
> -	mutex_lock(&qi->qi_quotaofflock);
>  	dqtype = xfs_scrub_quota_to_dqtype(sc);
> -	if (!xfs_this_quota_on(sc->mp, dqtype)) {
> -		error = -ENOENT;
> -		goto out_unlock_quota;
> -	}
> -
> -	/* Attach to the quota inode and set sc->ip so that reporting works. */
> -	ip = xfs_quota_inode(sc->mp, dqtype);
> -	sc->ip = ip;
>  
>  	/* Look for problem extents. */
> -	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	if (ip->i_d.di_flags & XFS_DIFLAG_REALTIME) {
> +	if (sc->ip->i_d.di_flags & XFS_DIFLAG_REALTIME) {
>  		xfs_scrub_ino_set_corrupt(sc, sc->ip->i_ino);
> -		goto out_unlock_inode;
> +		goto out;
>  	}
>  	max_dqid_off = ((xfs_dqid_t)-1) / qi->qi_dqperchunk;
>  	while (1) {
> @@ -238,11 +236,11 @@ xfs_scrub_quota(
>  
>  		off = irec.br_startoff + irec.br_blockcount;
>  		nimaps = 1;
> -		error = xfs_bmapi_read(ip, off, -1, &irec, &nimaps,
> +		error = xfs_bmapi_read(sc->ip, off, -1, &irec, &nimaps,
>  				XFS_BMAPI_ENTIRE);
>  		if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK, off,
>  				&error))
> -			goto out_unlock_inode;
> +			goto out;
>  		if (!nimaps)
>  			break;
>  		if (irec.br_startblock == HOLESTARTBLOCK)
> @@ -268,26 +266,18 @@ xfs_scrub_quota(
>  		    irec.br_startoff + irec.br_blockcount > max_dqid_off + 1)
>  			xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, off);
>  	}
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
>  		goto out;
>  
>  	/* Check all the quota items. */
>  	sqi.sc = sc;
>  	sqi.last_id = 0;
> -	error = xfs_dquot_iterate(mp, dqtype, 0, xfs_scrub_quota_item, &sqi);
> +	error = xfs_dquot_iterate(mp, dqtype, XFS_QMOPT_QUOTIP_LOCKED,
> +			xfs_scrub_quota_item, &sqi);
>  	if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK,
>  			sqi.last_id * qi->qi_dqperchunk, &error))
>  		goto out;
>  
>  out:
> -	/* We set sc->ip earlier, so make sure we clear it now. */
> -	sc->ip = NULL;
> -out_unlock_quota:
> -	mutex_unlock(&qi->qi_quotaofflock);
>  	return error;
> -
> -out_unlock_inode:
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -	goto out;
>  }
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index cb1e727..c43ee9e 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -42,6 +42,8 @@
>  #include "xfs_refcount_btree.h"
>  #include "xfs_rmap.h"
>  #include "xfs_rmap_btree.h"
> +#include "xfs_quota.h"
> +#include "xfs_qm.h"
>  #include "scrub/xfs_scrub.h"
>  #include "scrub/scrub.h"
>  #include "scrub/common.h"
> @@ -166,6 +168,8 @@ xfs_scrub_teardown(
>  			iput(VFS_I(sc->ip));
>  		sc->ip = NULL;
>  	}
> +	if (sc->has_quotaofflock)
> +		mutex_unlock(&sc->mp->m_quotainfo->qi_quotaofflock);
>  	if (sc->buf) {
>  		kmem_free(sc->buf);
>  		sc->buf = NULL;
> diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
> index 0d92af8..5d79731 100644
> --- a/fs/xfs/scrub/scrub.h
> +++ b/fs/xfs/scrub/scrub.h
> @@ -73,6 +73,7 @@ struct xfs_scrub_context {
>  	void				*buf;
>  	uint				ilock_flags;
>  	bool				try_harder;
> +	bool				has_quotaofflock;
>  
>  	/* State tracking for single-AG operations. */
>  	struct xfs_scrub_ag		sa;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html