[PATCH v2 0/2] btrfs/076: fix failure on device with small zone_append_max_bytes

[PATCH v2 0/2] btrfs/076: fix failure on device with small zone_append_max_bytes

[Naohiro Aota]

Running btrfs/072 on a zoned null_blk device fails with a mismatch of the
number of extents. That mismatch happens because the max size of extent is
limited by not only BTRFS_MAX_UNCOMPRESSED, but also zone_append_max_bytes
on the zoned mode.

Fix the issue by calculating the expected number of extents instead of
hard-coding it in the output file.

Also, use _fixed_by_kernel_commit to rewrite the fixing commit indication
in the comment.

- v2: Only fixed the subject lines

Naohiro Aota (2):
  btrfs/076: support smaller extent size limit
  btrfs/076: use _fixed_by_kernel_commit to tell the fixing kernel
    commit

 tests/btrfs/076     | 29 ++++++++++++++++++++++++-----
 tests/btrfs/076.out |  3 +--
 2 files changed, 25 insertions(+), 7 deletions(-)

-- 
2.42.0

[PATCH v2 1/2] btrfs/076: support smaller extent size limit

[Naohiro Aota]

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(-)

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
-- 
2.42.0

[PATCH v2 2/2] btrfs/076: use _fixed_by_kernel_commit to tell the fixing kernel commit

[Naohiro Aota]

The fix commit is written in the comment without a commit hash. Use
_fixed_by_kernel_commit command to describe it.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 tests/btrfs/076 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/btrfs/076 b/tests/btrfs/076
index a5cc3eb96b2f..dbb67bd1c241 100755
--- a/tests/btrfs/076
+++ b/tests/btrfs/076
@@ -5,10 +5,8 @@
 # FS QA Test No. btrfs/076
 #
 # Regression test for btrfs incorrect inode ratio detection.
-# This was fixed in the following linux kernel patch:
-#
-#     Btrfs: fix incorrect compression ratio detection
 #
+
 . ./common/preamble
 _begin_fstest auto quick compress
 
@@ -27,6 +25,8 @@ _cleanup()
 _supported_fs btrfs
 _require_test
 _require_scratch
+_fixed_by_kernel_commit 4bcbb3325513 \
+	"Btrfs: fix incorrect compression ratio detection"
 
 # An extent size can be up to BTRFS_MAX_UNCOMPRESSED
 max_extent_size=$(( 128 << 10 ))
-- 
2.42.0

Re: [PATCH v2 1/2] btrfs/076: support smaller extent size limit

[Filipe Manana]

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
>

Re: [PATCH v2 2/2] btrfs/076: use _fixed_by_kernel_commit to tell the fixing kernel commit

[Filipe Manana]

On Fri, Sep 15, 2023 at 8:28 AM Naohiro Aota <naohiro.aota@wdc.com> wrote:
>
> The fix commit is written in the comment without a commit hash. Use
> _fixed_by_kernel_commit command to describe it.
>
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>

Looks good,

Reviewed-by: Filipe Manana <fdmanana@suse.com>

> ---
>  tests/btrfs/076 | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tests/btrfs/076 b/tests/btrfs/076
> index a5cc3eb96b2f..dbb67bd1c241 100755
> --- a/tests/btrfs/076
> +++ b/tests/btrfs/076
> @@ -5,10 +5,8 @@
>  # FS QA Test No. btrfs/076
>  #
>  # Regression test for btrfs incorrect inode ratio detection.
> -# This was fixed in the following linux kernel patch:
> -#
> -#     Btrfs: fix incorrect compression ratio detection
>  #
> +
>  . ./common/preamble
>  _begin_fstest auto quick compress
>
> @@ -27,6 +25,8 @@ _cleanup()
>  _supported_fs btrfs
>  _require_test
>  _require_scratch
> +_fixed_by_kernel_commit 4bcbb3325513 \
> +       "Btrfs: fix incorrect compression ratio detection"
>
>  # An extent size can be up to BTRFS_MAX_UNCOMPRESSED
>  max_extent_size=$(( 128 << 10 ))
> --
> 2.42.0
>

Re: [PATCH v2 1/2] btrfs/076: support smaller extent size limit

[Naohiro Aota]

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.

Re: [PATCH v2 1/2] btrfs/076: support smaller extent size limit

[Zorro Lang]

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

Re: [PATCH v2 1/2] btrfs/076: support smaller extent size limit

[Naohiro Aota]

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.