Re: [PATCH v2 20/21] xfs: cross-reference the realtime bitmap

[Dave Chinner <david@fromorbit.com>]

On Tue, Jan 09, 2018 at 01:26:09PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> While we're scrubbing various btrees, cross-reference the records
> with the other metadata.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: streamline scrubber arguments, remove stack allocated objects
> ---
>  fs/xfs/libxfs/xfs_rtbitmap.c |   30 ++++++++++++++++++++++++++++++
>  fs/xfs/scrub/bmap.c          |    3 +++
>  fs/xfs/scrub/rtbitmap.c      |   20 ++++++++++++++++++++
>  fs/xfs/scrub/scrub.h         |    6 ++++++
>  fs/xfs/xfs_rtalloc.h         |    4 ++++
>  5 files changed, 63 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 3fb29a5..a3f25a2 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1097,3 +1097,33 @@ xfs_verify_rtbno(
>  {
>  	return rtbno < mp->m_sb.sb_rblocks;
>  }
> +
> +/* Is the given extent all free? */
> +int
> +xfs_rtalloc_extent_is_free(
> +	struct xfs_mount		*mp,
> +	struct xfs_trans		*tp,
> +	xfs_rtblock_t			start,
> +	xfs_rtblock_t			len,

shouldn't that be a xfs_extlen_t?

> +	bool				*is_free)
> +{
> +	xfs_rtblock_t			end;
> +	xfs_extlen_t			clen;

clen? Not sure what that is shorthand for.


> +	int				matches;
> +	int				error;
> +
> +	*is_free = false;
> +	while (len) {
> +		clen = len > ~0U ? ~0U : len;

So we're checking for a len > UINT_MAX and clamping it here because
the rt code uses 32 bit extent lengths internally? If we actually
need 64 bit extent lengths, then isn't using UINT_MAX more obvious
as to the purpose? i.e

	if (len > UINT_MAX)
		clen = UINT_MAX;
	else
		clen = len;

> +		error = xfs_rtcheck_range(mp, tp, start, clen, 1, &end,
> +				&matches);
> +		if (error || !matches || end < start + clen)
> +			return error;
> +
> +		len -= end - start;
> +		start = end + 1;
> +	}
> +
> +	*is_free = true;
> +	return error;
> +}
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index 5c34e025..2908a1b 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
> @@ -278,6 +278,9 @@ xfs_scrub_bmap_rt_extent_xref(
>  {
>  	if (info->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
>  		return;
> +
> +	xfs_scrub_xref_not_rtfree(info->sc, irec->br_startblock,
> +			irec->br_blockcount);

So, irec->br_blockcount is a xfs_extlen, which is 32 bits....

>  }
>  
>  /* Cross-reference a single datadev extent record. */
> diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
> index 6860d5d..1828b17 100644
> --- a/fs/xfs/scrub/rtbitmap.c
> +++ b/fs/xfs/scrub/rtbitmap.c
> @@ -98,3 +98,23 @@ xfs_scrub_rtsummary(
>  	/* XXX: implement this some day */
>  	return -ENOENT;
>  }
> +
> +
> +/* xref check that the extent is not free in the rtbitmap */
> +void
> +xfs_scrub_xref_not_rtfree(
> +	struct xfs_scrub_context	*sc,
> +	xfs_fsblock_t			fsbno,
> +	xfs_fsblock_t			len)

So we're quietly converting it to a 64 bit length here. ANd not even
using xfs_rtblock_t as you'd expect for a realtime block...

What caller needs/uses a 64 bit extent length? All the rt allocation
code uses xfs_extlen_t for lengths, so I'm a bit confused as to why
it's needed here.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com