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

[PATCH 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 the
max_open_zones limit. The third patch fixes the write target zone count logic.

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

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        |  4 ++--
 t/zbd/test-zbd-support | 37 +++++++++++++++++--------------------
 2 files changed, 19 insertions(+), 22 deletions(-)

-- 
2.31.1

[PATCH 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_zoned 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_zoned=0 is passed to fio, and it
makes fio use the max_open_zones reported by the device.

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

diff --git a/t/zbd/functions b/t/zbd/functions
index 40ffe1de..1a7ea970 100644
--- a/t/zbd/functions
+++ b/t/zbd/functions
@@ -180,8 +180,8 @@ max_open_zones() {
 	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
+	    # Specify 0 to indicate fio to get max open zones from the device.
+	    echo 0
 	else
 	    ${sg_inq} -e --page=0xB6 --len=20 --hex "$dev" | tail -1 |
 		{
-- 
2.31.1

[PATCH 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_zoned 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>
---
 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 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>
---
 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 1/3] t/zbd: Use max_open_zones that fio fetched from device

[Damien Le Moal]

On 2021/06/01 15:48, Shin'ichiro Kawasaki wrote:
> Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
> block device operation") modified fio to compare --max_open_zoned option

s/max_open_zoned/max_open_zones

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

If sysfs has the max_open_zones attribute file present, that value should be
retrieved first. libzbc and sg_inq should be used only if the sysfs attribute is
not present, that is, when running on older kernels.

> 
> To avoid the failure, modify the default value used in the test script
> from 128 to 0. With this, --max_open_zoned=0 is passed to fio, and it
> makes fio use the max_open_zones reported by the device.

Why pass anything at all since now fio will use the device reported limit by
default if the user does not specify anything ?

> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  t/zbd/functions | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/zbd/functions b/t/zbd/functions
> index 40ffe1de..1a7ea970 100644
> --- a/t/zbd/functions
> +++ b/t/zbd/functions
> @@ -180,8 +180,8 @@ max_open_zones() {
>  	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.

This comment is a little outdated since even nullblk now has a limit exposed
through sysfs (the limit may be 0 == no limit). See above comment. sysfs should
be the first thing to look at.

> -	    # Use default value.
> -	    echo 128
> +	    # Specify 0 to indicate fio to get max open zones from the device.
> +	    echo 0
>  	else
>  	    ${sg_inq} -e --page=0xB6 --len=20 --hex "$dev" | tail -1 |
>  		{
> 


-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 2021/06/01 15:48, Shin'ichiro Kawasaki wrote:
> Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
> block device operation") modified fio to compare --max_open_zoned 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>
> ---
>  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
>  
> 

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 2021/06/01 15:48, 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>
> ---
>  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")
> 

Looks OK to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>


-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

On Tue, Jun 01, 2021 at 07:01:18AM +0000, Damien Le Moal wrote:
> On 2021/06/01 15:48, Shin'ichiro Kawasaki wrote:
> > Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
> > block device operation") modified fio to compare --max_open_zoned option
> 
> s/max_open_zoned/max_open_zones
> 
> > 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.
> 
> If sysfs has the max_open_zones attribute file present, that value should be
> retrieved first. libzbc and sg_inq should be used only if the sysfs attribute is
> not present, that is, when running on older kernels.

Since we can assume that the test script and fio versions are updated in sync,
I don't see any point of duplicating the sysfs parsing in the test script.

If the test script either specifies --max_open_zones=0 or omits the parameter,
fio itself will either parse sysfs, or use libzbc to fetch the max limit.

So I think that the test script should only use those "methods" that fio
itself can't do.
Right now fio can parse sysfs and use libzbc.
The only remaining method is sg_inq. This only I assume that we want to
keep in the test. (I guess the alternative is to implement the
.get_max_open_zones() fio ioengine callback in engines/sg.c, that does
the same. But we probably don't want to do that, right?)

> 
> > 
> > To avoid the failure, modify the default value used in the test script
> > from 128 to 0. With this, --max_open_zoned=0 is passed to fio, and it
> > makes fio use the max_open_zones reported by the device.
> 
> Why pass anything at all since now fio will use the device reported limit by
> default if the user does not specify anything ?

Sure, we can just omit the --max_open_zones parameter completely.
That might be more clear.

> 
> > 
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >  t/zbd/functions | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/t/zbd/functions b/t/zbd/functions
> > index 40ffe1de..1a7ea970 100644
> > --- a/t/zbd/functions
> > +++ b/t/zbd/functions
> > @@ -180,8 +180,8 @@ max_open_zones() {
> >  	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.
> 
> This comment is a little outdated since even nullblk now has a limit exposed
> through sysfs (the limit may be 0 == no limit). See above comment. sysfs should
> be the first thing to look at.
> 
> > -	    # Use default value.
> > -	    echo 128
> > +	    # Specify 0 to indicate fio to get max open zones from the device.
> > +	    echo 0
> >  	else
> >  	    ${sg_inq} -e --page=0xB6 --len=20 --hex "$dev" | tail -1 |
> >  		{
> > 
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 

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

[Damien Le Moal]

On 2021/06/01 16:15, Niklas Cassel wrote:
> On Tue, Jun 01, 2021 at 07:01:18AM +0000, Damien Le Moal wrote:
>> On 2021/06/01 15:48, Shin'ichiro Kawasaki wrote:
>>> Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
>>> block device operation") modified fio to compare --max_open_zoned option
>>
>> s/max_open_zoned/max_open_zones
>>
>>> 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.
>>
>> If sysfs has the max_open_zones attribute file present, that value should be
>> retrieved first. libzbc and sg_inq should be used only if the sysfs attribute is
>> not present, that is, when running on older kernels.
> 
> Since we can assume that the test script and fio versions are updated in sync,
> I don't see any point of duplicating the sysfs parsing in the test script.
> 
> If the test script either specifies --max_open_zones=0 or omits the parameter,
> fio itself will either parse sysfs, or use libzbc to fetch the max limit.

True. Unless the user is running the tests with some special max_open_zones, we
should not try to grab any value then.

> 
> So I think that the test script should only use those "methods" that fio
> itself can't do.
> Right now fio can parse sysfs and use libzbc.
> The only remaining method is sg_inq. This only I assume that we want to
> keep in the test. (I guess the alternative is to implement the
> .get_max_open_zones() fio ioengine callback in engines/sg.c, that does
> the same. But we probably don't want to do that, right?)

No, we certainly do not. That would need a lot more code than that, e.g. to also
detect zone model. That would mean basically re-implementing what libzbc does
inside the sg engine. And since we now have the libzbc engine, I do not see the
point.

> 
>>
>>>
>>> To avoid the failure, modify the default value used in the test script
>>> from 128 to 0. With this, --max_open_zoned=0 is passed to fio, and it
>>> makes fio use the max_open_zones reported by the device.
>>
>> Why pass anything at all since now fio will use the device reported limit by
>> default if the user does not specify anything ?
> 
> Sure, we can just omit the --max_open_zones parameter completely.
> That might be more clear.
> 
>>
>>>
>>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>>> ---
>>>  t/zbd/functions | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/t/zbd/functions b/t/zbd/functions
>>> index 40ffe1de..1a7ea970 100644
>>> --- a/t/zbd/functions
>>> +++ b/t/zbd/functions
>>> @@ -180,8 +180,8 @@ max_open_zones() {
>>>  	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.
>>
>> This comment is a little outdated since even nullblk now has a limit exposed
>> through sysfs (the limit may be 0 == no limit). See above comment. sysfs should
>> be the first thing to look at.
>>
>>> -	    # Use default value.
>>> -	    echo 128
>>> +	    # Specify 0 to indicate fio to get max open zones from the device.
>>> +	    echo 0
>>>  	else
>>>  	    ${sg_inq} -e --page=0xB6 --len=20 --hex "$dev" | tail -1 |
>>>  		{
>>>
>>
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research
>>


-- 
Damien Le Moal
Western Digital Research

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

[Shinichiro Kawasaki]

On Jun 01, 2021 / 07:19, Damien Le Moal wrote:
> On 2021/06/01 16:15, Niklas Cassel wrote:
> > On Tue, Jun 01, 2021 at 07:01:18AM +0000, Damien Le Moal wrote:
> >> On 2021/06/01 15:48, Shin'ichiro Kawasaki wrote:
> >>> Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
> >>> block device operation") modified fio to compare --max_open_zoned option
> >>
> >> s/max_open_zoned/max_open_zones

Thanks. Will reflect.

> >>
> >>> 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.
> >>
> >> If sysfs has the max_open_zones attribute file present, that value should be
> >> retrieved first. libzbc and sg_inq should be used only if the sysfs attribute is
> >> not present, that is, when running on older kernels.
> > 
> > Since we can assume that the test script and fio versions are updated in sync,
> > I don't see any point of duplicating the sysfs parsing in the test script.
> > 
> > If the test script either specifies --max_open_zones=0 or omits the parameter,
> > fio itself will either parse sysfs, or use libzbc to fetch the max limit.
> 
> True. Unless the user is running the tests with some special max_open_zones, we
> should not try to grab any value then.
> 
> > 
> > So I think that the test script should only use those "methods" that fio
> > itself can't do.
> > Right now fio can parse sysfs and use libzbc.
> > The only remaining method is sg_inq. This only I assume that we want to
> > keep in the test. (I guess the alternative is to implement the
> > .get_max_open_zones() fio ioengine callback in engines/sg.c, that does
> > the same. But we probably don't want to do that, right?)
> 
> No, we certainly do not. That would need a lot more code than that, e.g. to also
> detect zone model. That would mean basically re-implementing what libzbc does
> inside the sg engine. And since we now have the libzbc engine, I do not see the
> point.
> 
> > 
> >>
> >>>
> >>> To avoid the failure, modify the default value used in the test script
> >>> from 128 to 0. With this, --max_open_zoned=0 is passed to fio, and it
> >>> makes fio use the max_open_zones reported by the device.
> >>
> >> Why pass anything at all since now fio will use the device reported limit by
> >> default if the user does not specify anything ?
> > 
> > Sure, we can just omit the --max_open_zones parameter completely.
> > That might be more clear.

I think still we need the --max_open_zones option in case the latest fio is
run on older kernel which does not have max_open_zones in sysfs. In such case,
fio is not able to obtain the correct max_open_zones of the device (unless it
uses libzbc I/O engine).

There are only two test cases which refers the max_open_zones that the test
script obtained: #31 and #32. The test case #31 has a check if the found
max_open_zones is zero or not to decide write target zone count. The
max_open_zones value is not used for --max_open_zones option (can not omit it).
As for the test case #32, it specifies "--max_open_zones=$max_open_zones" to the
fio command. I assume this option is still valid and required for the older
kernels. We could add a check to the test script to specify this option only
when max_open_zones is zero, but I think it's the better not to add such check
to keep the test script simpler.

> > 
> >>
> >>>
> >>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> >>> ---
> >>>  t/zbd/functions | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/t/zbd/functions b/t/zbd/functions
> >>> index 40ffe1de..1a7ea970 100644
> >>> --- a/t/zbd/functions
> >>> +++ b/t/zbd/functions
> >>> @@ -180,8 +180,8 @@ max_open_zones() {
> >>>  	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.
> >>
> >> This comment is a little outdated since even nullblk now has a limit exposed
> >> through sysfs (the limit may be 0 == no limit). See above comment. sysfs should
> >> be the first thing to look at.

Right. I will repharse as follows.

# Sg_inq can not get max_open_zones of non scsi devices.

> >>
> >>> -	    # Use default value.
> >>> -	    echo 128
> >>> +	    # Specify 0 to indicate fio to get max open zones from the device.
> >>> +	    echo 0
> >>>  	else
> >>>  	    ${sg_inq} -e --page=0xB6 --len=20 --hex "$dev" | tail -1 |
> >>>  		{
> >>>
> >>
> >>
> >> -- 
> >> Damien Le Moal
> >> Western Digital Research
> >>
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 


-- 
Best Regards,
Shin'ichiro Kawasaki

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

[Niklas Cassel]

On Tue, Jun 01, 2021 at 09:01:45AM +0000, Shinichiro Kawasaki wrote:
> On Jun 01, 2021 / 07:19, Damien Le Moal wrote:
> > On 2021/06/01 16:15, Niklas Cassel wrote:
> > > On Tue, Jun 01, 2021 at 07:01:18AM +0000, Damien Le Moal wrote:
> > >> On 2021/06/01 15:48, Shin'ichiro Kawasaki wrote:
> > >>> Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
> > >>> block device operation") modified fio to compare --max_open_zoned option
> > >>
> > >> s/max_open_zoned/max_open_zones
> 
> Thanks. Will reflect.
> 
> > >>
> > >>> 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.
> > >>
> > >> If sysfs has the max_open_zones attribute file present, that value should be
> > >> retrieved first. libzbc and sg_inq should be used only if the sysfs attribute is
> > >> not present, that is, when running on older kernels.
> > > 
> > > Since we can assume that the test script and fio versions are updated in sync,
> > > I don't see any point of duplicating the sysfs parsing in the test script.
> > > 
> > > If the test script either specifies --max_open_zones=0 or omits the parameter,
> > > fio itself will either parse sysfs, or use libzbc to fetch the max limit.
> > 
> > True. Unless the user is running the tests with some special max_open_zones, we
> > should not try to grab any value then.
> > 
> > > 
> > > So I think that the test script should only use those "methods" that fio
> > > itself can't do.
> > > Right now fio can parse sysfs and use libzbc.
> > > The only remaining method is sg_inq. This only I assume that we want to
> > > keep in the test. (I guess the alternative is to implement the
> > > .get_max_open_zones() fio ioengine callback in engines/sg.c, that does
> > > the same. But we probably don't want to do that, right?)
> > 
> > No, we certainly do not. That would need a lot more code than that, e.g. to also
> > detect zone model. That would mean basically re-implementing what libzbc does
> > inside the sg engine. And since we now have the libzbc engine, I do not see the
> > point.
> > 
> > > 
> > >>
> > >>>
> > >>> To avoid the failure, modify the default value used in the test script
> > >>> from 128 to 0. With this, --max_open_zoned=0 is passed to fio, and it
> > >>> makes fio use the max_open_zones reported by the device.
> > >>
> > >> Why pass anything at all since now fio will use the device reported limit by
> > >> default if the user does not specify anything ?
> > > 
> > > Sure, we can just omit the --max_open_zones parameter completely.
> > > That might be more clear.
> 
> I think still we need the --max_open_zones option in case the latest fio is
> run on older kernel which does not have max_open_zones in sysfs. In such case,
> fio is not able to obtain the correct max_open_zones of the device (unless it
> uses libzbc I/O engine).
> 
> There are only two test cases which refers the max_open_zones that the test
> script obtained: #31 and #32. The test case #31 has a check if the found
> max_open_zones is zero or not to decide write target zone count. The
> max_open_zones value is not used for --max_open_zones option (can not omit it).
> As for the test case #32, it specifies "--max_open_zones=$max_open_zones" to the
> fio command. I assume this option is still valid and required for the older
> kernels. We could add a check to the test script to specify this option only
> when max_open_zones is zero, but I think it's the better not to add such check
> to keep the test script simpler.

Test case 31 is just poorly designed IMHO.

I don't like that it wants to know max_open_zones, but then it doesn't even
send in that value to fio.
The only reason why it fetches max_open_zones is to save time.
We can come up with some other way, e.g. a random max limit, 10 GB, divide
that by zone_size. That is the amount of zones that we decide to write.
And each zone is written fully.

Then, since test case 31 is a random read test, just read the zones that we
wrote. Right now we will write <= (all zones)/2, but we read _all_ zones.

Since half zones will be empty, this is not an accurate test of the random
read performance.

(Perhaps you could make the argument that all zones that you read from
should be in zone state implicit open, but that does not seem to be the case
currently, as many zones will most likely be empty.
My suggestion is that we simply write each zone fully, that way, all zones
that we read from will be in zone state FULL.)




Test case 32:

On fio master the test script currently checks sg_inq (scsi generic) or libzbc,
else it defaults to 128.

The max_open_zones() could look like this (regardless of kernel version):
If sg_inq returns something: use the result as --max_open_zones.
If sq_inq doesn't return something omit --max_open_zones completely (or send
in 0), and fio will try to parse sysfs (or use libzbc if libzbc ioengine was
used). If fio fails (regardless of ioengine), it will default to 4096.

-The default of 128 can be removed from the test script. 4096 is just as good
random number when we cannot get the real value as 128.
-The libzbc parsing can be removed from the test script.
-No need to add sysfs parsing to the test script.

We still need a max_open_zones() function in the test script to call sg_inq.
So the max_open_zones() function always has to be there. It is just the
--max_open_zones parameter than sometimes can be excluded (or specified as
zero).


BTW, do we really need to keep the sg_inc call at all?
libzbc ioengine has both a SCSI and a SCSI generic backend.
So right now, it seems that the only reason max_open_zones() function has
for living is basically if someone runs fio against a SMR drive, on a kernel
older than v5.9, using an ioengine different from libzbc.

That is probably reason enough for living. But we should definitely have a
comment above the max_open_zones() function itself that states that this
function only exists in case the user runs against an SMR drive, on an old
kernel, with an ioengine != libzbc ioengine. (And libzbc ioengine is probably
the most appropriate ioengine is such a scenario.)


Kind regards,
Niklas

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

[Shinichiro Kawasaki]

On Jun 01, 2021 / 10:45, Niklas Cassel wrote:
> On Tue, Jun 01, 2021 at 09:01:45AM +0000, Shinichiro Kawasaki wrote:
> > On Jun 01, 2021 / 07:19, Damien Le Moal wrote:
> > > On 2021/06/01 16:15, Niklas Cassel wrote:
> > > > On Tue, Jun 01, 2021 at 07:01:18AM +0000, Damien Le Moal wrote:
> > > >> On 2021/06/01 15:48, Shin'ichiro Kawasaki wrote:
> > > >>> Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
> > > >>> block device operation") modified fio to compare --max_open_zoned option
> > > >>
> > > >> s/max_open_zoned/max_open_zones
> > 
> > Thanks. Will reflect.
> > 
> > > >>
> > > >>> 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.
> > > >>
> > > >> If sysfs has the max_open_zones attribute file present, that value should be
> > > >> retrieved first. libzbc and sg_inq should be used only if the sysfs attribute is
> > > >> not present, that is, when running on older kernels.
> > > > 
> > > > Since we can assume that the test script and fio versions are updated in sync,
> > > > I don't see any point of duplicating the sysfs parsing in the test script.
> > > > 
> > > > If the test script either specifies --max_open_zones=0 or omits the parameter,
> > > > fio itself will either parse sysfs, or use libzbc to fetch the max limit.
> > > 
> > > True. Unless the user is running the tests with some special max_open_zones, we
> > > should not try to grab any value then.
> > > 
> > > > 
> > > > So I think that the test script should only use those "methods" that fio
> > > > itself can't do.
> > > > Right now fio can parse sysfs and use libzbc.
> > > > The only remaining method is sg_inq. This only I assume that we want to
> > > > keep in the test. (I guess the alternative is to implement the
> > > > .get_max_open_zones() fio ioengine callback in engines/sg.c, that does
> > > > the same. But we probably don't want to do that, right?)
> > > 
> > > No, we certainly do not. That would need a lot more code than that, e.g. to also
> > > detect zone model. That would mean basically re-implementing what libzbc does
> > > inside the sg engine. And since we now have the libzbc engine, I do not see the
> > > point.
> > > 
> > > > 
> > > >>
> > > >>>
> > > >>> To avoid the failure, modify the default value used in the test script
> > > >>> from 128 to 0. With this, --max_open_zoned=0 is passed to fio, and it
> > > >>> makes fio use the max_open_zones reported by the device.
> > > >>
> > > >> Why pass anything at all since now fio will use the device reported limit by
> > > >> default if the user does not specify anything ?
> > > > 
> > > > Sure, we can just omit the --max_open_zones parameter completely.
> > > > That might be more clear.
> > 
> > I think still we need the --max_open_zones option in case the latest fio is
> > run on older kernel which does not have max_open_zones in sysfs. In such case,
> > fio is not able to obtain the correct max_open_zones of the device (unless it
> > uses libzbc I/O engine).
> > 
> > There are only two test cases which refers the max_open_zones that the test
> > script obtained: #31 and #32. The test case #31 has a check if the found
> > max_open_zones is zero or not to decide write target zone count. The
> > max_open_zones value is not used for --max_open_zones option (can not omit it).
> > As for the test case #32, it specifies "--max_open_zones=$max_open_zones" to the
> > fio command. I assume this option is still valid and required for the older
> > kernels. We could add a check to the test script to specify this option only
> > when max_open_zones is zero, but I think it's the better not to add such check
> > to keep the test script simpler.
> 
> Test case 31 is just poorly designed IMHO.
> 
> I don't like that it wants to know max_open_zones, but then it doesn't even
> send in that value to fio.
> The only reason why it fetches max_open_zones is to save time.
> We can come up with some other way, e.g. a random max limit, 10 GB, divide
> that by zone_size. That is the amount of zones that we decide to write.
> And each zone is written fully.
> 
> Then, since test case 31 is a random read test, just read the zones that we
> wrote. Right now we will write <= (all zones)/2, but we read _all_ zones.
> 
> Since half zones will be empty, this is not an accurate test of the random
> read performance.
> 
> (Perhaps you could make the argument that all zones that you read from
> should be in zone state implicit open, but that does not seem to be the case
> currently, as many zones will most likely be empty.
> My suggestion is that we simply write each zone fully, that way, all zones
> that we read from will be in zone state FULL.)

I think we are going beyond the scope of this patch series. The design of the
test case may not be the best, but it is somewhat reasonable and valid. I wish
we fix the test case failure first. Let's defer test design change or test
condition change at this moment until we really need to relook them.

> 
> 
> 
> 
> Test case 32:
> 
> On fio master the test script currently checks sg_inq (scsi generic) or libzbc,
> else it defaults to 128.
> 
> The max_open_zones() could look like this (regardless of kernel version):
> If sg_inq returns something: use the result as --max_open_zones.
> If sq_inq doesn't return something omit --max_open_zones completely (or send
> in 0), and fio will try to parse sysfs (or use libzbc if libzbc ioengine was
> used). If fio fails (regardless of ioengine), it will default to 4096.
> 
> -The default of 128 can be removed from the test script. 4096 is just as good
> random number when we cannot get the real value as 128.

Right, the patch does so.

> -The libzbc parsing can be removed from the test script.

Maybe this is a good idea, but I would like to defer it for later chance. It
will need certain amount of effort to check libzbc tool condition corner cases.

> -No need to add sysfs parsing to the test script.

Right, the patch does not add it.

> 
> We still need a max_open_zones() function in the test script to call sg_inq.
> So the max_open_zones() function always has to be there. It is just the
> --max_open_zones parameter than sometimes can be excluded (or specified as
> zero).
> 
> 
> BTW, do we really need to keep the sg_inc call at all?
> libzbc ioengine has both a SCSI and a SCSI generic backend.
> So right now, it seems that the only reason max_open_zones() function has
> for living is basically if someone runs fio against a SMR drive, on a kernel
> older than v5.9, using an ioengine different from libzbc.

I also think the conditions you described above is what we aim. When older
kernel gets the burden for us, we can discuss later.

> 
> That is probably reason enough for living. But we should definitely have a
> comment above the max_open_zones() function itself that states that this
> function only exists in case the user runs against an SMR drive, on an old
> kernel, with an ioengine != libzbc ioengine. (And libzbc ioengine is probably
> the most appropriate ioengine is such a scenario.)

I agree to add the comments. Will add it and resend v2. I don't think libzbc is
the only ioengine for SMR drives even for older Linux kernels. The block layer
has been supporting zoned block devices since kernel version 4.10. (This
discussion may depend on the definition of the word 'old', though :)

-- 
Best Regards,
Shin'ichiro Kawasaki