On Mon, Nov 16, 2020 at 04:07:23PM +0800, Gao Xiang wrote:
> If rootino is wrong and misspecified to a subdir inode #,
> the following assertion could be triggered:
> assert(ino != persp->p_rootino || hardh == persp->p_rooth);
>
> This patch adds a '-x' option (another awkward thing is that
> the codebase doesn't support long options) to address
> problamatic images by searching for the real dir, the reason
> that I don't enable it by default is that I'm not very confident
> with the xfsrestore codebase and xfsdump bulkstat issue will
> also be fixed immediately as well, so this function might be
> optional and only useful for pre-exist corrupted dumps.
>
> In details, my understanding of the original logic is
> 1) xfsrestore will create a rootdir node_t (p_rooth);
> 2) it will build the tree hierarchy from inomap and adopt
> the parent if needed (so inodes whose parent ino hasn't
> detected will be in the orphan dir, p_orphh);
> 3) during this period, if ino == rootino and
> hardh != persp->p_rooth (IOWs, another node_t with
> the same ino # is created), that'd be definitely wrong.
>
> So the proposal fix is that
> - considering the xfsdump root ino # is a subdir inode, it'll
> trigger ino == rootino && hardh != persp->p_rooth condition;
> - so we log this node_t as persp->p_rooth rather than the
> initial rootdir node_t created in 1);
> - we also know that this node is actually a subdir, and after
> the whole inomap is scanned (IOWs, the tree is built),
> the real root dir will have the orphan dir parent p_orphh;
> - therefore, we walk up by the parent until some node_t has
> the p_orphh, so that's it.
>
> Cc: Donald Douwsma <ddouwsma@xxxxxxxxxx>
> Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxx>
> ---
> changes since RFC v1:
> - fix non-dir fake rootino cases since tree_begindir()
> won't be triggered for these non-dir, and we could do
> that in tree_addent() instead (fault injection verified);
>
> - fix fake rootino case with gen = 0 as well, for more
> details, see the inlined comment in link_hardh()
> (fault injection verified as well).
>
> Anyway, all of this behaves like a workaround and I have
> no idea how to deal it more gracefully.
>
My manual fault injection patch:
- inject a non-dir fake rootino;
- inject all inode gen = 0 (to check the fake rootino case with gen = 0);
- disable the fake rootino case with gen = 0 workaround, and see
xfsrestore: tree.c:1003: tree_addent: Assertion `hardp->n_nrh != NRH_NULL' failed.
could happen.
diff --git a/dump/content.c b/dump/content.c
index c11d9b4..2d27d24 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -1509,6 +1509,7 @@ baseuuidbypass:
}
scwhdrtemplatep->cih_rootino = sc_rootxfsstatp->bs_ino;
+ scwhdrtemplatep->cih_rootino = 25166002; /* inject some real non-dir ino # */
inomap_writehdr(scwhdrtemplatep);
/* log the dump size. just a rough approx.
diff --git a/restore/content.c b/restore/content.c
index e807a83..f493fdb 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -8143,7 +8143,7 @@ read_filehdr(drive_t *drivep, filehdr_t *fhdrp, bool_t fhcs)
return RV_CORRUPT;
}
}
-
+ bstatp->bs_gen = 0;
return RV_OK;
}
@@ -8277,6 +8277,8 @@ read_dirent(drive_t *drivep,
}
}
+ dhdrp->dh_gen = 0;
+
mlog(MLOG_NITTY,
"read dirent hdr ino %llu gen %u size %u\n",
dhdrp->dh_ino,
diff --git a/restore/tree.c b/restore/tree.c
index 918fa01..2d8dec5 100644
--- a/restore/tree.c
+++ b/restore/tree.c
@@ -3886,6 +3886,7 @@ link_hardh(xfs_ino_t ino, gen_t gen)
{
nh_t tmp = hash_find(ino, gen);
+#if 0
/*
* XXX (another workaround): the simply way is that don't reuse node_t
* with gen = 0 created in tree_init(). Otherwise, it could cause
@@ -3903,6 +3904,7 @@ _("link out fake rootino %llu with gen=0 created in tree_init()\n"), ino);
orig_rooth = tmp;
return NH_NULL;
}
+#endif
return tmp;
}
--
2.18.4
