[PATCH v2 0/3] t/zbd: Fix max_open_zones related failures

[PATCH v2 0/3] t/zbd: Fix max_open_zones related failures

[Shin'ichiro Kawasaki]

Recent changes in fio [1] triggered three failures in the test script
t/zbd/test-zoned-support. This series address the failures.

The first failure was caused by difference between max_open_zones value that fio
refers and that test script refers. The first patch modifies the test script to
use the value fio refers.

The second failure was observed when users specify special max_open_zones values
larger than the max_open_zones limit of the test target device. The second patch
adds --ignore_zone_limits option to the fio command only when users specify such
special conditions.

The third failure was caused by write target zone count bug in the test case 31.
Data write to more zones than max_open_zones limit resulted in exceeding
max_open_zones limit. The third patch fixes the write target zone count logic.

[1] https://www.spinics.net/lists/fio/msg09502.html

Changes from v1:
* Added comment to explain why the test script gets max_open_zones
* Reflected other comments and added Reviewed-by tags

Shin'ichiro Kawasaki (3):
  t/zbd: Use max_open_zones that fio fetched from device
  t/zbd: Add ignore_zone_limit option to test with special
    max_open_zones
  t/zbd: Fix write target zones counting in test case #31

 t/zbd/functions        | 15 ++++++++++++---
 t/zbd/test-zbd-support | 37 +++++++++++++++++--------------------
 2 files changed, 29 insertions(+), 23 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] t/zbd: Use max_open_zones that fio fetched from device

[Shin'ichiro Kawasaki]

Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
block device operation") modified fio to compare --max_open_zones option
value and max_open_zones reported by the device. The device limit is
fetched through sysfs or through an ioengine specific implementation.

The test script currently try to fetch the max open zones limit using
libzbc tools or sg_inq. If either of these fail, default value 128 is
supplied. This default value can be too high when the test script is
run for certain zoned block devices, and can therefore result in fio
error and test case failure.

To avoid the failure, modify the default value used in the test script
from 128 to 0. With this, --max_open_zones=0 is passed to fio, and it
makes fio use the max_open_zones reported by the device. Also add
comments to describe why the test script gets max_open_zones with tools.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 t/zbd/functions | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/t/zbd/functions b/t/zbd/functions
index 40ffe1de..06973f79 100644
--- a/t/zbd/functions
+++ b/t/zbd/functions
@@ -173,15 +173,24 @@ last_online_zone() {
     fi
 }
 
+# Get max_open_zones of SMR drives using sg_inq or libzbc tools. Two test cases
+# 31 and 32 use this max_open_zones value. The test case 31 uses max_open_zones
+# to decide number of write target zones. The test case 32 passes max_open_zones
+# value to fio with --max_open_zones option. Of note is that fio itself as the
+# feature to get max_open_zones from the device through sysfs or ioengine
+# specific implementation. This max_open_zones fetch by test script is required
+# in case 1) ioengine lacks max_open_zones fetch feature, 2) Linux kernel lacks
+# zoned block device support, or 3) Linux kernel supports zoned block devices
+# but lacks max_open_zones in sysfs.
 max_open_zones() {
     local dev=$1
 
     if [ -n "${sg_inq}" ] && [ ! -n "${use_libzbc}" ]; then
 	if ! ${sg_inq} -e --page=0xB6 --len=20 --hex "$dev" \
 		 > /dev/null 2>&1; then
-	    # Non scsi device such as null_blk can not return max open zones.
-	    # Use default value.
-	    echo 128
+	    # When sg_inq can not get max open zones, specify 0 which indicates
+	    # fio to get max open zones limit from the device.
+	    echo 0
 	else
 	    ${sg_inq} -e --page=0xB6 --len=20 --hex "$dev" | tail -1 |
 		{
-- 
2.31.1

[PATCH v2 2/3] t/zbd: Add ignore_zone_limit option to test with special max_open_zones

[Shin'ichiro Kawasaki]

Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
block device operation") modified fio to compare --max_open_zones option
value and max_open_zones reported by the device. When the option
--max_open_zones is larger than the device limit, fio exits with an
error. However, sometimes it is useful to run fio with --max_open_zones
larger than the device limit to check performance impact of implicit
zone open and close by the zoned block devices. The test script
t/zbd/test-zbd-support has an option -o so that users can specify such
larger max_open_zones value. After the commit, such test runs fail with
the fio error.

To avoid the failure, modify the test script to specify another option
--ignore_zone_limits to fio command, which was added by the commit
575686bb85fa (zbd: add a new --ignore_zone_limits option). This option
is added to fio command only when users specify -o option and special
max_open_zones value to the test script. This change does not affect
default test conditions.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 t/zbd/test-zbd-support | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index 26aff373..015fa1dc 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -1348,6 +1348,7 @@ fi
 if [[ -n ${max_open_zones_opt} ]]; then
 	# Override max_open_zones with the script option value
 	max_open_zones="${max_open_zones_opt}"
+	global_var_opts+=("--ignore_zone_limits=1")
 	job_var_opts+=("--max_open_zones=${max_open_zones_opt}")
 fi
 
-- 
2.31.1

[PATCH v2 3/3] t/zbd: Fix write target zones counting in test case #31

[Shin'ichiro Kawasaki]

The test case #31 in t/zbd/test-zbd-support writes 128KB data to
sequential write required zones as the preparation for the following
random read test. The data write leaves the target zones in open status.
The test case refers the variable 'nz', which has max_open_zones value,
to decide how many zones to write the data. However, the end condition
of the write target zone loop has a bug. The disk end offset is used as
the loop end condition, which does not match the last target zone when
number of sequential write required zones divided by nz has remainder.
This results in write to more zones than nz=max_open_zones limit and the
test case failure. To fix the bug and to simplify the script, avoid the
loop and utilize zonemode strided to achieve the same data write
pattern. Also specify size and io_size using nz to reliably count the
write target zones.

Even with the fix above, still the number of open zones may exceed
max_open_zones since other test cases executed before the test case 31
may leave open zones on the test target device. To avoid this failure,
reset all zones before the data write.

The failures were observed with libzbc I/O engine after the commit
e8267436fd7a ("engines/libzbc: add support for the get_max_open_zones io
op"), which changed the max_open_zones value fio refers.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 t/zbd/test-zbd-support | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index 015fa1dc..a684f988 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -731,32 +731,28 @@ test30() {
 test31() {
     local bs inc nz off opts size
 
-    prep_write
-    # Start with writing 128 KB to max_open_zones sequential zones.
-    bs=128K
+    [ -n "$is_zbd" ] && reset_zone "$dev" -1
+
+    # As preparation, write 128 KB to sequential write required zones. Limit
+    # write target zones up to max_open_zones to keep test time reasonable.
+    # To distribute the write target zones evenly, skip certain zones for every
+    # write. Utilize zonemode strided for such write patterns.
+    bs=$((128 * 1024))
     nz=$((max_open_zones))
     if [[ $nz -eq 0 ]]; then
 	nz=128
     fi
-    # shellcheck disable=SC2017
-    inc=$(((disk_size - (first_sequential_zone_sector * 512)) / (nz * zone_size)
-	   * zone_size))
-    if [ "$inc" -eq 0 ]; then
-	require_seq_zones $nz || return $SKIP_TESTCASE
-    fi
-    opts=()
-    for ((off = first_sequential_zone_sector * 512; off < disk_size;
-	  off += inc)); do
-	opts+=("--name=$dev" "--filename=$dev" "--offset=$off" "--io_size=$bs")
-	opts+=("--bs=$bs" "--size=$zone_size" "$(ioengine "libaio")")
-	opts+=("--rw=write" "--direct=1" "--thread=1" "--stats=0")
-	opts+=("--zonemode=zbd" "--zonesize=${zone_size}")
-	opts+=(${job_var_opts[@]})
-    done
-    "$(dirname "$0")/../../fio" "${opts[@]}" >> "${logfile}.${test_number}" 2>&1
-    # Next, run the test.
     off=$((first_sequential_zone_sector * 512))
     size=$((disk_size - off))
+    inc=$(((size / nz / zone_size) * zone_size))
+    opts=("--name=$dev" "--filename=$dev" "--rw=write" "--bs=${bs}")
+    opts+=("--offset=$off" "--size=$((inc * nz))" "--io_size=$((bs * nz))")
+    opts+=("--zonemode=strided" "--zonesize=${bs}" "--zonerange=${inc}")
+    opts+=("--direct=1")
+    echo "fio ${opts[@]}" >> "${logfile}.${test_number}"
+    "$(dirname "$0")/../../fio" "${opts[@]}" >> "${logfile}.${test_number}" 2>&1
+
+    # Next, run the test.
     opts=("--name=$dev" "--filename=$dev" "--offset=$off" "--size=$size")
     opts+=("--bs=$bs" "$(ioengine "psync")" "--rw=randread" "--direct=1")
     opts+=("--thread=1" "--time_based" "--runtime=30" "--zonemode=zbd")
-- 
2.31.1

Re: [PATCH v2 2/3] t/zbd: Add ignore_zone_limit option to test with special max_open_zones

[Niklas Cassel]

On Fri, Jun 04, 2021 at 02:33:50PM +0900, Shin'ichiro Kawasaki wrote:
> Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
> block device operation") modified fio to compare --max_open_zones option
> value and max_open_zones reported by the device. When the option
> --max_open_zones is larger than the device limit, fio exits with an
> error. However, sometimes it is useful to run fio with --max_open_zones
> larger than the device limit to check performance impact of implicit
> zone open and close by the zoned block devices. The test script
> t/zbd/test-zbd-support has an option -o so that users can specify such
> larger max_open_zones value. After the commit, such test runs fail with
> the fio error.
> 
> To avoid the failure, modify the test script to specify another option
> --ignore_zone_limits to fio command, which was added by the commit
> 575686bb85fa (zbd: add a new --ignore_zone_limits option). This option
> is added to fio command only when users specify -o option and special
> max_open_zones value to the test script. This change does not affect
> default test conditions.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  t/zbd/test-zbd-support | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
> index 26aff373..015fa1dc 100755
> --- a/t/zbd/test-zbd-support
> +++ b/t/zbd/test-zbd-support
> @@ -1348,6 +1348,7 @@ fi
>  if [[ -n ${max_open_zones_opt} ]]; then
>  	# Override max_open_zones with the script option value
>  	max_open_zones="${max_open_zones_opt}"
> +	global_var_opts+=("--ignore_zone_limits=1")
>  	job_var_opts+=("--max_open_zones=${max_open_zones_opt}")
>  fi
>  
> -- 
> 2.31.1
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v2 3/3] t/zbd: Fix write target zones counting in test case #31

[Niklas Cassel]

On Fri, Jun 04, 2021 at 02:33:51PM +0900, Shin'ichiro Kawasaki wrote:
> The test case #31 in t/zbd/test-zbd-support writes 128KB data to
> sequential write required zones as the preparation for the following
> random read test. The data write leaves the target zones in open status.
> The test case refers the variable 'nz', which has max_open_zones value,
> to decide how many zones to write the data. However, the end condition
> of the write target zone loop has a bug. The disk end offset is used as
> the loop end condition, which does not match the last target zone when
> number of sequential write required zones divided by nz has remainder.
> This results in write to more zones than nz=max_open_zones limit and the
> test case failure. To fix the bug and to simplify the script, avoid the
> loop and utilize zonemode strided to achieve the same data write
> pattern. Also specify size and io_size using nz to reliably count the
> write target zones.
> 
> Even with the fix above, still the number of open zones may exceed
> max_open_zones since other test cases executed before the test case 31
> may leave open zones on the test target device. To avoid this failure,
> reset all zones before the data write.
> 
> The failures were observed with libzbc I/O engine after the commit
> e8267436fd7a ("engines/libzbc: add support for the get_max_open_zones io
> op"), which changed the max_open_zones value fio refers.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  t/zbd/test-zbd-support | 36 ++++++++++++++++--------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
> index 015fa1dc..a684f988 100755
> --- a/t/zbd/test-zbd-support
> +++ b/t/zbd/test-zbd-support
> @@ -731,32 +731,28 @@ test30() {
>  test31() {
>      local bs inc nz off opts size
>  
> -    prep_write
> -    # Start with writing 128 KB to max_open_zones sequential zones.
> -    bs=128K
> +    [ -n "$is_zbd" ] && reset_zone "$dev" -1
> +
> +    # As preparation, write 128 KB to sequential write required zones. Limit
> +    # write target zones up to max_open_zones to keep test time reasonable.
> +    # To distribute the write target zones evenly, skip certain zones for every
> +    # write. Utilize zonemode strided for such write patterns.
> +    bs=$((128 * 1024))
>      nz=$((max_open_zones))
>      if [[ $nz -eq 0 ]]; then
>  	nz=128
>      fi
> -    # shellcheck disable=SC2017
> -    inc=$(((disk_size - (first_sequential_zone_sector * 512)) / (nz * zone_size)
> -	   * zone_size))
> -    if [ "$inc" -eq 0 ]; then
> -	require_seq_zones $nz || return $SKIP_TESTCASE
> -    fi
> -    opts=()
> -    for ((off = first_sequential_zone_sector * 512; off < disk_size;
> -	  off += inc)); do
> -	opts+=("--name=$dev" "--filename=$dev" "--offset=$off" "--io_size=$bs")
> -	opts+=("--bs=$bs" "--size=$zone_size" "$(ioengine "libaio")")
> -	opts+=("--rw=write" "--direct=1" "--thread=1" "--stats=0")
> -	opts+=("--zonemode=zbd" "--zonesize=${zone_size}")
> -	opts+=(${job_var_opts[@]})
> -    done
> -    "$(dirname "$0")/../../fio" "${opts[@]}" >> "${logfile}.${test_number}" 2>&1
> -    # Next, run the test.
>      off=$((first_sequential_zone_sector * 512))
>      size=$((disk_size - off))
> +    inc=$(((size / nz / zone_size) * zone_size))
> +    opts=("--name=$dev" "--filename=$dev" "--rw=write" "--bs=${bs}")
> +    opts+=("--offset=$off" "--size=$((inc * nz))" "--io_size=$((bs * nz))")
> +    opts+=("--zonemode=strided" "--zonesize=${bs}" "--zonerange=${inc}")
> +    opts+=("--direct=1")
> +    echo "fio ${opts[@]}" >> "${logfile}.${test_number}"
> +    "$(dirname "$0")/../../fio" "${opts[@]}" >> "${logfile}.${test_number}" 2>&1
> +
> +    # Next, run the test.
>      opts=("--name=$dev" "--filename=$dev" "--offset=$off" "--size=$size")
>      opts+=("--bs=$bs" "$(ioengine "psync")" "--rw=randread" "--direct=1")
>      opts+=("--thread=1" "--time_based" "--runtime=30" "--zonemode=zbd")
> -- 
> 2.31.1
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v2 1/3] t/zbd: Use max_open_zones that fio fetched from device

[Niklas Cassel]

On Fri, Jun 04, 2021 at 02:33:49PM +0900, Shin'ichiro Kawasaki wrote:
> Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
> block device operation") modified fio to compare --max_open_zones option
> value and max_open_zones reported by the device. The device limit is
> fetched through sysfs or through an ioengine specific implementation.
> 
> The test script currently try to fetch the max open zones limit using
> libzbc tools or sg_inq. If either of these fail, default value 128 is
> supplied. This default value can be too high when the test script is
> run for certain zoned block devices, and can therefore result in fio
> error and test case failure.
> 
> To avoid the failure, modify the default value used in the test script
> from 128 to 0. With this, --max_open_zones=0 is passed to fio, and it
> makes fio use the max_open_zones reported by the device. Also add
> comments to describe why the test script gets max_open_zones with tools.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  t/zbd/functions | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/t/zbd/functions b/t/zbd/functions
> index 40ffe1de..06973f79 100644
> --- a/t/zbd/functions
> +++ b/t/zbd/functions
> @@ -173,15 +173,24 @@ last_online_zone() {
>      fi
>  }
>  
> +# Get max_open_zones of SMR drives using sg_inq or libzbc tools. Two test cases
> +# 31 and 32 use this max_open_zones value. The test case 31 uses max_open_zones
> +# to decide number of write target zones. The test case 32 passes max_open_zones
> +# value to fio with --max_open_zones option. Of note is that fio itself as the
> +# feature to get max_open_zones from the device through sysfs or ioengine

has the feature?

> +# specific implementation. This max_open_zones fetch by test script is required
> +# in case 1) ioengine lacks max_open_zones fetch feature, 2) Linux kernel lacks
> +# zoned block device support, or 3) Linux kernel supports zoned block devices
> +# but lacks max_open_zones in sysfs.

1) ioengine lacks max_open_zones
makes it sound like that is something that is needed,
it is not, there is the oslib implementation.

I think that you can replace 1, 2, 3 with something simpler like:

... is required in case fio is running on an old Linux kernel version which
lacks max_open_zones in sysfs.

Or if you want something more specific:

... is required in case fio is running on an old Linux kernel version which
lacks max_oper_zones in sysfs, or which lacks zbd support completely.

>  max_open_zones() {
>      local dev=$1
>  
>      if [ -n "${sg_inq}" ] && [ ! -n "${use_libzbc}" ]; then
>  	if ! ${sg_inq} -e --page=0xB6 --len=20 --hex "$dev" \
>  		 > /dev/null 2>&1; then
> -	    # Non scsi device such as null_blk can not return max open zones.
> -	    # Use default value.
> -	    echo 128
> +	    # When sg_inq can not get max open zones, specify 0 which indicates
> +	    # fio to get max open zones limit from the device.
> +	    echo 0
>  	else
>  	    ${sg_inq} -e --page=0xB6 --len=20 --hex "$dev" | tail -1 |
>  		{
> -- 
> 2.31.1
> 

Re: [PATCH v2 1/3] t/zbd: Use max_open_zones that fio fetched from device

[Shinichiro Kawasaki]

On Jun 04, 2021 / 08:03, Niklas Cassel wrote:
> On Fri, Jun 04, 2021 at 02:33:49PM +0900, Shin'ichiro Kawasaki wrote:
> > Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
> > block device operation") modified fio to compare --max_open_zones option
> > value and max_open_zones reported by the device. The device limit is
> > fetched through sysfs or through an ioengine specific implementation.
> > 
> > The test script currently try to fetch the max open zones limit using
> > libzbc tools or sg_inq. If either of these fail, default value 128 is
> > supplied. This default value can be too high when the test script is
> > run for certain zoned block devices, and can therefore result in fio
> > error and test case failure.
> > 
> > To avoid the failure, modify the default value used in the test script
> > from 128 to 0. With this, --max_open_zones=0 is passed to fio, and it
> > makes fio use the max_open_zones reported by the device. Also add
> > comments to describe why the test script gets max_open_zones with tools.
> > 
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >  t/zbd/functions | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/t/zbd/functions b/t/zbd/functions
> > index 40ffe1de..06973f79 100644
> > --- a/t/zbd/functions
> > +++ b/t/zbd/functions
> > @@ -173,15 +173,24 @@ last_online_zone() {
> >      fi
> >  }
> >  
> > +# Get max_open_zones of SMR drives using sg_inq or libzbc tools. Two test cases
> > +# 31 and 32 use this max_open_zones value. The test case 31 uses max_open_zones
> > +# to decide number of write target zones. The test case 32 passes max_open_zones
> > +# value to fio with --max_open_zones option. Of note is that fio itself as the
> > +# feature to get max_open_zones from the device through sysfs or ioengine
> 
> has the feature?

Oops. Thanks for the catch.

> 
> > +# specific implementation. This max_open_zones fetch by test script is required
> > +# in case 1) ioengine lacks max_open_zones fetch feature, 2) Linux kernel lacks
> > +# zoned block device support, or 3) Linux kernel supports zoned block devices
> > +# but lacks max_open_zones in sysfs.
> 
> 1) ioengine lacks max_open_zones
> makes it sound like that is something that is needed,
> it is not, there is the oslib implementation.

Indeed.

> 
> I think that you can replace 1, 2, 3 with something simpler like:
> 
> ... is required in case fio is running on an old Linux kernel version which
> lacks max_open_zones in sysfs.
> 
> Or if you want something more specific:
> 
> ... is required in case fio is running on an old Linux kernel version which
> lacks max_oper_zones in sysfs, or which lacks zbd support completely.

Will reflect to v3. Thank you!

-- 
Best Regards,
Shin'ichiro Kawasaki