Re: [PATCH 05/11] xfs: pass flags and return gc errors from xfs_blockgc_free_quota

From: Brian Foster <bfoster@redhat.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org, hch@infradead.org, david@fromorbit.com
Subject: Re: [PATCH 05/11] xfs: pass flags and return gc errors from xfs_blockgc_free_quota
Date: Mon, 25 Jan 2021 13:15:15 -0500	[thread overview]
Message-ID: <20210125181515.GK2047559@bfoster> (raw)
In-Reply-To: <161142794741.2171939.10175775024910240954.stgit@magnolia>

On Sat, Jan 23, 2021 at 10:52:27AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Change the signature of xfs_blockgc_free_quota in preparation for the
> next few patches.  Callers can now pass EOF_FLAGS into the function to
> control scan parameters; and the function will now pass back any
> corruption errors seen while scanning, though for our retry loops we'll
> just try again unconditionally.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_file.c   |    7 +++----
>  fs/xfs/xfs_icache.c |   20 ++++++++++++--------
>  fs/xfs/xfs_icache.h |    2 +-
>  3 files changed, 16 insertions(+), 13 deletions(-)
> 
> 
...
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index aba901d5637b..68b6f72593dc 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -1651,19 +1651,21 @@ xfs_start_block_reaping(
>   * failure. We make a best effort by including each quota under low free space
>   * conditions (less than 1% free space) in the scan.
>   */
> -bool
> +int
>  xfs_blockgc_free_quota(
> -	struct xfs_inode	*ip)
> +	struct xfs_inode	*ip,
> +	unsigned int		eof_flags)
>  {
>  	struct xfs_eofblocks	eofb = {0};
>  	struct xfs_dquot	*dq;
>  	bool			do_work = false;
> +	int			error;
>  
>  	/*
> -	 * Run a sync scan to increase effectiveness and use the union filter to
> +	 * Run a scan to increase effectiveness and use the union filter to

The original comment referred to the increased effectiveness of a sync
scan. It doesn't make a whole lot of sense without that qualification
IMO (even though the scan is still sync). We could move that bit of
comment to the caller where the flag is now set, but it's probably fine
to just remove that text also. With that tweak:

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

>  	 * cover all applicable quotas in a single scan.
>  	 */
> -	eofb.eof_flags = XFS_EOF_FLAGS_UNION | XFS_EOF_FLAGS_SYNC;
> +	eofb.eof_flags = XFS_EOF_FLAGS_UNION | eof_flags;
>  
>  	if (XFS_IS_UQUOTA_ENFORCED(ip->i_mount)) {
>  		dq = xfs_inode_dquot(ip, XFS_DQTYPE_USER);
> @@ -1693,9 +1695,11 @@ xfs_blockgc_free_quota(
>  	}
>  
>  	if (!do_work)
> -		return false;
> +		return 0;
>  
> -	xfs_icache_free_eofblocks(ip->i_mount, &eofb);
> -	xfs_icache_free_cowblocks(ip->i_mount, &eofb);
> -	return true;
> +	error = xfs_icache_free_eofblocks(ip->i_mount, &eofb);
> +	if (error)
> +		return error;
> +
> +	return xfs_icache_free_cowblocks(ip->i_mount, &eofb);
>  }
> diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
> index 21b726a05b0d..d64ea8f5c589 100644
> --- a/fs/xfs/xfs_icache.h
> +++ b/fs/xfs/xfs_icache.h
> @@ -54,7 +54,7 @@ long xfs_reclaim_inodes_nr(struct xfs_mount *mp, int nr_to_scan);
>  
>  void xfs_inode_set_reclaim_tag(struct xfs_inode *ip);
>  
> -bool xfs_blockgc_free_quota(struct xfs_inode *ip);
> +int xfs_blockgc_free_quota(struct xfs_inode *ip, unsigned int eof_flags);
>  
>  void xfs_inode_set_eofblocks_tag(struct xfs_inode *ip);
>  void xfs_inode_clear_eofblocks_tag(struct xfs_inode *ip);
> 