[PATCH 0/3 v2] btrfs-progs: dump-super: fix dump-super on aarch64

[PATCH 0/3 v2] btrfs-progs: dump-super: fix dump-super on aarch64

[Anand Jain]

v2: Skip sbreads() for sb bytenr greater the device size.

The command "btrfs inspect dump-super -a" is failing on aarch64 systems.
The following set of patches resolves the issue. Patch 1/3 is enhancing
the error log it helped debug the issue. Patch 2/3 preparatory. Patch
3/3 provides the fix.

Anand Jain (3):
  btrfs-progs: dump-super: improve error log
  btrfs-progs: dump_super: drop the label out and variable ret
  btrfs-progs: dump-super: fix read beyond device size

 cmds/inspect-dump-super.c | 40 ++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

-- 
2.39.3

[PATCH 1/3] btrfs-progs: dump-super: improve error log

[Anand Jain]

Add more error info to help debug.

  $ ./btrfs inspect-internal dump-super -Ffa /dev/vdb10

  Before:
  ERROR: failed to read the superblock on /dev/vdb10 at 274877906944

  After:
  ERROR: failed to read the superblock on /dev/vdb10 at 274877906944
  read 0/4096 bytes

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds/inspect-dump-super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cmds/inspect-dump-super.c b/cmds/inspect-dump-super.c
index d62c3a85d9ca..4529b2308d7e 100644
--- a/cmds/inspect-dump-super.c
+++ b/cmds/inspect-dump-super.c
@@ -41,7 +41,8 @@ static int load_and_dump_sb(char *filename, int fd, u64 sb_bytenr, int full,
 		if (ret == 0 && errno == 0)
 			return 0;
 
-		error("failed to read the superblock on %s at %llu", filename, sb_bytenr);
+		error("Failed to read the superblock on %s at %llu read %llu/%d bytes",
+		       filename, sb_bytenr, ret, BTRFS_SUPER_INFO_SIZE);
 		error("error = '%m', errno = %d", errno);
 		return 1;
 	}
-- 
2.39.3

Re: [PATCH 1/3] btrfs-progs: dump-super: improve error log

[Qu Wenruo]

On 2023/6/27 16:53, Anand Jain wrote:
> Add more error info to help debug.
>
>    $ ./btrfs inspect-internal dump-super -Ffa /dev/vdb10
>
>    Before:
>    ERROR: failed to read the superblock on /dev/vdb10 at 274877906944
>
>    After:
>    ERROR: failed to read the superblock on /dev/vdb10 at 274877906944
>    read 0/4096 bytes
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   cmds/inspect-dump-super.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/cmds/inspect-dump-super.c b/cmds/inspect-dump-super.c
> index d62c3a85d9ca..4529b2308d7e 100644
> --- a/cmds/inspect-dump-super.c
> +++ b/cmds/inspect-dump-super.c
> @@ -41,7 +41,8 @@ static int load_and_dump_sb(char *filename, int fd, u64 sb_bytenr, int full,
>   		if (ret == 0 && errno == 0)
>   			return 0;
>
> -		error("failed to read the superblock on %s at %llu", filename, sb_bytenr);
> +		error("Failed to read the superblock on %s at %llu read %llu/%d bytes",
> +		       filename, sb_bytenr, ret, BTRFS_SUPER_INFO_SIZE);
>   		error("error = '%m', errno = %d", errno);
>   		return 1;
>   	}

[PATCH 2/3] btrfs-progs: dump_super: drop the label out and variable ret

[Anand Jain]

In cmd_inspect_dump_super(), at the label 'out', nothing much happens
other than returning ret.

At the goto statement to the label, in the for loop, we perform close(fd).
However, moving the close(fd) to 'out' as well is not a good idea because
close(fd) doesn't make sense outside the for loop.

Instead, simply return 1 instead of ret=1 and then returning it. Drop both
the 'out' label and ret.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds/inspect-dump-super.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/cmds/inspect-dump-super.c b/cmds/inspect-dump-super.c
index 4529b2308d7e..f32c67fd5c4d 100644
--- a/cmds/inspect-dump-super.c
+++ b/cmds/inspect-dump-super.c
@@ -84,7 +84,6 @@ static int cmd_inspect_dump_super(const struct cmd_struct *cmd,
 	char *filename;
 	int fd = -1;
 	int i;
-	int ret = 0;
 	u64 arg;
 	u64 sb_bytenr = btrfs_sb_offset(0);
 
@@ -156,8 +155,7 @@ static int cmd_inspect_dump_super(const struct cmd_struct *cmd,
 		fd = open(filename, O_RDONLY);
 		if (fd < 0) {
 			error("cannot open %s: %m", filename);
-			ret = 1;
-			goto out;
+			return 1;
 		}
 
 		if (all) {
@@ -168,8 +166,7 @@ static int cmd_inspect_dump_super(const struct cmd_struct *cmd,
 				if (load_and_dump_sb(filename, fd,
 						sb_bytenr, full, force)) {
 					close(fd);
-					ret = 1;
-					goto out;
+					return 1;
 				}
 
 				putchar('\n');
@@ -177,15 +174,13 @@ static int cmd_inspect_dump_super(const struct cmd_struct *cmd,
 		} else {
 			if (load_and_dump_sb(filename, fd, sb_bytenr, full, force)) {
 				close(fd);
-				ret = 1;
-				goto out;
+				return 1;
 			}
 			putchar('\n');
 		}
 		close(fd);
 	}
 
-out:
-	return ret;
+	return 0;
 }
 DEFINE_SIMPLE_COMMAND(inspect_dump_super, "dump-super");
-- 
2.39.3

Re: [PATCH 2/3] btrfs-progs: dump_super: drop the label out and variable ret

[Qu Wenruo]

On 2023/6/27 16:53, Anand Jain wrote:
> In cmd_inspect_dump_super(), at the label 'out', nothing much happens
> other than returning ret.
>
> At the goto statement to the label, in the for loop, we perform close(fd).
> However, moving the close(fd) to 'out' as well is not a good idea because
> close(fd) doesn't make sense outside the for loop.
>
> Instead, simply return 1 instead of ret=1 and then returning it. Drop both
> the 'out' label and ret.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   cmds/inspect-dump-super.c | 13 ++++---------
>   1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/cmds/inspect-dump-super.c b/cmds/inspect-dump-super.c
> index 4529b2308d7e..f32c67fd5c4d 100644
> --- a/cmds/inspect-dump-super.c
> +++ b/cmds/inspect-dump-super.c
> @@ -84,7 +84,6 @@ static int cmd_inspect_dump_super(const struct cmd_struct *cmd,
>   	char *filename;
>   	int fd = -1;
>   	int i;
> -	int ret = 0;
>   	u64 arg;
>   	u64 sb_bytenr = btrfs_sb_offset(0);
>
> @@ -156,8 +155,7 @@ static int cmd_inspect_dump_super(const struct cmd_struct *cmd,
>   		fd = open(filename, O_RDONLY);
>   		if (fd < 0) {
>   			error("cannot open %s: %m", filename);
> -			ret = 1;
> -			goto out;
> +			return 1;
>   		}
>
>   		if (all) {
> @@ -168,8 +166,7 @@ static int cmd_inspect_dump_super(const struct cmd_struct *cmd,
>   				if (load_and_dump_sb(filename, fd,
>   						sb_bytenr, full, force)) {
>   					close(fd);
> -					ret = 1;
> -					goto out;
> +					return 1;
>   				}
>
>   				putchar('\n');
> @@ -177,15 +174,13 @@ static int cmd_inspect_dump_super(const struct cmd_struct *cmd,
>   		} else {
>   			if (load_and_dump_sb(filename, fd, sb_bytenr, full, force)) {
>   				close(fd);
> -				ret = 1;
> -				goto out;
> +				return 1;
>   			}
>   			putchar('\n');
>   		}
>   		close(fd);
>   	}
>
> -out:
> -	return ret;
> +	return 0;
>   }
>   DEFINE_SIMPLE_COMMAND(inspect_dump_super, "dump-super");

[PATCH 3/3] btrfs-progs: dump-super: fix read beyond device size

[Anand Jain]

On aarch64 systems with glibc 2.28, several btrfs-progs test cases are
failing because the command 'btrfs inspect dump-super -a <dev>' reports
an error when it attempts to read beyond the disk/file-image size.

      $ btrfs inspect dump-super -a /dev/vdb12
      <snap>
      ERROR: Failed to read the superblock on /dev/vdb12 at 274877906944
      ERROR: Error = 'No such file or directory', errno = 2

And btrfs/184 also fails, as it uses -s 2 option to dump the last super
block.

	$ ./check btrfs/184
	FSTYP         -- btrfs
	PLATFORM      -- Linux/aarch64 a4k 6.4.0-rc7+ #7 SMP PREEMPT Sat Jun 24 02:47:24 EDT 2023
	MKFS_OPTIONS  -- /dev/vdb2
	MOUNT_OPTIONS -- /dev/vdb2 /mnt/scratch

	btrfs/184 1s ... [failed, exit status 1]- output mismatch (see /Volumes/ws/xfstests-dev/results//btrfs/184.out.bad)
	    --- tests/btrfs/184.out    2020-03-03 00:26:40.172081468 -0500
	    +++ /Volumes/ws/xfstests-dev/results//btrfs/184.out.bad    2023-06-24 05:54:40.868210737 -0400
	    @@ -1,2 +1,3 @@
	     QA output created by 184
	    -Silence is golden
	    +Deleted dev superblocks not scratched
	    +(see /Volumes/ws/xfstests-dev/results//btrfs/184.full for details)
	    ...
	    (Run 'diff -u /Volumes/ws/xfstests-dev/tests/btrfs/184.out /Volumes/ws/xfstests-dev/results//btrfs/184.out.bad'  to see the entire diff)
	Ran: btrfs/184
	Failures: btrfs/184
	Failed 1 of 1 tests

This is because `pread()` behaves differently on aarch64 and sets
`errno = 2` instead of the usual `errno = 0`.

To fix check if the sb offset is beyond the device size or regular file
size and skip the corresponding sbread().

Also, move putchar('\n') after a successful call to load_and_dump_sb() to
the load_and_dump_sb() itself.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds/inspect-dump-super.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/cmds/inspect-dump-super.c b/cmds/inspect-dump-super.c
index f32c67fd5c4d..a1c3dcd9d90b 100644
--- a/cmds/inspect-dump-super.c
+++ b/cmds/inspect-dump-super.c
@@ -20,6 +20,7 @@
 #include <fcntl.h>
 #include <errno.h>
 #include <getopt.h>
+#include <sys/stat.h>
 #include "kernel-shared/ctree.h"
 #include "kernel-shared/disk-io.h"
 #include "kernel-shared/print-tree.h"
@@ -33,8 +34,27 @@ static int load_and_dump_sb(char *filename, int fd, u64 sb_bytenr, int full,
 		int force)
 {
 	struct btrfs_super_block sb;
+	struct stat st;
 	u64 ret;
 
+	if (fstat(fd, &st)) {
+		error("error = '%m', errno = %d", errno);
+		return 1;
+	}
+
+	if (S_ISBLK(st.st_mode) || S_ISREG(st.st_mode)) {
+		off_t last_byte;
+
+		last_byte = lseek(fd, 0, SEEK_END);
+		if (last_byte == -1) {
+			error("error = '%m', errno = %d", errno);
+			return 1;
+		}
+
+		if (sb_bytenr > last_byte)
+			return 0;
+	}
+
 	ret = sbread(fd, &sb, sb_bytenr);
 	if (ret != BTRFS_SUPER_INFO_SIZE) {
 		/* check if the disk if too short for further superblock */
@@ -54,6 +74,7 @@ static int load_and_dump_sb(char *filename, int fd, u64 sb_bytenr, int full,
 		return 1;
 	}
 	btrfs_print_superblock(&sb, full);
+	putchar('\n');
 	return 0;
 }
 
@@ -168,15 +189,12 @@ static int cmd_inspect_dump_super(const struct cmd_struct *cmd,
 					close(fd);
 					return 1;
 				}
-
-				putchar('\n');
 			}
 		} else {
 			if (load_and_dump_sb(filename, fd, sb_bytenr, full, force)) {
 				close(fd);
 				return 1;
 			}
-			putchar('\n');
 		}
 		close(fd);
 	}
-- 
2.39.3

Re: [PATCH 3/3] btrfs-progs: dump-super: fix read beyond device size

[Qu Wenruo]

On 2023/6/27 16:53, Anand Jain wrote:
> On aarch64 systems with glibc 2.28, several btrfs-progs test cases are
> failing because the command 'btrfs inspect dump-super -a <dev>' reports
> an error when it attempts to read beyond the disk/file-image size.
>
>        $ btrfs inspect dump-super -a /dev/vdb12
>        <snap>
>        ERROR: Failed to read the superblock on /dev/vdb12 at 274877906944
>        ERROR: Error = 'No such file or directory', errno = 2
>
> And btrfs/184 also fails, as it uses -s 2 option to dump the last super
> block.
>
> 	$ ./check btrfs/184
> 	FSTYP         -- btrfs
> 	PLATFORM      -- Linux/aarch64 a4k 6.4.0-rc7+ #7 SMP PREEMPT Sat Jun 24 02:47:24 EDT 2023
> 	MKFS_OPTIONS  -- /dev/vdb2
> 	MOUNT_OPTIONS -- /dev/vdb2 /mnt/scratch
>
> 	btrfs/184 1s ... [failed, exit status 1]- output mismatch (see /Volumes/ws/xfstests-dev/results//btrfs/184.out.bad)
> 	    --- tests/btrfs/184.out    2020-03-03 00:26:40.172081468 -0500
> 	    +++ /Volumes/ws/xfstests-dev/results//btrfs/184.out.bad    2023-06-24 05:54:40.868210737 -0400
> 	    @@ -1,2 +1,3 @@
> 	     QA output created by 184
> 	    -Silence is golden
> 	    +Deleted dev superblocks not scratched
> 	    +(see /Volumes/ws/xfstests-dev/results//btrfs/184.full for details)
> 	    ...
> 	    (Run 'diff -u /Volumes/ws/xfstests-dev/tests/btrfs/184.out /Volumes/ws/xfstests-dev/results//btrfs/184.out.bad'  to see the entire diff)
> 	Ran: btrfs/184
> 	Failures: btrfs/184
> 	Failed 1 of 1 tests
>
> This is because `pread()` behaves differently on aarch64 and sets
> `errno = 2` instead of the usual `errno = 0`.
>
> To fix check if the sb offset is beyond the device size or regular file
> size and skip the corresponding sbread().
>
> Also, move putchar('\n') after a successful call to load_and_dump_sb() to
> the load_and_dump_sb() itself.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   cmds/inspect-dump-super.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/cmds/inspect-dump-super.c b/cmds/inspect-dump-super.c
> index f32c67fd5c4d..a1c3dcd9d90b 100644
> --- a/cmds/inspect-dump-super.c
> +++ b/cmds/inspect-dump-super.c
> @@ -20,6 +20,7 @@
>   #include <fcntl.h>
>   #include <errno.h>
>   #include <getopt.h>
> +#include <sys/stat.h>
>   #include "kernel-shared/ctree.h"
>   #include "kernel-shared/disk-io.h"
>   #include "kernel-shared/print-tree.h"
> @@ -33,8 +34,27 @@ static int load_and_dump_sb(char *filename, int fd, u64 sb_bytenr, int full,
>   		int force)
>   {
>   	struct btrfs_super_block sb;
> +	struct stat st;
>   	u64 ret;
>
> +	if (fstat(fd, &st)) {
> +		error("error = '%m', errno = %d", errno);
> +		return 1;
> +	}
> +
> +	if (S_ISBLK(st.st_mode) || S_ISREG(st.st_mode)) {
> +		off_t last_byte;
> +
> +		last_byte = lseek(fd, 0, SEEK_END);
> +		if (last_byte == -1) {
> +			error("error = '%m', errno = %d", errno);
> +			return 1;
> +		}
> +
> +		if (sb_bytenr > last_byte)
> +			return 0;
> +	}
> +
>   	ret = sbread(fd, &sb, sb_bytenr);
>   	if (ret != BTRFS_SUPER_INFO_SIZE) {
>   		/* check if the disk if too short for further superblock */
> @@ -54,6 +74,7 @@ static int load_and_dump_sb(char *filename, int fd, u64 sb_bytenr, int full,
>   		return 1;
>   	}
>   	btrfs_print_superblock(&sb, full);
> +	putchar('\n');
>   	return 0;
>   }
>
> @@ -168,15 +189,12 @@ static int cmd_inspect_dump_super(const struct cmd_struct *cmd,
>   					close(fd);
>   					return 1;
>   				}
> -
> -				putchar('\n');
>   			}
>   		} else {
>   			if (load_and_dump_sb(filename, fd, sb_bytenr, full, force)) {
>   				close(fd);
>   				return 1;
>   			}
> -			putchar('\n');
>   		}
>   		close(fd);
>   	}

Re: [PATCH 3/3] btrfs-progs: dump-super: fix read beyond device size

[David Sterba]

On Tue, Jun 27, 2023 at 04:53:15PM +0800, Anand Jain wrote:
> @@ -33,8 +34,27 @@ static int load_and_dump_sb(char *filename, int fd, u64 sb_bytenr, int full,
>  		int force)
>  {
>  	struct btrfs_super_block sb;
> +	struct stat st;
>  	u64 ret;
>  
> +	if (fstat(fd, &st)) {
> +		error("error = '%m', errno = %d", errno);
> +		return 1;
> +	}
> +
> +	if (S_ISBLK(st.st_mode) || S_ISREG(st.st_mode)) {
> +		off_t last_byte;
> +
> +		last_byte = lseek(fd, 0, SEEK_END);
> +		if (last_byte == -1) {
> +			error("error = '%m', errno = %d", errno);

Such error messages are not useful, there should be a description of the
problem with %m for the error text. I've updated it.

> +			return 1;
> +		}
> +
> +		if (sb_bytenr > last_byte)
> +			return 0;
> +	}
> +
>  	ret = sbread(fd, &sb, sb_bytenr);
>  	if (ret != BTRFS_SUPER_INFO_SIZE) {
>  		/* check if the disk if too short for further superblock */
> @@ -54,6 +74,7 @@ static int load_and_dump_sb(char *filename, int fd, u64 sb_bytenr, int full,
>  		return 1;
>  	}
>  	btrfs_print_superblock(&sb, full);
> +	putchar('\n');
>  	return 0;
>  }

Re: [PATCH 3/3] btrfs-progs: dump-super: fix read beyond device size

[Anand Jain]

>> +			error("error = '%m', errno = %d", errno);
> 
> Such error messages are not useful, there should be a description of the
> problem with %m for the error text. I've updated it.
> 

          error("cannot read end of file %s: %m", filename);

Yep. Now much better.

Thanks, Anand

Re: [PATCH 0/3 v2] btrfs-progs: dump-super: fix dump-super on aarch64

[David Sterba]

On Tue, Jun 27, 2023 at 04:53:12PM +0800, Anand Jain wrote:
> v2: Skip sbreads() for sb bytenr greater the device size.
> 
> The command "btrfs inspect dump-super -a" is failing on aarch64 systems.
> The following set of patches resolves the issue. Patch 1/3 is enhancing
> the error log it helped debug the issue. Patch 2/3 preparatory. Patch
> 3/3 provides the fix.
> 
> Anand Jain (3):
>   btrfs-progs: dump-super: improve error log
>   btrfs-progs: dump_super: drop the label out and variable ret
>   btrfs-progs: dump-super: fix read beyond device size

Added to devel, thanks.