[PATCH 0/2] xfs_db rdump: papercut fixes

[PATCH 0/2] xfs_db rdump: papercut fixes

[Alexander Monakov]

Hello,

I've been using the new 'rdump' command of xfs_db for manual recovery of
a substantially damaged filesystem. I'd like to propose two patches that
would have made that work a bit easier.

I'd also like to suggest to remove the one-argument form of rdump that
dumps from root. I'm honestly unsure what it's for, considering that
'rdump / dest' is also supposed to work.

Alexander Monakov (2):
  xfs_db rdump: remove sole_path logic
  xfs_db: use push_cur_and_set_type more

 db/namei.c        |  2 +-
 db/rdump.c        | 21 +++------------------
 man/man8/xfs_db.8 |  8 +-------
 3 files changed, 5 insertions(+), 26 deletions(-)

-- 
2.51.0

[PATCH 1/2] xfs_db rdump: remove sole_path logic

[Alexander Monakov]

Eliminate special handling of the case where rdump'ing one directory
does not create the corresponding directory in the destination, but
instead modifies the destination's attributes and creates children
alongside the pre-existing children.

This can be a trap for the unwary (the effect on attributes can be
particularly surprising and non-trivial to undo), and, in general, fewer
special cases in such a low-level tool should be desirable.

Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
---
 db/rdump.c        | 19 ++-----------------
 man/man8/xfs_db.8 |  8 +-------
 2 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/db/rdump.c b/db/rdump.c
index 9ff83355..73295dbe 100644
--- a/db/rdump.c
+++ b/db/rdump.c
@@ -852,7 +852,6 @@ rdump_file(
 static int
 rdump_path(
 	struct xfs_mount	*mp,
-	bool			sole_path,
 	const char		*path,
 	const struct destdir	*destdir)
 {
@@ -890,20 +889,6 @@ rdump_path(
 			dbprintf(_("%s: %s\n"), path, strerror(ret));
 			return 1;
 		}
-
-		if (sole_path) {
-			struct xfs_dinode	*dip = iocur_top->data;
-
-			/*
-			 * If this is the only path to copy out and it's a dir,
-			 * then we can copy the children directly into the
-			 * target.
-			 */
-			if (S_ISDIR(be16_to_cpu(dip->di_mode))) {
-				pbuf->len = 0;
-				pbuf->path[0] = 0;
-			}
-		}
 	} else {
 		set_cur_inode(mp->m_sb.sb_rootino);
 	}
@@ -980,7 +965,7 @@ rdump_f(
 	if (optind == argc - 1) {
 		/* no dirs given, just do the whole fs */
 		push_cur();
-		ret = rdump_path(mp, false, "", &destdir);
+		ret = rdump_path(mp, "", &destdir);
 		pop_cur();
 		if (ret)
 			exitcode = 1;
@@ -996,7 +981,7 @@ rdump_f(
 		argv[i][len] = 0;
 
 		push_cur();
-		ret = rdump_path(mp, argc == optind + 2, argv[i], &destdir);
+		ret = rdump_path(mp, argv[i], &destdir);
 		pop_cur();
 
 		if (ret) {
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index 1e85aebb..920b2b3e 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -1147,13 +1147,7 @@ the filesystem as possible.
 If zero
 .B paths
 are specified, the entire filesystem is dumped.
-If only one
-.B path
-is specified and it is a directory, the children of that directory will be
-copied directly to the destination.
-If multiple
-.B paths
-are specified, each file is copied into the directory as a new child.
+Otherwise, each entry is copied into the destination as a new child.
 
 If possible, sparse holes, xfs file attributes, and extended attributes will be
 preserved.
-- 
2.51.0

Re: [PATCH 1/2] xfs_db rdump: remove sole_path logic

[Darrick J. Wong]

On Wed, Nov 12, 2025 at 06:19:31PM +0300, Alexander Monakov wrote:
> Eliminate special handling of the case where rdump'ing one directory
> does not create the corresponding directory in the destination, but
> instead modifies the destination's attributes and creates children
> alongside the pre-existing children.

It sounds like what happened is that you ran rdump with a non-empty
destdir, only to have rdump mutate the existing children in that
directory.  Is that correct?

If so, then I think what you really wanted was for rdump to check for
that and error out, unless explicit --overwrite permission had been
granted.  Because...

> This can be a trap for the unwary (the effect on attributes can be
> particularly surprising and non-trivial to undo), and, in general, fewer
> special cases in such a low-level tool should be desirable.

...I use this "special case" and don't understand why you decided that
removing functionality was the solution here.  This is a filesystem
debugger, there are weird functions and sharp edges everywhere.

--D

> Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
> ---
>  db/rdump.c        | 19 ++-----------------
>  man/man8/xfs_db.8 |  8 +-------
>  2 files changed, 3 insertions(+), 24 deletions(-)
> 
> diff --git a/db/rdump.c b/db/rdump.c
> index 9ff83355..73295dbe 100644
> --- a/db/rdump.c
> +++ b/db/rdump.c
> @@ -852,7 +852,6 @@ rdump_file(
>  static int
>  rdump_path(
>  	struct xfs_mount	*mp,
> -	bool			sole_path,
>  	const char		*path,
>  	const struct destdir	*destdir)
>  {
> @@ -890,20 +889,6 @@ rdump_path(
>  			dbprintf(_("%s: %s\n"), path, strerror(ret));
>  			return 1;
>  		}
> -
> -		if (sole_path) {
> -			struct xfs_dinode	*dip = iocur_top->data;
> -
> -			/*
> -			 * If this is the only path to copy out and it's a dir,
> -			 * then we can copy the children directly into the
> -			 * target.
> -			 */
> -			if (S_ISDIR(be16_to_cpu(dip->di_mode))) {
> -				pbuf->len = 0;
> -				pbuf->path[0] = 0;
> -			}
> -		}
>  	} else {
>  		set_cur_inode(mp->m_sb.sb_rootino);
>  	}
> @@ -980,7 +965,7 @@ rdump_f(
>  	if (optind == argc - 1) {
>  		/* no dirs given, just do the whole fs */
>  		push_cur();
> -		ret = rdump_path(mp, false, "", &destdir);
> +		ret = rdump_path(mp, "", &destdir);
>  		pop_cur();
>  		if (ret)
>  			exitcode = 1;
> @@ -996,7 +981,7 @@ rdump_f(
>  		argv[i][len] = 0;
>  
>  		push_cur();
> -		ret = rdump_path(mp, argc == optind + 2, argv[i], &destdir);
> +		ret = rdump_path(mp, argv[i], &destdir);
>  		pop_cur();
>  
>  		if (ret) {
> diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> index 1e85aebb..920b2b3e 100644
> --- a/man/man8/xfs_db.8
> +++ b/man/man8/xfs_db.8
> @@ -1147,13 +1147,7 @@ the filesystem as possible.
>  If zero
>  .B paths
>  are specified, the entire filesystem is dumped.
> -If only one
> -.B path
> -is specified and it is a directory, the children of that directory will be
> -copied directly to the destination.
> -If multiple
> -.B paths
> -are specified, each file is copied into the directory as a new child.
> +Otherwise, each entry is copied into the destination as a new child.
>  
>  If possible, sparse holes, xfs file attributes, and extended attributes will be
>  preserved.
> -- 
> 2.51.0
> 
> 

Re: [PATCH 1/2] xfs_db rdump: remove sole_path logic

[Alexander Monakov]

On Wed, 12 Nov 2025, Darrick J. Wong wrote:

> On Wed, Nov 12, 2025 at 06:19:31PM +0300, Alexander Monakov wrote:
> > Eliminate special handling of the case where rdump'ing one directory
> > does not create the corresponding directory in the destination, but
> > instead modifies the destination's attributes and creates children
> > alongside the pre-existing children.
> 
> It sounds like what happened is that you ran rdump with a non-empty
> destdir, only to have rdump mutate the existing children in that
> directory.  Is that correct?

Not really (what you describe would have been even worse).

In my case, I was rdump'ing into a directory that had rtinherit set, and
rdump undid that (generally speaking, all attributes of the destdir were
overwritten, not just rtinherit).

> If so, then I think what you really wanted was for rdump to check for
> that and error out, unless explicit --overwrite permission had been
> granted.  Because...

(I would still hit the above issue with attributes)

> > This can be a trap for the unwary (the effect on attributes can be
> > particularly surprising and non-trivial to undo), and, in general, fewer
> > special cases in such a low-level tool should be desirable.
> 
> ...I use this "special case" and don't understand why you decided that
> removing functionality was the solution here.  This is a filesystem
> debugger, there are weird functions and sharp edges everywhere.

Shouldn't we strive to have fewer surprises? For instance, if there was
an explicit flag for that, there wouldn't be an issue.

Out of curiosity, what is your use-case for this?

Thank you.
Alexander

Re: [PATCH 1/2] xfs_db rdump: remove sole_path logic

[Darrick J. Wong]

On Wed, Nov 12, 2025 at 10:45:37PM +0300, Alexander Monakov wrote:
> 
> On Wed, 12 Nov 2025, Darrick J. Wong wrote:
> 
> > On Wed, Nov 12, 2025 at 06:19:31PM +0300, Alexander Monakov wrote:
> > > Eliminate special handling of the case where rdump'ing one directory
> > > does not create the corresponding directory in the destination, but
> > > instead modifies the destination's attributes and creates children
> > > alongside the pre-existing children.
> > 
> > It sounds like what happened is that you ran rdump with a non-empty
> > destdir, only to have rdump mutate the existing children in that
> > directory.  Is that correct?
> 
> Not really (what you describe would have been even worse).
> 
> In my case, I was rdump'ing into a directory that had rtinherit set, and
> rdump undid that (generally speaking, all attributes of the destdir were
> overwritten, not just rtinherit).
> 
> > If so, then I think what you really wanted was for rdump to check for
> > that and error out, unless explicit --overwrite permission had been
> > granted.  Because...
> 
> (I would still hit the above issue with attributes)
> 
> > > This can be a trap for the unwary (the effect on attributes can be
> > > particularly surprising and non-trivial to undo), and, in general, fewer
> > > special cases in such a low-level tool should be desirable.
> > 
> > ...I use this "special case" and don't understand why you decided that
> > removing functionality was the solution here.  This is a filesystem
> > debugger, there are weird functions and sharp edges everywhere.
> 
> Shouldn't we strive to have fewer surprises? For instance, if there was
> an explicit flag for that, there wouldn't be an issue.
> 
> Out of curiosity, what is your use-case for this?

Copying build artifacts out of a directory on an xfs filesystem into
some other directory, e.g.

# xfs_db -c 'rdump /rootfs /srv/containers/whatever' /dev/sda1

Without then having to do:

# mv /srv/containers/whatever/rootfs/* /srv/containers/whatever/
# rmdir /srv/containers/whatever/rootfs

I'd be willing to accept more rsync-like behavior where a trailing slash
means "copy children directly into the target dir" and no trailing slash
means "create new child subdir in target dir and copy stuff into that".

IOWs,

xfs_db> rdump /rootfs /srv/containers/whatever

Would always create /srv/containers/whatever/rootfs and copy the
children of /rootfs into that path, but

xfs_db> rdump /rootfs/ /srv/containers/whatever/boot

Would copy the children of /rootfs directly into
/srv/containers/whatever/boot.

--D


> Thank you.
> Alexander
> 

[PATCH 2/2] xfs_db: use push_cur_and_set_type more

[Alexander Monakov]

Since push_cur unsets cur_typ, 'ls' and 'rdump' with paths relative to
current address do not work with a mysterious diagnostic (claiming that
the supplied name is not a directory). Use push_cur_and_set_type
instead.

Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
---
 db/namei.c | 2 +-
 db/rdump.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/db/namei.c b/db/namei.c
index 1d9581c3..4149503c 100644
--- a/db/namei.c
+++ b/db/namei.c
@@ -629,7 +629,7 @@ ls_f(
 	}
 
 	for (c = optind; c < argc; c++) {
-		push_cur();
+		push_cur_and_set_type();
 
 		error = path_walk(rootino, argv[c]);
 		if (error)
diff --git a/db/rdump.c b/db/rdump.c
index 73295dbe..146f829b 100644
--- a/db/rdump.c
+++ b/db/rdump.c
@@ -980,7 +980,7 @@ rdump_f(
 			len--;
 		argv[i][len] = 0;
 
-		push_cur();
+		push_cur_and_set_type();
 		ret = rdump_path(mp, argv[i], &destdir);
 		pop_cur();
 
-- 
2.51.0

Re: [PATCH 2/2] xfs_db: use push_cur_and_set_type more

[Darrick J. Wong]

On Wed, Nov 12, 2025 at 06:19:32PM +0300, Alexander Monakov wrote:
> Since push_cur unsets cur_typ, 'ls' and 'rdump' with paths relative to
> current address do not work with a mysterious diagnostic (claiming that
> the supplied name is not a directory). Use push_cur_and_set_type
> instead.

IOWs, you're trying to fix this, correct?

# xfs_db /dev/sdf
xfs_db> ls /
/:
8          128                directory      0x0000002e   1 . (good)
10         128                directory      0x0000172e   2 .. (good)
314        50337826           directory      0x0a152956   5 PPTRS (good)
xfs_db> ls ./PPTRS
./PPTRS: Not a directory

If so, then
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D


> Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
> ---
>  db/namei.c | 2 +-
>  db/rdump.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/db/namei.c b/db/namei.c
> index 1d9581c3..4149503c 100644
> --- a/db/namei.c
> +++ b/db/namei.c
> @@ -629,7 +629,7 @@ ls_f(
>  	}
>  
>  	for (c = optind; c < argc; c++) {
> -		push_cur();
> +		push_cur_and_set_type();
>  
>  		error = path_walk(rootino, argv[c]);
>  		if (error)
> diff --git a/db/rdump.c b/db/rdump.c
> index 73295dbe..146f829b 100644
> --- a/db/rdump.c
> +++ b/db/rdump.c
> @@ -980,7 +980,7 @@ rdump_f(
>  			len--;
>  		argv[i][len] = 0;
>  
> -		push_cur();
> +		push_cur_and_set_type();
>  		ret = rdump_path(mp, argv[i], &destdir);
>  		pop_cur();
>  
> -- 
> 2.51.0
> 
> 

Re: [PATCH 2/2] xfs_db: use push_cur_and_set_type more

[Alexander Monakov]

On Wed, 12 Nov 2025, Darrick J. Wong wrote:

> On Wed, Nov 12, 2025 at 06:19:32PM +0300, Alexander Monakov wrote:
> > Since push_cur unsets cur_typ, 'ls' and 'rdump' with paths relative to
> > current address do not work with a mysterious diagnostic (claiming that
> > the supplied name is not a directory). Use push_cur_and_set_type
> > instead.
> 
> IOWs, you're trying to fix this, correct?

Mostly correct, but with 'path /' in place of initial 'ls /' so that
current address is set to the root inode.

> # xfs_db /dev/sdf
> xfs_db> ls /
> /:
> 8          128                directory      0x0000002e   1 . (good)
> 10         128                directory      0x0000172e   2 .. (good)
> 314        50337826           directory      0x0a152956   5 PPTRS (good)
> xfs_db> ls ./PPTRS
> ./PPTRS: Not a directory
> 
> If so, then
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

Thanks.
Alexander