| Message ID | f7fed92047412c7e8f89e94c10ec80af564fe9cb.1687361649.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: dump-super: fix dump-super on aarch64 | expand |
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",
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
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
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",
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.
>>>>> 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?
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",
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(-)