All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] btrfs: test active zone tracking
@ 2022-09-29  4:19 Naohiro Aota
  2022-09-29  4:19 ` [PATCH v2 1/2] common: introduce zone_capacity() to return a zone capacity Naohiro Aota
  2022-09-29  4:19 ` [PATCH v2 2/2] btrfs: test active zone tracking Naohiro Aota
  0 siblings, 2 replies; 10+ messages in thread
From: Naohiro Aota @ 2022-09-29  4:19 UTC (permalink / raw)
  To: fstests; +Cc: linux-btrfs, 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.

Changes:
- v2:
  - Rename to common/zoned.
  - Move _filter_blkzone_report() as well to common/zoned.
  - Drop _require_fio as it was already unnecessary.

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

 common/filter       |  13 -----
 common/zoned        |  39 +++++++++++++
 tests/btrfs/237     |   8 +--
 tests/btrfs/292     | 136 ++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/292.out |   2 +
 5 files changed, 179 insertions(+), 19 deletions(-)
 create mode 100644 common/zoned
 create mode 100755 tests/btrfs/292
 create mode 100644 tests/btrfs/292.out

-- 
2.37.3


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] common: introduce zone_capacity() to return a zone capacity
  2022-09-29  4:19 [PATCH v2 0/2] btrfs: test active zone tracking Naohiro Aota
@ 2022-09-29  4:19 ` Naohiro Aota
  2022-09-29  5:15   ` Zorro Lang
  2022-09-29  4:19 ` [PATCH v2 2/2] btrfs: test active zone tracking Naohiro Aota
  1 sibling, 1 reply; 10+ messages in thread
From: Naohiro Aota @ 2022-09-29  4:19 UTC (permalink / raw)
  To: fstests; +Cc: linux-btrfs, Naohiro Aota

Introduce _zone_capacity() to return a zone capacity of the given address
in the given device (optional). Move _filter_blkzone_report() for it, and
rewrite btrfs/237 with it.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 common/filter   | 13 -------------
 common/zoned    | 28 ++++++++++++++++++++++++++++
 tests/btrfs/237 |  8 ++------
 3 files changed, 30 insertions(+), 19 deletions(-)
 create mode 100644 common/zoned

diff --git a/common/filter b/common/filter
index 28dea64662dc..ac5c93422567 100644
--- a/common/filter
+++ b/common/filter
@@ -651,18 +651,5 @@ _filter_bash()
 	sed -e "s/^bash: line 1: /bash: /"
 }
 
-#
-# blkzone report added zone capacity to be printed from v2.37.
-# This filter will add an extra column 'cap' with the same value of
-# 'len'(zone size) for blkzone version < 2.37
-#
-# Before: start: 0x000100000, len 0x040000, wptr 0x000000 ..
-# After: start: 0x000100000, len 0x040000, cap 0x040000, wptr 0x000000 ..
-_filter_blkzone_report()
-{
-	$AWK_PROG -F "," 'BEGIN{OFS=",";} $3 !~ /cap/ {$2=$2","$2;} {print;}' |\
-	sed -e 's/len/cap/2'
-}
-
 # make sure this script returns success
 /bin/true
diff --git a/common/zoned b/common/zoned
new file mode 100644
index 000000000000..d1bc60f784a1
--- /dev/null
+++ b/common/zoned
@@ -0,0 +1,28 @@
+#
+# Common zoned block device specific functions
+#
+
+#
+# blkzone report added zone capacity to be printed from v2.37.
+# This filter will add an extra column 'cap' with the same value of
+# 'len'(zone size) for blkzone version < 2.37
+#
+# Before: start: 0x000100000, len 0x040000, wptr 0x000000 ..
+# After: start: 0x000100000, len 0x040000, cap 0x040000, wptr 0x000000 ..
+_filter_blkzone_report()
+{
+	$AWK_PROG -F "," 'BEGIN{OFS=",";} $3 !~ /cap/ {$2=$2","$2;} {print;}' |\
+	sed -e 's/len/cap/2'
+}
+
+_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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] btrfs: test active zone tracking
  2022-09-29  4:19 [PATCH v2 0/2] btrfs: test active zone tracking Naohiro Aota
  2022-09-29  4:19 ` [PATCH v2 1/2] common: introduce zone_capacity() to return a zone capacity Naohiro Aota
@ 2022-09-29  4:19 ` Naohiro Aota
  2022-09-29  6:01   ` Zorro Lang
  1 sibling, 1 reply; 10+ messages in thread
From: Naohiro Aota @ 2022-09-29  4:19 UTC (permalink / raw)
  To: fstests; +Cc: linux-btrfs, 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/zoned        |  11 ++++
 tests/btrfs/292     | 136 ++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/292.out |   2 +
 3 files changed, 149 insertions(+)
 create mode 100755 tests/btrfs/292
 create mode 100644 tests/btrfs/292.out

diff --git a/common/zoned b/common/zoned
index d1bc60f784a1..eed0082a15cf 100644
--- a/common/zoned
+++ b/common/zoned
@@ -15,6 +15,17 @@ _filter_blkzone_report()
 	sed -e 's/len/cap/2'
 }
 
+_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..6cfd6b18c299
--- /dev/null
+++ b/tests/btrfs/292
@@ -0,0 +1,136 @@
+#! /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/zoned
+
+# 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_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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] common: introduce zone_capacity() to return a zone capacity
  2022-09-29  4:19 ` [PATCH v2 1/2] common: introduce zone_capacity() to return a zone capacity Naohiro Aota
@ 2022-09-29  5:15   ` Zorro Lang
  2022-09-30  6:19     ` Naohiro Aota
  0 siblings, 1 reply; 10+ messages in thread
From: Zorro Lang @ 2022-09-29  5:15 UTC (permalink / raw)
  To: Naohiro Aota; +Cc: fstests, linux-btrfs

On Thu, Sep 29, 2022 at 01:19:24PM +0900, Naohiro Aota wrote:
> Introduce _zone_capacity() to return a zone capacity of the given address
> in the given device (optional). Move _filter_blkzone_report() for it, and
> rewrite btrfs/237 with it.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  common/filter   | 13 -------------
>  common/zoned    | 28 ++++++++++++++++++++++++++++
>  tests/btrfs/237 |  8 ++------
>  3 files changed, 30 insertions(+), 19 deletions(-)
>  create mode 100644 common/zoned
> 
> diff --git a/common/filter b/common/filter
> index 28dea64662dc..ac5c93422567 100644
> --- a/common/filter
> +++ b/common/filter
> @@ -651,18 +651,5 @@ _filter_bash()
>  	sed -e "s/^bash: line 1: /bash: /"
>  }
>  
> -#
> -# blkzone report added zone capacity to be printed from v2.37.
> -# This filter will add an extra column 'cap' with the same value of
> -# 'len'(zone size) for blkzone version < 2.37
> -#
> -# Before: start: 0x000100000, len 0x040000, wptr 0x000000 ..
> -# After: start: 0x000100000, len 0x040000, cap 0x040000, wptr 0x000000 ..
> -_filter_blkzone_report()
> -{
> -	$AWK_PROG -F "," 'BEGIN{OFS=",";} $3 !~ /cap/ {$2=$2","$2;} {print;}' |\
> -	sed -e 's/len/cap/2'
> -}
> -
>  # make sure this script returns success
>  /bin/true
> diff --git a/common/zoned b/common/zoned
> new file mode 100644
> index 000000000000..d1bc60f784a1
> --- /dev/null
> +++ b/common/zoned
> @@ -0,0 +1,28 @@
> +#
> +# Common zoned block device specific functions
> +#
> +
> +#
> +# blkzone report added zone capacity to be printed from v2.37.
> +# This filter will add an extra column 'cap' with the same value of
> +# 'len'(zone size) for blkzone version < 2.37
> +#
> +# Before: start: 0x000100000, len 0x040000, wptr 0x000000 ..
> +# After: start: 0x000100000, len 0x040000, cap 0x040000, wptr 0x000000 ..
> +_filter_blkzone_report()
> +{
> +	$AWK_PROG -F "," 'BEGIN{OFS=",";} $3 !~ /cap/ {$2=$2","$2;} {print;}' |\
> +	sed -e 's/len/cap/2'
> +}
> +
> +_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

I'm a little surprised this line doesn't report error :) Anyway, it should be
common/zoned as above. Others look good to me. With this fix, you can add:

Reviewed-by: Zorro Lang <zlang@redhat.com>

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] btrfs: test active zone tracking
  2022-09-29  4:19 ` [PATCH v2 2/2] btrfs: test active zone tracking Naohiro Aota
@ 2022-09-29  6:01   ` Zorro Lang
  2022-09-29 12:04     ` David Sterba
  2022-09-30  6:29     ` Naohiro Aota
  0 siblings, 2 replies; 10+ messages in thread
From: Zorro Lang @ 2022-09-29  6:01 UTC (permalink / raw)
  To: Naohiro Aota; +Cc: fstests, linux-btrfs

On Thu, Sep 29, 2022 at 01:19:25PM +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")).

If this's a regression test case for known fix, we'd better to use:
_fixed_by_kernel_commit 65ea1b66482f block: add bdev_max_segments (patchset)

to remind downstream testers about that known issue.

> 
> [1] https://lore.kernel.org/linux-btrfs/cover.1657321126.git.naohiro.aota@wdc.com/
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  common/zoned        |  11 ++++
>  tests/btrfs/292     | 136 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/292.out |   2 +
>  3 files changed, 149 insertions(+)
>  create mode 100755 tests/btrfs/292
>  create mode 100644 tests/btrfs/292.out
> 
> diff --git a/common/zoned b/common/zoned
> index d1bc60f784a1..eed0082a15cf 100644
> --- a/common/zoned
> +++ b/common/zoned
> @@ -15,6 +15,17 @@ _filter_blkzone_report()
>  	sed -e 's/len/cap/2'
>  }
>  
> +_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..6cfd6b18c299
> --- /dev/null
> +++ b/tests/btrfs/292
> @@ -0,0 +1,136 @@
> +#! /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

It's not necessary, due to fs specified common file is included automatically
by _source_specific_fs() according to $FSTYP.

> +. ./common/zoned
> +
> +# 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_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} \

What kind of indentation do you use? 4 spaces? 2 spaces? or a tab? Although that's
not a big deal, 8 characters width tab is recommended in fstests :)

> +	   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 &

If we used background processes, we'd better to deal with them in cleanup time.
But above two background processes are simple enough, I think you can add a
"wait" in _cleanup to make sure these backgroud processes are done.
Or you'd like to remove the "local" for pid1 and pid2, then does below in
_cleanup:

_cleanup()
{
	[ -n "$pid1" ] && kill $pid1
	[ -n "$pid2" ] && kill $pid2
	wait
	...
}

And ...

> +    local pid2=$!
> +
> +    wait $pid1; local ret1=$?

unset pid1

> +    wait $pid2; local ret2=$?

unset pid2

At here

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

The $status is 1 at the beginning of each case, and we set it to 0 at the end
of each case. If a test case _fail exit in the middle, then status keep be 1.
For your case, I think you don't need to touch the $status, you _fail directly
if anyone *worktout* returns failure. Or just let the "${work} failed" output
breaks the golden image(.out) to cause a test failure (no matter the status=0/1).

> +
> +# success, all done
> +if [ $status -eq 0 ]; then
> +    echo "Silence is golden"
> +fi

You can output "Silence is golden" at here directly, due to this case expect
that "Silence".

I'm not the best reviewer for a zoned&btrfs related case, so I tried to review
from fstests side. I saw Johannes Thumshirn <johannes.thumshirn@wdc.com> has
given you a RVB last time, I think you can keep that as a review from btrfs and
zbd side (except he has more review points now).

Thanks,
Zorro

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] btrfs: test active zone tracking
  2022-09-29  6:01   ` Zorro Lang
@ 2022-09-29 12:04     ` David Sterba
  2022-09-29 14:06       ` Zorro Lang
  2022-09-30  6:29     ` Naohiro Aota
  1 sibling, 1 reply; 10+ messages in thread
From: David Sterba @ 2022-09-29 12:04 UTC (permalink / raw)
  To: Zorro Lang; +Cc: Naohiro Aota, fstests, linux-btrfs

On Thu, Sep 29, 2022 at 02:01:06PM +0800, Zorro Lang wrote:
> On Thu, Sep 29, 2022 at 01:19:25PM +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")).
> 
> If this's a regression test case for known fix, we'd better to use:
> _fixed_by_kernel_commit 65ea1b66482f block: add bdev_max_segments (patchset)

This is very misleading as the commit only adds a helper that's used in
later commits. If anything, the last commit in the series should be
mentioned.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] btrfs: test active zone tracking
  2022-09-29 12:04     ` David Sterba
@ 2022-09-29 14:06       ` Zorro Lang
  2022-09-30  6:21         ` Naohiro Aota
  0 siblings, 1 reply; 10+ messages in thread
From: Zorro Lang @ 2022-09-29 14:06 UTC (permalink / raw)
  To: David Sterba; +Cc: Naohiro Aota, fstests, linux-btrfs

On Thu, Sep 29, 2022 at 02:04:00PM +0200, David Sterba wrote:
> On Thu, Sep 29, 2022 at 02:01:06PM +0800, Zorro Lang wrote:
> > On Thu, Sep 29, 2022 at 01:19:25PM +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")).
> > 
> > If this's a regression test case for known fix, we'd better to use:
> > _fixed_by_kernel_commit 65ea1b66482f block: add bdev_max_segments (patchset)
> 
> This is very misleading as the commit only adds a helper that's used in
> later commits. If anything, the last commit in the series should be
> mentioned.

Sure, I just gave an example, you learned about that patchset more than me, so
feel free to pick up a proper commit and description :)

> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] common: introduce zone_capacity() to return a zone capacity
  2022-09-29  5:15   ` Zorro Lang
@ 2022-09-30  6:19     ` Naohiro Aota
  0 siblings, 0 replies; 10+ messages in thread
From: Naohiro Aota @ 2022-09-30  6:19 UTC (permalink / raw)
  To: Zorro Lang; +Cc: fstests, linux-btrfs

On Thu, Sep 29, 2022 at 01:15:07PM +0800, Zorro Lang wrote:
> On Thu, Sep 29, 2022 at 01:19:24PM +0900, Naohiro Aota wrote:
> > Introduce _zone_capacity() to return a zone capacity of the given address
> > in the given device (optional). Move _filter_blkzone_report() for it, and
> > rewrite btrfs/237 with it.
> > 
> > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> > ---
> >  common/filter   | 13 -------------
> >  common/zoned    | 28 ++++++++++++++++++++++++++++
> >  tests/btrfs/237 |  8 ++------
> >  3 files changed, 30 insertions(+), 19 deletions(-)
> >  create mode 100644 common/zoned
> > 
> > diff --git a/common/filter b/common/filter
> > index 28dea64662dc..ac5c93422567 100644
> > --- a/common/filter
> > +++ b/common/filter
> > @@ -651,18 +651,5 @@ _filter_bash()
> >  	sed -e "s/^bash: line 1: /bash: /"
> >  }
> >  
> > -#
> > -# blkzone report added zone capacity to be printed from v2.37.
> > -# This filter will add an extra column 'cap' with the same value of
> > -# 'len'(zone size) for blkzone version < 2.37
> > -#
> > -# Before: start: 0x000100000, len 0x040000, wptr 0x000000 ..
> > -# After: start: 0x000100000, len 0x040000, cap 0x040000, wptr 0x000000 ..
> > -_filter_blkzone_report()
> > -{
> > -	$AWK_PROG -F "," 'BEGIN{OFS=",";} $3 !~ /cap/ {$2=$2","$2;} {print;}' |\
> > -	sed -e 's/len/cap/2'
> > -}
> > -
> >  # make sure this script returns success
> >  /bin/true
> > diff --git a/common/zoned b/common/zoned
> > new file mode 100644
> > index 000000000000..d1bc60f784a1
> > --- /dev/null
> > +++ b/common/zoned
> > @@ -0,0 +1,28 @@
> > +#
> > +# Common zoned block device specific functions
> > +#
> > +
> > +#
> > +# blkzone report added zone capacity to be printed from v2.37.
> > +# This filter will add an extra column 'cap' with the same value of
> > +# 'len'(zone size) for blkzone version < 2.37
> > +#
> > +# Before: start: 0x000100000, len 0x040000, wptr 0x000000 ..
> > +# After: start: 0x000100000, len 0x040000, cap 0x040000, wptr 0x000000 ..
> > +_filter_blkzone_report()
> > +{
> > +	$AWK_PROG -F "," 'BEGIN{OFS=",";} $3 !~ /cap/ {$2=$2","$2;} {print;}' |\
> > +	sed -e 's/len/cap/2'
> > +}
> > +
> > +_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
> 
> I'm a little surprised this line doesn't report error :) Anyway, it should be
> common/zoned as above. Others look good to me. With this fix, you can add:

Oops, I forgot to run this one. Thank you for catching this.

> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> >  
> >  # 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
> > 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] btrfs: test active zone tracking
  2022-09-29 14:06       ` Zorro Lang
@ 2022-09-30  6:21         ` Naohiro Aota
  0 siblings, 0 replies; 10+ messages in thread
From: Naohiro Aota @ 2022-09-30  6:21 UTC (permalink / raw)
  To: Zorro Lang; +Cc: David Sterba, fstests, linux-btrfs

On Thu, Sep 29, 2022 at 10:06:04PM +0800, Zorro Lang wrote:
> On Thu, Sep 29, 2022 at 02:04:00PM +0200, David Sterba wrote:
> > On Thu, Sep 29, 2022 at 02:01:06PM +0800, Zorro Lang wrote:
> > > On Thu, Sep 29, 2022 at 01:19:25PM +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")).
> > > 
> > > If this's a regression test case for known fix, we'd better to use:
> > > _fixed_by_kernel_commit 65ea1b66482f block: add bdev_max_segments (patchset)
> > 
> > This is very misleading as the commit only adds a helper that's used in
> > later commits. If anything, the last commit in the series should be
> > mentioned.
> 
> Sure, I just gave an example, you learned about that patchset more than me, so
> feel free to pick up a proper commit and description :)

Indeed. I'll use the last commit here.

> > 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] btrfs: test active zone tracking
  2022-09-29  6:01   ` Zorro Lang
  2022-09-29 12:04     ` David Sterba
@ 2022-09-30  6:29     ` Naohiro Aota
  1 sibling, 0 replies; 10+ messages in thread
From: Naohiro Aota @ 2022-09-30  6:29 UTC (permalink / raw)
  To: Zorro Lang; +Cc: fstests, linux-btrfs

On Thu, Sep 29, 2022 at 02:01:06PM +0800, Zorro Lang wrote:
> On Thu, Sep 29, 2022 at 01:19:25PM +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")).
> 
> If this's a regression test case for known fix, we'd better to use:
> _fixed_by_kernel_commit 65ea1b66482f block: add bdev_max_segments (patchset)
> 
> to remind downstream testers about that known issue.
> 
> > 
> > [1] https://lore.kernel.org/linux-btrfs/cover.1657321126.git.naohiro.aota@wdc.com/
> > 
> > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> > ---
> >  common/zoned        |  11 ++++
> >  tests/btrfs/292     | 136 ++++++++++++++++++++++++++++++++++++++++++++
> >  tests/btrfs/292.out |   2 +
> >  3 files changed, 149 insertions(+)
> >  create mode 100755 tests/btrfs/292
> >  create mode 100644 tests/btrfs/292.out
> > 
> > diff --git a/common/zoned b/common/zoned
> > index d1bc60f784a1..eed0082a15cf 100644
> > --- a/common/zoned
> > +++ b/common/zoned
> > @@ -15,6 +15,17 @@ _filter_blkzone_report()
> >  	sed -e 's/len/cap/2'
> >  }
> >  
> > +_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..6cfd6b18c299
> > --- /dev/null
> > +++ b/tests/btrfs/292
> > @@ -0,0 +1,136 @@
> > +#! /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
> 
> It's not necessary, due to fs specified common file is included automatically
> by _source_specific_fs() according to $FSTYP.

Yep. I'll drop this.

> > +. ./common/zoned
> > +
> > +# 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_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} \
> 
> What kind of indentation do you use? 4 spaces? 2 spaces? or a tab? Although that's
> not a big deal, 8 characters width tab is recommended in fstests :)

Ah, I use both vim and emacs and there must be confusing. I'll reformat.

> 
> > +	   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 &
> 
> If we used background processes, we'd better to deal with them in cleanup time.
> But above two background processes are simple enough, I think you can add a
> "wait" in _cleanup to make sure these backgroud processes are done.
> Or you'd like to remove the "local" for pid1 and pid2, then does below in
> _cleanup:
> 
> _cleanup()
> {
> 	[ -n "$pid1" ] && kill $pid1
> 	[ -n "$pid2" ] && kill $pid2
> 	wait
> 	...
> }
> 
> And ...
> 
> > +    local pid2=$!
> > +
> > +    wait $pid1; local ret1=$?
> 
> unset pid1
> 
> > +    wait $pid2; local ret2=$?
> 
> unset pid2
> 
> At here

Sure. I'll take this. Thank you.

> > +
> > +    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
> 
> The $status is 1 at the beginning of each case, and we set it to 0 at the end
> of each case. If a test case _fail exit in the middle, then status keep be 1.
> For your case, I think you don't need to touch the $status, you _fail directly
> if anyone *worktout* returns failure. Or just let the "${work} failed" output
> breaks the golden image(.out) to cause a test failure (no matter the status=0/1).
> 
> > +
> > +# success, all done
> > +if [ $status -eq 0 ]; then
> > +    echo "Silence is golden"
> > +fi
> 
> You can output "Silence is golden" at here directly, due to this case expect
> that "Silence".

Ah, yes. I tried to run as many tests as possible, even if one test
fails. But, I now think it's not much worthwhile. I'll just _fail any
failed work.

> I'm not the best reviewer for a zoned&btrfs related case, so I tried to review
> from fstests side. I saw Johannes Thumshirn <johannes.thumshirn@wdc.com> has
> given you a RVB last time, I think you can keep that as a review from btrfs and
> zbd side (except he has more review points now).

Well, I forgot to add the tag. Thanks.

> Thanks,
> Zorro
> 
> > +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
> > 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-09-30  6:29 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-29  4:19 [PATCH v2 0/2] btrfs: test active zone tracking Naohiro Aota
2022-09-29  4:19 ` [PATCH v2 1/2] common: introduce zone_capacity() to return a zone capacity Naohiro Aota
2022-09-29  5:15   ` Zorro Lang
2022-09-30  6:19     ` Naohiro Aota
2022-09-29  4:19 ` [PATCH v2 2/2] btrfs: test active zone tracking Naohiro Aota
2022-09-29  6:01   ` Zorro Lang
2022-09-29 12:04     ` David Sterba
2022-09-29 14:06       ` Zorro Lang
2022-09-30  6:21         ` Naohiro Aota
2022-09-30  6:29     ` Naohiro Aota

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.