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

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

[Anand Jain]

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

Anand Jain (2):
  btrfs-progs: dump-super: improve error log
  btrfs-progs: dump-super: fix read beyond device size

 cmds/inspect-dump-super.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH 1/2] 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.31.1

[PATCH 2/2] 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 /dev/vdb12
  <snap>
  ERROR: Failed to read the superblock on /dev/vdb12 at 274877906944
  ERROR: Error = 'No such file or directory', errno = 2

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

To fix include `errno = 2` as the expected error and return success.

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

diff --git a/cmds/inspect-dump-super.c b/cmds/inspect-dump-super.c
index 4529b2308d7e..1121d9af93b9 100644
--- a/cmds/inspect-dump-super.c
+++ b/cmds/inspect-dump-super.c
@@ -37,8 +37,12 @@ static int load_and_dump_sb(char *filename, int fd, u64 sb_bytenr, int full,
 
 	ret = sbread(fd, &sb, sb_bytenr);
 	if (ret != BTRFS_SUPER_INFO_SIZE) {
-		/* check if the disk if too short for further superblock */
-		if (ret == 0 && errno == 0)
+		/*
+		 * Check if the disk if too short for further superblock.
+		 * On aarch64 glibc 2.28, pread() would set errno = 2 if read
+		 * beyond the disk size.
+		 */
+		if (ret == 0 && (errno == 0 || errno == 2))
 			return 0;
 
 		error("Failed to read the superblock on %s at %llu read %llu/%d bytes",
-- 
2.31.1

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

[Qu Wenruo]

On 2023/6/21 23:41, 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 /dev/vdb12
>    <snap>
>    ERROR: Failed to read the superblock on /dev/vdb12 at 274877906944
>    ERROR: Error = 'No such file or directory', errno = 2
> 
> This is because `pread()` behaves differently on aarch64 and sets
> `errno = 2` instead of the usual `errno = 0`.

I don't think that's the proper way to handle certain glibc quirks.

Instead we should do extra checks before the read, and reject any read 
beyond the device size.

Thanks,
Qu
> 
> To fix include `errno = 2` as the expected error and return success.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>   cmds/inspect-dump-super.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds/inspect-dump-super.c b/cmds/inspect-dump-super.c
> index 4529b2308d7e..1121d9af93b9 100644
> --- a/cmds/inspect-dump-super.c
> +++ b/cmds/inspect-dump-super.c
> @@ -37,8 +37,12 @@ static int load_and_dump_sb(char *filename, int fd, u64 sb_bytenr, int full,
>   
>   	ret = sbread(fd, &sb, sb_bytenr);
>   	if (ret != BTRFS_SUPER_INFO_SIZE) {
> -		/* check if the disk if too short for further superblock */
> -		if (ret == 0 && errno == 0)
> +		/*
> +		 * Check if the disk if too short for further superblock.
> +		 * On aarch64 glibc 2.28, pread() would set errno = 2 if read
> +		 * beyond the disk size.
> +		 */
> +		if (ret == 0 && (errno == 0 || errno == 2))
>   			return 0;
>   
>   		error("Failed to read the superblock on %s at %llu read %llu/%d bytes",

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

[Anand Jain]

On 22/06/2023 09:02, Qu Wenruo wrote:
> 
> 
> On 2023/6/21 23:41, 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 /dev/vdb12
>>    <snap>
>>    ERROR: Failed to read the superblock on /dev/vdb12 at 274877906944
>>    ERROR: Error = 'No such file or directory', errno = 2
>>
>> This is because `pread()` behaves differently on aarch64 and sets
>> `errno = 2` instead of the usual `errno = 0`.
> 
> I don't think that's the proper way to handle certain glibc quirks.
> 
> Instead we should do extra checks before the read, and reject any read 
> beyond the device size.

I implemented that in a local version, following the kernel's approach.
However, I didn't send it out because the test case misc-tests/015*
requires dump-super to work on character devices like /dev/urandom,
which is an interesting approach I didn't want to disrupt by modifying
the testcase. Another approach is to check only for regular files and
block devices, but it's not a generic any device solution.

Thanks, Anand

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

[Qu Wenruo]

On 2023/6/22 09:29, Anand Jain wrote:
>
>
> On 22/06/2023 09:02, Qu Wenruo wrote:
>>
>>
>> On 2023/6/21 23:41, 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 /dev/vdb12
>>>    <snap>
>>>    ERROR: Failed to read the superblock on /dev/vdb12 at 274877906944
>>>    ERROR: Error = 'No such file or directory', errno = 2
>>>
>>> This is because `pread()` behaves differently on aarch64 and sets
>>> `errno = 2` instead of the usual `errno = 0`.
>>
>> I don't think that's the proper way to handle certain glibc quirks.
>>
>> Instead we should do extra checks before the read, and reject any read
>> beyond the device size.
>
> I implemented that in a local version, following the kernel's approach.
> However, I didn't send it out because the test case misc-tests/015*
> requires dump-super to work on character devices like /dev/urandom,
> which is an interesting approach I didn't want to disrupt by modifying
> the testcase. Another approach is to check only for regular files and
> block devices, but it's not a generic any device solution.

I think it's completely sane to update that misc/015 test case, so that
we put some garbage into the backup super blocks other than relying on
the support to run super-dump on char devices.

Thanks,
Qu
>
> Thanks, Anand

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

[Anand Jain]

Just a note:

This patch fixes the following xfstests btrfs/184 failure on aarch64.

$ ./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



On 21/06/2023 23:41, 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 /dev/vdb12
>    <snap>
>    ERROR: Failed to read the superblock on /dev/vdb12 at 274877906944
>    ERROR: Error = 'No such file or directory', errno = 2
> 
> This is because `pread()` behaves differently on aarch64 and sets
> `errno = 2` instead of the usual `errno = 0`.
> 
> To fix include `errno = 2` as the expected error and return success.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>   cmds/inspect-dump-super.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds/inspect-dump-super.c b/cmds/inspect-dump-super.c
> index 4529b2308d7e..1121d9af93b9 100644
> --- a/cmds/inspect-dump-super.c
> +++ b/cmds/inspect-dump-super.c
> @@ -37,8 +37,12 @@ static int load_and_dump_sb(char *filename, int fd, u64 sb_bytenr, int full,
>   
>   	ret = sbread(fd, &sb, sb_bytenr);
>   	if (ret != BTRFS_SUPER_INFO_SIZE) {
> -		/* check if the disk if too short for further superblock */
> -		if (ret == 0 && errno == 0)
> +		/*
> +		 * Check if the disk if too short for further superblock.
> +		 * On aarch64 glibc 2.28, pread() would set errno = 2 if read
> +		 * beyond the disk size.
> +		 */
> +		if (ret == 0 && (errno == 0 || errno == 2))
>   			return 0;
>   
>   		error("Failed to read the superblock on %s at %llu read %llu/%d bytes",

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

[Anand Jain]

On 22/06/2023 10:20, Qu Wenruo wrote:
> 
> 
> On 2023/6/22 09:29, Anand Jain wrote:
>>
>>
>> On 22/06/2023 09:02, Qu Wenruo wrote:
>>>
>>>
>>> On 2023/6/21 23:41, 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 /dev/vdb12
>>>>    <snap>
>>>>    ERROR: Failed to read the superblock on /dev/vdb12 at 274877906944
>>>>    ERROR: Error = 'No such file or directory', errno = 2
>>>>
>>>> This is because `pread()` behaves differently on aarch64 and sets
>>>> `errno = 2` instead of the usual `errno = 0`.
>>>
>>> I don't think that's the proper way to handle certain glibc quirks.
>>>
>>> Instead we should do extra checks before the read, and reject any read
>>> beyond the device size.
>>
>> I implemented that in a local version, following the kernel's approach.
>> However, I didn't send it out because the test case misc-tests/015*
>> requires dump-super to work on character devices like /dev/urandom,
>> which is an interesting approach I didn't want to disrupt by modifying
>> the testcase. Another approach is to check only for regular files and
>> block devices, but it's not a generic any device solution.
> 
> I think it's completely sane to update that misc/015 test case, so that
> we put some garbage into the backup super blocks other than relying on
> the support to run super-dump on char devices.

Yeah, I follow a rule of thumb to avoid removing a feature, even if it's
not useful. It may just be that I don't know how it is used.

But I'm okay with removing the facility to pass the character device
to the btrfs inspect dump-super <char/device> as you suggest. I'm just
wondering if we have any more comments about that?

Thanks.

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

[Anand Jain]

>>>>> This is because `pread()` behaves differently on aarch64 and sets
>>>>> `errno = 2` instead of the usual `errno = 0`.
>>>>
>>>> I don't think that's the proper way to handle certain glibc quirks.
>>>>
>>>> Instead we should do extra checks before the read, and reject any read
>>>> beyond the device size.
>>>
>>> I implemented that in a local version, following the kernel's approach.
>>> However, I didn't send it out because the test case misc-tests/015*
>>> requires dump-super to work on character devices like /dev/urandom,
>>> which is an interesting approach I didn't want to disrupt by modifying
>>> the testcase.

>>> Another approach is to check only for regular files and
>>> block devices, but it's not a generic any device solution.

Sent v2 with this change.

>> I think it's completely sane to update that misc/015 test case, so that
>> we put some garbage into the backup super blocks other than relying on
>> the support to run super-dump on char devices.
> 
> Yeah, I follow a rule of thumb to avoid removing a feature, even if it's
> not useful. It may just be that I don't know how it is used.
> 
> But I'm okay with removing the facility to pass the character device
> to the btrfs inspect dump-super <char/device> as you suggest. I'm just
> wondering if we have any more comments about that?