On Tue, Jan 09, 2018 at 10:55:25AM +1100, Dave Chinner wrote:
> On Fri, Dec 22, 2017 at 04:44:24PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Scrub should make sure that each bnobt record has a corresponding
> > cntbt record.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/scrub/agheader.c | 26 ++++++++++++++++++++++++++
> > fs/xfs/scrub/alloc.c | 37 +++++++++++++++++++++++++++++++++++++
> > 2 files changed, 63 insertions(+)
> >
> >
> > diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
> > index 3bb0f96..e87022f 100644
> > --- a/fs/xfs/scrub/agheader.c
> > +++ b/fs/xfs/scrub/agheader.c
> > @@ -444,6 +444,7 @@ xfs_scrub_agf_xref(
> > struct xfs_btree_cur **pcur;
> > xfs_agblock_t bno;
> > xfs_extlen_t blocks;
> > + int have;
> > int error;
> >
> > bno = XFS_AGF_BLOCK(mp);
> > @@ -465,6 +466,31 @@ xfs_scrub_agf_xref(
> > xfs_scrub_block_xref_set_corrupt(sc, sc->sa.agf_bp);
> > }
> >
> > + /* Cross-reference with the cntbt. */
> > + pcur = &sc->sa.cnt_cur;
> > + while (*pcur) {
> > + xfs_agblock_t agbno;
> > +
> > + /* Any freespace at all? */
> > + error = xfs_alloc_lookup_le(*pcur, 0, -1U, &have);
> > + if (!xfs_scrub_should_xref(sc, &error, pcur))
> > + break;
> > + if (!have) {
> > + if (agf->agf_freeblks != be32_to_cpu(0))
>
> No need to endian convert 0.
Fixed.
> > + xfs_scrub_block_xref_set_corrupt(sc,
> > + sc->sa.agf_bp);
> > + break;
> > + }
> > +
> > + /* Check agf_longest */
> > + error = xfs_alloc_get_rec(*pcur, &agbno, &blocks, &have);
> > + if (!xfs_scrub_should_xref(sc, &error, pcur))
> > + break;
> > + if (!have || blocks != be32_to_cpu(agf->agf_longest))
> > + xfs_scrub_block_xref_set_corrupt(sc, sc->sa.agf_bp);
> > + break;
> > + }
>
> Why is this a while loop? It'll break out at the end of the first
> loop - did you forget to remove that last break statement?
Curious use of while loops to avoid having a goto dontcareaboutcntbt label.
Time to make this (and the other one) its own function....
--D
> > +
> > /* scrub teardown will take care of sc->sa for us */
> > }
> >
> > diff --git a/fs/xfs/scrub/alloc.c b/fs/xfs/scrub/alloc.c
> > index 3d6f8cc..9a28e3d 100644
> > --- a/fs/xfs/scrub/alloc.c
> > +++ b/fs/xfs/scrub/alloc.c
> > @@ -31,6 +31,7 @@
> > #include "xfs_sb.h"
> > #include "xfs_alloc.h"
> > #include "xfs_rmap.h"
> > +#include "xfs_alloc.h"
> > #include "scrub/xfs_scrub.h"
> > #include "scrub/scrub.h"
> > #include "scrub/common.h"
> > @@ -57,6 +58,42 @@ xfs_scrub_allocbt_xref(
> > xfs_agblock_t bno,
> > xfs_extlen_t len)
> > {
> > + struct xfs_btree_cur **pcur;
> > + struct xfs_scrub_ag *psa = &sc->sa;
> > + xfs_agblock_t fbno;
> > + xfs_extlen_t flen;
> > + int has_otherrec;
> > + int error;
> > +
> > + /*
> > + * Ensure there's a corresponding cntbt/bnobt record matching
> > + * this bnobt/cntbt record, respectively.
> > + */
> > + if (sc->sm->sm_type == XFS_SCRUB_TYPE_BNOBT)
> > + pcur = &psa->cnt_cur;
> > + else
> > + pcur = &psa->bno_cur;
> > + while (*pcur) {
> > + error = xfs_alloc_lookup_le(*pcur, bno, len, &has_otherrec);
> > + if (!xfs_scrub_should_xref(sc, &error, pcur))
> > + break;
> > + if (!has_otherrec) {
> > + xfs_scrub_btree_xref_set_corrupt(sc, *pcur, 0);
> > + break;
> > + }
> > +
> > + error = xfs_alloc_get_rec(*pcur, &fbno, &flen, &has_otherrec);
> > + if (!xfs_scrub_should_xref(sc, &error, pcur))
> > + break;
> > + if (!has_otherrec) {
> > + xfs_scrub_btree_xref_set_corrupt(sc, *pcur, 0);
> > + break;
> > + }
> > +
> > + if (fbno != bno || flen != len)
> > + xfs_scrub_btree_xref_set_corrupt(sc, *pcur, 0);
> > + break;
> > + }
>
> Same again - I don't see why this is a while loop.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
> --
> 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
