On Thu, May 07, 2020 at 09:43:55AM -0400, Brian Foster wrote:
> On Thu, May 07, 2020 at 02:34:11PM +0200, Christoph Hellwig wrote:
> > On Fri, May 01, 2020 at 02:23:16PM -0400, Brian Foster wrote:
> > > Can we use another dummy parent inode value in xfs_repair? It looks to
> > > me that we set it to zero in phase 4 if it fails verification and set
> > > the parent to NULLFSINO (i.e. unknown) in repair's in-core tracking.
> > > Phase 6 walks the directory entries and explicitly sets the parent inode
> > > number of entries with an unknown parent (according to the in-core
> > > tracking). IOW, I don't see where we actually rely on the directory
> > > header having a parent inode of zero outside of detecting it in the
> > > custom verifier. If that's the only functional purpose, I wonder if we
> > > could do something like set the bogus parent field of a sf dir to the
> > > root inode or to itself, that way the default verifier wouldn't trip
> > > over it..
> >
> > I don't think we need a dummy parent at all - we can just skip the
> > parent validation entirely, which is what my incremental patch does.
> >
>
> xfs_repair already skips the parent validation, this patch just
> refactors it. What I was considering above is whether repair uses the
> current dummy value of zero for any functional reason. If not, it kind
> of looks like the earlier phase of repair checks the parent, sees that
> it would fail a verifier, replaces it with zero (which would also fail
> the verifier) and then eventually replaces zero with a valid parent or
> ditches the entry in phase 6. If we placed a temporary parent value in
> the early phase that wouldn't explicitly fail a verifier by being an
> invalid inode number (instead of using 0 to notify the verifier to skip
> the validation), then we wouldn't need to skip the parent validation in
> phase 6 when we look up the inode again.
>
...
To demonstrate, I hacked on repair a bit using an fs with an
intentionally corrupted shortform parent inode and had to make the
following tweaks to work around the custom fork verifier. The
ino_discovery checks were added because phases 3 and 4 toggle that flag
such that the former clears the parent value in the inode, but the
latter actually updates the external parent tracking. IOW, setting a
"valid" inode in phase 3 would otherwise trick phase 4 into using it.
I'd probably try to think of something cleaner for that issue if we were
to take such an approach.
Brian
diff --git a/repair/dir2.c b/repair/dir2.c
index cbbce601..c30ccb37 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -165,7 +165,7 @@ process_sf_dir2(
int tmp_elen;
int tmp_len;
xfs_dir2_sf_entry_t *tmp_sfep;
- xfs_ino_t zero = 0;
+ xfs_ino_t zero = mp->m_sb.sb_rootino;
sfp = (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip);
max_size = XFS_DFORK_DSIZE(dip, mp);
@@ -494,7 +494,8 @@ _("bogus .. inode number (%" PRIu64 ") in directory inode %" PRIu64 ", "),
if (!no_modify) {
do_warn(_("clearing inode number\n"));
- libxfs_dir2_sf_put_parent_ino(sfp, zero);
+ if (!ino_discovery)
+ libxfs_dir2_sf_put_parent_ino(sfp, zero);
*dino_dirty = 1;
*repair = 1;
} else {
@@ -528,8 +529,8 @@ _("bad .. entry in directory inode %" PRIu64 ", points to self, "),
ino);
if (!no_modify) {
do_warn(_("clearing inode number\n"));
-
- libxfs_dir2_sf_put_parent_ino(sfp, zero);
+ if (!ino_discovery)
+ libxfs_dir2_sf_put_parent_ino(sfp, zero);
*dino_dirty = 1;
*repair = 1;
} else {
diff --git a/repair/phase6.c b/repair/phase6.c
index beceea9a..613ca578 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1104,7 +1104,7 @@ mv_orphanage(
(unsigned long long)ino, ++incr);
/* Orphans may not have a proper parent, so use custom ops here */
- err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &phase6_ifork_ops);
+ err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &xfs_default_ifork_ops);
if (err)
do_error(_("%d - couldn't iget disconnected inode\n"), err);
@@ -2875,7 +2875,7 @@ process_dir_inode(
ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
- error = -libxfs_iget(mp, NULL, ino, 0, &ip, &phase6_ifork_ops);
+ error = -libxfs_iget(mp, NULL, ino, 0, &ip, &xfs_default_ifork_ops);
if (error) {
if (!no_modify)
do_error(
