Re: [PATCH 1/6] xfs: clean up xchk_bmap_check_rmaps usage of XFS_IFORK_Q

[Brian Foster <bfoster@redhat.com>]

On Mon, May 18, 2020 at 09:33:53AM +0200, Christoph Hellwig wrote:
> From: "Darrick J. Wong" <darrick.wong@oracle.com>
> 
> XFS_IFORK_Q is supposed to be a predicate, not a function returning a
> value.  Its usage is in xchk_bmap_check_rmaps is incorrect, but that
> function only cares about whether or not the "size" of the data is zero
> or not.  Convert that logic to use a proper boolean, and teach the
> caller to skip the call entirely if the end result would be that we'd do
> nothing anyway.  This avoids a crash later in this series.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> [hch: generalized the NULL ifor check]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

I'm still rather confused about the i_size logic, but this seems like a
good cleanup regardless:

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

>  fs/xfs/scrub/bmap.c | 35 +++++++++++++----------------------
>  1 file changed, 13 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index add8598eacd5d..d0daf3de9fde5 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
> @@ -566,8 +566,8 @@ xchk_bmap_check_rmaps(
>  	struct xfs_scrub	*sc,
>  	int			whichfork)
>  {
> -	loff_t			size;
>  	xfs_agnumber_t		agno;
> +	bool			zero_size;
>  	int			error;
>  
>  	if (!xfs_sb_version_hasrmapbt(&sc->mp->m_sb) ||
> @@ -579,6 +579,8 @@ xchk_bmap_check_rmaps(
>  	if (XFS_IS_REALTIME_INODE(sc->ip) && whichfork == XFS_DATA_FORK)
>  		return 0;
>  
> +	ASSERT(XFS_IFORK_PTR(sc->ip, whichfork) != NULL);
> +
>  	/*
>  	 * Only do this for complex maps that are in btree format, or for
>  	 * situations where we would seem to have a size but zero extents.
> @@ -586,19 +588,13 @@ xchk_bmap_check_rmaps(
>  	 * to flag this bmap as corrupt if there are rmaps that need to be
>  	 * reattached.
>  	 */
> -	switch (whichfork) {
> -	case XFS_DATA_FORK:
> -		size = i_size_read(VFS_I(sc->ip));
> -		break;
> -	case XFS_ATTR_FORK:
> -		size = XFS_IFORK_Q(sc->ip);
> -		break;
> -	default:
> -		size = 0;
> -		break;
> -	}
> +	if (whichfork == XFS_DATA_FORK)
> +		zero_size = i_size_read(VFS_I(sc->ip)) == 0;
> +	else
> +		zero_size = false;
> +
>  	if (XFS_IFORK_FORMAT(sc->ip, whichfork) != XFS_DINODE_FMT_BTREE &&
> -	    (size == 0 || XFS_IFORK_NEXTENTS(sc->ip, whichfork) > 0))
> +	    (zero_size || XFS_IFORK_NEXTENTS(sc->ip, whichfork) > 0))
>  		return 0;
>  
>  	for (agno = 0; agno < sc->mp->m_sb.sb_agcount; agno++) {
> @@ -627,12 +623,14 @@ xchk_bmap(
>  	struct xchk_bmap_info	info = { NULL };
>  	struct xfs_mount	*mp = sc->mp;
>  	struct xfs_inode	*ip = sc->ip;
> -	struct xfs_ifork	*ifp;
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
>  	xfs_fileoff_t		endoff;
>  	struct xfs_iext_cursor	icur;
>  	int			error = 0;
>  
> -	ifp = XFS_IFORK_PTR(ip, whichfork);
> +	/* Non-existent forks can be ignored. */
> +	if (!ifp)
> +		goto out;
>  
>  	info.is_rt = whichfork == XFS_DATA_FORK && XFS_IS_REALTIME_INODE(ip);
>  	info.whichfork = whichfork;
> @@ -641,9 +639,6 @@ xchk_bmap(
>  
>  	switch (whichfork) {
>  	case XFS_COW_FORK:
> -		/* Non-existent CoW forks are ignorable. */
> -		if (!ifp)
> -			goto out;
>  		/* No CoW forks on non-reflink inodes/filesystems. */
>  		if (!xfs_is_reflink_inode(ip)) {
>  			xchk_ino_set_corrupt(sc, sc->ip->i_ino);
> @@ -651,8 +646,6 @@ xchk_bmap(
>  		}
>  		break;
>  	case XFS_ATTR_FORK:
> -		if (!ifp)
> -			goto out_check_rmap;
>  		if (!xfs_sb_version_hasattr(&mp->m_sb) &&
>  		    !xfs_sb_version_hasattr2(&mp->m_sb))
>  			xchk_ino_set_corrupt(sc, sc->ip->i_ino);
> @@ -700,7 +693,6 @@ xchk_bmap(
>  
>  	/* Scrub extent records. */
>  	info.lastoff = 0;
> -	ifp = XFS_IFORK_PTR(ip, whichfork);
>  	for_each_xfs_iext(ifp, &icur, &irec) {
>  		if (xchk_should_terminate(sc, &error) ||
>  		    (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> @@ -717,7 +709,6 @@ xchk_bmap(
>  			goto out;
>  	}
>  
> -out_check_rmap:
>  	error = xchk_bmap_check_rmaps(sc, whichfork);
>  	if (!xchk_fblock_xref_process_error(sc, whichfork, 0, &error))
>  		goto out;
> -- 
> 2.26.2
>