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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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, <rec, &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, <rec, &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(<rec, owner, flags))
> > - have_lt = 0;
> > + if (!xfs_rmap_is_mergeable(<rec, 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@xxxxxxxxxx>
>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
