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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Jan 16, 2018 at 01:57:46PM +1100, Dave Chinner wrote:
> 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.

Clamped length, and it ought to be xfs_extlen_t as you point out.

> 
> > +	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;

Yes.
> 
> > +		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.

I think I confused myself when wandering through the rt code... will fix.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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