[PATCH 0/2] btrfs: test active zone tracking

[PATCH 0/2] btrfs: test active zone tracking

[Naohiro Aota]

This series adds a test for checking if an active zone tracking feature of
btrfs's zoned mode. The first patch introduces _zone_capacity() helper to
get the zone capacity of a specified zone. It rewrites btrfs/237 with the
helper and use the helper in a newly added test btrfs/292.

Naohiro Aota (2):
  common: introduce zone_capacity() to return a zone capacity
  btrfs: test active zone tracking

 common/zbd          |  29 ++++++++++
 tests/btrfs/237     |   8 +--
 tests/btrfs/292     | 137 ++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/292.out |   2 +
 4 files changed, 170 insertions(+), 6 deletions(-)
 create mode 100644 common/zbd
 create mode 100755 tests/btrfs/292
 create mode 100644 tests/btrfs/292.out

-- 
2.37.3

[PATCH 1/2] common: introduce zone_capacity() to return a zone capacity

[Naohiro Aota]

Introduce _zone_capacity() to return a zone capacity of the given address
in the given device (optional). Rewrite btrfs/237 with it.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 common/zbd      | 17 +++++++++++++++++
 tests/btrfs/237 |  8 ++------
 2 files changed, 19 insertions(+), 6 deletions(-)
 create mode 100644 common/zbd

diff --git a/common/zbd b/common/zbd
new file mode 100644
index 000000000000..329bb7be6b7b
--- /dev/null
+++ b/common/zbd
@@ -0,0 +1,17 @@
+#
+# Common zoned block device specific functions
+#
+
+. common/filter
+
+_zone_capacity() {
+    local phy=$1
+    local dev=$2
+
+    [ -z "$dev" ] && dev=$SCRATCH_DEV
+
+    size=$($BLKZONE_PROG report -o $phy -l 1 $dev |\
+	       _filter_blkzone_report |\
+	       grep -Po "cap 0x[[:xdigit:]]+" | cut -d ' ' -f 2)
+    echo $((size << 9))
+}
diff --git a/tests/btrfs/237 b/tests/btrfs/237
index bc6522e2200a..101094b5ce70 100755
--- a/tests/btrfs/237
+++ b/tests/btrfs/237
@@ -13,7 +13,7 @@
 _begin_fstest auto quick zone balance
 
 # Import common functions.
-. ./common/filter
+. ./common/zbd
 
 # real QA test starts here
 
@@ -56,11 +56,7 @@ fi
 
 start_data_bg_phy=$(get_data_bg_physical)
 start_data_bg_phy=$((start_data_bg_phy >> 9))
-
-size=$($BLKZONE_PROG report -o $start_data_bg_phy -l 1 $SCRATCH_DEV |\
-	_filter_blkzone_report |\
-	grep -Po "cap 0x[[:xdigit:]]+" | cut -d ' ' -f 2)
-size=$((size << 9))
+size=$(_zone_capacity $start_data_bg_phy)
 
 reclaim_threshold=75
 echo $reclaim_threshold > /sys/fs/btrfs/"$uuid"/bg_reclaim_threshold
-- 
2.37.3

[PATCH 2/2] btrfs: test active zone tracking

[Naohiro Aota]

A ZNS device limits the number of active zones, which is the number of
zones can be written at the same time. To deal with the limit, btrfs's
zoned mode tracks which zone (corresponds to a block group on the SINGLE
profile) is active, and finish a zone if necessary.

This test checks if the active zone tracking and the finishing of zones
works properly. First, it fills <number of max active zones> zones
mostly. And, run some data/metadata stress workload to force btrfs to use a
new zone.

This test fails on an older kernel (e.g, 5.18.2) like below.

btrfs/292
[failed, exit status 1]- output mismatch (see /host/btrfs/292.out.bad)
    --- tests/btrfs/292.out     2022-09-15 07:52:18.000000000 +0000
    +++ /host/btrfs/292.out.bad 2022-09-15 07:59:14.290967793 +0000
    @@ -1,2 +1,5 @@
     QA output created by 292
    -Silence is golden
    +stress_data_bgs failed
    +stress_data_bgs_2 failed
    +failed: '/bin/btrfs subvolume snapshot /mnt/scratch /mnt/scratch/snap825'
    +(see /host/btrfs/292.full for details)
    ...
    (Run 'diff -u /var/lib/xfstests/tests/btrfs/292.out /host/btrfs/292.out.bad'  to see the entire diff)

The failure is fixed with a series "btrfs: zoned: fix active zone tracking
issues" [1] (upstream commits from 65ea1b66482f ("block: add bdev_max_segments()
helper") to 2ce543f47843 ("btrfs: zoned: wait until zone is finished when
allocation didn't progress")).

[1] https://lore.kernel.org/linux-btrfs/cover.1657321126.git.naohiro.aota@wdc.com/

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 common/zbd          |  12 ++++
 tests/btrfs/292     | 137 ++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/292.out |   2 +
 3 files changed, 151 insertions(+)
 create mode 100755 tests/btrfs/292
 create mode 100644 tests/btrfs/292.out

diff --git a/common/zbd b/common/zbd
index 329bb7be6b7b..19a59fd27e69 100644
--- a/common/zbd
+++ b/common/zbd
@@ -2,8 +2,20 @@
 # Common zoned block device specific functions
 #
 
+. common/rc
 . common/filter
 
+_require_limited_active_zones() {
+    local dev=$1
+    local sysfs=$(_sysfs_dev ${dev})
+    local attr="${sysfs}/queue/max_active_zones"
+
+    [ -e "${attr}" ] || _notrun "cannot find queue/max_active_zones. Maybe non-zoned device?"
+    if [ $(cat "${attr}") == 0 ]; then
+	_notrun "this test requires limited active zones"
+    fi
+}
+
 _zone_capacity() {
     local phy=$1
     local dev=$2
diff --git a/tests/btrfs/292 b/tests/btrfs/292
new file mode 100755
index 000000000000..30c027c4ba5f
--- /dev/null
+++ b/tests/btrfs/292
@@ -0,0 +1,137 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Western Digital Corporation.  All Rights Reserved.
+#
+# FS QA Test 292
+#
+# Test that an active zone is properly reclaimed to allow the further
+# allocations, even if the active zones are mostly filled.
+#
+. ./common/preamble
+_begin_fstest auto quick snapshot zone
+
+# Import common functions.
+. ./common/btrfs
+. ./common/zbd
+
+# real QA test starts here
+
+_supported_fs btrfs
+_require_scratch
+_require_zoned_device "$SCRATCH_DEV"
+_require_limited_active_zones "$SCRATCH_DEV"
+
+_require_command "$BLKZONE_PROG" blkzone
+_require_fio
+_require_btrfs_command inspect-internal dump-tree
+
+# This test requires specific data space usage, skip if we have compression
+# enabled.
+_require_no_compress
+
+max_active=$(cat $(_sysfs_dev ${SCRATCH_DEV})/queue/max_active_zones)
+
+# Fill the zones leaving the last 1MB
+fill_active_zones() {
+    # Asuumes we have the same capacity between zones.
+    local capacity=$(_zone_capacity 0)
+    local fill_size=$((capacity - 1024 * 1024))
+
+    for x in $(seq ${max_active}); do
+	dd if=/dev/zero of=${SCRATCH_MNT}/fill$(printf "%02d" $x) bs=${fill_size} \
+	   count=1 oflag=direct 2>/dev/null
+	$BTRFS_UTIL_PROG filesystem sync ${SCRATCH_MNT}
+
+	local nactive=$($BLKZONE_PROG report ${SCRATCH_DEV} | grep oi | wc -l)
+	if [[ ${nactive} == ${max_active} ]]; then
+	    break
+	fi
+    done
+
+    echo "max active zones: ${max_active}" >> $seqres.full
+    $BLKZONE_PROG report ${SCRATCH_DEV} | grep oi | cat -n >> $seqres.full
+}
+
+workout() {
+    local func="$1"
+
+    _scratch_mkfs >/dev/null 2>&1
+    _scratch_mount
+
+    fill_active_zones
+    eval "$func"
+    local ret=$?
+
+    _scratch_unmount
+    _check_btrfs_filesystem ${SCRATCH_DEV}
+
+    return $ret
+}
+
+stress_data_bgs() {
+    # This dd fails with ENOSPC, which should not :(
+    dd if=/dev/zero of=${SCRATCH_MNT}/large bs=64M count=1 oflag=sync \
+       >>$seqres.full 2>&1
+}
+
+stress_data_bgs_2() {
+    # This dd fails with ENOSPC, which should not :(
+    dd if=/dev/zero of=${SCRATCH_MNT}/large bs=64M count=10 conv=fsync \
+       >>$seqres.full 2>&1 &
+    local pid1=$!
+
+    dd if=/dev/zero of=${SCRATCH_MNT}/large2 bs=64M count=10 conv=fsync \
+       >>$seqres.full 2>&1 &
+    local pid2=$!
+
+    wait $pid1; local ret1=$?
+    wait $pid2; local ret2=$?
+
+    if [ $ret1 -ne 0 -o $ret2 -ne 0 ]; then
+	return 1
+    fi
+    return 0
+}
+
+get_meta_bgs() {
+    $BTRFS_UTIL_PROG inspect-internal dump-tree -t EXTENT ${SCRATCH_DEV} |
+        grep BLOCK_GROUP -A 1 |grep -B1 'METADATA|' |
+        grep -oP '\(\d+ BLOCK_GROUP_ITEM \d+\)'
+}
+
+# This test case does not return the result because
+# _run_btrfs_util_prog will call _fail() in the error case anyway.
+stress_metadata_bgs() {
+    local metabgs=$(get_meta_bgs)
+    local count=0
+
+    while : ; do
+        _run_btrfs_util_prog subvolume snapshot ${SCRATCH_MNT} ${SCRATCH_MNT}/snap$i
+        _run_btrfs_util_prog filesystem sync ${SCRATCH_MNT}
+        cur_metabgs=$(get_meta_bgs)
+        if [[ "${cur_metabgs}" != "${metabgs}" ]]; then
+            break
+        fi
+        i=$((i + 1))
+    done
+}
+
+WORKS=(
+    stress_data_bgs
+    stress_data_bgs_2
+    stress_metadata_bgs
+)
+
+status=0
+for work in "${WORKS[@]}"; do
+    if ! workout "${work}"; then
+	echo "${work} failed"
+	status=1
+    fi
+done
+
+# success, all done
+if [ $status -eq 0 ]; then
+    echo "Silence is golden"
+fi
+exit
diff --git a/tests/btrfs/292.out b/tests/btrfs/292.out
new file mode 100644
index 000000000000..627309d3fbd2
--- /dev/null
+++ b/tests/btrfs/292.out
@@ -0,0 +1,2 @@
+QA output created by 292
+Silence is golden
-- 
2.37.3

Re: [PATCH 0/2] btrfs: test active zone tracking

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 1/2] common: introduce zone_capacity() to return a zone capacity

[Zorro Lang]

On Thu, Sep 22, 2022 at 02:54:58PM +0900, Naohiro Aota wrote:
> Introduce _zone_capacity() to return a zone capacity of the given address
> in the given device (optional). Rewrite btrfs/237 with it.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  common/zbd      | 17 +++++++++++++++++
>  tests/btrfs/237 |  8 ++------
>  2 files changed, 19 insertions(+), 6 deletions(-)
>  create mode 100644 common/zbd
> 
> diff --git a/common/zbd b/common/zbd
> new file mode 100644
> index 000000000000..329bb7be6b7b
> --- /dev/null
> +++ b/common/zbd

I don't like this abbreviation :-P If others don't open this file and read the
comment in it, they nearly no chance to guess what's this file for.

> @@ -0,0 +1,17 @@
> +#
> +# Common zoned block device specific functions
> +#
> +
> +. common/filter
> +
> +_zone_capacity() {
> +    local phy=$1
> +    local dev=$2
> +
> +    [ -z "$dev" ] && dev=$SCRATCH_DEV
> +
> +    size=$($BLKZONE_PROG report -o $phy -l 1 $dev |\
> +	       _filter_blkzone_report |\
> +	       grep -Po "cap 0x[[:xdigit:]]+" | cut -d ' ' -f 2)
> +    echo $((size << 9))
> +}

Do you have more zone related helpers are going to provide? If only this
single one helper, I think it's not worth adding a new common file. You
can put it into common/rc simply, as there're already several *zone*
related helpers in it.

We can pick up these *zone* related function into a separate include file in
one day we feel it's time (too many related helpers, and better to be maintained
individually).

Thanks,
Zorro

> diff --git a/tests/btrfs/237 b/tests/btrfs/237
> index bc6522e2200a..101094b5ce70 100755
> --- a/tests/btrfs/237
> +++ b/tests/btrfs/237
> @@ -13,7 +13,7 @@
>  _begin_fstest auto quick zone balance
>  
>  # Import common functions.
> -. ./common/filter
> +. ./common/zbd
>  
>  # real QA test starts here
>  
> @@ -56,11 +56,7 @@ fi
>  
>  start_data_bg_phy=$(get_data_bg_physical)
>  start_data_bg_phy=$((start_data_bg_phy >> 9))
> -
> -size=$($BLKZONE_PROG report -o $start_data_bg_phy -l 1 $SCRATCH_DEV |\
> -	_filter_blkzone_report |\
> -	grep -Po "cap 0x[[:xdigit:]]+" | cut -d ' ' -f 2)
> -size=$((size << 9))
> +size=$(_zone_capacity $start_data_bg_phy)
>  
>  reclaim_threshold=75
>  echo $reclaim_threshold > /sys/fs/btrfs/"$uuid"/bg_reclaim_threshold
> -- 
> 2.37.3
> 

Re: [PATCH 2/2] btrfs: test active zone tracking

[Zorro Lang]

On Thu, Sep 22, 2022 at 02:54:59PM +0900, Naohiro Aota wrote:
> A ZNS device limits the number of active zones, which is the number of
> zones can be written at the same time. To deal with the limit, btrfs's
> zoned mode tracks which zone (corresponds to a block group on the SINGLE
> profile) is active, and finish a zone if necessary.
> 
> This test checks if the active zone tracking and the finishing of zones
> works properly. First, it fills <number of max active zones> zones
> mostly. And, run some data/metadata stress workload to force btrfs to use a
> new zone.
> 
> This test fails on an older kernel (e.g, 5.18.2) like below.
> 
> btrfs/292
> [failed, exit status 1]- output mismatch (see /host/btrfs/292.out.bad)
>     --- tests/btrfs/292.out     2022-09-15 07:52:18.000000000 +0000
>     +++ /host/btrfs/292.out.bad 2022-09-15 07:59:14.290967793 +0000
>     @@ -1,2 +1,5 @@
>      QA output created by 292
>     -Silence is golden
>     +stress_data_bgs failed
>     +stress_data_bgs_2 failed
>     +failed: '/bin/btrfs subvolume snapshot /mnt/scratch /mnt/scratch/snap825'
>     +(see /host/btrfs/292.full for details)
>     ...
>     (Run 'diff -u /var/lib/xfstests/tests/btrfs/292.out /host/btrfs/292.out.bad'  to see the entire diff)
> 
> The failure is fixed with a series "btrfs: zoned: fix active zone tracking
> issues" [1] (upstream commits from 65ea1b66482f ("block: add bdev_max_segments()
> helper") to 2ce543f47843 ("btrfs: zoned: wait until zone is finished when
> allocation didn't progress")).
> 
> [1] https://lore.kernel.org/linux-btrfs/cover.1657321126.git.naohiro.aota@wdc.com/
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  common/zbd          |  12 ++++
>  tests/btrfs/292     | 137 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/292.out |   2 +
>  3 files changed, 151 insertions(+)
>  create mode 100755 tests/btrfs/292
>  create mode 100644 tests/btrfs/292.out
> 
> diff --git a/common/zbd b/common/zbd
> index 329bb7be6b7b..19a59fd27e69 100644
> --- a/common/zbd
> +++ b/common/zbd
> @@ -2,8 +2,20 @@
>  # Common zoned block device specific functions
>  #
>  
> +. common/rc

Did hit anything wrong if you don't import this file here? I think generally
we don't need to import common/rc in other common files (except common/preamble)
, due to it's always be imported at the beginning of each test cases.

>  . common/filter

As I said in patch 1/2, if you'd like to create this separated common file
about zone device, are you willing to move those extant zone related helpers
into this file, and change related cases to turn to import this file? I don't
have objection if you'd like to organize that.

Thanks,
Zorro

>  
> +_require_limited_active_zones() {
> +    local dev=$1
> +    local sysfs=$(_sysfs_dev ${dev})
> +    local attr="${sysfs}/queue/max_active_zones"
> +
> +    [ -e "${attr}" ] || _notrun "cannot find queue/max_active_zones. Maybe non-zoned device?"
> +    if [ $(cat "${attr}") == 0 ]; then
> +	_notrun "this test requires limited active zones"
> +    fi
> +}
> +
>  _zone_capacity() {
>      local phy=$1
>      local dev=$2
> diff --git a/tests/btrfs/292 b/tests/btrfs/292
> new file mode 100755
> index 000000000000..30c027c4ba5f
> --- /dev/null
> +++ b/tests/btrfs/292
> @@ -0,0 +1,137 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Western Digital Corporation.  All Rights Reserved.
> +#
> +# FS QA Test 292
> +#
> +# Test that an active zone is properly reclaimed to allow the further
> +# allocations, even if the active zones are mostly filled.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick snapshot zone
> +
> +# Import common functions.
> +. ./common/btrfs
> +. ./common/zbd
> +
> +# real QA test starts here
> +
> +_supported_fs btrfs
> +_require_scratch
> +_require_zoned_device "$SCRATCH_DEV"
> +_require_limited_active_zones "$SCRATCH_DEV"
> +
> +_require_command "$BLKZONE_PROG" blkzone
> +_require_fio
> +_require_btrfs_command inspect-internal dump-tree
> +
> +# This test requires specific data space usage, skip if we have compression
> +# enabled.
> +_require_no_compress
> +
> +max_active=$(cat $(_sysfs_dev ${SCRATCH_DEV})/queue/max_active_zones)
> +
> +# Fill the zones leaving the last 1MB
> +fill_active_zones() {
> +    # Asuumes we have the same capacity between zones.
> +    local capacity=$(_zone_capacity 0)
> +    local fill_size=$((capacity - 1024 * 1024))
> +
> +    for x in $(seq ${max_active}); do
> +	dd if=/dev/zero of=${SCRATCH_MNT}/fill$(printf "%02d" $x) bs=${fill_size} \
> +	   count=1 oflag=direct 2>/dev/null
> +	$BTRFS_UTIL_PROG filesystem sync ${SCRATCH_MNT}
> +
> +	local nactive=$($BLKZONE_PROG report ${SCRATCH_DEV} | grep oi | wc -l)
> +	if [[ ${nactive} == ${max_active} ]]; then
> +	    break
> +	fi
> +    done
> +
> +    echo "max active zones: ${max_active}" >> $seqres.full
> +    $BLKZONE_PROG report ${SCRATCH_DEV} | grep oi | cat -n >> $seqres.full
> +}
> +
> +workout() {
> +    local func="$1"
> +
> +    _scratch_mkfs >/dev/null 2>&1
> +    _scratch_mount
> +
> +    fill_active_zones
> +    eval "$func"
> +    local ret=$?
> +
> +    _scratch_unmount
> +    _check_btrfs_filesystem ${SCRATCH_DEV}
> +
> +    return $ret
> +}
> +
> +stress_data_bgs() {
> +    # This dd fails with ENOSPC, which should not :(
> +    dd if=/dev/zero of=${SCRATCH_MNT}/large bs=64M count=1 oflag=sync \
> +       >>$seqres.full 2>&1
> +}
> +
> +stress_data_bgs_2() {
> +    # This dd fails with ENOSPC, which should not :(
> +    dd if=/dev/zero of=${SCRATCH_MNT}/large bs=64M count=10 conv=fsync \
> +       >>$seqres.full 2>&1 &
> +    local pid1=$!
> +
> +    dd if=/dev/zero of=${SCRATCH_MNT}/large2 bs=64M count=10 conv=fsync \
> +       >>$seqres.full 2>&1 &
> +    local pid2=$!
> +
> +    wait $pid1; local ret1=$?
> +    wait $pid2; local ret2=$?
> +
> +    if [ $ret1 -ne 0 -o $ret2 -ne 0 ]; then
> +	return 1
> +    fi
> +    return 0
> +}
> +
> +get_meta_bgs() {
> +    $BTRFS_UTIL_PROG inspect-internal dump-tree -t EXTENT ${SCRATCH_DEV} |
> +        grep BLOCK_GROUP -A 1 |grep -B1 'METADATA|' |
> +        grep -oP '\(\d+ BLOCK_GROUP_ITEM \d+\)'
> +}
> +
> +# This test case does not return the result because
> +# _run_btrfs_util_prog will call _fail() in the error case anyway.
> +stress_metadata_bgs() {
> +    local metabgs=$(get_meta_bgs)
> +    local count=0
> +
> +    while : ; do
> +        _run_btrfs_util_prog subvolume snapshot ${SCRATCH_MNT} ${SCRATCH_MNT}/snap$i
> +        _run_btrfs_util_prog filesystem sync ${SCRATCH_MNT}
> +        cur_metabgs=$(get_meta_bgs)
> +        if [[ "${cur_metabgs}" != "${metabgs}" ]]; then
> +            break
> +        fi
> +        i=$((i + 1))
> +    done
> +}
> +
> +WORKS=(
> +    stress_data_bgs
> +    stress_data_bgs_2
> +    stress_metadata_bgs
> +)
> +
> +status=0
> +for work in "${WORKS[@]}"; do
> +    if ! workout "${work}"; then
> +	echo "${work} failed"
> +	status=1
> +    fi
> +done
> +
> +# success, all done
> +if [ $status -eq 0 ]; then
> +    echo "Silence is golden"
> +fi
> +exit
> diff --git a/tests/btrfs/292.out b/tests/btrfs/292.out
> new file mode 100644
> index 000000000000..627309d3fbd2
> --- /dev/null
> +++ b/tests/btrfs/292.out
> @@ -0,0 +1,2 @@
> +QA output created by 292
> +Silence is golden
> -- 
> 2.37.3
> 

Re: [PATCH 1/2] common: introduce zone_capacity() to return a zone capacity

[Johannes Thumshirn]

On 22.09.22 17:42, Zorro Lang wrote:
>> --- /dev/null
>> +++ b/common/zbd
> I don't like this abbreviation :-P If others don't open this file and read the
> comment in it, they nearly no chance to guess what's this file for.
> 

zbd is a well known abbreviation for zoned block devices. I think most
people in storage and filesystems know it.

Re: [PATCH 1/2] common: introduce zone_capacity() to return a zone capacity

[Zorro Lang]

On Fri, Sep 23, 2022 at 08:02:10AM +0000, Johannes Thumshirn wrote:
> On 22.09.22 17:42, Zorro Lang wrote:
> >> --- /dev/null
> >> +++ b/common/zbd
> > I don't like this abbreviation :-P If others don't open this file and read the
> > comment in it, they nearly no chance to guess what's this file for.
> > 
> 
> zbd is a well known abbreviation for zoned block devices. I think most
> people in storage and filesystems know it.

OK, but we haven't been that "a single character is worth a thousand
pieces of gold", so we can use a longer name, likes common/zone,
common/zoned, common/zoned_block, common/zoned_device or something likes
that. Anyway, that's just my personal opinion, if most of people prefer
using "common/zbd", I'm fine to have that :) 

But I hope you can move all zoned block device related helpers to the new
common file if you'd like to bring in this file, likes what Darrick did in:

commit 67afd5c742464607994316acb2c6e8303b8af4c5
Author: Darrick J. Wong <djwong@kernel.org>
Date:   Tue Aug 9 14:00:46 2022 -0700

    common/rc: move ext4-specific helpers into a separate common/ext4 file

Thanks,
Zorro

> 
> 

Re: [PATCH 1/2] common: introduce zone_capacity() to return a zone capacity

[Naohiro Aota]

On Fri, Sep 23, 2022 at 07:51:26PM +0800, Zorro Lang wrote:
> On Fri, Sep 23, 2022 at 08:02:10AM +0000, Johannes Thumshirn wrote:
> > On 22.09.22 17:42, Zorro Lang wrote:
> > >> --- /dev/null
> > >> +++ b/common/zbd
> > > I don't like this abbreviation :-P If others don't open this file and read the
> > > comment in it, they nearly no chance to guess what's this file for.
> > > 
> > 
> > zbd is a well known abbreviation for zoned block devices. I think most
> > people in storage and filesystems know it.
> 
> OK, but we haven't been that "a single character is worth a thousand
> pieces of gold", so we can use a longer name, likes common/zone,
> common/zoned, common/zoned_block, common/zoned_device or something likes
> that. Anyway, that's just my personal opinion, if most of people prefer
> using "common/zbd", I'm fine to have that :) 

Sure. I'll use "zoned" as it is more common in the kernel code.

> But I hope you can move all zoned block device related helpers to the new
> common file if you'd like to bring in this file, likes what Darrick did in:
> 
> commit 67afd5c742464607994316acb2c6e8303b8af4c5
> Author: Darrick J. Wong <djwong@kernel.org>
> Date:   Tue Aug 9 14:00:46 2022 -0700
> 
>     common/rc: move ext4-specific helpers into a separate common/ext4 file

Yes, that will be better to have things in common/zoned. I considered
moving zoned functions (_zone_type, _require_{,non_}zoned_device), but
_require_loop() and _require_dm_target() use _require_non_zoned_device() in
them. So, moving _require_non_zoned_device() will make a dependency from
common/rc to common/zoned, which I considered not much clean. How do you
think of it?

Moving _filter_blkzone_report() would be fine, though.

> Thanks,
> Zorro
> 
> > 
> > 
> 

Re: [PATCH 2/2] btrfs: test active zone tracking

[Naohiro Aota]

On Fri, Sep 23, 2022 at 12:03:31AM +0800, Zorro Lang wrote:
> On Thu, Sep 22, 2022 at 02:54:59PM +0900, Naohiro Aota wrote:
> > A ZNS device limits the number of active zones, which is the number of
> > zones can be written at the same time. To deal with the limit, btrfs's
> > zoned mode tracks which zone (corresponds to a block group on the SINGLE
> > profile) is active, and finish a zone if necessary.
> > 
> > This test checks if the active zone tracking and the finishing of zones
> > works properly. First, it fills <number of max active zones> zones
> > mostly. And, run some data/metadata stress workload to force btrfs to use a
> > new zone.
> > 
> > This test fails on an older kernel (e.g, 5.18.2) like below.
> > 
> > btrfs/292
> > [failed, exit status 1]- output mismatch (see /host/btrfs/292.out.bad)
> >     --- tests/btrfs/292.out     2022-09-15 07:52:18.000000000 +0000
> >     +++ /host/btrfs/292.out.bad 2022-09-15 07:59:14.290967793 +0000
> >     @@ -1,2 +1,5 @@
> >      QA output created by 292
> >     -Silence is golden
> >     +stress_data_bgs failed
> >     +stress_data_bgs_2 failed
> >     +failed: '/bin/btrfs subvolume snapshot /mnt/scratch /mnt/scratch/snap825'
> >     +(see /host/btrfs/292.full for details)
> >     ...
> >     (Run 'diff -u /var/lib/xfstests/tests/btrfs/292.out /host/btrfs/292.out.bad'  to see the entire diff)
> > 
> > The failure is fixed with a series "btrfs: zoned: fix active zone tracking
> > issues" [1] (upstream commits from 65ea1b66482f ("block: add bdev_max_segments()
> > helper") to 2ce543f47843 ("btrfs: zoned: wait until zone is finished when
> > allocation didn't progress")).
> > 
> > [1] https://lore.kernel.org/linux-btrfs/cover.1657321126.git.naohiro.aota@wdc.com/
> > 
> > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> > ---
> >  common/zbd          |  12 ++++
> >  tests/btrfs/292     | 137 ++++++++++++++++++++++++++++++++++++++++++++
> >  tests/btrfs/292.out |   2 +
> >  3 files changed, 151 insertions(+)
> >  create mode 100755 tests/btrfs/292
> >  create mode 100644 tests/btrfs/292.out
> > 
> > diff --git a/common/zbd b/common/zbd
> > index 329bb7be6b7b..19a59fd27e69 100644
> > --- a/common/zbd
> > +++ b/common/zbd
> > @@ -2,8 +2,20 @@
> >  # Common zoned block device specific functions
> >  #
> >  
> > +. common/rc
> 
> Did hit anything wrong if you don't import this file here? I think generally
> we don't need to import common/rc in other common files (except common/preamble)
> , due to it's always be imported at the beginning of each test cases.

Thank you for the comment. I'll check removing it affects the test.

> >  . common/filter
> 
> As I said in patch 1/2, if you'd like to create this separated common file
> about zone device, are you willing to move those extant zone related helpers
> into this file, and change related cases to turn to import this file? I don't
> have objection if you'd like to organize that.

Yeah, moving _filter_blkzone_report() would be fine and clean. I'll at
least move it to here.

> Thanks,
> Zorro
> 
> >  
> > +_require_limited_active_zones() {
> > +    local dev=$1
> > +    local sysfs=$(_sysfs_dev ${dev})
> > +    local attr="${sysfs}/queue/max_active_zones"
> > +
> > +    [ -e "${attr}" ] || _notrun "cannot find queue/max_active_zones. Maybe non-zoned device?"
> > +    if [ $(cat "${attr}") == 0 ]; then
> > +	_notrun "this test requires limited active zones"
> > +    fi
> > +}
> > +
> >  _zone_capacity() {
> >      local phy=$1
> >      local dev=$2
> > diff --git a/tests/btrfs/292 b/tests/btrfs/292
> > new file mode 100755
> > index 000000000000..30c027c4ba5f
> > --- /dev/null
> > +++ b/tests/btrfs/292
> > @@ -0,0 +1,137 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 Western Digital Corporation.  All Rights Reserved.
> > +#
> > +# FS QA Test 292
> > +#
> > +# Test that an active zone is properly reclaimed to allow the further
> > +# allocations, even if the active zones are mostly filled.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick snapshot zone
> > +
> > +# Import common functions.
> > +. ./common/btrfs
> > +. ./common/zbd
> > +
> > +# real QA test starts here
> > +
> > +_supported_fs btrfs
> > +_require_scratch
> > +_require_zoned_device "$SCRATCH_DEV"
> > +_require_limited_active_zones "$SCRATCH_DEV"
> > +
> > +_require_command "$BLKZONE_PROG" blkzone
> > +_require_fio
> > +_require_btrfs_command inspect-internal dump-tree
> > +
> > +# This test requires specific data space usage, skip if we have compression
> > +# enabled.
> > +_require_no_compress
> > +
> > +max_active=$(cat $(_sysfs_dev ${SCRATCH_DEV})/queue/max_active_zones)
> > +
> > +# Fill the zones leaving the last 1MB
> > +fill_active_zones() {
> > +    # Asuumes we have the same capacity between zones.
> > +    local capacity=$(_zone_capacity 0)
> > +    local fill_size=$((capacity - 1024 * 1024))
> > +
> > +    for x in $(seq ${max_active}); do
> > +	dd if=/dev/zero of=${SCRATCH_MNT}/fill$(printf "%02d" $x) bs=${fill_size} \
> > +	   count=1 oflag=direct 2>/dev/null
> > +	$BTRFS_UTIL_PROG filesystem sync ${SCRATCH_MNT}
> > +
> > +	local nactive=$($BLKZONE_PROG report ${SCRATCH_DEV} | grep oi | wc -l)
> > +	if [[ ${nactive} == ${max_active} ]]; then
> > +	    break
> > +	fi
> > +    done
> > +
> > +    echo "max active zones: ${max_active}" >> $seqres.full
> > +    $BLKZONE_PROG report ${SCRATCH_DEV} | grep oi | cat -n >> $seqres.full
> > +}
> > +
> > +workout() {
> > +    local func="$1"
> > +
> > +    _scratch_mkfs >/dev/null 2>&1
> > +    _scratch_mount
> > +
> > +    fill_active_zones
> > +    eval "$func"
> > +    local ret=$?
> > +
> > +    _scratch_unmount
> > +    _check_btrfs_filesystem ${SCRATCH_DEV}
> > +
> > +    return $ret
> > +}
> > +
> > +stress_data_bgs() {
> > +    # This dd fails with ENOSPC, which should not :(
> > +    dd if=/dev/zero of=${SCRATCH_MNT}/large bs=64M count=1 oflag=sync \
> > +       >>$seqres.full 2>&1
> > +}
> > +
> > +stress_data_bgs_2() {
> > +    # This dd fails with ENOSPC, which should not :(
> > +    dd if=/dev/zero of=${SCRATCH_MNT}/large bs=64M count=10 conv=fsync \
> > +       >>$seqres.full 2>&1 &
> > +    local pid1=$!
> > +
> > +    dd if=/dev/zero of=${SCRATCH_MNT}/large2 bs=64M count=10 conv=fsync \
> > +       >>$seqres.full 2>&1 &
> > +    local pid2=$!
> > +
> > +    wait $pid1; local ret1=$?
> > +    wait $pid2; local ret2=$?
> > +
> > +    if [ $ret1 -ne 0 -o $ret2 -ne 0 ]; then
> > +	return 1
> > +    fi
> > +    return 0
> > +}
> > +
> > +get_meta_bgs() {
> > +    $BTRFS_UTIL_PROG inspect-internal dump-tree -t EXTENT ${SCRATCH_DEV} |
> > +        grep BLOCK_GROUP -A 1 |grep -B1 'METADATA|' |
> > +        grep -oP '\(\d+ BLOCK_GROUP_ITEM \d+\)'
> > +}
> > +
> > +# This test case does not return the result because
> > +# _run_btrfs_util_prog will call _fail() in the error case anyway.
> > +stress_metadata_bgs() {
> > +    local metabgs=$(get_meta_bgs)
> > +    local count=0
> > +
> > +    while : ; do
> > +        _run_btrfs_util_prog subvolume snapshot ${SCRATCH_MNT} ${SCRATCH_MNT}/snap$i
> > +        _run_btrfs_util_prog filesystem sync ${SCRATCH_MNT}
> > +        cur_metabgs=$(get_meta_bgs)
> > +        if [[ "${cur_metabgs}" != "${metabgs}" ]]; then
> > +            break
> > +        fi
> > +        i=$((i + 1))
> > +    done
> > +}
> > +
> > +WORKS=(
> > +    stress_data_bgs
> > +    stress_data_bgs_2
> > +    stress_metadata_bgs
> > +)
> > +
> > +status=0
> > +for work in "${WORKS[@]}"; do
> > +    if ! workout "${work}"; then
> > +	echo "${work} failed"
> > +	status=1
> > +    fi
> > +done
> > +
> > +# success, all done
> > +if [ $status -eq 0 ]; then
> > +    echo "Silence is golden"
> > +fi
> > +exit
> > diff --git a/tests/btrfs/292.out b/tests/btrfs/292.out
> > new file mode 100644
> > index 000000000000..627309d3fbd2
> > --- /dev/null
> > +++ b/tests/btrfs/292.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 292
> > +Silence is golden
> > -- 
> > 2.37.3
> > 
> 

Re: [PATCH 1/2] common: introduce zone_capacity() to return a zone capacity

[Zorro Lang]

On Wed, Sep 28, 2022 at 03:57:08AM +0000, Naohiro Aota wrote:
> On Fri, Sep 23, 2022 at 07:51:26PM +0800, Zorro Lang wrote:
> > On Fri, Sep 23, 2022 at 08:02:10AM +0000, Johannes Thumshirn wrote:
> > > On 22.09.22 17:42, Zorro Lang wrote:
> > > >> --- /dev/null
> > > >> +++ b/common/zbd
> > > > I don't like this abbreviation :-P If others don't open this file and read the
> > > > comment in it, they nearly no chance to guess what's this file for.
> > > > 
> > > 
> > > zbd is a well known abbreviation for zoned block devices. I think most
> > > people in storage and filesystems know it.
> > 
> > OK, but we haven't been that "a single character is worth a thousand
> > pieces of gold", so we can use a longer name, likes common/zone,
> > common/zoned, common/zoned_block, common/zoned_device or something likes
> > that. Anyway, that's just my personal opinion, if most of people prefer
> > using "common/zbd", I'm fine to have that :) 
> 
> Sure. I'll use "zoned" as it is more common in the kernel code.
> 
> > But I hope you can move all zoned block device related helpers to the new
> > common file if you'd like to bring in this file, likes what Darrick did in:
> > 
> > commit 67afd5c742464607994316acb2c6e8303b8af4c5
> > Author: Darrick J. Wong <djwong@kernel.org>
> > Date:   Tue Aug 9 14:00:46 2022 -0700
> > 
> >     common/rc: move ext4-specific helpers into a separate common/ext4 file
> 
> Yes, that will be better to have things in common/zoned. I considered
> moving zoned functions (_zone_type, _require_{,non_}zoned_device), but
> _require_loop() and _require_dm_target() use _require_non_zoned_device() in
> them. So, moving _require_non_zoned_device() will make a dependency from
> common/rc to common/zoned, which I considered not much clean. How do you
> think of it?

Oh, below commit [1] brought in the coupling of common/rc and zoned helpers.
Hmm... that cause all the 3 helpers (_zone_type, _require_{,non_}zoned_device)
have to be in common/rc or be imported in common/rc. Looks like we have to
keep them in common/rc, except we make a bigger refactor to common/rc, or you'd
like to make your 2 new helpers in common/rc too (likes these 3 old ones:)

BTW I doubt if we might need to use more zoned related helpers in common/rc, due
to we deal with test devices in common/rc mostly, likes dax. Someone might want
a seperated common/dax or common/pmem file one day. The common/rc imports
specific fs helpers according to $FSTYP (common/config: _source_specific_fs()).
If we need to deal with different kind of device types in common/rc one day, is
there a better idea to determine which one should be imported? Welcome any
suggestions if anyone has :)

[1]
commit 952310a57d9323ae0bb174b50be93107a8895e0c
Author: Naohiro Aota <naohiro.aota@wdc.com>
Date:   Mon Aug 16 20:35:08 2021 +0900

    common: add zoned block device checks

> 
> Moving _filter_blkzone_report() would be fine, though.

Yeah, moving this is fine.

Thanks,
Zorro

> 
> > Thanks,
> > Zorro
> > 
> > > 
> > > 
> > 
> 

Re: [PATCH 1/2] common: introduce zone_capacity() to return a zone capacity

[Darrick J. Wong]

On Wed, Sep 28, 2022 at 01:47:15PM +0800, Zorro Lang wrote:
> On Wed, Sep 28, 2022 at 03:57:08AM +0000, Naohiro Aota wrote:
> > On Fri, Sep 23, 2022 at 07:51:26PM +0800, Zorro Lang wrote:
> > > On Fri, Sep 23, 2022 at 08:02:10AM +0000, Johannes Thumshirn wrote:
> > > > On 22.09.22 17:42, Zorro Lang wrote:
> > > > >> --- /dev/null
> > > > >> +++ b/common/zbd
> > > > > I don't like this abbreviation :-P If others don't open this file and read the
> > > > > comment in it, they nearly no chance to guess what's this file for.
> > > > > 
> > > > 
> > > > zbd is a well known abbreviation for zoned block devices. I think most
> > > > people in storage and filesystems know it.
> > > 
> > > OK, but we haven't been that "a single character is worth a thousand
> > > pieces of gold", so we can use a longer name, likes common/zone,
> > > common/zoned, common/zoned_block, common/zoned_device or something likes
> > > that. Anyway, that's just my personal opinion, if most of people prefer
> > > using "common/zbd", I'm fine to have that :) 
> > 
> > Sure. I'll use "zoned" as it is more common in the kernel code.
> > 
> > > But I hope you can move all zoned block device related helpers to the new
> > > common file if you'd like to bring in this file, likes what Darrick did in:
> > > 
> > > commit 67afd5c742464607994316acb2c6e8303b8af4c5
> > > Author: Darrick J. Wong <djwong@kernel.org>
> > > Date:   Tue Aug 9 14:00:46 2022 -0700
> > > 
> > >     common/rc: move ext4-specific helpers into a separate common/ext4 file
> > 
> > Yes, that will be better to have things in common/zoned. I considered
> > moving zoned functions (_zone_type, _require_{,non_}zoned_device), but
> > _require_loop() and _require_dm_target() use _require_non_zoned_device() in
> > them. So, moving _require_non_zoned_device() will make a dependency from
> > common/rc to common/zoned, which I considered not much clean. How do you
> > think of it?
> 
> Oh, below commit [1] brought in the coupling of common/rc and zoned helpers.
> Hmm... that cause all the 3 helpers (_zone_type, _require_{,non_}zoned_device)
> have to be in common/rc or be imported in common/rc. Looks like we have to
> keep them in common/rc, except we make a bigger refactor to common/rc, or you'd
> like to make your 2 new helpers in common/rc too (likes these 3 old ones:)
> 
> BTW I doubt if we might need to use more zoned related helpers in common/rc, due
> to we deal with test devices in common/rc mostly, likes dax. Someone might want
> a seperated common/dax or common/pmem file one day. The common/rc imports
> specific fs helpers according to $FSTYP (common/config: _source_specific_fs()).
> If we need to deal with different kind of device types in common/rc one day, is
> there a better idea to determine which one should be imported? Welcome any
> suggestions if anyone has :)

Leave those three in common/rc and put/move the rest to common/zoned ?

I think it's fine for common/rc to have helpers that *detect* the
presence of a blockdev feature, and require tests to source
common/$feature if they want to do anything clever with that feature.
After all, the _require_non_zoned_device tests don't care about
_zone_capacity, right?

--D

> [1]
> commit 952310a57d9323ae0bb174b50be93107a8895e0c
> Author: Naohiro Aota <naohiro.aota@wdc.com>
> Date:   Mon Aug 16 20:35:08 2021 +0900
> 
>     common: add zoned block device checks
> 
> > 
> > Moving _filter_blkzone_report() would be fine, though.
> 
> Yeah, moving this is fine.
> 
> Thanks,
> Zorro
> 
> > 
> > > Thanks,
> > > Zorro
> > > 
> > > > 
> > > > 
> > > 
> > 
> 

Re: [PATCH 1/2] common: introduce zone_capacity() to return a zone capacity

[Naohiro Aota]

On Wed, Sep 28, 2022 at 08:32:47AM -0700, Darrick J. Wong wrote:
> On Wed, Sep 28, 2022 at 01:47:15PM +0800, Zorro Lang wrote:
> > On Wed, Sep 28, 2022 at 03:57:08AM +0000, Naohiro Aota wrote:
> > > On Fri, Sep 23, 2022 at 07:51:26PM +0800, Zorro Lang wrote:
> > > > On Fri, Sep 23, 2022 at 08:02:10AM +0000, Johannes Thumshirn wrote:
> > > > > On 22.09.22 17:42, Zorro Lang wrote:
> > > > > >> --- /dev/null
> > > > > >> +++ b/common/zbd
> > > > > > I don't like this abbreviation :-P If others don't open this file and read the
> > > > > > comment in it, they nearly no chance to guess what's this file for.
> > > > > > 
> > > > > 
> > > > > zbd is a well known abbreviation for zoned block devices. I think most
> > > > > people in storage and filesystems know it.
> > > > 
> > > > OK, but we haven't been that "a single character is worth a thousand
> > > > pieces of gold", so we can use a longer name, likes common/zone,
> > > > common/zoned, common/zoned_block, common/zoned_device or something likes
> > > > that. Anyway, that's just my personal opinion, if most of people prefer
> > > > using "common/zbd", I'm fine to have that :) 
> > > 
> > > Sure. I'll use "zoned" as it is more common in the kernel code.
> > > 
> > > > But I hope you can move all zoned block device related helpers to the new
> > > > common file if you'd like to bring in this file, likes what Darrick did in:
> > > > 
> > > > commit 67afd5c742464607994316acb2c6e8303b8af4c5
> > > > Author: Darrick J. Wong <djwong@kernel.org>
> > > > Date:   Tue Aug 9 14:00:46 2022 -0700
> > > > 
> > > >     common/rc: move ext4-specific helpers into a separate common/ext4 file
> > > 
> > > Yes, that will be better to have things in common/zoned. I considered
> > > moving zoned functions (_zone_type, _require_{,non_}zoned_device), but
> > > _require_loop() and _require_dm_target() use _require_non_zoned_device() in
> > > them. So, moving _require_non_zoned_device() will make a dependency from
> > > common/rc to common/zoned, which I considered not much clean. How do you
> > > think of it?
> > 
> > Oh, below commit [1] brought in the coupling of common/rc and zoned helpers.
> > Hmm... that cause all the 3 helpers (_zone_type, _require_{,non_}zoned_device)
> > have to be in common/rc or be imported in common/rc. Looks like we have to
> > keep them in common/rc, except we make a bigger refactor to common/rc, or you'd
> > like to make your 2 new helpers in common/rc too (likes these 3 old ones:)
> > 
> > BTW I doubt if we might need to use more zoned related helpers in common/rc, due
> > to we deal with test devices in common/rc mostly, likes dax. Someone might want
> > a seperated common/dax or common/pmem file one day. The common/rc imports
> > specific fs helpers according to $FSTYP (common/config: _source_specific_fs()).
> > If we need to deal with different kind of device types in common/rc one day, is
> > there a better idea to determine which one should be imported? Welcome any
> > suggestions if anyone has :)
> 
> Leave those three in common/rc and put/move the rest to common/zoned ?
> 
> I think it's fine for common/rc to have helpers that *detect* the
> presence of a blockdev feature, and require tests to source
> common/$feature if they want to do anything clever with that feature.
> After all, the _require_non_zoned_device tests don't care about
> _zone_capacity, right?

Thank you for your suggestions. Yes, _require_non_zoned_device() just read
/sys/block/${sdev}/queue/zoned. I'll do so to leave the three functions in
common/rc and move/create other helpers in common/zoned.

Thanks,

> --D
> 
> > [1]
> > commit 952310a57d9323ae0bb174b50be93107a8895e0c
> > Author: Naohiro Aota <naohiro.aota@wdc.com>
> > Date:   Mon Aug 16 20:35:08 2021 +0900
> > 
> >     common: add zoned block device checks
> > 
> > > 
> > > Moving _filter_blkzone_report() would be fine, though.
> > 
> > Yeah, moving this is fine.
> > 
> > Thanks,
> > Zorro
> > 
> > > 
> > > > Thanks,
> > > > Zorro
> > > > 
> > > > > 
> > > > > 
> > > > 
> > > 
> >