xfs
[Top] [All Lists]

Re: [PATCH 01/14] repair: merge scanfunc_bno and scanfunc_cnt

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 01/14] repair: merge scanfunc_bno and scanfunc_cnt
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Mon, 12 Oct 2009 11:53:00 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20090902175839.915684396@xxxxxxxxxxxxxxxxxxxxxx>
References: <20090902175531.469184575@xxxxxxxxxxxxxxxxxxxxxx> <20090902175839.915684396@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Thunderbird 2.0.0.23 (Macintosh/20090812)
Christoph Hellwig wrote:

Those two functions are almost identical. The big difference is that we only
move blocks from XR_E_FREE1 to XR_E_FREE state when processing the cnt btree.

Besides that we print bno vs cnt in the messages and obviously validate a
slightly different magic number in the header.


Signed-off-by: Christoph Hellwig <hch@xxxxxx>


Generally seems fine to me, a couple of nitpicks below, take 'em or leave 'em.

Index: xfsprogs-dev/repair/scan.c
===================================================================
--- xfsprogs-dev.orig/repair/scan.c     2009-08-21 18:24:26.000000000 +0000
+++ xfsprogs-dev/repair/scan.c  2009-08-21 18:40:59.000000000 +0000
@@ -439,15 +439,16 @@ _("out-of-order bmap key (file offset) i
 }
void
-scanfunc_bno(
+scanfunc_allocbt(
        struct xfs_btree_block  *block,
        int                     level,
        xfs_agblock_t           bno,
        xfs_agnumber_t          agno,
        int                     suspect,
-       int                     isroot
-       )
+       int                     isroot,
+       __uint32_t              magic)
 {
+       const char              *name;
        xfs_agblock_t           b, e;
        int                     i;
        xfs_alloc_ptr_t         *pp;
@@ -456,16 +457,18 @@ scanfunc_bno(
        int                     numrecs;
        int                     state;
- if (be32_to_cpu(block->bb_magic) != XFS_ABTB_MAGIC) {
-               do_warn(_("bad magic # %#x in btbno block %d/%d\n"),
-                       be32_to_cpu(block->bb_magic), agno, bno);
+       name = (magic == XFS_ABTB_MAGIC) ? "bno" : "cnt";

Should we explicitly test that this is either XFS_ABTC_MAGIC or XFS_ABTB_MAGIC here to avoid any programming-error
type problems?
+
+       if (be32_to_cpu(block->bb_magic) != magic) {
+               do_warn(_("bad magic # %#x in bt%s block %d/%d\n"),
+                       be32_to_cpu(block->bb_magic), name, agno, bno);
                hdr_errors++;
                if (suspect)
                        return;
        }
        if (be16_to_cpu(block->bb_level) != level) {
-               do_warn(_("expected level %d got %d in btbno block %d/%d\n"),
-                       level, be16_to_cpu(block->bb_level), agno, bno);
+               do_warn(_("expected level %d got %d in bt%s block %d/%d\n"),
+                       level, be16_to_cpu(block->bb_level), name, agno, bno);
                hdr_errors++;
                if (suspect)
                        return;
@@ -483,8 +486,8 @@ scanfunc_bno(
        default:
                set_agbno_state(mp, agno, bno, XR_E_MULT);
                do_warn(
-_("bno freespace btree block claimed (state %d), agno %d, bno %d, suspect 
%d\n"),
-                               state, agno, bno, suspect);
+_("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect 
%d\n"),
+                               name, state, agno, bno, suspect);
                return;
        }
@@ -520,15 +523,27 @@ _("bno freespace btree block claimed (st
                                continue;
                        for (b = be32_to_cpu(rp[i].ar_startblock);
                             b < e; b++)  {
-                               if (get_agbno_state(mp, agno, b)
-                                                       == XR_E_UNKNOWN)
+                               state = get_agbno_state(mp, agno, b);
+                               switch (state) {
+                               case XR_E_UNKNOWN:
                                        set_agbno_state(mp, agno, b,
                                                        XR_E_FREE1);
-                               else  {
+                                       break;
+                               case XR_E_FREE1:
+                                       /*
+                                        * no warning messages -- we'll catch
+                                        * FREE1 blocks later
+                                        */
+                                       if (magic != XFS_ABTB_MAGIC) {

Why not make this explicitly "if (magic == XFS_ABTC_MAGIC)" - I guess it seems 
potentially
more future-proof to me though I don't suppose we'll ever get a new type here.  
:)
The positive test seems clearer to me but *shrug*.

Rest looks fine.  I suppose we should do the same to the functions in db/* 
someday.

Thanks,
-Eric

<Prev in Thread] Current Thread [Next in Thread>