| Message ID | f03093d83baa5bcd4229a0dc9a473add534ee016.1694762532.git.naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs/076: fix failure on device with small zone_append_max_bytes | expand |
On Fri, Sep 15, 2023 at 8:28 AM Naohiro Aota <naohiro.aota@wdc.com> wrote: > > Running btrfs/076 on a zoned null_blk device will fail with the following error. > > - output mismatch (see /host/results/btrfs/076.out.bad) > --- tests/btrfs/076.out 2021-02-05 01:44:20.000000000 +0000 > +++ /host/results/btrfs/076.out.bad 2023-09-15 01:49:36.000000000 +0000 > @@ -1,3 +1,3 @@ > QA output created by 076 > -80 > -80 > +83 > +83 > ... > > This is because the default value of zone_append_max_bytes is 127.5 KB > which is smaller than BTRFS_MAX_UNCOMPRESSED (128K). So, the extent size is > limited to 126976 (= ROUND_DOWN(127.5K, 4096)), which makes the number of > extents larger, and fails the test. > > Instead of hard-coding the number of extents, we can calculate it using the > max extent size of an extent. It is limited by either > BTRFS_MAX_UNCOMPRESSED or zone_append_max_bytes. > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> Looks good, Reviewed-by: Filipe Manana <fdmanana@suse.com> Just two minor comments below. > --- > tests/btrfs/076 | 23 +++++++++++++++++++++-- > tests/btrfs/076.out | 3 +-- > 2 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/tests/btrfs/076 b/tests/btrfs/076 > index 89e9672d09e2..a5cc3eb96b2f 100755 > --- a/tests/btrfs/076 > +++ b/tests/btrfs/076 > @@ -28,13 +28,28 @@ _supported_fs btrfs > _require_test > _require_scratch > > +# An extent size can be up to BTRFS_MAX_UNCOMPRESSED > +max_extent_size=$(( 128 << 10 )) For consistency with every other test and common files, using 128 * 1024 would be perhaps better. I certainly find it easier to read, but that's a personal preference only. > +if _scratch_btrfs_is_zoned; then > + zone_append_max=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/zone_append_max_bytes") > + if [[ $zone_append_max -gt 0 && $zone_append_max -lt $max_extent_size ]]; then > + # Round down to PAGE_SIZE > + max_extent_size=$(( $zone_append_max / 4096 * 4096 )) > + fi > +fi > +file_size=$(( 10 << 20 )) And this one it's even less immediate to understand, having 1 * 1024 * 1024 would make it much more easier to read. Thanks. > +expect=$(( (file_size + max_extent_size - 1) / max_extent_size )) > + > _scratch_mkfs >> $seqres.full 2>&1 > _scratch_mount "-o compress=lzo" > > $XFS_IO_PROG -f -c "pwrite 0 10M" -c "fsync" \ > $SCRATCH_MNT/data >> $seqres.full 2>&1 > > -_extent_count $SCRATCH_MNT/data > +res=$(_extent_count $SCRATCH_MNT/data) > +if [[ $res -ne $expect ]]; then > + _fail "Expected $expect extents, got $res" > +fi > > $XFS_IO_PROG -f -c "pwrite 0 $((4096*33))" -c "fsync" \ > $SCRATCH_MNT/data >> $seqres.full 2>&1 > @@ -42,7 +57,11 @@ $XFS_IO_PROG -f -c "pwrite 0 $((4096*33))" -c "fsync" \ > $XFS_IO_PROG -f -c "pwrite 0 10M" -c "fsync" \ > $SCRATCH_MNT/data >> $seqres.full 2>&1 > > -_extent_count $SCRATCH_MNT/data > +res=$(_extent_count $SCRATCH_MNT/data) > +if [[ $res -ne $expect ]]; then > + _fail "Expected $expect extents, got $res" > +fi > > +echo "Silence is golden" > status=0 > exit > diff --git a/tests/btrfs/076.out b/tests/btrfs/076.out > index b99f7eb10a16..248e095d91af 100644 > --- a/tests/btrfs/076.out > +++ b/tests/btrfs/076.out > @@ -1,3 +1,2 @@ > QA output created by 076 > -80 > -80 > +Silence is golden > -- > 2.42.0 >
On Fri, Sep 15, 2023 at 10:16:32AM +0100, Filipe Manana wrote: > On Fri, Sep 15, 2023 at 8:28 AM Naohiro Aota <naohiro.aota@wdc.com> wrote: > > > > Running btrfs/076 on a zoned null_blk device will fail with the following error. > > > > - output mismatch (see /host/results/btrfs/076.out.bad) > > --- tests/btrfs/076.out 2021-02-05 01:44:20.000000000 +0000 > > +++ /host/results/btrfs/076.out.bad 2023-09-15 01:49:36.000000000 +0000 > > @@ -1,3 +1,3 @@ > > QA output created by 076 > > -80 > > -80 > > +83 > > +83 > > ... > > > > This is because the default value of zone_append_max_bytes is 127.5 KB > > which is smaller than BTRFS_MAX_UNCOMPRESSED (128K). So, the extent size is > > limited to 126976 (= ROUND_DOWN(127.5K, 4096)), which makes the number of > > extents larger, and fails the test. > > > > Instead of hard-coding the number of extents, we can calculate it using the > > max extent size of an extent. It is limited by either > > BTRFS_MAX_UNCOMPRESSED or zone_append_max_bytes. > > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > Looks good, > > Reviewed-by: Filipe Manana <fdmanana@suse.com> > > Just two minor comments below. > > > --- > > tests/btrfs/076 | 23 +++++++++++++++++++++-- > > tests/btrfs/076.out | 3 +-- > > 2 files changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/tests/btrfs/076 b/tests/btrfs/076 > > index 89e9672d09e2..a5cc3eb96b2f 100755 > > --- a/tests/btrfs/076 > > +++ b/tests/btrfs/076 > > @@ -28,13 +28,28 @@ _supported_fs btrfs > > _require_test > > _require_scratch > > > > +# An extent size can be up to BTRFS_MAX_UNCOMPRESSED > > +max_extent_size=$(( 128 << 10 )) > > For consistency with every other test and common files, using 128 * > 1024 would be perhaps better. I certainly find it easier to read, but > that's a personal preference only. > > > +if _scratch_btrfs_is_zoned; then > > + zone_append_max=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/zone_append_max_bytes") > > + if [[ $zone_append_max -gt 0 && $zone_append_max -lt $max_extent_size ]]; then > > + # Round down to PAGE_SIZE > > + max_extent_size=$(( $zone_append_max / 4096 * 4096 )) > > + fi > > +fi > > +file_size=$(( 10 << 20 )) > > And this one it's even less immediate to understand, having 1 * 1024 * > 1024 would make it much more easier to read. Agreed. I'll use 1024 and repost. Thanks.
On Thu, Sep 21, 2023 at 07:41:19AM +0000, Naohiro Aota wrote: > On Fri, Sep 15, 2023 at 10:16:32AM +0100, Filipe Manana wrote: > > On Fri, Sep 15, 2023 at 8:28 AM Naohiro Aota <naohiro.aota@wdc.com> wrote: > > > > > > Running btrfs/076 on a zoned null_blk device will fail with the following error. > > > > > > - output mismatch (see /host/results/btrfs/076.out.bad) > > > --- tests/btrfs/076.out 2021-02-05 01:44:20.000000000 +0000 > > > +++ /host/results/btrfs/076.out.bad 2023-09-15 01:49:36.000000000 +0000 > > > @@ -1,3 +1,3 @@ > > > QA output created by 076 > > > -80 > > > -80 > > > +83 > > > +83 > > > ... > > > > > > This is because the default value of zone_append_max_bytes is 127.5 KB > > > which is smaller than BTRFS_MAX_UNCOMPRESSED (128K). So, the extent size is > > > limited to 126976 (= ROUND_DOWN(127.5K, 4096)), which makes the number of > > > extents larger, and fails the test. > > > > > > Instead of hard-coding the number of extents, we can calculate it using the > > > max extent size of an extent. It is limited by either > > > BTRFS_MAX_UNCOMPRESSED or zone_append_max_bytes. > > > > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > > > Looks good, > > > > Reviewed-by: Filipe Manana <fdmanana@suse.com> > > > > Just two minor comments below. > > > > > --- > > > tests/btrfs/076 | 23 +++++++++++++++++++++-- > > > tests/btrfs/076.out | 3 +-- > > > 2 files changed, 22 insertions(+), 4 deletions(-) > > > > > > diff --git a/tests/btrfs/076 b/tests/btrfs/076 > > > index 89e9672d09e2..a5cc3eb96b2f 100755 > > > --- a/tests/btrfs/076 > > > +++ b/tests/btrfs/076 > > > @@ -28,13 +28,28 @@ _supported_fs btrfs > > > _require_test > > > _require_scratch > > > > > > +# An extent size can be up to BTRFS_MAX_UNCOMPRESSED > > > +max_extent_size=$(( 128 << 10 )) > > > > For consistency with every other test and common files, using 128 * > > 1024 would be perhaps better. I certainly find it easier to read, but > > that's a personal preference only. > > > > > +if _scratch_btrfs_is_zoned; then > > > + zone_append_max=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/zone_append_max_bytes") > > > + if [[ $zone_append_max -gt 0 && $zone_append_max -lt $max_extent_size ]]; then > > > + # Round down to PAGE_SIZE > > > + max_extent_size=$(( $zone_append_max / 4096 * 4096 )) > > > + fi > > > +fi > > > +file_size=$(( 10 << 20 )) > > > > And this one it's even less immediate to understand, having 1 * 1024 * > > 1024 would make it much more easier to read. > > Agreed. I'll use 1024 and repost. Thanks. I've changed that part when I merged this patch (haven't pushed), so you don't need to send this patch again, save that time :) Thanks, Zorro
On Fri, Sep 22, 2023 at 01:09:38AM +0800, Zorro Lang wrote: > On Thu, Sep 21, 2023 at 07:41:19AM +0000, Naohiro Aota wrote: > > On Fri, Sep 15, 2023 at 10:16:32AM +0100, Filipe Manana wrote: > > > On Fri, Sep 15, 2023 at 8:28 AM Naohiro Aota <naohiro.aota@wdc.com> wrote: > > > > > > > > Running btrfs/076 on a zoned null_blk device will fail with the following error. > > > > > > > > - output mismatch (see /host/results/btrfs/076.out.bad) > > > > --- tests/btrfs/076.out 2021-02-05 01:44:20.000000000 +0000 > > > > +++ /host/results/btrfs/076.out.bad 2023-09-15 01:49:36.000000000 +0000 > > > > @@ -1,3 +1,3 @@ > > > > QA output created by 076 > > > > -80 > > > > -80 > > > > +83 > > > > +83 > > > > ... > > > > > > > > This is because the default value of zone_append_max_bytes is 127.5 KB > > > > which is smaller than BTRFS_MAX_UNCOMPRESSED (128K). So, the extent size is > > > > limited to 126976 (= ROUND_DOWN(127.5K, 4096)), which makes the number of > > > > extents larger, and fails the test. > > > > > > > > Instead of hard-coding the number of extents, we can calculate it using the > > > > max extent size of an extent. It is limited by either > > > > BTRFS_MAX_UNCOMPRESSED or zone_append_max_bytes. > > > > > > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > > > > > Looks good, > > > > > > Reviewed-by: Filipe Manana <fdmanana@suse.com> > > > > > > Just two minor comments below. > > > > > > > --- > > > > tests/btrfs/076 | 23 +++++++++++++++++++++-- > > > > tests/btrfs/076.out | 3 +-- > > > > 2 files changed, 22 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/tests/btrfs/076 b/tests/btrfs/076 > > > > index 89e9672d09e2..a5cc3eb96b2f 100755 > > > > --- a/tests/btrfs/076 > > > > +++ b/tests/btrfs/076 > > > > @@ -28,13 +28,28 @@ _supported_fs btrfs > > > > _require_test > > > > _require_scratch > > > > > > > > +# An extent size can be up to BTRFS_MAX_UNCOMPRESSED > > > > +max_extent_size=$(( 128 << 10 )) > > > > > > For consistency with every other test and common files, using 128 * > > > 1024 would be perhaps better. I certainly find it easier to read, but > > > that's a personal preference only. > > > > > > > +if _scratch_btrfs_is_zoned; then > > > > + zone_append_max=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/zone_append_max_bytes") > > > > + if [[ $zone_append_max -gt 0 && $zone_append_max -lt $max_extent_size ]]; then > > > > + # Round down to PAGE_SIZE > > > > + max_extent_size=$(( $zone_append_max / 4096 * 4096 )) > > > > + fi > > > > +fi > > > > +file_size=$(( 10 << 20 )) > > > > > > And this one it's even less immediate to understand, having 1 * 1024 * > > > 1024 would make it much more easier to read. > > > > Agreed. I'll use 1024 and repost. Thanks. > > I've changed that part when I merged this patch (haven't pushed), so you > don't need to send this patch again, save that time :) > > Thanks, > Zorro > Oops, I could not read this as my subscription is depending on vger + l2md... Anyway, thank you fixing the patch.
diff --git a/tests/btrfs/076 b/tests/btrfs/076 index 89e9672d09e2..a5cc3eb96b2f 100755 --- a/tests/btrfs/076 +++ b/tests/btrfs/076 @@ -28,13 +28,28 @@ _supported_fs btrfs _require_test _require_scratch +# An extent size can be up to BTRFS_MAX_UNCOMPRESSED +max_extent_size=$(( 128 << 10 )) +if _scratch_btrfs_is_zoned; then + zone_append_max=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/zone_append_max_bytes") + if [[ $zone_append_max -gt 0 && $zone_append_max -lt $max_extent_size ]]; then + # Round down to PAGE_SIZE + max_extent_size=$(( $zone_append_max / 4096 * 4096 )) + fi +fi +file_size=$(( 10 << 20 )) +expect=$(( (file_size + max_extent_size - 1) / max_extent_size )) + _scratch_mkfs >> $seqres.full 2>&1 _scratch_mount "-o compress=lzo" $XFS_IO_PROG -f -c "pwrite 0 10M" -c "fsync" \ $SCRATCH_MNT/data >> $seqres.full 2>&1 -_extent_count $SCRATCH_MNT/data +res=$(_extent_count $SCRATCH_MNT/data) +if [[ $res -ne $expect ]]; then + _fail "Expected $expect extents, got $res" +fi $XFS_IO_PROG -f -c "pwrite 0 $((4096*33))" -c "fsync" \ $SCRATCH_MNT/data >> $seqres.full 2>&1 @@ -42,7 +57,11 @@ $XFS_IO_PROG -f -c "pwrite 0 $((4096*33))" -c "fsync" \ $XFS_IO_PROG -f -c "pwrite 0 10M" -c "fsync" \ $SCRATCH_MNT/data >> $seqres.full 2>&1 -_extent_count $SCRATCH_MNT/data +res=$(_extent_count $SCRATCH_MNT/data) +if [[ $res -ne $expect ]]; then + _fail "Expected $expect extents, got $res" +fi +echo "Silence is golden" status=0 exit diff --git a/tests/btrfs/076.out b/tests/btrfs/076.out index b99f7eb10a16..248e095d91af 100644 --- a/tests/btrfs/076.out +++ b/tests/btrfs/076.out @@ -1,3 +1,2 @@ QA output created by 076 -80 -80 +Silence is golden
Running btrfs/076 on a zoned null_blk device will fail with the following error. - output mismatch (see /host/results/btrfs/076.out.bad) --- tests/btrfs/076.out 2021-02-05 01:44:20.000000000 +0000 +++ /host/results/btrfs/076.out.bad 2023-09-15 01:49:36.000000000 +0000 @@ -1,3 +1,3 @@ QA output created by 076 -80 -80 +83 +83 ... This is because the default value of zone_append_max_bytes is 127.5 KB which is smaller than BTRFS_MAX_UNCOMPRESSED (128K). So, the extent size is limited to 126976 (= ROUND_DOWN(127.5K, 4096)), which makes the number of extents larger, and fails the test. Instead of hard-coding the number of extents, we can calculate it using the max extent size of an extent. It is limited by either BTRFS_MAX_UNCOMPRESSED or zone_append_max_bytes. Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> --- tests/btrfs/076 | 23 +++++++++++++++++++++-- tests/btrfs/076.out | 3 +-- 2 files changed, 22 insertions(+), 4 deletions(-)