xfs
[Top] [All Lists]

[PATCH] xfs_repair - do not attempt to set shortform attr header when cl

To: Michael Monnerie <michael.monnerie@xxxxxxxxxxxxxxxxxxx>
Subject: [PATCH] xfs_repair - do not attempt to set shortform attr header when clearing
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Mon, 13 Jul 2009 23:13:58 -0500
Cc: xfs mailing list <xfs@xxxxxxxxxxx>
In-reply-to: <200907031320.48358@xxxxxx>
References: <200907031320.48358@xxxxxx>
User-agent: Thunderbird 2.0.0.22 (Macintosh/20090605)
As reported in "bad fs - xfs_repair 3.01 crashes on it" ...

The filesystem encountered a bad attribute fork which is cleared:

local inode 3857051697 attr too small (size = 3, min size = 4)
bad attribute fork in inode 3857051697, clearing attr fork
clearing inode 3857051697 attributes

and then later this inode failed an assertion:

data fork in regular inode 3857051697 claims used block 537147998
xfs_repair: dinode.c:2108: process_inode_data_fork: Assertion `err == 0' failed.

The ASSERT is there because process_inode_data_fork() calls 
process_exinode() twice; once with check_dups == 1, and again with 
check_dups == 0.  The assertion is that they should both return the
the same answer about whether the inode contained duplicate blocks.

However, they are tested in different ways; with check_dups set,
process_exinode() simply does search_dup_extent() when it gets
to process_bmbt_reclist_int(); without check_dups set, it utilizes
the ba_bmap[][] array of bitmaps, compared against the current
extent record.

Long story short(er), when we cleared the bad attribute in
clear_dinode_attr(), it used XFS_DFORK_APTR() to get to the
shortform attribute header, and set some fields.  However,
di_forkoff must have been corrupt as well, because setting these
fields corrupted the extent list, and the 4th extent on the inode
got its physical block modified from:
431241822 / 0x19B43A5E
to:
537147998 / 0x20043A5E

and this new (corrupt) physical block matched another inode's
block, triggering the dup & return 1, triggering the ASSERT.

Whew.

Anyway, simply setting di_forkoff to 0 should be enough to flag
the inode as having no attr fork, and messing with where we
think the shortform attribute header may be is now shown to be
dangerous.  Simply not mucking w/ the header seems to fix
the problem, based on testing with the metadump image.

Almost.

process_inode_attr_fork() calls clear_dinode_attr() which puts
it into the XFS_DINODE_FMT_EXTENTS state, but upon return resets
that to XFS_DINODE_FMT_LOCAL.  Later, it's checked that if 
!XFS_DFORK_Q(), the format is XFS_DINODE_FMT_EXTENTS (!)
and it gets reset.

So drop the setting to XFS_DINODE_FMT_LOCAL; for whatever reason,
"no attributes" seems to expect _EXTENTS format, see for example
xfs_attr_shortform_remove(), clear_dinode_core(), and 
xfs_attr_fork_reset() in the kernel, which all set it to _EXTENTS
in this circumstance.

Fix this up after both calls to clear_dinode_attr().

Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxxx>
---

diff --git a/repair/dinode.c b/repair/dinode.c
index 84e1d05..23de0a8 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -103,23 +103,8 @@ clear_dinode_attr(xfs_mount_t *mp, xfs_dinode_t *dino, 
xfs_ino_t ino_num)
        }
 
        /* get rid of the fork by clearing forkoff */
-
-       /* Originally, when the attr repair code was added, the fork was cleared
-        * by turning it into shortform status.  This meant clearing the
-        * hdr.totsize/count fields and also changing aformat to LOCAL
-        * (vs EXTENTS).  Over various fixes, the aformat and forkoff have
-        * been updated to not show an attribute fork at all, however.
-        * It could be possible that resetting totsize/count are not needed,
-        * but just to be safe, leave it in for now.
-        */
-
-       if (!no_modify) {
-               xfs_attr_shortform_t *asf = (xfs_attr_shortform_t *)
-                               XFS_DFORK_APTR(dino);
-               asf->hdr.totsize = cpu_to_be16(sizeof(xfs_attr_sf_hdr_t));
-               asf->hdr.count = 0;
-               dinoc->di_forkoff = 0;  /* got to do this after asf is set */
-       }
+       if (!no_modify)
+               dinoc->di_forkoff = 0;
 
        /*
         * always returns 1 since the fork gets zapped
@@ -2195,7 +2180,6 @@ process_inode_attr_fork(
                        if (delete_attr_ok)  {
                                do_warn(_(", clearing attr fork\n"));
                                *dirty += clear_dinode_attr(mp, dino, lino);
-                               dinoc->di_aformat = XFS_DINODE_FMT_LOCAL;
                        } else  {
                                do_warn("\n");
                                *dirty += clear_dinode(mp, dino, lino);
@@ -2253,12 +2237,10 @@ process_inode_attr_fork(
                        lino);
                if (!repair) {
                        /* clear attributes if not done already */
-                       if (!no_modify)  {
+                       if (!no_modify)
                                *dirty += clear_dinode_attr(mp, dino, lino);
-                               dinoc->di_aformat = XFS_DINODE_FMT_LOCAL;
-                       } else  {
+                       else
                                do_warn(_("would clear attr fork\n"));
-                       }
                        *atotblocks = 0;
                        *anextents = 0;
                }


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