Re: [PATCH 01/21] xfs: don't assume a left rmap when allocating a new rmap

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

On Thu, Jun 28, 2018 at 02:11:38PM -0700, Allison Henderson wrote:
> On 06/24/2018 12:23 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The original rmap code assumed that there would always be at least one
> > rmap in the rmapbt (the AG sb/agf/agi) and so errored out if it didn't
> > find one.  This assumption isn't true for the rmapbt repair function
> > (and it won't be true for realtime rmap either), so remove the check and
> > just deal with the situation.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >   fs/xfs/libxfs/xfs_rmap.c |   24 ++++++++++++------------
> >   1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> > index d4460b0d2d81..8b2a2f81d110 100644
> > --- a/fs/xfs/libxfs/xfs_rmap.c
> > +++ b/fs/xfs/libxfs/xfs_rmap.c
> > @@ -753,19 +753,19 @@ xfs_rmap_map(
> >   			&have_lt);
> >   	if (error)
> >   		goto out_error;
> > -	XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
> > -
> > -	error = xfs_rmap_get_rec(cur, &ltrec, &have_lt);
> > -	if (error)
> > -		goto out_error;
> > -	XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
> > -	trace_xfs_rmap_lookup_le_range_result(cur->bc_mp,
> > -			cur->bc_private.a.agno, ltrec.rm_startblock,
> > -			ltrec.rm_blockcount, ltrec.rm_owner,
> > -			ltrec.rm_offset, ltrec.rm_flags);
> > +	if (have_lt) {
> > +		error = xfs_rmap_get_rec(cur, &ltrec, &have_lt);
> > +		if (error)
> > +			goto out_error;
> > +		XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
> > +		trace_xfs_rmap_lookup_le_range_result(cur->bc_mp,
> > +				cur->bc_private.a.agno, ltrec.rm_startblock,
> > +				ltrec.rm_blockcount, ltrec.rm_owner,
> > +				ltrec.rm_offset, ltrec.rm_flags);
> > -	if (!xfs_rmap_is_mergeable(&ltrec, owner, flags))
> > -		have_lt = 0;
> > +		if (!xfs_rmap_is_mergeable(&ltrec, owner, flags))
> > +			have_lt = 0;
> > +	}
> >   	XFS_WANT_CORRUPTED_GOTO(mp,
> >   		have_lt == 0 ||
> > 
> 
> Alrighty, looks ok after some digging around. I'm still a little puzzled as
> to why the original code raised the assert without checking to see whats on
> the other side of the cursor?  Assuming the error condition
> was supposed to be the case when the tree was empty.  In any case, it looks
> correct now.

At the time (~2014?) I don't think either Dave or I were thinking about
rmapbt being extended into the realtime device, so we thought that
assumption was a reasonable one to make.  That was, of course, long
before I got far enough along in designing online check to realize that
"hey, maybe we should be able to rebuild things from scratch too"... :)

Anyway, thank you both for the review.

--D

> Reviewed-by: Allison Henderson <allison.henderson@oracle.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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=Q2PdVMOGp7_huNLFbP6xty0mgocZk65leUyLVRvSsSY&s=6-FSoklyIhTEtg811gAG43N9-7Z-sYFsm7zv33EadgQ&e=
> > 
> --
> 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