[PATCH 0/2] zbd: fix open zone status check for non-write jobs

[PATCH 0/2] zbd: fix open zone status check for non-write jobs

[Shin'ichiro Kawasaki]

Currently, fio checks if each zone within the IO range is in an open
condition or not. This check is done even if the job does not perform
any write operations, and causes confusion among users.

This series addresses the problem. The first patch fixes it, and the
second patch adds a test case to confirm the fix.

Shin'ichiro Kawasaki (2):
  zbd: do not check open zones status and limits when jobs do not write
  t/zbd: add test case to confirm no max_open_zones limit check

 t/zbd/test-zbd-support | 29 +++++++++++++++++++++++++++++
 zbd.c                  | 10 ++++++++++
 2 files changed, 39 insertions(+)

-- 
2.47.0

[PATCH 1/2] zbd: do not check open zones status and limits when jobs do not write

[Shin'ichiro Kawasaki]

Currently, fio checks the conditions of each zone within the IO range at
the job start. If a zone is in an open condition, it is added to the
write target zone array. If the number of write target zones exceeds the
max_open_zones or job_max_open_zones limit, fio terminates with the
error message "Number of open zones exceeds max_open_zones limit". This
check for zone condition and the resulting termination occur even when
the job does not perform a write operation, leading to confusion among
users.

To avoid the confusion, skip the check when jobs do not perform write
operations. Additionally, print the message to inform that the
job_max_open_zones limit does not work for non-write jobs.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 zbd.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/zbd.c b/zbd.c
index 8a092cbe..ee095b1d 100644
--- a/zbd.c
+++ b/zbd.c
@@ -1264,6 +1264,16 @@ int zbd_setup_files(struct thread_data *td)
 			return 1;
 		}
 
+		/*
+		 * If this job does not do write operations, skip open zone
+		 * condition check.
+		 */
+		if (!td_write(td)) {
+			if (td->o.job_max_open_zones)
+				log_info("'job_max_open_zones' is valid only for write jobs\n");
+			continue;
+		}
+
 		/*
 		 * The per job max open zones limit cannot be used without a
 		 * global max open zones limit. (As the tracking of open zones
-- 
2.47.0

Re: [PATCH 1/2] zbd: do not check open zones status and limits when jobs do not write

[Niklas Cassel]

On Tue, Jan 21, 2025 at 05:39:12PM +0900, Shin'ichiro Kawasaki wrote:
> Currently, fio checks the conditions of each zone within the IO range at
> the job start. If a zone is in an open condition, it is added to the
> write target zone array. If the number of write target zones exceeds the
> max_open_zones or job_max_open_zones limit, fio terminates with the
> error message "Number of open zones exceeds max_open_zones limit". This
> check for zone condition and the resulting termination occur even when
> the job does not perform a write operation, leading to confusion among
> users.
> 
> To avoid the confusion, skip the check when jobs do not perform write
> operations. Additionally, print the message to inform that the
> job_max_open_zones limit does not work for non-write jobs.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  zbd.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/zbd.c b/zbd.c
> index 8a092cbe..ee095b1d 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -1264,6 +1264,16 @@ int zbd_setup_files(struct thread_data *td)
>  			return 1;
>  		}
>  
> +		/*
> +		 * If this job does not do write operations, skip open zone
> +		 * condition check.
> +		 */
> +		if (!td_write(td)) {
> +			if (td->o.job_max_open_zones)
> +				log_info("'job_max_open_zones' is valid only for write jobs\n");
> +			continue;
> +		}
> +

zbd_setup_files() currently loops from f->min_zone to f->max_zone.

Since this check is now skipped if the job does notdo write operations,
could perhaps zbd_setup_files() be changed to loop from f->write_min_zone
to f->write_max_zone instead?


Kind regards,
Niklas

Re: [PATCH 1/2] zbd: do not check open zones status and limits when jobs do not write

[Shinichiro Kawasaki]

On Jan 21, 2025 / 09:17, Niklas Cassel wrote:
> On Tue, Jan 21, 2025 at 05:39:12PM +0900, Shin'ichiro Kawasaki wrote:
> > Currently, fio checks the conditions of each zone within the IO range at
> > the job start. If a zone is in an open condition, it is added to the
> > write target zone array. If the number of write target zones exceeds the
> > max_open_zones or job_max_open_zones limit, fio terminates with the
> > error message "Number of open zones exceeds max_open_zones limit". This
> > check for zone condition and the resulting termination occur even when
> > the job does not perform a write operation, leading to confusion among
> > users.
> > 
> > To avoid the confusion, skip the check when jobs do not perform write
> > operations. Additionally, print the message to inform that the
> > job_max_open_zones limit does not work for non-write jobs.
> > 
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >  zbd.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/zbd.c b/zbd.c
> > index 8a092cbe..ee095b1d 100644
> > --- a/zbd.c
> > +++ b/zbd.c
> > @@ -1264,6 +1264,16 @@ int zbd_setup_files(struct thread_data *td)
> >  			return 1;
> >  		}
> >  
> > +		/*
> > +		 * If this job does not do write operations, skip open zone
> > +		 * condition check.
> > +		 */
> > +		if (!td_write(td)) {
> > +			if (td->o.job_max_open_zones)
> > +				log_info("'job_max_open_zones' is valid only for write jobs\n");
> > +			continue;
> > +		}
> > +
> 
> zbd_setup_files() currently loops from f->min_zone to f->max_zone.
> 
> Since this check is now skipped if the job does notdo write operations,
> could perhaps zbd_setup_files() be changed to loop from f->write_min_zone
> to f->write_max_zone instead?

I don't think so. Actually, write_in_zone and write_max_zone are not the
mebmer of struct fio_file. They are member of struct zoned_block_device_info,
which is shared across jobs (td) and files (f). zbd_setup_files() is called
in the loop for jobs (td), so I think the loop in zbd_setup_files() should
refer to f->min_zone and f->max_zone, which are unique to each job (td) and
file (f).

[PATCH 2/2] t/zbd: add test case to confirm no max_open_zones limit check

[Shin'ichiro Kawasaki]

The previous commit fixed the max_open_zones limit check for non-write
jobs. Add a test case to confirm the fix.

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

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index e0b2a755..468fce70 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -1608,6 +1608,35 @@ test69() {
 		>> "${logfile}.${test_number}" 2>&1 || return $?
 }
 
+# Test max_open_zones and job_max_open_zones do not error out for non-write jobs
+test70() {
+	require_zbd || return "$SKIP_TESTCASE"
+
+	reset_zone "${dev}" -1
+
+	# Write data to two zones and make them open
+	run_fio_on_seq "$(ioengine "psync")" --io_size="$min_seq_write_size" \
+		       --rw=write --offset_increment=1z --numjobs=2 \
+		       --group_reporting=1 >> "${logfile}.${test_number}" 2>&1
+
+	# Confirm max_open_zones=1 for read workload does not fail
+	run_fio_on_seq "$(ioengine "psync")" --io_size="$min_seq_write_size" \
+		       --rw=read --max_open_zones=1 \
+		       >> "${logfile}.${test_number}" 2>&1 || return $?
+
+	# Confirm job_max_open_zones=1 for read workload does not fail
+	run_fio_on_seq "$(ioengine "psync")" --io_size="$min_seq_write_size" \
+		       --rw=read --job_max_open_zones=1 \
+		       >> "${logfile}.${test_number}" 2>&1
+	grep -q 'valid only for write jobs' \
+	     "${logfile}.${test_number}" || return $?
+
+	# Confirm max_open_zones=1 for trim workload does not fail
+	run_fio_on_seq "$(ioengine "psync")" --rw=trim --io_size=1z \
+		       --bs="$zone_size" --max_open_zones=1 \
+		       >> "${logfile}.${test_number}" 2>&1
+}
+
 SECONDS=0
 tests=()
 dynamic_analyzer=()
-- 
2.47.0

Re: [PATCH 0/2] zbd: fix open zone status check for non-write jobs

[fiotestbot]

[-- Attachment #1: Type: text/plain, Size: 141 bytes --]


The result of fio's continuous integration tests was: None

For more details see https://github.com/fiotestbot/fio/actions/runs/12883126116

Re: [PATCH 0/2] zbd: fix open zone status check for non-write jobs

[Niklas Cassel]

Hello Shin'ichiro,

On Tue, Jan 21, 2025 at 05:39:11PM +0900, Shin'ichiro Kawasaki wrote:
> Currently, fio checks if each zone within the IO range is in an open
> condition or not. This check is done even if the job does not perform
> any write operations, and causes confusion among users.
> 
> This series addresses the problem. The first patch fixes it, and the
> second patch adds a test case to confirm the fix.

If the first patch fixes something, then perhaps it should have a Fixes tag?

I guess the commit
8ac768899d63 ("zbd: do not reset extra zones in open conditions")

is the proper one to reference here?


Kind regards,
Niklas


> 
> Shin'ichiro Kawasaki (2):
>   zbd: do not check open zones status and limits when jobs do not write
>   t/zbd: add test case to confirm no max_open_zones limit check
> 
>  t/zbd/test-zbd-support | 29 +++++++++++++++++++++++++++++
>  zbd.c                  | 10 ++++++++++
>  2 files changed, 39 insertions(+)
> 
> -- 
> 2.47.0
> 
> 

Re: [PATCH 0/2] zbd: fix open zone status check for non-write jobs

[Shinichiro Kawasaki]

On Jan 21, 2025 / 09:14, Niklas Cassel wrote:
> Hello Shin'ichiro,
> 
> On Tue, Jan 21, 2025 at 05:39:11PM +0900, Shin'ichiro Kawasaki wrote:
> > Currently, fio checks if each zone within the IO range is in an open
> > condition or not. This check is done even if the job does not perform
> > any write operations, and causes confusion among users.
> > 
> > This series addresses the problem. The first patch fixes it, and the
> > second patch adds a test case to confirm the fix.
> 
> If the first patch fixes something, then perhaps it should have a Fixes tag?

Yes, agreed.

> 
> I guess the commit
> 8ac768899d63 ("zbd: do not reset extra zones in open conditions")
> 
> is the proper one to reference here?

Another candidate is this:
954217b90191 ("zbd: Initialize open zones list referring zone status at fio start")

I think it will be the better reference to add two Fixes tags for both. Will do
so in v2.