[RFC PATCH] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[RFC PATCH] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Gao Xiang]

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
    been 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.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 restore/content.c |  7 +++++++
 restore/getopt.h  |  4 ++--
 restore/tree.c    | 44 +++++++++++++++++++++++++++++++++++++++++++-
 restore/tree.h    |  2 ++
 4 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/restore/content.c b/restore/content.c
index 6b22965..e807a83 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -865,6 +865,7 @@ static int quotafilecheck(char *type, char *dstdir, char *quotafile);
 
 bool_t content_media_change_needed;
 bool_t restore_rootdir_permissions;
+bool_t need_fixrootdir;
 char *media_change_alert_program = NULL;
 size_t perssz;
 
@@ -964,6 +965,7 @@ content_init(int argc, char *argv[], size64_t vmsz)
 	stsz = 0;
 	interpr = BOOL_FALSE;
 	restore_rootdir_permissions = BOOL_FALSE;
+	need_fixrootdir = BOOL_FALSE;
 	optind = 1;
 	opterr = 0;
 	while ((c = getopt(argc, argv, GETOPT_CMDSTRING)) != EOF) {
@@ -1189,6 +1191,9 @@ content_init(int argc, char *argv[], size64_t vmsz)
 		case GETOPT_FMT2COMPAT:
 			tranp->t_truncategenpr = BOOL_TRUE;
 			break;
+		case GETOPT_FIXROOTDIR:
+			need_fixrootdir = BOOL_TRUE;
+			break;
 		}
 	}
 
@@ -3140,6 +3145,8 @@ applydirdump(drive_t *drivep,
 			return rv;
 		}
 
+		if (need_fixrootdir)
+			tree_fixroot();
 		persp->s.dirdonepr = BOOL_TRUE;
 	}
 
diff --git a/restore/getopt.h b/restore/getopt.h
index b5bc004..b0c0c7d 100644
--- a/restore/getopt.h
+++ b/restore/getopt.h
@@ -26,7 +26,7 @@
  * purpose is to contain that command string.
  */
 
-#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
+#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wxABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
 
 #define GETOPT_WORKSPACE	'a'	/* workspace dir (content.c) */
 #define GETOPT_BLOCKSIZE        'b'     /* blocksize for rmt */
@@ -51,7 +51,7 @@
 /*				'u' */
 #define	GETOPT_VERBOSITY	'v'	/* verbosity level (0 to 4) */
 #define	GETOPT_SMALLWINDOW	'w'	/* use a small window for dir entries */
-/*				'x' */
+#define GETOPT_FIXROOTDIR	'x'	/* try to fix rootdir due to bulkstat misuse */
 /*				'y' */
 /*				'z' */
 #define	GETOPT_NOEXTATTR	'A'	/* do not restore ext. file attr. */
diff --git a/restore/tree.c b/restore/tree.c
index 0670318..c1e0461 100644
--- a/restore/tree.c
+++ b/restore/tree.c
@@ -265,6 +265,7 @@ extern void usage(void);
 extern size_t pgsz;
 extern size_t pgmask;
 extern bool_t restore_rootdir_permissions;
+extern bool_t need_fixrootdir;
 
 /* forward declarations of locally defined static functions ******************/
 
@@ -335,6 +336,36 @@ static xfs_ino_t orphino = ORPH_INO;
 
 /* definition of locally defined global functions ****************************/
 
+void
+tree_fixroot(void)
+{
+	nh_t		rooth = persp->p_rooth;
+	xfs_ino_t 	rootino;
+
+	while (1) {
+		nh_t	parh;
+		node_t *rootp = Node_map(rooth);
+
+		rootino = rootp->n_ino;
+		parh = rootp->n_parh;
+		Node_unmap(rooth, &rootp);
+
+		if (parh == rooth ||
+		/* since all new node (including non-parent) would be adopted into orphh */
+		    parh == persp->p_orphh ||
+		    parh == NH_NULL)
+			break;
+		rooth = parh;
+	}
+
+	if (rooth != persp->p_rooth) {
+		persp->p_rooth = rooth;
+		persp->p_rootino = rootino;
+
+		mlog(MLOG_NORMAL, "fix root # to %llu (bind mount?)\n", rootino);
+	}
+}
+
 /* ARGSUSED */
 bool_t
 tree_init(char *hkdir,
@@ -753,8 +784,13 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
 
 	/* lookup head of hardlink list
 	 */
+	/* do not use the old fake root node to prevent potential loop */
+	if (need_fixrootdir == BOOL_TRUE && ino == persp->p_rootino && !gen)
+		gen = -1;
+
 	hardh = link_hardh(ino, gen);
-	assert(ino != persp->p_rootino || hardh == persp->p_rooth);
+	if (need_fixrootdir == BOOL_FALSE)
+		assert(ino != persp->p_rootino || hardh == persp->p_rooth);
 
 	/* already present
 	 */
@@ -824,6 +860,12 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
 		*dahp = dah;
 	}
 
+	/* found the fake rootino from subdir, need fix p_rooth. */
+	if (need_fixrootdir == BOOL_TRUE &&
+	    persp->p_rootino == ino && hardh != persp->p_rooth) {
+		mlog(MLOG_NORMAL, "found fake rootino #%llu, will fix.\n", ino);
+		persp->p_rooth = hardh;
+	}
 	return hardh;
 }
 
diff --git a/restore/tree.h b/restore/tree.h
index 4f9ffe8..5d0c346 100644
--- a/restore/tree.h
+++ b/restore/tree.h
@@ -18,6 +18,8 @@
 #ifndef TREE_H
 #define TREE_H
 
+void tree_fixroot(void);
+
 /* tree_init - creates a new tree abstraction.
  */
 extern bool_t tree_init(char *hkdir,
-- 
2.18.4

Re: [RFC PATCH] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Eric Sandeen]

On 11/13/20 6:51 AM, 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
>     been 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.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Thank you for looking into this - I think you now understand xfsdump &
xfsrestore better than anyone else on the planet.  ;)

One question - what happens if the wrong "root inode" is not a directory?
I think that it is possible from the old "get the first active inode" heuristic
to find any type of file and save it as the root inode.

I think that your approach still works in this case, but wanted to double check
and see what you think.

Thanks,
-Eric

Re: [RFC PATCH] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Gao Xiang]

On Fri, Nov 13, 2020 at 08:10:39AM -0600, Eric Sandeen wrote:
> On 11/13/20 6:51 AM, Gao Xiang wrote:

...

> 
> Thank you for looking into this - I think you now understand xfsdump &
> xfsrestore better than anyone else on the planet.  ;)
> 
> One question - what happens if the wrong "root inode" is not a directory?
> I think that it is possible from the old "get the first active inode" heuristic
> to find any type of file and save it as the root inode.
> 
> I think that your approach still works in this case, but wanted to double check
> and see what you think.

Yeah, good question. I also think it works too, but just in case let me
do fault injection on a regular inode later (Donald's image is /var
subdir...)

Thanks,
Gao Xiang

> 
> Thanks,
> -Eric
> 

Re: [RFC PATCH] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Gao Xiang]

Hi Eric,

On Fri, Nov 13, 2020 at 10:15:53PM +0800, Gao Xiang wrote:
> On Fri, Nov 13, 2020 at 08:10:39AM -0600, Eric Sandeen wrote:
> > On 11/13/20 6:51 AM, Gao Xiang wrote:
> 
> ...
> 
> > 
> > Thank you for looking into this - I think you now understand xfsdump &
> > xfsrestore better than anyone else on the planet.  ;)
> > 
> > One question - what happens if the wrong "root inode" is not a directory?
> > I think that it is possible from the old "get the first active inode" heuristic
> > to find any type of file and save it as the root inode.
> > 
> > I think that your approach still works in this case, but wanted to double check
> > and see what you think.
> 
> Yeah, good question. I also think it works too, but just in case let me
> do fault injection on a regular inode later (Donald's image is /var
> subdir...)
> 

Sorry for the previous wrong conclusion...

From the code itself, tree_begindir() only triggers for node_t == dir but all
dirents can be trigged by tree_addent(), so I update the patch and verified
with manual fault injection code as well...

RFC v2: https://lore.kernel.org/linux-xfs/20201116080723.1486270-1-hsiangkao@redhat.com/
fault injection: https://lore.kernel.org/linux-xfs/20201116081359.GA1486562@xiangao.remote.csb/

Thanks,
Gao Xiang

> Thanks,
> Gao Xiang
> 
> > 
> > Thanks,
> > -Eric
> > 

[RFC PATCH v2] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Gao Xiang]

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@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
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.

 restore/content.c |  7 +++++
 restore/getopt.h  |  4 +--
 restore/tree.c    | 70 ++++++++++++++++++++++++++++++++++++++++++++---
 restore/tree.h    |  2 ++
 4 files changed, 77 insertions(+), 6 deletions(-)

diff --git a/restore/content.c b/restore/content.c
index 6b22965..e807a83 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -865,6 +865,7 @@ static int quotafilecheck(char *type, char *dstdir, char *quotafile);
 
 bool_t content_media_change_needed;
 bool_t restore_rootdir_permissions;
+bool_t need_fixrootdir;
 char *media_change_alert_program = NULL;
 size_t perssz;
 
@@ -964,6 +965,7 @@ content_init(int argc, char *argv[], size64_t vmsz)
 	stsz = 0;
 	interpr = BOOL_FALSE;
 	restore_rootdir_permissions = BOOL_FALSE;
+	need_fixrootdir = BOOL_FALSE;
 	optind = 1;
 	opterr = 0;
 	while ((c = getopt(argc, argv, GETOPT_CMDSTRING)) != EOF) {
@@ -1189,6 +1191,9 @@ content_init(int argc, char *argv[], size64_t vmsz)
 		case GETOPT_FMT2COMPAT:
 			tranp->t_truncategenpr = BOOL_TRUE;
 			break;
+		case GETOPT_FIXROOTDIR:
+			need_fixrootdir = BOOL_TRUE;
+			break;
 		}
 	}
 
@@ -3140,6 +3145,8 @@ applydirdump(drive_t *drivep,
 			return rv;
 		}
 
+		if (need_fixrootdir)
+			tree_fixroot();
 		persp->s.dirdonepr = BOOL_TRUE;
 	}
 
diff --git a/restore/getopt.h b/restore/getopt.h
index b5bc004..b0c0c7d 100644
--- a/restore/getopt.h
+++ b/restore/getopt.h
@@ -26,7 +26,7 @@
  * purpose is to contain that command string.
  */
 
-#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
+#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wxABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
 
 #define GETOPT_WORKSPACE	'a'	/* workspace dir (content.c) */
 #define GETOPT_BLOCKSIZE        'b'     /* blocksize for rmt */
@@ -51,7 +51,7 @@
 /*				'u' */
 #define	GETOPT_VERBOSITY	'v'	/* verbosity level (0 to 4) */
 #define	GETOPT_SMALLWINDOW	'w'	/* use a small window for dir entries */
-/*				'x' */
+#define GETOPT_FIXROOTDIR	'x'	/* try to fix rootdir due to bulkstat misuse */
 /*				'y' */
 /*				'z' */
 #define	GETOPT_NOEXTATTR	'A'	/* do not restore ext. file attr. */
diff --git a/restore/tree.c b/restore/tree.c
index 0670318..918fa01 100644
--- a/restore/tree.c
+++ b/restore/tree.c
@@ -15,7 +15,6 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
-
 #include <stdio.h>
 #include <unistd.h>
 #include <stdlib.h>
@@ -265,6 +264,7 @@ extern void usage(void);
 extern size_t pgsz;
 extern size_t pgmask;
 extern bool_t restore_rootdir_permissions;
+extern bool_t need_fixrootdir;
 
 /* forward declarations of locally defined static functions ******************/
 
@@ -331,10 +331,45 @@ static tran_t *tranp = 0;
 static char *persname = PERS_NAME;
 static char *orphname = ORPH_NAME;
 static xfs_ino_t orphino = ORPH_INO;
+static nh_t orig_rooth = NH_NULL;
 
 
 /* definition of locally defined global functions ****************************/
 
+void
+tree_fixroot(void)
+{
+	nh_t		rooth = persp->p_rooth;
+	xfs_ino_t 	rootino;
+
+	while (1) {
+		nh_t	parh;
+		node_t *rootp = Node_map(rooth);
+
+		rootino = rootp->n_ino;
+		parh = rootp->n_parh;
+		Node_unmap(rooth, &rootp);
+
+		if (parh == rooth ||
+		/*
+		 * since all new node (including non-parent)
+		 * would be adopted into orphh
+		 */
+		    parh == persp->p_orphh ||
+		    parh == NH_NULL)
+			break;
+		rooth = parh;
+	}
+
+	if (rooth != persp->p_rooth) {
+		persp->p_rooth = rooth;
+		persp->p_rootino = rootino;
+
+		mlog(MLOG_NORMAL, _("fix root # to %llu (bind mount?)\n"),
+		     rootino);
+	}
+}
+
 /* ARGSUSED */
 bool_t
 tree_init(char *hkdir,
@@ -754,7 +789,8 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
 	/* lookup head of hardlink list
 	 */
 	hardh = link_hardh(ino, gen);
-	assert(ino != persp->p_rootino || hardh == persp->p_rooth);
+	if (need_fixrootdir == BOOL_FALSE)
+		assert(ino != persp->p_rootino || hardh == persp->p_rooth);
 
 	/* already present
 	 */
@@ -823,7 +859,6 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
 		adopt(persp->p_orphh, hardh, NRH_NULL);
 		*dahp = dah;
 	}
-
 	return hardh;
 }
 
@@ -968,6 +1003,7 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
 				}
 			} else {
 				assert(hardp->n_nrh != NRH_NULL);
+
 				namebuflen
 				=
 				namreg_get(hardp->n_nrh,
@@ -1118,6 +1154,13 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
 		      ino,
 		      gen);
 	}
+	/* found the fake rootino from subdir, need fix p_rooth. */
+	if (need_fixrootdir == BOOL_TRUE &&
+	    persp->p_rootino == ino && hardh != persp->p_rooth) {
+		mlog(MLOG_NORMAL,
+		     _("found fake rootino #%llu, will fix.\n"), ino);
+		persp->p_rooth = hardh;
+	}
 	return RV_OK;
 }
 
@@ -3841,7 +3884,26 @@ selsubtree_recurse_down(nh_t nh, bool_t sensepr)
 static nh_t
 link_hardh(xfs_ino_t ino, gen_t gen)
 {
-	return hash_find(ino, gen);
+	nh_t tmp = hash_find(ino, gen);
+
+	/*
+	 * XXX (another workaround): the simply way is that don't reuse node_t
+	 * with gen = 0 created in tree_init(). Otherwise, it could cause
+	 * xfsrestore: tree.c:1003: tree_addent: Assertion
+	 * `hardp->n_nrh != NRH_NULL' failed.
+	 * and that node_t is a dir node but the fake rootino could be a non-dir
+	 * plus reusing it could cause potential loop in tree hierarchy.
+	 */
+	if (need_fixrootdir == BOOL_TRUE &&
+	    ino == persp->p_rootino && gen == 0 &&
+	    orig_rooth == NH_NULL) {
+		mlog(MLOG_NORMAL,
+_("link out fake rootino %llu with gen=0 created in tree_init()\n"), ino);
+		link_out(tmp);
+		orig_rooth = tmp;
+		return NH_NULL;
+	}
+	return tmp;
 }
 
 /* returns following node in hard link list
diff --git a/restore/tree.h b/restore/tree.h
index 4f9ffe8..5d0c346 100644
--- a/restore/tree.h
+++ b/restore/tree.h
@@ -18,6 +18,8 @@
 #ifndef TREE_H
 #define TREE_H
 
+void tree_fixroot(void);
+
 /* tree_init - creates a new tree abstraction.
  */
 extern bool_t tree_init(char *hkdir,
-- 
2.18.4

Re: [RFC PATCH v2] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Gao Xiang]

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@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> 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

Re: [RFC PATCH v2] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Bill O'Donnell]

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@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

I used your manual fault injection patch to test, and this solution
works for me. Thanks!

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
> 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.
> 
>  restore/content.c |  7 +++++
>  restore/getopt.h  |  4 +--
>  restore/tree.c    | 70 ++++++++++++++++++++++++++++++++++++++++++++---
>  restore/tree.h    |  2 ++
>  4 files changed, 77 insertions(+), 6 deletions(-)
> 
> diff --git a/restore/content.c b/restore/content.c
> index 6b22965..e807a83 100644
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -865,6 +865,7 @@ static int quotafilecheck(char *type, char *dstdir, char *quotafile);
>  
>  bool_t content_media_change_needed;
>  bool_t restore_rootdir_permissions;
> +bool_t need_fixrootdir;
>  char *media_change_alert_program = NULL;
>  size_t perssz;
>  
> @@ -964,6 +965,7 @@ content_init(int argc, char *argv[], size64_t vmsz)
>  	stsz = 0;
>  	interpr = BOOL_FALSE;
>  	restore_rootdir_permissions = BOOL_FALSE;
> +	need_fixrootdir = BOOL_FALSE;
>  	optind = 1;
>  	opterr = 0;
>  	while ((c = getopt(argc, argv, GETOPT_CMDSTRING)) != EOF) {
> @@ -1189,6 +1191,9 @@ content_init(int argc, char *argv[], size64_t vmsz)
>  		case GETOPT_FMT2COMPAT:
>  			tranp->t_truncategenpr = BOOL_TRUE;
>  			break;
> +		case GETOPT_FIXROOTDIR:
> +			need_fixrootdir = BOOL_TRUE;
> +			break;
>  		}
>  	}
>  
> @@ -3140,6 +3145,8 @@ applydirdump(drive_t *drivep,
>  			return rv;
>  		}
>  
> +		if (need_fixrootdir)
> +			tree_fixroot();
>  		persp->s.dirdonepr = BOOL_TRUE;
>  	}
>  
> diff --git a/restore/getopt.h b/restore/getopt.h
> index b5bc004..b0c0c7d 100644
> --- a/restore/getopt.h
> +++ b/restore/getopt.h
> @@ -26,7 +26,7 @@
>   * purpose is to contain that command string.
>   */
>  
> -#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
> +#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wxABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
>  
>  #define GETOPT_WORKSPACE	'a'	/* workspace dir (content.c) */
>  #define GETOPT_BLOCKSIZE        'b'     /* blocksize for rmt */
> @@ -51,7 +51,7 @@
>  /*				'u' */
>  #define	GETOPT_VERBOSITY	'v'	/* verbosity level (0 to 4) */
>  #define	GETOPT_SMALLWINDOW	'w'	/* use a small window for dir entries */
> -/*				'x' */
> +#define GETOPT_FIXROOTDIR	'x'	/* try to fix rootdir due to bulkstat misuse */
>  /*				'y' */
>  /*				'z' */
>  #define	GETOPT_NOEXTATTR	'A'	/* do not restore ext. file attr. */
> diff --git a/restore/tree.c b/restore/tree.c
> index 0670318..918fa01 100644
> --- a/restore/tree.c
> +++ b/restore/tree.c
> @@ -15,7 +15,6 @@
>   * along with this program; if not, write the Free Software Foundation,
>   * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>   */
> -
>  #include <stdio.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> @@ -265,6 +264,7 @@ extern void usage(void);
>  extern size_t pgsz;
>  extern size_t pgmask;
>  extern bool_t restore_rootdir_permissions;
> +extern bool_t need_fixrootdir;
>  
>  /* forward declarations of locally defined static functions ******************/
>  
> @@ -331,10 +331,45 @@ static tran_t *tranp = 0;
>  static char *persname = PERS_NAME;
>  static char *orphname = ORPH_NAME;
>  static xfs_ino_t orphino = ORPH_INO;
> +static nh_t orig_rooth = NH_NULL;
>  
>  
>  /* definition of locally defined global functions ****************************/
>  
> +void
> +tree_fixroot(void)
> +{
> +	nh_t		rooth = persp->p_rooth;
> +	xfs_ino_t 	rootino;
> +
> +	while (1) {
> +		nh_t	parh;
> +		node_t *rootp = Node_map(rooth);
> +
> +		rootino = rootp->n_ino;
> +		parh = rootp->n_parh;
> +		Node_unmap(rooth, &rootp);
> +
> +		if (parh == rooth ||
> +		/*
> +		 * since all new node (including non-parent)
> +		 * would be adopted into orphh
> +		 */
> +		    parh == persp->p_orphh ||
> +		    parh == NH_NULL)
> +			break;
> +		rooth = parh;
> +	}
> +
> +	if (rooth != persp->p_rooth) {
> +		persp->p_rooth = rooth;
> +		persp->p_rootino = rootino;
> +
> +		mlog(MLOG_NORMAL, _("fix root # to %llu (bind mount?)\n"),
> +		     rootino);
> +	}
> +}
> +
>  /* ARGSUSED */
>  bool_t
>  tree_init(char *hkdir,
> @@ -754,7 +789,8 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>  	/* lookup head of hardlink list
>  	 */
>  	hardh = link_hardh(ino, gen);
> -	assert(ino != persp->p_rootino || hardh == persp->p_rooth);
> +	if (need_fixrootdir == BOOL_FALSE)
> +		assert(ino != persp->p_rootino || hardh == persp->p_rooth);
>  
>  	/* already present
>  	 */
> @@ -823,7 +859,6 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>  		adopt(persp->p_orphh, hardh, NRH_NULL);
>  		*dahp = dah;
>  	}
> -
>  	return hardh;
>  }
>  
> @@ -968,6 +1003,7 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>  				}
>  			} else {
>  				assert(hardp->n_nrh != NRH_NULL);
> +
>  				namebuflen
>  				=
>  				namreg_get(hardp->n_nrh,
> @@ -1118,6 +1154,13 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>  		      ino,
>  		      gen);
>  	}
> +	/* found the fake rootino from subdir, need fix p_rooth. */
> +	if (need_fixrootdir == BOOL_TRUE &&
> +	    persp->p_rootino == ino && hardh != persp->p_rooth) {
> +		mlog(MLOG_NORMAL,
> +		     _("found fake rootino #%llu, will fix.\n"), ino);
> +		persp->p_rooth = hardh;
> +	}
>  	return RV_OK;
>  }
>  
> @@ -3841,7 +3884,26 @@ selsubtree_recurse_down(nh_t nh, bool_t sensepr)
>  static nh_t
>  link_hardh(xfs_ino_t ino, gen_t gen)
>  {
> -	return hash_find(ino, gen);
> +	nh_t tmp = hash_find(ino, gen);
> +
> +	/*
> +	 * XXX (another workaround): the simply way is that don't reuse node_t
> +	 * with gen = 0 created in tree_init(). Otherwise, it could cause
> +	 * xfsrestore: tree.c:1003: tree_addent: Assertion
> +	 * `hardp->n_nrh != NRH_NULL' failed.
> +	 * and that node_t is a dir node but the fake rootino could be a non-dir
> +	 * plus reusing it could cause potential loop in tree hierarchy.
> +	 */
> +	if (need_fixrootdir == BOOL_TRUE &&
> +	    ino == persp->p_rootino && gen == 0 &&
> +	    orig_rooth == NH_NULL) {
> +		mlog(MLOG_NORMAL,
> +_("link out fake rootino %llu with gen=0 created in tree_init()\n"), ino);
> +		link_out(tmp);
> +		orig_rooth = tmp;
> +		return NH_NULL;
> +	}
> +	return tmp;
>  }
>  
>  /* returns following node in hard link list
> diff --git a/restore/tree.h b/restore/tree.h
> index 4f9ffe8..5d0c346 100644
> --- a/restore/tree.h
> +++ b/restore/tree.h
> @@ -18,6 +18,8 @@
>  #ifndef TREE_H
>  #define TREE_H
>  
> +void tree_fixroot(void);
> +
>  /* tree_init - creates a new tree abstraction.
>   */
>  extern bool_t tree_init(char *hkdir,
> -- 
> 2.18.4
> 

Re: [RFC PATCH v2] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Masayoshi Mizuma]

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@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Hello,
This patch looks good to me. The option should be useful as
the workaround of the xfsdump bulkstat issue.

How about if we add the help of the option?
Something like this:

diff --git a/common/main.c b/common/main.c
index cb2caf7..9f98226 100644
--- a/common/main.c
+++ b/common/main.c
@@ -988,6 +988,7 @@ usage(void)
 	ULO(_("(contents only)"),			GETOPT_TOC);
 	ULO(_("<verbosity {silent, verbose, trace}>"),	GETOPT_VERBOSITY);
 	ULO(_("(use small tree window)"),		GETOPT_SMALLWINDOW);
+	ULO(_("(try to fix rootdir due to xfsdump issue)"),GETOPT_FIXROOTDIR);
 	ULO(_("(don't restore extended file attributes)"), GETOPT_NOEXTATTR);
 	ULO(_("(restore root dir owner/permissions)"),	GETOPT_ROOTPERM);
 	ULO(_("(restore DMAPI event settings)"),	GETOPT_SETDM);
diff --git a/man/man8/xfsrestore.8 b/man/man8/xfsrestore.8
index 60e4309..5e58434 100644
--- a/man/man8/xfsrestore.8
+++ b/man/man8/xfsrestore.8
@@ -240,6 +240,10 @@ but does not create or modify any files or directories.
 It may be desirable to set the verbosity level to \f3silent\f1
 when using this option.
 .TP 5
+.B \-x
+This option may be useful to fix an issue which the files are restored
+to orphanage directory because of xfsdump (v3.1.7 - v3.1.9) problem.
+.TP 5
 \f3\-v\f1 \f2verbosity\f1
 .\" set inter-paragraph distance to 0
 .PD 0
---

Thanks!
Masa

> ---
> 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.
> 
>  restore/content.c |  7 +++++
>  restore/getopt.h  |  4 +--
>  restore/tree.c    | 70 ++++++++++++++++++++++++++++++++++++++++++++---
>  restore/tree.h    |  2 ++
>  4 files changed, 77 insertions(+), 6 deletions(-)
> 
> diff --git a/restore/content.c b/restore/content.c
> index 6b22965..e807a83 100644
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -865,6 +865,7 @@ static int quotafilecheck(char *type, char *dstdir, char *quotafile);
>  
>  bool_t content_media_change_needed;
>  bool_t restore_rootdir_permissions;
> +bool_t need_fixrootdir;
>  char *media_change_alert_program = NULL;
>  size_t perssz;
>  
> @@ -964,6 +965,7 @@ content_init(int argc, char *argv[], size64_t vmsz)
>  	stsz = 0;
>  	interpr = BOOL_FALSE;
>  	restore_rootdir_permissions = BOOL_FALSE;
> +	need_fixrootdir = BOOL_FALSE;
>  	optind = 1;
>  	opterr = 0;
>  	while ((c = getopt(argc, argv, GETOPT_CMDSTRING)) != EOF) {
> @@ -1189,6 +1191,9 @@ content_init(int argc, char *argv[], size64_t vmsz)
>  		case GETOPT_FMT2COMPAT:
>  			tranp->t_truncategenpr = BOOL_TRUE;
>  			break;
> +		case GETOPT_FIXROOTDIR:
> +			need_fixrootdir = BOOL_TRUE;
> +			break;
>  		}
>  	}
>  
> @@ -3140,6 +3145,8 @@ applydirdump(drive_t *drivep,
>  			return rv;
>  		}
>  
> +		if (need_fixrootdir)
> +			tree_fixroot();
>  		persp->s.dirdonepr = BOOL_TRUE;
>  	}
>  
> diff --git a/restore/getopt.h b/restore/getopt.h
> index b5bc004..b0c0c7d 100644
> --- a/restore/getopt.h
> +++ b/restore/getopt.h
> @@ -26,7 +26,7 @@
>   * purpose is to contain that command string.
>   */
>  
> -#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
> +#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wxABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
>  
>  #define GETOPT_WORKSPACE	'a'	/* workspace dir (content.c) */
>  #define GETOPT_BLOCKSIZE        'b'     /* blocksize for rmt */
> @@ -51,7 +51,7 @@
>  /*				'u' */
>  #define	GETOPT_VERBOSITY	'v'	/* verbosity level (0 to 4) */
>  #define	GETOPT_SMALLWINDOW	'w'	/* use a small window for dir entries */
> -/*				'x' */
> +#define GETOPT_FIXROOTDIR	'x'	/* try to fix rootdir due to bulkstat misuse */
>  /*				'y' */
>  /*				'z' */
>  #define	GETOPT_NOEXTATTR	'A'	/* do not restore ext. file attr. */
> diff --git a/restore/tree.c b/restore/tree.c
> index 0670318..918fa01 100644
> --- a/restore/tree.c
> +++ b/restore/tree.c
> @@ -15,7 +15,6 @@
>   * along with this program; if not, write the Free Software Foundation,
>   * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>   */
> -
>  #include <stdio.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> @@ -265,6 +264,7 @@ extern void usage(void);
>  extern size_t pgsz;
>  extern size_t pgmask;
>  extern bool_t restore_rootdir_permissions;
> +extern bool_t need_fixrootdir;
>  
>  /* forward declarations of locally defined static functions ******************/
>  
> @@ -331,10 +331,45 @@ static tran_t *tranp = 0;
>  static char *persname = PERS_NAME;
>  static char *orphname = ORPH_NAME;
>  static xfs_ino_t orphino = ORPH_INO;
> +static nh_t orig_rooth = NH_NULL;
>  
>  
>  /* definition of locally defined global functions ****************************/
>  
> +void
> +tree_fixroot(void)
> +{
> +	nh_t		rooth = persp->p_rooth;
> +	xfs_ino_t 	rootino;
> +
> +	while (1) {
> +		nh_t	parh;
> +		node_t *rootp = Node_map(rooth);
> +
> +		rootino = rootp->n_ino;
> +		parh = rootp->n_parh;
> +		Node_unmap(rooth, &rootp);
> +
> +		if (parh == rooth ||
> +		/*
> +		 * since all new node (including non-parent)
> +		 * would be adopted into orphh
> +		 */
> +		    parh == persp->p_orphh ||
> +		    parh == NH_NULL)
> +			break;
> +		rooth = parh;
> +	}
> +
> +	if (rooth != persp->p_rooth) {
> +		persp->p_rooth = rooth;
> +		persp->p_rootino = rootino;
> +
> +		mlog(MLOG_NORMAL, _("fix root # to %llu (bind mount?)\n"),
> +		     rootino);
> +	}
> +}
> +
>  /* ARGSUSED */
>  bool_t
>  tree_init(char *hkdir,
> @@ -754,7 +789,8 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>  	/* lookup head of hardlink list
>  	 */
>  	hardh = link_hardh(ino, gen);
> -	assert(ino != persp->p_rootino || hardh == persp->p_rooth);
> +	if (need_fixrootdir == BOOL_FALSE)
> +		assert(ino != persp->p_rootino || hardh == persp->p_rooth);
>  
>  	/* already present
>  	 */
> @@ -823,7 +859,6 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>  		adopt(persp->p_orphh, hardh, NRH_NULL);
>  		*dahp = dah;
>  	}
> -
>  	return hardh;
>  }
>  
> @@ -968,6 +1003,7 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>  				}
>  			} else {
>  				assert(hardp->n_nrh != NRH_NULL);
> +
>  				namebuflen
>  				=
>  				namreg_get(hardp->n_nrh,
> @@ -1118,6 +1154,13 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>  		      ino,
>  		      gen);
>  	}
> +	/* found the fake rootino from subdir, need fix p_rooth. */
> +	if (need_fixrootdir == BOOL_TRUE &&
> +	    persp->p_rootino == ino && hardh != persp->p_rooth) {
> +		mlog(MLOG_NORMAL,
> +		     _("found fake rootino #%llu, will fix.\n"), ino);
> +		persp->p_rooth = hardh;
> +	}
>  	return RV_OK;
>  }
>  
> @@ -3841,7 +3884,26 @@ selsubtree_recurse_down(nh_t nh, bool_t sensepr)
>  static nh_t
>  link_hardh(xfs_ino_t ino, gen_t gen)
>  {
> -	return hash_find(ino, gen);
> +	nh_t tmp = hash_find(ino, gen);
> +
> +	/*
> +	 * XXX (another workaround): the simply way is that don't reuse node_t
> +	 * with gen = 0 created in tree_init(). Otherwise, it could cause
> +	 * xfsrestore: tree.c:1003: tree_addent: Assertion
> +	 * `hardp->n_nrh != NRH_NULL' failed.
> +	 * and that node_t is a dir node but the fake rootino could be a non-dir
> +	 * plus reusing it could cause potential loop in tree hierarchy.
> +	 */
> +	if (need_fixrootdir == BOOL_TRUE &&
> +	    ino == persp->p_rootino && gen == 0 &&
> +	    orig_rooth == NH_NULL) {
> +		mlog(MLOG_NORMAL,
> +_("link out fake rootino %llu with gen=0 created in tree_init()\n"), ino);
> +		link_out(tmp);
> +		orig_rooth = tmp;
> +		return NH_NULL;
> +	}
> +	return tmp;
>  }
>  
>  /* returns following node in hard link list
> diff --git a/restore/tree.h b/restore/tree.h
> index 4f9ffe8..5d0c346 100644
> --- a/restore/tree.h
> +++ b/restore/tree.h
> @@ -18,6 +18,8 @@
>  #ifndef TREE_H
>  #define TREE_H
>  
> +void tree_fixroot(void);
> +
>  /* tree_init - creates a new tree abstraction.
>   */
>  extern bool_t tree_init(char *hkdir,
> -- 
> 2.18.4
> 

Re: [RFC PATCH v2] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Hironori Shiina]

On 11/16/20 03:07, 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.

I agree that this function is optional for another reason. This fix
cannot be the default behavior because of a workaround for a case where
a fake root's gen is zero. This workaround prevents a correct dump
without a fake root from being restored.

> 
> 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@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> 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.
> 
>  restore/content.c |  7 +++++
>  restore/getopt.h  |  4 +--
>  restore/tree.c    | 70 ++++++++++++++++++++++++++++++++++++++++++++---
>  restore/tree.h    |  2 ++
>  4 files changed, 77 insertions(+), 6 deletions(-)
> 
> diff --git a/restore/content.c b/restore/content.c
> index 6b22965..e807a83 100644
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -865,6 +865,7 @@ static int quotafilecheck(char *type, char *dstdir, char *quotafile);
>  
>  bool_t content_media_change_needed;
>  bool_t restore_rootdir_permissions;
> +bool_t need_fixrootdir;
>  char *media_change_alert_program = NULL;
>  size_t perssz;
>  
> @@ -964,6 +965,7 @@ content_init(int argc, char *argv[], size64_t vmsz)
>  	stsz = 0;
>  	interpr = BOOL_FALSE;
>  	restore_rootdir_permissions = BOOL_FALSE;
> +	need_fixrootdir = BOOL_FALSE;
>  	optind = 1;
>  	opterr = 0;
>  	while ((c = getopt(argc, argv, GETOPT_CMDSTRING)) != EOF) {
> @@ -1189,6 +1191,9 @@ content_init(int argc, char *argv[], size64_t vmsz)
>  		case GETOPT_FMT2COMPAT:
>  			tranp->t_truncategenpr = BOOL_TRUE;
>  			break;
> +		case GETOPT_FIXROOTDIR:
> +			need_fixrootdir = BOOL_TRUE;
> +			break;
>  		}
>  	}
>  
> @@ -3140,6 +3145,8 @@ applydirdump(drive_t *drivep,
>  			return rv;
>  		}
>  
> +		if (need_fixrootdir)
> +			tree_fixroot();
>  		persp->s.dirdonepr = BOOL_TRUE;
>  	}
>  
> diff --git a/restore/getopt.h b/restore/getopt.h
> index b5bc004..b0c0c7d 100644
> --- a/restore/getopt.h
> +++ b/restore/getopt.h
> @@ -26,7 +26,7 @@
>   * purpose is to contain that command string.
>   */
>  
> -#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
> +#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wxABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
>  
>  #define GETOPT_WORKSPACE	'a'	/* workspace dir (content.c) */
>  #define GETOPT_BLOCKSIZE        'b'     /* blocksize for rmt */
> @@ -51,7 +51,7 @@
>  /*				'u' */
>  #define	GETOPT_VERBOSITY	'v'	/* verbosity level (0 to 4) */
>  #define	GETOPT_SMALLWINDOW	'w'	/* use a small window for dir entries */
> -/*				'x' */
> +#define GETOPT_FIXROOTDIR	'x'	/* try to fix rootdir due to bulkstat misuse */
>  /*				'y' */
>  /*				'z' */
>  #define	GETOPT_NOEXTATTR	'A'	/* do not restore ext. file attr. */
> diff --git a/restore/tree.c b/restore/tree.c
> index 0670318..918fa01 100644
> --- a/restore/tree.c
> +++ b/restore/tree.c
> @@ -15,7 +15,6 @@
>   * along with this program; if not, write the Free Software Foundation,
>   * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>   */
> -
>  #include <stdio.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> @@ -265,6 +264,7 @@ extern void usage(void);
>  extern size_t pgsz;
>  extern size_t pgmask;
>  extern bool_t restore_rootdir_permissions;
> +extern bool_t need_fixrootdir;
>  
>  /* forward declarations of locally defined static functions ******************/
>  
> @@ -331,10 +331,45 @@ static tran_t *tranp = 0;
>  static char *persname = PERS_NAME;
>  static char *orphname = ORPH_NAME;
>  static xfs_ino_t orphino = ORPH_INO;
> +static nh_t orig_rooth = NH_NULL;
>  
>  
>  /* definition of locally defined global functions ****************************/
>  
> +void
> +tree_fixroot(void)
> +{
> +	nh_t		rooth = persp->p_rooth;
> +	xfs_ino_t 	rootino;
> +
> +	while (1) {
> +		nh_t	parh;
> +		node_t *rootp = Node_map(rooth);
> +
> +		rootino = rootp->n_ino;
> +		parh = rootp->n_parh;
> +		Node_unmap(rooth, &rootp);
> +
> +		if (parh == rooth ||
> +		/*
> +		 * since all new node (including non-parent)
> +		 * would be adopted into orphh
> +		 */
> +		    parh == persp->p_orphh ||
> +		    parh == NH_NULL)
> +			break;
> +		rooth = parh;
> +	}
> +
> +	if (rooth != persp->p_rooth) {
> +		persp->p_rooth = rooth;
> +		persp->p_rootino = rootino;
> +

As far as I see intialization for a root in tree_init(), isn't it
necessary to adopt the orphanage node(persp->p_orphh) to the new root?

> +		mlog(MLOG_NORMAL, _("fix root # to %llu (bind mount?)\n"),
> +		     rootino);
> +	}
> +}
> +
>  /* ARGSUSED */
>  bool_t
>  tree_init(char *hkdir,
> @@ -754,7 +789,8 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>  	/* lookup head of hardlink list
>  	 */
>  	hardh = link_hardh(ino, gen);
> -	assert(ino != persp->p_rootino || hardh == persp->p_rooth);
> +	if (need_fixrootdir == BOOL_FALSE)
> +		assert(ino != persp->p_rootino || hardh == persp->p_rooth);
>  
>  	/* already present
>  	 */
> @@ -823,7 +859,6 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>  		adopt(persp->p_orphh, hardh, NRH_NULL);
>  		*dahp = dah;
>  	}
> -
>  	return hardh;
>  }
>  
> @@ -968,6 +1003,7 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>  				}
>  			} else {
>  				assert(hardp->n_nrh != NRH_NULL);
> +
>  				namebuflen
>  				=
>  				namreg_get(hardp->n_nrh,
> @@ -1118,6 +1154,13 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>  		      ino,
>  		      gen);
>  	}
> +	/* found the fake rootino from subdir, need fix p_rooth. */
> +	if (need_fixrootdir == BOOL_TRUE &&
> +	    persp->p_rootino == ino && hardh != persp->p_rooth) {
> +		mlog(MLOG_NORMAL,
> +		     _("found fake rootino #%llu, will fix.\n"), ino);
> +		persp->p_rooth = hardh;
> +	}
>  	return RV_OK;
>  }
>  
> @@ -3841,7 +3884,26 @@ selsubtree_recurse_down(nh_t nh, bool_t sensepr)
>  static nh_t
>  link_hardh(xfs_ino_t ino, gen_t gen)
>  {
> -	return hash_find(ino, gen);
> +	nh_t tmp = hash_find(ino, gen);
> +
> +	/*
> +	 * XXX (another workaround): the simply way is that don't reuse node_t
> +	 * with gen = 0 created in tree_init(). Otherwise, it could cause
> +	 * xfsrestore: tree.c:1003: tree_addent: Assertion
> +	 * `hardp->n_nrh != NRH_NULL' failed.
> +	 * and that node_t is a dir node but the fake rootino could be a non-dir
> +	 * plus reusing it could cause potential loop in tree hierarchy.
> +	 */
> +	if (need_fixrootdir == BOOL_TRUE &&
> +	    ino == persp->p_rootino && gen == 0 &&
> +	    orig_rooth == NH_NULL) {

As I mentioned above, this workaround makes this correction optional.
This workaround assumes the initially created root is fake. If a dump is
created correctly without a fake root, this function returns NH_NULL for
the correct root.


Thanks,
Hironori

> +		mlog(MLOG_NORMAL,
> +_("link out fake rootino %llu with gen=0 created in tree_init()\n"), ino);
> +		link_out(tmp);
> +		orig_rooth = tmp;
> +		return NH_NULL;
> +	}
> +	return tmp;
>  }
>  
>  /* returns following node in hard link list
> diff --git a/restore/tree.h b/restore/tree.h
> index 4f9ffe8..5d0c346 100644
> --- a/restore/tree.h
> +++ b/restore/tree.h
> @@ -18,6 +18,8 @@
>  #ifndef TREE_H
>  #define TREE_H
>  
> +void tree_fixroot(void);
> +
>  /* tree_init - creates a new tree abstraction.
>   */
>  extern bool_t tree_init(char *hkdir,

Re: [RFC PATCH v2] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Hironori Shiina]

On 6/10/22 19:05, Hironori Shiina wrote:
> 
> 
> On 11/16/20 03:07, 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.
> 
> I agree that this function is optional for another reason. This fix
> cannot be the default behavior because of a workaround for a case where
> a fake root's gen is zero. This workaround prevents a correct dump
> without a fake root from being restored.
> 
>>
>> 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@redhat.com>
>> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
>> ---
>> 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.
>>
>>  restore/content.c |  7 +++++
>>  restore/getopt.h  |  4 +--
>>  restore/tree.c    | 70 ++++++++++++++++++++++++++++++++++++++++++++---
>>  restore/tree.h    |  2 ++
>>  4 files changed, 77 insertions(+), 6 deletions(-)
>>
>> diff --git a/restore/content.c b/restore/content.c
>> index 6b22965..e807a83 100644
>> --- a/restore/content.c
>> +++ b/restore/content.c
>> @@ -865,6 +865,7 @@ static int quotafilecheck(char *type, char *dstdir, char *quotafile);
>>  
>>  bool_t content_media_change_needed;
>>  bool_t restore_rootdir_permissions;
>> +bool_t need_fixrootdir;
>>  char *media_change_alert_program = NULL;
>>  size_t perssz;
>>  
>> @@ -964,6 +965,7 @@ content_init(int argc, char *argv[], size64_t vmsz)
>>  	stsz = 0;
>>  	interpr = BOOL_FALSE;
>>  	restore_rootdir_permissions = BOOL_FALSE;
>> +	need_fixrootdir = BOOL_FALSE;
>>  	optind = 1;
>>  	opterr = 0;
>>  	while ((c = getopt(argc, argv, GETOPT_CMDSTRING)) != EOF) {
>> @@ -1189,6 +1191,9 @@ content_init(int argc, char *argv[], size64_t vmsz)
>>  		case GETOPT_FMT2COMPAT:
>>  			tranp->t_truncategenpr = BOOL_TRUE;
>>  			break;
>> +		case GETOPT_FIXROOTDIR:
>> +			need_fixrootdir = BOOL_TRUE;
>> +			break;
>>  		}
>>  	}
>>  
>> @@ -3140,6 +3145,8 @@ applydirdump(drive_t *drivep,
>>  			return rv;
>>  		}
>>  
>> +		if (need_fixrootdir)
>> +			tree_fixroot();
>>  		persp->s.dirdonepr = BOOL_TRUE;
>>  	}
>>  
>> diff --git a/restore/getopt.h b/restore/getopt.h
>> index b5bc004..b0c0c7d 100644
>> --- a/restore/getopt.h
>> +++ b/restore/getopt.h
>> @@ -26,7 +26,7 @@
>>   * purpose is to contain that command string.
>>   */
>>  
>> -#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
>> +#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wxABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
>>  
>>  #define GETOPT_WORKSPACE	'a'	/* workspace dir (content.c) */
>>  #define GETOPT_BLOCKSIZE        'b'     /* blocksize for rmt */
>> @@ -51,7 +51,7 @@
>>  /*				'u' */
>>  #define	GETOPT_VERBOSITY	'v'	/* verbosity level (0 to 4) */
>>  #define	GETOPT_SMALLWINDOW	'w'	/* use a small window for dir entries */
>> -/*				'x' */
>> +#define GETOPT_FIXROOTDIR	'x'	/* try to fix rootdir due to bulkstat misuse */
>>  /*				'y' */
>>  /*				'z' */
>>  #define	GETOPT_NOEXTATTR	'A'	/* do not restore ext. file attr. */
>> diff --git a/restore/tree.c b/restore/tree.c
>> index 0670318..918fa01 100644
>> --- a/restore/tree.c
>> +++ b/restore/tree.c
>> @@ -15,7 +15,6 @@
>>   * along with this program; if not, write the Free Software Foundation,
>>   * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>>   */
>> -
>>  #include <stdio.h>
>>  #include <unistd.h>
>>  #include <stdlib.h>
>> @@ -265,6 +264,7 @@ extern void usage(void);
>>  extern size_t pgsz;
>>  extern size_t pgmask;
>>  extern bool_t restore_rootdir_permissions;
>> +extern bool_t need_fixrootdir;
>>  
>>  /* forward declarations of locally defined static functions ******************/
>>  
>> @@ -331,10 +331,45 @@ static tran_t *tranp = 0;
>>  static char *persname = PERS_NAME;
>>  static char *orphname = ORPH_NAME;
>>  static xfs_ino_t orphino = ORPH_INO;
>> +static nh_t orig_rooth = NH_NULL;
>>  
>>  
>>  /* definition of locally defined global functions ****************************/
>>  
>> +void
>> +tree_fixroot(void)
>> +{
>> +	nh_t		rooth = persp->p_rooth;
>> +	xfs_ino_t 	rootino;
>> +
>> +	while (1) {
>> +		nh_t	parh;
>> +		node_t *rootp = Node_map(rooth);
>> +
>> +		rootino = rootp->n_ino;
>> +		parh = rootp->n_parh;
>> +		Node_unmap(rooth, &rootp);
>> +
>> +		if (parh == rooth ||
>> +		/*
>> +		 * since all new node (including non-parent)
>> +		 * would be adopted into orphh
>> +		 */
>> +		    parh == persp->p_orphh ||
>> +		    parh == NH_NULL)
>> +			break;
>> +		rooth = parh;
>> +	}
>> +
>> +	if (rooth != persp->p_rooth) {
>> +		persp->p_rooth = rooth;
>> +		persp->p_rootino = rootino;
>> +
> 
> As far as I see intialization for a root in tree_init(), isn't it
> necessary to adopt the orphanage node(persp->p_orphh) to the new root?
> 

These two steps are necessary here:
+               disown(rooth);
+               adopt(persp->p_rooth, persp->p_orphh, NH_NULL);

Otherwise, we hit an assertion error when restroing a renamed file in
the cumulative mode. We need to re-adopt the orphanage dir to the fixed
root for creating a correct path of a node put to orphanage here:

https://git.kernel.org/pub/scm/fs/xfs/xfsdump-dev.git/tree/restore/tree.c#n1498

>> +		mlog(MLOG_NORMAL, _("fix root # to %llu (bind mount?)\n"),
>> +		     rootino);
>> +	}
>> +}
>> +
>>  /* ARGSUSED */
>>  bool_t
>>  tree_init(char *hkdir,
>> @@ -754,7 +789,8 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>>  	/* lookup head of hardlink list
>>  	 */
>>  	hardh = link_hardh(ino, gen);
>> -	assert(ino != persp->p_rootino || hardh == persp->p_rooth);
>> +	if (need_fixrootdir == BOOL_FALSE)
>> +		assert(ino != persp->p_rootino || hardh == persp->p_rooth);
>>  
>>  	/* already present
>>  	 */
>> @@ -823,7 +859,6 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>>  		adopt(persp->p_orphh, hardh, NRH_NULL);
>>  		*dahp = dah;
>>  	}
>> -
>>  	return hardh;
>>  }
>>  
>> @@ -968,6 +1003,7 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>>  				}
>>  			} else {
>>  				assert(hardp->n_nrh != NRH_NULL);
>> +
>>  				namebuflen
>>  				=
>>  				namreg_get(hardp->n_nrh,
>> @@ -1118,6 +1154,13 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>>  		      ino,
>>  		      gen);
>>  	}
>> +	/* found the fake rootino from subdir, need fix p_rooth. */
>> +	if (need_fixrootdir == BOOL_TRUE &&
>> +	    persp->p_rootino == ino && hardh != persp->p_rooth) {
>> +		mlog(MLOG_NORMAL,
>> +		     _("found fake rootino #%llu, will fix.\n"), ino);
>> +		persp->p_rooth = hardh;
>> +	}
>>  	return RV_OK;
>>  }
>>  
>> @@ -3841,7 +3884,26 @@ selsubtree_recurse_down(nh_t nh, bool_t sensepr)
>>  static nh_t
>>  link_hardh(xfs_ino_t ino, gen_t gen)
>>  {
>> -	return hash_find(ino, gen);
>> +	nh_t tmp = hash_find(ino, gen);
>> +
>> +	/*
>> +	 * XXX (another workaround): the simply way is that don't reuse node_t
>> +	 * with gen = 0 created in tree_init(). Otherwise, it could cause
>> +	 * xfsrestore: tree.c:1003: tree_addent: Assertion
>> +	 * `hardp->n_nrh != NRH_NULL' failed.
>> +	 * and that node_t is a dir node but the fake rootino could be a non-dir
>> +	 * plus reusing it could cause potential loop in tree hierarchy.
>> +	 */
>> +	if (need_fixrootdir == BOOL_TRUE &&
>> +	    ino == persp->p_rootino && gen == 0 &&
>> +	    orig_rooth == NH_NULL) {
> 
> As I mentioned above, this workaround makes this correction optional.
> This workaround assumes the initially created root is fake. If a dump is
> created correctly without a fake root, this function returns NH_NULL for
> the correct root.
> 
> 

Due to this part, '-x' flag does not work for the correct dump. We need
to provide procudure for a user who may have a wrong dump with man or
the help message like this:
---------
A user who has a dump created by xfsdump with this bug should see a
table of contents of the dump file before restoring:
  xfsrestore -t -f <dumpfile>
If a similar message to the following one is displayed, '-x' is required
to restore the dump:
  NOTE: ino 128 salvaging file, placing in orphanage/1024.0/FILE_056
Otherwise, '-x' is not required.
---------

I tried the cumulative mode locally with this fix by combining xfs/064,
xfs/065 and xfs/545 in xfstests. Then, the issue regarding a renamed
file, which is mentioned above, was found. Based on the results of the
tests, the following information also should be provied to users:
---------
In the cumulative mode, a user needs to check with '-t' and use '-x'
flag only for the level 0 dump. Once the root is fixed in restoring the
level 0 dump, '-x' flag is no longer required for level 1+ dumps.
---------

Thanks,
Hironori

> Thanks,
> Hironori
> 
>> +		mlog(MLOG_NORMAL,
>> +_("link out fake rootino %llu with gen=0 created in tree_init()\n"), ino);
>> +		link_out(tmp);
>> +		orig_rooth = tmp;
>> +		return NH_NULL;
>> +	}
>> +	return tmp;
>>  }
>>  
>>  /* returns following node in hard link list
>> diff --git a/restore/tree.h b/restore/tree.h
>> index 4f9ffe8..5d0c346 100644
>> --- a/restore/tree.h
>> +++ b/restore/tree.h
>> @@ -18,6 +18,8 @@
>>  #ifndef TREE_H
>>  #define TREE_H
>>  
>> +void tree_fixroot(void);
>> +
>>  /* tree_init - creates a new tree abstraction.
>>   */
>>  extern bool_t tree_init(char *hkdir,

[RFC PATCH V3] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Hironori Shiina]

From: Gao Xiang <hsiangkao@redhat.com>

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@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
---

changes since RFC v2:
 - re-adopt the orphanage dir to the fixed root for creating a
   correct path of the orphanage dir.

 - add description of the new '-x' option to the man page.

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).

 common/main.c         |  1 +
 man/man8/xfsrestore.8 | 14 +++++++++
 restore/content.c     |  7 +++++
 restore/getopt.h      |  4 +--
 restore/tree.c        | 72 ++++++++++++++++++++++++++++++++++++++++---
 restore/tree.h        |  2 ++
 6 files changed, 94 insertions(+), 6 deletions(-)

diff --git a/common/main.c b/common/main.c
index 1db07d4..6141ffb 100644
--- a/common/main.c
+++ b/common/main.c
@@ -988,6 +988,7 @@ usage(void)
 	ULO(_("(contents only)"),			GETOPT_TOC);
 	ULO(_("<verbosity {silent, verbose, trace}>"),	GETOPT_VERBOSITY);
 	ULO(_("(use small tree window)"),		GETOPT_SMALLWINDOW);
+	ULO(_("(try to fix rootdir due to xfsdump issue)"),GETOPT_FIXROOTDIR);
 	ULO(_("(don't restore extended file attributes)"), GETOPT_NOEXTATTR);
 	ULO(_("(restore root dir owner/permissions)"),	GETOPT_ROOTPERM);
 	ULO(_("(restore DMAPI event settings)"),	GETOPT_SETDM);
diff --git a/man/man8/xfsrestore.8 b/man/man8/xfsrestore.8
index 60e4309..df7dde0 100644
--- a/man/man8/xfsrestore.8
+++ b/man/man8/xfsrestore.8
@@ -240,6 +240,20 @@ but does not create or modify any files or directories.
 It may be desirable to set the verbosity level to \f3silent\f1
 when using this option.
 .TP 5
+.B \-x
+This option may be useful to fix an issue which the files are restored
+to orphanage directory because of xfsdump (v3.1.7 - v3.1.9) problem.
+A normal dump cannot be restored with this option. This option works
+only for a corrupted dump.
+If a dump is created by problematic xfsdump (v3.1.7 - v3.1.9), you
+should see the contents of the dump with \f3\-t\f1 option before
+restoring. Then, if a file is placed to the orphanage directory, you need to
+use this \f3\-x\f1 option to restore the dump. Otherwise, you can restore
+the dump without this option.
+
+In the cumulative mode, this option is required only for a base (level 0)
+dump. You no longer need this option for level 1+ dumps.
+.TP 5
 \f3\-v\f1 \f2verbosity\f1
 .\" set inter-paragraph distance to 0
 .PD 0
diff --git a/restore/content.c b/restore/content.c
index b19bb90..fdf26dd 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -861,6 +861,7 @@ static int quotafilecheck(char *type, char *dstdir, char *quotafile);
 
 bool_t content_media_change_needed;
 bool_t restore_rootdir_permissions;
+bool_t need_fixrootdir;
 char *media_change_alert_program = NULL;
 size_t perssz;
 
@@ -958,6 +959,7 @@ content_init(int argc, char *argv[], size64_t vmsz)
 	stsz = 0;
 	interpr = BOOL_FALSE;
 	restore_rootdir_permissions = BOOL_FALSE;
+	need_fixrootdir = BOOL_FALSE;
 	optind = 1;
 	opterr = 0;
 	while ((c = getopt(argc, argv, GETOPT_CMDSTRING)) != EOF) {
@@ -1186,6 +1188,9 @@ content_init(int argc, char *argv[], size64_t vmsz)
 		case GETOPT_FMT2COMPAT:
 			tranp->t_truncategenpr = BOOL_TRUE;
 			break;
+		case GETOPT_FIXROOTDIR:
+			need_fixrootdir = BOOL_TRUE;
+			break;
 		}
 	}
 
@@ -3129,6 +3134,8 @@ applydirdump(drive_t *drivep,
 			return rv;
 		}
 
+		if (need_fixrootdir)
+			tree_fixroot();
 		persp->s.dirdonepr = BOOL_TRUE;
 	}
 
diff --git a/restore/getopt.h b/restore/getopt.h
index b5bc004..b0c0c7d 100644
--- a/restore/getopt.h
+++ b/restore/getopt.h
@@ -26,7 +26,7 @@
  * purpose is to contain that command string.
  */
 
-#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
+#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wxABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
 
 #define GETOPT_WORKSPACE	'a'	/* workspace dir (content.c) */
 #define GETOPT_BLOCKSIZE        'b'     /* blocksize for rmt */
@@ -51,7 +51,7 @@
 /*				'u' */
 #define	GETOPT_VERBOSITY	'v'	/* verbosity level (0 to 4) */
 #define	GETOPT_SMALLWINDOW	'w'	/* use a small window for dir entries */
-/*				'x' */
+#define GETOPT_FIXROOTDIR	'x'	/* try to fix rootdir due to bulkstat misuse */
 /*				'y' */
 /*				'z' */
 #define	GETOPT_NOEXTATTR	'A'	/* do not restore ext. file attr. */
diff --git a/restore/tree.c b/restore/tree.c
index 5429b74..bfa07fe 100644
--- a/restore/tree.c
+++ b/restore/tree.c
@@ -15,7 +15,6 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
-
 #include <stdio.h>
 #include <unistd.h>
 #include <stdlib.h>
@@ -262,6 +261,7 @@ extern void usage(void);
 extern size_t pgsz;
 extern size_t pgmask;
 extern bool_t restore_rootdir_permissions;
+extern bool_t need_fixrootdir;
 
 /* forward declarations of locally defined static functions ******************/
 
@@ -328,10 +328,47 @@ static tran_t *tranp = 0;
 static char *persname = PERS_NAME;
 static char *orphname = ORPH_NAME;
 static xfs_ino_t orphino = ORPH_INO;
+static nh_t orig_rooth = NH_NULL;
 
 
 /* definition of locally defined global functions ****************************/
 
+void
+tree_fixroot(void)
+{
+	nh_t		rooth = persp->p_rooth;
+	xfs_ino_t 	rootino;
+
+	while (1) {
+		nh_t	parh;
+		node_t *rootp = Node_map(rooth);
+
+		rootino = rootp->n_ino;
+		parh = rootp->n_parh;
+		Node_unmap(rooth, &rootp);
+
+		if (parh == rooth ||
+		/*
+		 * since all new node (including non-parent)
+		 * would be adopted into orphh
+		 */
+		    parh == persp->p_orphh ||
+		    parh == NH_NULL)
+			break;
+		rooth = parh;
+	}
+
+	if (rooth != persp->p_rooth) {
+		persp->p_rooth = rooth;
+		persp->p_rootino = rootino;
+		disown(rooth);
+		adopt(persp->p_rooth, persp->p_orphh, NH_NULL);
+
+		mlog(MLOG_NORMAL, _("fix root # to %llu (bind mount?)\n"),
+		     rootino);
+	}
+}
+
 /* ARGSUSED */
 bool_t
 tree_init(char *hkdir,
@@ -746,7 +783,8 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
 	/* lookup head of hardlink list
 	 */
 	hardh = link_hardh(ino, gen);
-	assert(ino != persp->p_rootino || hardh == persp->p_rooth);
+	if (need_fixrootdir == BOOL_FALSE)
+		assert(ino != persp->p_rootino || hardh == persp->p_rooth);
 
 	/* already present
 	 */
@@ -815,7 +853,6 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
 		adopt(persp->p_orphh, hardh, NRH_NULL);
 		*dahp = dah;
 	}
-
 	return hardh;
 }
 
@@ -960,6 +997,7 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
 				}
 			} else {
 				assert(hardp->n_nrh != NRH_NULL);
+
 				namebuflen
 				=
 				namreg_get(hardp->n_nrh,
@@ -1110,6 +1148,13 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
 		      ino,
 		      gen);
 	}
+	/* found the fake rootino from subdir, need fix p_rooth. */
+	if (need_fixrootdir == BOOL_TRUE &&
+	    persp->p_rootino == ino && hardh != persp->p_rooth) {
+		mlog(MLOG_NORMAL,
+		     _("found fake rootino #%llu, will fix.\n"), ino);
+		persp->p_rooth = hardh;
+	}
 	return RV_OK;
 }
 
@@ -3808,7 +3853,26 @@ selsubtree_recurse_down(nh_t nh, bool_t sensepr)
 static nh_t
 link_hardh(xfs_ino_t ino, gen_t gen)
 {
-	return hash_find(ino, gen);
+	nh_t tmp = hash_find(ino, gen);
+
+	/*
+	 * XXX (another workaround): the simply way is that don't reuse node_t
+	 * with gen = 0 created in tree_init(). Otherwise, it could cause
+	 * xfsrestore: tree.c:1003: tree_addent: Assertion
+	 * `hardp->n_nrh != NRH_NULL' failed.
+	 * and that node_t is a dir node but the fake rootino could be a non-dir
+	 * plus reusing it could cause potential loop in tree hierarchy.
+	 */
+	if (need_fixrootdir == BOOL_TRUE &&
+	    ino == persp->p_rootino && gen == 0 &&
+	    orig_rooth == NH_NULL) {
+		mlog(MLOG_NORMAL,
+_("link out fake rootino %llu with gen=0 created in tree_init()\n"), ino);
+		link_out(tmp);
+		orig_rooth = tmp;
+		return NH_NULL;
+	}
+	return tmp;
 }
 
 /* returns following node in hard link list
diff --git a/restore/tree.h b/restore/tree.h
index bf66e3d..f5bd4ff 100644
--- a/restore/tree.h
+++ b/restore/tree.h
@@ -18,6 +18,8 @@
 #ifndef TREE_H
 #define TREE_H
 
+void tree_fixroot(void);
+
 /* tree_init - creates a new tree abstraction.
  */
 extern bool_t tree_init(char *hkdir,
-- 
2.37.3

Re: [RFC PATCH V3] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Darrick J. Wong]

On Wed, Sep 28, 2022 at 03:10:52PM -0400, Hironori Shiina wrote:
> From: Gao Xiang <hsiangkao@redhat.com>
> 
> 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.

As far as fixing xfsdump -- wasn't XFS_BULK_IREQ_SPECIAL_ROOT supposed
to solve that problem by enabling dump to discover it it's really been
passed the fs root directory?

--D

> 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@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
> ---
> 
> changes since RFC v2:
>  - re-adopt the orphanage dir to the fixed root for creating a
>    correct path of the orphanage dir.
> 
>  - add description of the new '-x' option to the man page.
> 
> 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).
> 
>  common/main.c         |  1 +
>  man/man8/xfsrestore.8 | 14 +++++++++
>  restore/content.c     |  7 +++++
>  restore/getopt.h      |  4 +--
>  restore/tree.c        | 72 ++++++++++++++++++++++++++++++++++++++++---
>  restore/tree.h        |  2 ++
>  6 files changed, 94 insertions(+), 6 deletions(-)
> 
> diff --git a/common/main.c b/common/main.c
> index 1db07d4..6141ffb 100644
> --- a/common/main.c
> +++ b/common/main.c
> @@ -988,6 +988,7 @@ usage(void)
>  	ULO(_("(contents only)"),			GETOPT_TOC);
>  	ULO(_("<verbosity {silent, verbose, trace}>"),	GETOPT_VERBOSITY);
>  	ULO(_("(use small tree window)"),		GETOPT_SMALLWINDOW);
> +	ULO(_("(try to fix rootdir due to xfsdump issue)"),GETOPT_FIXROOTDIR);
>  	ULO(_("(don't restore extended file attributes)"), GETOPT_NOEXTATTR);
>  	ULO(_("(restore root dir owner/permissions)"),	GETOPT_ROOTPERM);
>  	ULO(_("(restore DMAPI event settings)"),	GETOPT_SETDM);
> diff --git a/man/man8/xfsrestore.8 b/man/man8/xfsrestore.8
> index 60e4309..df7dde0 100644
> --- a/man/man8/xfsrestore.8
> +++ b/man/man8/xfsrestore.8
> @@ -240,6 +240,20 @@ but does not create or modify any files or directories.
>  It may be desirable to set the verbosity level to \f3silent\f1
>  when using this option.
>  .TP 5
> +.B \-x
> +This option may be useful to fix an issue which the files are restored
> +to orphanage directory because of xfsdump (v3.1.7 - v3.1.9) problem.
> +A normal dump cannot be restored with this option. This option works
> +only for a corrupted dump.
> +If a dump is created by problematic xfsdump (v3.1.7 - v3.1.9), you
> +should see the contents of the dump with \f3\-t\f1 option before
> +restoring. Then, if a file is placed to the orphanage directory, you need to
> +use this \f3\-x\f1 option to restore the dump. Otherwise, you can restore
> +the dump without this option.
> +
> +In the cumulative mode, this option is required only for a base (level 0)
> +dump. You no longer need this option for level 1+ dumps.
> +.TP 5
>  \f3\-v\f1 \f2verbosity\f1
>  .\" set inter-paragraph distance to 0
>  .PD 0
> diff --git a/restore/content.c b/restore/content.c
> index b19bb90..fdf26dd 100644
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -861,6 +861,7 @@ static int quotafilecheck(char *type, char *dstdir, char *quotafile);
>  
>  bool_t content_media_change_needed;
>  bool_t restore_rootdir_permissions;
> +bool_t need_fixrootdir;
>  char *media_change_alert_program = NULL;
>  size_t perssz;
>  
> @@ -958,6 +959,7 @@ content_init(int argc, char *argv[], size64_t vmsz)
>  	stsz = 0;
>  	interpr = BOOL_FALSE;
>  	restore_rootdir_permissions = BOOL_FALSE;
> +	need_fixrootdir = BOOL_FALSE;
>  	optind = 1;
>  	opterr = 0;
>  	while ((c = getopt(argc, argv, GETOPT_CMDSTRING)) != EOF) {
> @@ -1186,6 +1188,9 @@ content_init(int argc, char *argv[], size64_t vmsz)
>  		case GETOPT_FMT2COMPAT:
>  			tranp->t_truncategenpr = BOOL_TRUE;
>  			break;
> +		case GETOPT_FIXROOTDIR:
> +			need_fixrootdir = BOOL_TRUE;
> +			break;
>  		}
>  	}
>  
> @@ -3129,6 +3134,8 @@ applydirdump(drive_t *drivep,
>  			return rv;
>  		}
>  
> +		if (need_fixrootdir)
> +			tree_fixroot();
>  		persp->s.dirdonepr = BOOL_TRUE;
>  	}
>  
> diff --git a/restore/getopt.h b/restore/getopt.h
> index b5bc004..b0c0c7d 100644
> --- a/restore/getopt.h
> +++ b/restore/getopt.h
> @@ -26,7 +26,7 @@
>   * purpose is to contain that command string.
>   */
>  
> -#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
> +#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wxABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
>  
>  #define GETOPT_WORKSPACE	'a'	/* workspace dir (content.c) */
>  #define GETOPT_BLOCKSIZE        'b'     /* blocksize for rmt */
> @@ -51,7 +51,7 @@
>  /*				'u' */
>  #define	GETOPT_VERBOSITY	'v'	/* verbosity level (0 to 4) */
>  #define	GETOPT_SMALLWINDOW	'w'	/* use a small window for dir entries */
> -/*				'x' */
> +#define GETOPT_FIXROOTDIR	'x'	/* try to fix rootdir due to bulkstat misuse */
>  /*				'y' */
>  /*				'z' */
>  #define	GETOPT_NOEXTATTR	'A'	/* do not restore ext. file attr. */
> diff --git a/restore/tree.c b/restore/tree.c
> index 5429b74..bfa07fe 100644
> --- a/restore/tree.c
> +++ b/restore/tree.c
> @@ -15,7 +15,6 @@
>   * along with this program; if not, write the Free Software Foundation,
>   * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>   */
> -
>  #include <stdio.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> @@ -262,6 +261,7 @@ extern void usage(void);
>  extern size_t pgsz;
>  extern size_t pgmask;
>  extern bool_t restore_rootdir_permissions;
> +extern bool_t need_fixrootdir;
>  
>  /* forward declarations of locally defined static functions ******************/
>  
> @@ -328,10 +328,47 @@ static tran_t *tranp = 0;
>  static char *persname = PERS_NAME;
>  static char *orphname = ORPH_NAME;
>  static xfs_ino_t orphino = ORPH_INO;
> +static nh_t orig_rooth = NH_NULL;
>  
>  
>  /* definition of locally defined global functions ****************************/
>  
> +void
> +tree_fixroot(void)
> +{
> +	nh_t		rooth = persp->p_rooth;
> +	xfs_ino_t 	rootino;
> +
> +	while (1) {
> +		nh_t	parh;
> +		node_t *rootp = Node_map(rooth);
> +
> +		rootino = rootp->n_ino;
> +		parh = rootp->n_parh;
> +		Node_unmap(rooth, &rootp);
> +
> +		if (parh == rooth ||
> +		/*
> +		 * since all new node (including non-parent)
> +		 * would be adopted into orphh
> +		 */
> +		    parh == persp->p_orphh ||
> +		    parh == NH_NULL)
> +			break;
> +		rooth = parh;
> +	}
> +
> +	if (rooth != persp->p_rooth) {
> +		persp->p_rooth = rooth;
> +		persp->p_rootino = rootino;
> +		disown(rooth);
> +		adopt(persp->p_rooth, persp->p_orphh, NH_NULL);
> +
> +		mlog(MLOG_NORMAL, _("fix root # to %llu (bind mount?)\n"),
> +		     rootino);
> +	}
> +}
> +
>  /* ARGSUSED */
>  bool_t
>  tree_init(char *hkdir,
> @@ -746,7 +783,8 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>  	/* lookup head of hardlink list
>  	 */
>  	hardh = link_hardh(ino, gen);
> -	assert(ino != persp->p_rootino || hardh == persp->p_rooth);
> +	if (need_fixrootdir == BOOL_FALSE)
> +		assert(ino != persp->p_rootino || hardh == persp->p_rooth);
>  
>  	/* already present
>  	 */
> @@ -815,7 +853,6 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>  		adopt(persp->p_orphh, hardh, NRH_NULL);
>  		*dahp = dah;
>  	}
> -
>  	return hardh;
>  }
>  
> @@ -960,6 +997,7 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>  				}
>  			} else {
>  				assert(hardp->n_nrh != NRH_NULL);
> +
>  				namebuflen
>  				=
>  				namreg_get(hardp->n_nrh,
> @@ -1110,6 +1148,13 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>  		      ino,
>  		      gen);
>  	}
> +	/* found the fake rootino from subdir, need fix p_rooth. */
> +	if (need_fixrootdir == BOOL_TRUE &&
> +	    persp->p_rootino == ino && hardh != persp->p_rooth) {
> +		mlog(MLOG_NORMAL,
> +		     _("found fake rootino #%llu, will fix.\n"), ino);
> +		persp->p_rooth = hardh;
> +	}
>  	return RV_OK;
>  }
>  
> @@ -3808,7 +3853,26 @@ selsubtree_recurse_down(nh_t nh, bool_t sensepr)
>  static nh_t
>  link_hardh(xfs_ino_t ino, gen_t gen)
>  {
> -	return hash_find(ino, gen);
> +	nh_t tmp = hash_find(ino, gen);
> +
> +	/*
> +	 * XXX (another workaround): the simply way is that don't reuse node_t
> +	 * with gen = 0 created in tree_init(). Otherwise, it could cause
> +	 * xfsrestore: tree.c:1003: tree_addent: Assertion
> +	 * `hardp->n_nrh != NRH_NULL' failed.
> +	 * and that node_t is a dir node but the fake rootino could be a non-dir
> +	 * plus reusing it could cause potential loop in tree hierarchy.
> +	 */
> +	if (need_fixrootdir == BOOL_TRUE &&
> +	    ino == persp->p_rootino && gen == 0 &&
> +	    orig_rooth == NH_NULL) {
> +		mlog(MLOG_NORMAL,
> +_("link out fake rootino %llu with gen=0 created in tree_init()\n"), ino);
> +		link_out(tmp);
> +		orig_rooth = tmp;
> +		return NH_NULL;
> +	}
> +	return tmp;
>  }
>  
>  /* returns following node in hard link list
> diff --git a/restore/tree.h b/restore/tree.h
> index bf66e3d..f5bd4ff 100644
> --- a/restore/tree.h
> +++ b/restore/tree.h
> @@ -18,6 +18,8 @@
>  #ifndef TREE_H
>  #define TREE_H
>  
> +void tree_fixroot(void);
> +
>  /* tree_init - creates a new tree abstraction.
>   */
>  extern bool_t tree_init(char *hkdir,
> -- 
> 2.37.3
> 

Re: [RFC PATCH V3] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Eric Sandeen]

On 9/29/22 1:17 PM, Darrick J. Wong wrote:
> On Wed, Sep 28, 2022 at 03:10:52PM -0400, Hironori Shiina wrote:
>> From: Gao Xiang <hsiangkao@redhat.com>
>>
>> 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.
> 
> As far as fixing xfsdump -- wasn't XFS_BULK_IREQ_SPECIAL_ROOT supposed
> to solve that problem by enabling dump to discover it it's really been
> passed the fs root directory?

Yes, but as I understand it this patch is to allow the user to recover
from an already corrupted dump, at restore time, right?

This still feels like deep magic in xfsdump that most people struggle
to understand, but it seems clear to me that the changes here are truly
isolated to the new "-x" option - IOWs if "-x" is not specified, there is
no behavior change at all.

Since this is intended to attempt recovery from an already-corrupted
dump image as a last resort, and given that there are already some xfstests
in place to validate the behavior, I feel reasonably comfortable with
merging this.

Reviwed-by: Eric Sandeen <sandeen@redhat.com>

-Eric

> --D
> 
>> 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@redhat.com>
>> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
>> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
>> ---
>>
>> changes since RFC v2:
>>  - re-adopt the orphanage dir to the fixed root for creating a
>>    correct path of the orphanage dir.
>>
>>  - add description of the new '-x' option to the man page.
>>
>> 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).
>>
>>  common/main.c         |  1 +
>>  man/man8/xfsrestore.8 | 14 +++++++++
>>  restore/content.c     |  7 +++++
>>  restore/getopt.h      |  4 +--
>>  restore/tree.c        | 72 ++++++++++++++++++++++++++++++++++++++++---
>>  restore/tree.h        |  2 ++
>>  6 files changed, 94 insertions(+), 6 deletions(-)
>>
>> diff --git a/common/main.c b/common/main.c
>> index 1db07d4..6141ffb 100644
>> --- a/common/main.c
>> +++ b/common/main.c
>> @@ -988,6 +988,7 @@ usage(void)
>>  	ULO(_("(contents only)"),			GETOPT_TOC);
>>  	ULO(_("<verbosity {silent, verbose, trace}>"),	GETOPT_VERBOSITY);
>>  	ULO(_("(use small tree window)"),		GETOPT_SMALLWINDOW);
>> +	ULO(_("(try to fix rootdir due to xfsdump issue)"),GETOPT_FIXROOTDIR);
>>  	ULO(_("(don't restore extended file attributes)"), GETOPT_NOEXTATTR);
>>  	ULO(_("(restore root dir owner/permissions)"),	GETOPT_ROOTPERM);
>>  	ULO(_("(restore DMAPI event settings)"),	GETOPT_SETDM);
>> diff --git a/man/man8/xfsrestore.8 b/man/man8/xfsrestore.8
>> index 60e4309..df7dde0 100644
>> --- a/man/man8/xfsrestore.8
>> +++ b/man/man8/xfsrestore.8
>> @@ -240,6 +240,20 @@ but does not create or modify any files or directories.
>>  It may be desirable to set the verbosity level to \f3silent\f1
>>  when using this option.
>>  .TP 5
>> +.B \-x
>> +This option may be useful to fix an issue which the files are restored
>> +to orphanage directory because of xfsdump (v3.1.7 - v3.1.9) problem.
>> +A normal dump cannot be restored with this option. This option works
>> +only for a corrupted dump.
>> +If a dump is created by problematic xfsdump (v3.1.7 - v3.1.9), you
>> +should see the contents of the dump with \f3\-t\f1 option before
>> +restoring. Then, if a file is placed to the orphanage directory, you need to
>> +use this \f3\-x\f1 option to restore the dump. Otherwise, you can restore
>> +the dump without this option.
>> +
>> +In the cumulative mode, this option is required only for a base (level 0)
>> +dump. You no longer need this option for level 1+ dumps.
>> +.TP 5
>>  \f3\-v\f1 \f2verbosity\f1
>>  .\" set inter-paragraph distance to 0
>>  .PD 0
>> diff --git a/restore/content.c b/restore/content.c
>> index b19bb90..fdf26dd 100644
>> --- a/restore/content.c
>> +++ b/restore/content.c
>> @@ -861,6 +861,7 @@ static int quotafilecheck(char *type, char *dstdir, char *quotafile);
>>  
>>  bool_t content_media_change_needed;
>>  bool_t restore_rootdir_permissions;
>> +bool_t need_fixrootdir;
>>  char *media_change_alert_program = NULL;
>>  size_t perssz;
>>  
>> @@ -958,6 +959,7 @@ content_init(int argc, char *argv[], size64_t vmsz)
>>  	stsz = 0;
>>  	interpr = BOOL_FALSE;
>>  	restore_rootdir_permissions = BOOL_FALSE;
>> +	need_fixrootdir = BOOL_FALSE;
>>  	optind = 1;
>>  	opterr = 0;
>>  	while ((c = getopt(argc, argv, GETOPT_CMDSTRING)) != EOF) {
>> @@ -1186,6 +1188,9 @@ content_init(int argc, char *argv[], size64_t vmsz)
>>  		case GETOPT_FMT2COMPAT:
>>  			tranp->t_truncategenpr = BOOL_TRUE;
>>  			break;
>> +		case GETOPT_FIXROOTDIR:
>> +			need_fixrootdir = BOOL_TRUE;
>> +			break;
>>  		}
>>  	}
>>  
>> @@ -3129,6 +3134,8 @@ applydirdump(drive_t *drivep,
>>  			return rv;
>>  		}
>>  
>> +		if (need_fixrootdir)
>> +			tree_fixroot();
>>  		persp->s.dirdonepr = BOOL_TRUE;
>>  	}
>>  
>> diff --git a/restore/getopt.h b/restore/getopt.h
>> index b5bc004..b0c0c7d 100644
>> --- a/restore/getopt.h
>> +++ b/restore/getopt.h
>> @@ -26,7 +26,7 @@
>>   * purpose is to contain that command string.
>>   */
>>  
>> -#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
>> +#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wxABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
>>  
>>  #define GETOPT_WORKSPACE	'a'	/* workspace dir (content.c) */
>>  #define GETOPT_BLOCKSIZE        'b'     /* blocksize for rmt */
>> @@ -51,7 +51,7 @@
>>  /*				'u' */
>>  #define	GETOPT_VERBOSITY	'v'	/* verbosity level (0 to 4) */
>>  #define	GETOPT_SMALLWINDOW	'w'	/* use a small window for dir entries */
>> -/*				'x' */
>> +#define GETOPT_FIXROOTDIR	'x'	/* try to fix rootdir due to bulkstat misuse */
>>  /*				'y' */
>>  /*				'z' */
>>  #define	GETOPT_NOEXTATTR	'A'	/* do not restore ext. file attr. */
>> diff --git a/restore/tree.c b/restore/tree.c
>> index 5429b74..bfa07fe 100644
>> --- a/restore/tree.c
>> +++ b/restore/tree.c
>> @@ -15,7 +15,6 @@
>>   * along with this program; if not, write the Free Software Foundation,
>>   * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>>   */
>> -
>>  #include <stdio.h>
>>  #include <unistd.h>
>>  #include <stdlib.h>
>> @@ -262,6 +261,7 @@ extern void usage(void);
>>  extern size_t pgsz;
>>  extern size_t pgmask;
>>  extern bool_t restore_rootdir_permissions;
>> +extern bool_t need_fixrootdir;
>>  
>>  /* forward declarations of locally defined static functions ******************/
>>  
>> @@ -328,10 +328,47 @@ static tran_t *tranp = 0;
>>  static char *persname = PERS_NAME;
>>  static char *orphname = ORPH_NAME;
>>  static xfs_ino_t orphino = ORPH_INO;
>> +static nh_t orig_rooth = NH_NULL;
>>  
>>  
>>  /* definition of locally defined global functions ****************************/
>>  
>> +void
>> +tree_fixroot(void)
>> +{
>> +	nh_t		rooth = persp->p_rooth;
>> +	xfs_ino_t 	rootino;
>> +
>> +	while (1) {
>> +		nh_t	parh;
>> +		node_t *rootp = Node_map(rooth);
>> +
>> +		rootino = rootp->n_ino;
>> +		parh = rootp->n_parh;
>> +		Node_unmap(rooth, &rootp);
>> +
>> +		if (parh == rooth ||
>> +		/*
>> +		 * since all new node (including non-parent)
>> +		 * would be adopted into orphh
>> +		 */
>> +		    parh == persp->p_orphh ||
>> +		    parh == NH_NULL)
>> +			break;
>> +		rooth = parh;
>> +	}
>> +
>> +	if (rooth != persp->p_rooth) {
>> +		persp->p_rooth = rooth;
>> +		persp->p_rootino = rootino;
>> +		disown(rooth);
>> +		adopt(persp->p_rooth, persp->p_orphh, NH_NULL);
>> +
>> +		mlog(MLOG_NORMAL, _("fix root # to %llu (bind mount?)\n"),
>> +		     rootino);
>> +	}
>> +}
>> +
>>  /* ARGSUSED */
>>  bool_t
>>  tree_init(char *hkdir,
>> @@ -746,7 +783,8 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>>  	/* lookup head of hardlink list
>>  	 */
>>  	hardh = link_hardh(ino, gen);
>> -	assert(ino != persp->p_rootino || hardh == persp->p_rooth);
>> +	if (need_fixrootdir == BOOL_FALSE)
>> +		assert(ino != persp->p_rootino || hardh == persp->p_rooth);
>>  
>>  	/* already present
>>  	 */
>> @@ -815,7 +853,6 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>>  		adopt(persp->p_orphh, hardh, NRH_NULL);
>>  		*dahp = dah;
>>  	}
>> -
>>  	return hardh;
>>  }
>>  
>> @@ -960,6 +997,7 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>>  				}
>>  			} else {
>>  				assert(hardp->n_nrh != NRH_NULL);
>> +
>>  				namebuflen
>>  				=
>>  				namreg_get(hardp->n_nrh,
>> @@ -1110,6 +1148,13 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
>>  		      ino,
>>  		      gen);
>>  	}
>> +	/* found the fake rootino from subdir, need fix p_rooth. */
>> +	if (need_fixrootdir == BOOL_TRUE &&
>> +	    persp->p_rootino == ino && hardh != persp->p_rooth) {
>> +		mlog(MLOG_NORMAL,
>> +		     _("found fake rootino #%llu, will fix.\n"), ino);
>> +		persp->p_rooth = hardh;
>> +	}
>>  	return RV_OK;
>>  }
>>  
>> @@ -3808,7 +3853,26 @@ selsubtree_recurse_down(nh_t nh, bool_t sensepr)
>>  static nh_t
>>  link_hardh(xfs_ino_t ino, gen_t gen)
>>  {
>> -	return hash_find(ino, gen);
>> +	nh_t tmp = hash_find(ino, gen);
>> +
>> +	/*
>> +	 * XXX (another workaround): the simply way is that don't reuse node_t
>> +	 * with gen = 0 created in tree_init(). Otherwise, it could cause
>> +	 * xfsrestore: tree.c:1003: tree_addent: Assertion
>> +	 * `hardp->n_nrh != NRH_NULL' failed.
>> +	 * and that node_t is a dir node but the fake rootino could be a non-dir
>> +	 * plus reusing it could cause potential loop in tree hierarchy.
>> +	 */
>> +	if (need_fixrootdir == BOOL_TRUE &&
>> +	    ino == persp->p_rootino && gen == 0 &&
>> +	    orig_rooth == NH_NULL) {
>> +		mlog(MLOG_NORMAL,
>> +_("link out fake rootino %llu with gen=0 created in tree_init()\n"), ino);
>> +		link_out(tmp);
>> +		orig_rooth = tmp;
>> +		return NH_NULL;
>> +	}
>> +	return tmp;
>>  }
>>  
>>  /* returns following node in hard link list
>> diff --git a/restore/tree.h b/restore/tree.h
>> index bf66e3d..f5bd4ff 100644
>> --- a/restore/tree.h
>> +++ b/restore/tree.h
>> @@ -18,6 +18,8 @@
>>  #ifndef TREE_H
>>  #define TREE_H
>>  
>> +void tree_fixroot(void);
>> +
>>  /* tree_init - creates a new tree abstraction.
>>   */
>>  extern bool_t tree_init(char *hkdir,
>> -- 
>> 2.37.3
>>
> 

Re: [RFC PATCH V3] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Darrick J. Wong]

On Wed, Apr 19, 2023 at 09:16:28AM -0500, Eric Sandeen wrote:
> 
> 
> On 9/29/22 1:17 PM, Darrick J. Wong wrote:
> > On Wed, Sep 28, 2022 at 03:10:52PM -0400, Hironori Shiina wrote:
> >> From: Gao Xiang <hsiangkao@redhat.com>
> >>
> >> 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.
> > 
> > As far as fixing xfsdump -- wasn't XFS_BULK_IREQ_SPECIAL_ROOT supposed
> > to solve that problem by enabling dump to discover it it's really been
> > passed the fs root directory?
> 
> Yes, but as I understand it this patch is to allow the user to recover
> from an already corrupted dump, at restore time, right?

Right, though I still haven't seen any patches to dump to employ
XFS_BULK_IREQ_SPECIAL_ROOT to avoid spitting out bad dumps in the first
place.  I think the heuristic that we applied is probably good enough,
but we might as well query the kernel when possible.

> This still feels like deep magic in xfsdump that most people struggle
> to understand, but it seems clear to me that the changes here are truly
> isolated to the new "-x" option - IOWs if "-x" is not specified, there is
> no behavior change at all.
> 
> Since this is intended to attempt recovery from an already-corrupted
> dump image as a last resort, and given that there are already some xfstests
> in place to validate the behavior, I feel reasonably comfortable with
> merging this.

Documentation nit: Can restore detect that it's been given a corrupt
dump, and if so, should it warn the user to rerun with -x?

--D

> Reviwed-by: Eric Sandeen <sandeen@redhat.com>
> 
> -Eric
> 
> > --D
> > 
> >> 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@redhat.com>
> >> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> >> Signed-off-by: Hironori Shiina <shiina.hironori@fujitsu.com>
> >> ---
> >>
> >> changes since RFC v2:
> >>  - re-adopt the orphanage dir to the fixed root for creating a
> >>    correct path of the orphanage dir.
> >>
> >>  - add description of the new '-x' option to the man page.
> >>
> >> 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).
> >>
> >>  common/main.c         |  1 +
> >>  man/man8/xfsrestore.8 | 14 +++++++++
> >>  restore/content.c     |  7 +++++
> >>  restore/getopt.h      |  4 +--
> >>  restore/tree.c        | 72 ++++++++++++++++++++++++++++++++++++++++---
> >>  restore/tree.h        |  2 ++
> >>  6 files changed, 94 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/common/main.c b/common/main.c
> >> index 1db07d4..6141ffb 100644
> >> --- a/common/main.c
> >> +++ b/common/main.c
> >> @@ -988,6 +988,7 @@ usage(void)
> >>  	ULO(_("(contents only)"),			GETOPT_TOC);
> >>  	ULO(_("<verbosity {silent, verbose, trace}>"),	GETOPT_VERBOSITY);
> >>  	ULO(_("(use small tree window)"),		GETOPT_SMALLWINDOW);
> >> +	ULO(_("(try to fix rootdir due to xfsdump issue)"),GETOPT_FIXROOTDIR);
> >>  	ULO(_("(don't restore extended file attributes)"), GETOPT_NOEXTATTR);
> >>  	ULO(_("(restore root dir owner/permissions)"),	GETOPT_ROOTPERM);
> >>  	ULO(_("(restore DMAPI event settings)"),	GETOPT_SETDM);
> >> diff --git a/man/man8/xfsrestore.8 b/man/man8/xfsrestore.8
> >> index 60e4309..df7dde0 100644
> >> --- a/man/man8/xfsrestore.8
> >> +++ b/man/man8/xfsrestore.8
> >> @@ -240,6 +240,20 @@ but does not create or modify any files or directories.
> >>  It may be desirable to set the verbosity level to \f3silent\f1
> >>  when using this option.
> >>  .TP 5
> >> +.B \-x
> >> +This option may be useful to fix an issue which the files are restored
> >> +to orphanage directory because of xfsdump (v3.1.7 - v3.1.9) problem.
> >> +A normal dump cannot be restored with this option. This option works
> >> +only for a corrupted dump.
> >> +If a dump is created by problematic xfsdump (v3.1.7 - v3.1.9), you
> >> +should see the contents of the dump with \f3\-t\f1 option before
> >> +restoring. Then, if a file is placed to the orphanage directory, you need to
> >> +use this \f3\-x\f1 option to restore the dump. Otherwise, you can restore
> >> +the dump without this option.
> >> +
> >> +In the cumulative mode, this option is required only for a base (level 0)
> >> +dump. You no longer need this option for level 1+ dumps.
> >> +.TP 5
> >>  \f3\-v\f1 \f2verbosity\f1
> >>  .\" set inter-paragraph distance to 0
> >>  .PD 0
> >> diff --git a/restore/content.c b/restore/content.c
> >> index b19bb90..fdf26dd 100644
> >> --- a/restore/content.c
> >> +++ b/restore/content.c
> >> @@ -861,6 +861,7 @@ static int quotafilecheck(char *type, char *dstdir, char *quotafile);
> >>  
> >>  bool_t content_media_change_needed;
> >>  bool_t restore_rootdir_permissions;
> >> +bool_t need_fixrootdir;
> >>  char *media_change_alert_program = NULL;
> >>  size_t perssz;
> >>  
> >> @@ -958,6 +959,7 @@ content_init(int argc, char *argv[], size64_t vmsz)
> >>  	stsz = 0;
> >>  	interpr = BOOL_FALSE;
> >>  	restore_rootdir_permissions = BOOL_FALSE;
> >> +	need_fixrootdir = BOOL_FALSE;
> >>  	optind = 1;
> >>  	opterr = 0;
> >>  	while ((c = getopt(argc, argv, GETOPT_CMDSTRING)) != EOF) {
> >> @@ -1186,6 +1188,9 @@ content_init(int argc, char *argv[], size64_t vmsz)
> >>  		case GETOPT_FMT2COMPAT:
> >>  			tranp->t_truncategenpr = BOOL_TRUE;
> >>  			break;
> >> +		case GETOPT_FIXROOTDIR:
> >> +			need_fixrootdir = BOOL_TRUE;
> >> +			break;
> >>  		}
> >>  	}
> >>  
> >> @@ -3129,6 +3134,8 @@ applydirdump(drive_t *drivep,
> >>  			return rv;
> >>  		}
> >>  
> >> +		if (need_fixrootdir)
> >> +			tree_fixroot();
> >>  		persp->s.dirdonepr = BOOL_TRUE;
> >>  	}
> >>  
> >> diff --git a/restore/getopt.h b/restore/getopt.h
> >> index b5bc004..b0c0c7d 100644
> >> --- a/restore/getopt.h
> >> +++ b/restore/getopt.h
> >> @@ -26,7 +26,7 @@
> >>   * purpose is to contain that command string.
> >>   */
> >>  
> >> -#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
> >> +#define GETOPT_CMDSTRING	"a:b:c:def:himn:op:qrs:tv:wxABCDEFG:H:I:JKL:M:NO:PQRS:TUVWX:Y:"
> >>  
> >>  #define GETOPT_WORKSPACE	'a'	/* workspace dir (content.c) */
> >>  #define GETOPT_BLOCKSIZE        'b'     /* blocksize for rmt */
> >> @@ -51,7 +51,7 @@
> >>  /*				'u' */
> >>  #define	GETOPT_VERBOSITY	'v'	/* verbosity level (0 to 4) */
> >>  #define	GETOPT_SMALLWINDOW	'w'	/* use a small window for dir entries */
> >> -/*				'x' */
> >> +#define GETOPT_FIXROOTDIR	'x'	/* try to fix rootdir due to bulkstat misuse */
> >>  /*				'y' */
> >>  /*				'z' */
> >>  #define	GETOPT_NOEXTATTR	'A'	/* do not restore ext. file attr. */
> >> diff --git a/restore/tree.c b/restore/tree.c
> >> index 5429b74..bfa07fe 100644
> >> --- a/restore/tree.c
> >> +++ b/restore/tree.c
> >> @@ -15,7 +15,6 @@
> >>   * along with this program; if not, write the Free Software Foundation,
> >>   * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> >>   */
> >> -
> >>  #include <stdio.h>
> >>  #include <unistd.h>
> >>  #include <stdlib.h>
> >> @@ -262,6 +261,7 @@ extern void usage(void);
> >>  extern size_t pgsz;
> >>  extern size_t pgmask;
> >>  extern bool_t restore_rootdir_permissions;
> >> +extern bool_t need_fixrootdir;
> >>  
> >>  /* forward declarations of locally defined static functions ******************/
> >>  
> >> @@ -328,10 +328,47 @@ static tran_t *tranp = 0;
> >>  static char *persname = PERS_NAME;
> >>  static char *orphname = ORPH_NAME;
> >>  static xfs_ino_t orphino = ORPH_INO;
> >> +static nh_t orig_rooth = NH_NULL;
> >>  
> >>  
> >>  /* definition of locally defined global functions ****************************/
> >>  
> >> +void
> >> +tree_fixroot(void)
> >> +{
> >> +	nh_t		rooth = persp->p_rooth;
> >> +	xfs_ino_t 	rootino;
> >> +
> >> +	while (1) {
> >> +		nh_t	parh;
> >> +		node_t *rootp = Node_map(rooth);
> >> +
> >> +		rootino = rootp->n_ino;
> >> +		parh = rootp->n_parh;
> >> +		Node_unmap(rooth, &rootp);
> >> +
> >> +		if (parh == rooth ||
> >> +		/*
> >> +		 * since all new node (including non-parent)
> >> +		 * would be adopted into orphh
> >> +		 */
> >> +		    parh == persp->p_orphh ||
> >> +		    parh == NH_NULL)
> >> +			break;
> >> +		rooth = parh;
> >> +	}
> >> +
> >> +	if (rooth != persp->p_rooth) {
> >> +		persp->p_rooth = rooth;
> >> +		persp->p_rootino = rootino;
> >> +		disown(rooth);
> >> +		adopt(persp->p_rooth, persp->p_orphh, NH_NULL);
> >> +
> >> +		mlog(MLOG_NORMAL, _("fix root # to %llu (bind mount?)\n"),
> >> +		     rootino);
> >> +	}
> >> +}
> >> +
> >>  /* ARGSUSED */
> >>  bool_t
> >>  tree_init(char *hkdir,
> >> @@ -746,7 +783,8 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
> >>  	/* lookup head of hardlink list
> >>  	 */
> >>  	hardh = link_hardh(ino, gen);
> >> -	assert(ino != persp->p_rootino || hardh == persp->p_rooth);
> >> +	if (need_fixrootdir == BOOL_FALSE)
> >> +		assert(ino != persp->p_rootino || hardh == persp->p_rooth);
> >>  
> >>  	/* already present
> >>  	 */
> >> @@ -815,7 +853,6 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
> >>  		adopt(persp->p_orphh, hardh, NRH_NULL);
> >>  		*dahp = dah;
> >>  	}
> >> -
> >>  	return hardh;
> >>  }
> >>  
> >> @@ -960,6 +997,7 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
> >>  				}
> >>  			} else {
> >>  				assert(hardp->n_nrh != NRH_NULL);
> >> +
> >>  				namebuflen
> >>  				=
> >>  				namreg_get(hardp->n_nrh,
> >> @@ -1110,6 +1148,13 @@ tree_addent(nh_t parh, xfs_ino_t ino, gen_t gen, char *name, size_t namelen)
> >>  		      ino,
> >>  		      gen);
> >>  	}
> >> +	/* found the fake rootino from subdir, need fix p_rooth. */
> >> +	if (need_fixrootdir == BOOL_TRUE &&
> >> +	    persp->p_rootino == ino && hardh != persp->p_rooth) {
> >> +		mlog(MLOG_NORMAL,
> >> +		     _("found fake rootino #%llu, will fix.\n"), ino);
> >> +		persp->p_rooth = hardh;
> >> +	}
> >>  	return RV_OK;
> >>  }
> >>  
> >> @@ -3808,7 +3853,26 @@ selsubtree_recurse_down(nh_t nh, bool_t sensepr)
> >>  static nh_t
> >>  link_hardh(xfs_ino_t ino, gen_t gen)
> >>  {
> >> -	return hash_find(ino, gen);
> >> +	nh_t tmp = hash_find(ino, gen);
> >> +
> >> +	/*
> >> +	 * XXX (another workaround): the simply way is that don't reuse node_t
> >> +	 * with gen = 0 created in tree_init(). Otherwise, it could cause
> >> +	 * xfsrestore: tree.c:1003: tree_addent: Assertion
> >> +	 * `hardp->n_nrh != NRH_NULL' failed.
> >> +	 * and that node_t is a dir node but the fake rootino could be a non-dir
> >> +	 * plus reusing it could cause potential loop in tree hierarchy.
> >> +	 */
> >> +	if (need_fixrootdir == BOOL_TRUE &&
> >> +	    ino == persp->p_rootino && gen == 0 &&
> >> +	    orig_rooth == NH_NULL) {
> >> +		mlog(MLOG_NORMAL,
> >> +_("link out fake rootino %llu with gen=0 created in tree_init()\n"), ino);
> >> +		link_out(tmp);
> >> +		orig_rooth = tmp;
> >> +		return NH_NULL;
> >> +	}
> >> +	return tmp;
> >>  }
> >>  
> >>  /* returns following node in hard link list
> >> diff --git a/restore/tree.h b/restore/tree.h
> >> index bf66e3d..f5bd4ff 100644
> >> --- a/restore/tree.h
> >> +++ b/restore/tree.h
> >> @@ -18,6 +18,8 @@
> >>  #ifndef TREE_H
> >>  #define TREE_H
> >>  
> >> +void tree_fixroot(void);
> >> +
> >>  /* tree_init - creates a new tree abstraction.
> >>   */
> >>  extern bool_t tree_init(char *hkdir,
> >> -- 
> >> 2.37.3
> >>
> > 

Re: [RFC PATCH V3] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Carlos Maiolino]

Hi Darrick.

> > >> 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.
> > >
> > > As far as fixing xfsdump -- wasn't XFS_BULK_IREQ_SPECIAL_ROOT supposed
> > > to solve that problem by enabling dump to discover it it's really been
> > > passed the fs root directory?
> >
> > Yes, but as I understand it this patch is to allow the user to recover
> > from an already corrupted dump, at restore time, right?
> 
> Right, though I still haven't seen any patches to dump to employ
> XFS_BULK_IREQ_SPECIAL_ROOT to avoid spitting out bad dumps in the first
> place.  I think the heuristic that we applied is probably good enough,
> but we might as well query the kernel when possible.
> 
> > This still feels like deep magic in xfsdump that most people struggle
> > to understand, but it seems clear to me that the changes here are truly
> > isolated to the new "-x" option - IOWs if "-x" is not specified, there is
> > no behavior change at all.
> >
> > Since this is intended to attempt recovery from an already-corrupted
> > dump image as a last resort, and given that there are already some xfstests
> > in place to validate the behavior, I feel reasonably comfortable with
> > merging this.
> 
> Documentation nit: Can restore detect that it's been given a corrupt
> dump, and if so, should it warn the user to rerun with -x?
> 
> --D

I am assuming that even though you have concerns about not having
XFS_BULK_IREQ_SPECIAL_ROOT employed in dump yet (and the documentation nit :),
you are not opposed to have this patch merged?


-- 
Carlos Maiolino

Re: [RFC PATCH V3] xfsrestore: fix rootdir due to xfsdump bulkstat misuse

[Darrick J. Wong]

On Fri, Apr 21, 2023 at 11:10:06AM +0200, Carlos Maiolino wrote:
> Hi Darrick.
> 
> > > >> 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.
> > > >
> > > > As far as fixing xfsdump -- wasn't XFS_BULK_IREQ_SPECIAL_ROOT supposed
> > > > to solve that problem by enabling dump to discover it it's really been
> > > > passed the fs root directory?
> > >
> > > Yes, but as I understand it this patch is to allow the user to recover
> > > from an already corrupted dump, at restore time, right?
> > 
> > Right, though I still haven't seen any patches to dump to employ
> > XFS_BULK_IREQ_SPECIAL_ROOT to avoid spitting out bad dumps in the first
> > place.  I think the heuristic that we applied is probably good enough,
> > but we might as well query the kernel when possible.
> > 
> > > This still feels like deep magic in xfsdump that most people struggle
> > > to understand, but it seems clear to me that the changes here are truly
> > > isolated to the new "-x" option - IOWs if "-x" is not specified, there is
> > > no behavior change at all.
> > >
> > > Since this is intended to attempt recovery from an already-corrupted
> > > dump image as a last resort, and given that there are already some xfstests
> > > in place to validate the behavior, I feel reasonably comfortable with
> > > merging this.
> > 
> > Documentation nit: Can restore detect that it's been given a corrupt
> > dump, and if so, should it warn the user to rerun with -x?
> > 
> > --D
> 
> I am assuming that even though you have concerns about not having
> XFS_BULK_IREQ_SPECIAL_ROOT employed in dump yet (and the documentation nit :),
> you are not opposed to have this patch merged?

Correct.  Consider this my somewhat passive-aggressive prod not to
forget to improve dump's "I'm doing something stupid" detector.

--D

> 
> -- 
> Carlos Maiolino