* [PATCH] zbd: Fix max_open_zones checks
@ 2020-05-27 1:20 Shin'ichiro Kawasaki
2020-05-27 3:50 ` Damien Le Moal
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Shin'ichiro Kawasaki @ 2020-05-27 1:20 UTC (permalink / raw)
To: fio, Jens Axboe; +Cc: Alexey Dobriyan, Damien Le Moal, Shinichiro Kawasaki
Commit 219c662d3b12 ("zbd: introduce per job maximum open zones limit")
introduced job_max_open_zones option which limits the number of open
zones per job. It has similar role as max_open_zones option which limits
the number of open zones for all jobs. It was intended that these two
options both work, but the commit replaced some checks for max_open_zones
simply with checks for job_max_open_zones. Because of this, when
max_open_zones is set and job_max_open_zones is not set, fio fails to
limit the number of open zones. This resulted in test case #29 failure
of t/zbd/test-zbd-support script for regular null_blk devices.
To fix the failure, modify the checks to target both job_max_open_zones
and max_open_zones.
Fixes: 219c662d3b12 ("zbd: introduce per job maximum open zones limit")
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
zbd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/zbd.c b/zbd.c
index 72352db0..2a725c1f 100644
--- a/zbd.c
+++ b/zbd.c
@@ -997,7 +997,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
assert(is_valid_offset(f, io_u->offset));
- if (td->o.job_max_open_zones) {
+ if (td->o.max_open_zones || td->o.job_max_open_zones) {
/*
* This statement accesses f->zbd_info->open_zones[] on purpose
* without locking.
@@ -1026,7 +1026,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
zone_lock(td, f, z);
pthread_mutex_lock(&f->zbd_info->mutex);
- if (td->o.job_max_open_zones == 0)
+ if (td->o.max_open_zones == 0 && td->o.job_max_open_zones == 0)
goto examine_zone;
if (f->zbd_info->num_open_zones == 0) {
pthread_mutex_unlock(&f->zbd_info->mutex);
@@ -1082,7 +1082,7 @@ examine_zone:
}
dprint(FD_ZBD, "%s(%s): closing zone %d\n", __func__, f->file_name,
zone_idx);
- if (td->o.job_max_open_zones)
+ if (td->o.max_open_zones || td->o.job_max_open_zones)
zbd_close_zone(td, f, open_zone_idx);
pthread_mutex_unlock(&f->zbd_info->mutex);
--
2.25.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] zbd: Fix max_open_zones checks
2020-05-27 1:20 [PATCH] zbd: Fix max_open_zones checks Shin'ichiro Kawasaki
@ 2020-05-27 3:50 ` Damien Le Moal
2020-06-04 1:43 ` Shinichiro Kawasaki
2020-05-27 8:34 ` Alexey Dobriyan
2020-06-04 2:15 ` Jens Axboe
2 siblings, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2020-05-27 3:50 UTC (permalink / raw)
To: Shinichiro Kawasaki, fio, Jens Axboe; +Cc: Alexey Dobriyan
On 2020/05/27 10:20, Shin'ichiro Kawasaki wrote:
> Commit 219c662d3b12 ("zbd: introduce per job maximum open zones limit")
> introduced job_max_open_zones option which limits the number of open
> zones per job. It has similar role as max_open_zones option which limits
> the number of open zones for all jobs. It was intended that these two
> options both work, but the commit replaced some checks for max_open_zones
> simply with checks for job_max_open_zones. Because of this, when
> max_open_zones is set and job_max_open_zones is not set, fio fails to
> limit the number of open zones. This resulted in test case #29 failure
> of t/zbd/test-zbd-support script for regular null_blk devices.
>
> To fix the failure, modify the checks to target both job_max_open_zones
> and max_open_zones.
>
> Fixes: 219c662d3b12 ("zbd: introduce per job maximum open zones limit")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
> zbd.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/zbd.c b/zbd.c
> index 72352db0..2a725c1f 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -997,7 +997,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
>
> assert(is_valid_offset(f, io_u->offset));
>
> - if (td->o.job_max_open_zones) {
> + if (td->o.max_open_zones || td->o.job_max_open_zones) {
> /*
> * This statement accesses f->zbd_info->open_zones[] on purpose
> * without locking.
> @@ -1026,7 +1026,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
>
> zone_lock(td, f, z);
> pthread_mutex_lock(&f->zbd_info->mutex);
> - if (td->o.job_max_open_zones == 0)
> + if (td->o.max_open_zones == 0 && td->o.job_max_open_zones == 0)
> goto examine_zone;
> if (f->zbd_info->num_open_zones == 0) {
> pthread_mutex_unlock(&f->zbd_info->mutex);
> @@ -1082,7 +1082,7 @@ examine_zone:
> }
> dprint(FD_ZBD, "%s(%s): closing zone %d\n", __func__, f->file_name,
> zone_idx);
> - if (td->o.job_max_open_zones)
> + if (td->o.max_open_zones || td->o.job_max_open_zones)
> zbd_close_zone(td, f, open_zone_idx);
> pthread_mutex_unlock(&f->zbd_info->mutex);
>
>
Looks good.
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] zbd: Fix max_open_zones checks
2020-05-27 1:20 [PATCH] zbd: Fix max_open_zones checks Shin'ichiro Kawasaki
2020-05-27 3:50 ` Damien Le Moal
@ 2020-05-27 8:34 ` Alexey Dobriyan
2020-05-27 8:40 ` Damien Le Moal
2020-06-04 2:15 ` Jens Axboe
2 siblings, 1 reply; 6+ messages in thread
From: Alexey Dobriyan @ 2020-05-27 8:34 UTC (permalink / raw)
To: Shin'ichiro Kawasaki; +Cc: fio, Jens Axboe, Damien Le Moal
On Wed, May 27, 2020 at 10:20:13AM +0900, Shin'ichiro Kawasaki wrote:
> Commit 219c662d3b12 ("zbd: introduce per job maximum open zones limit")
> introduced job_max_open_zones option which limits the number of open
> zones per job. It has similar role as max_open_zones option which limits
> the number of open zones for all jobs. It was intended that these two
> options both work, but the commit replaced some checks for max_open_zones
> simply with checks for job_max_open_zones. Because of this, when
> max_open_zones is set and job_max_open_zones is not set, fio fails to
> limit the number of open zones. This resulted in test case #29 failure
> of t/zbd/test-zbd-support script for regular null_blk devices.
Isn't this test broken for the rationale behind "job_max_open_zones="?
opts+=("--zonemode=zbd"...
opts+=("--ioengine=psync" "--rw=randwrite" "--direct=1")
opts+=("--max_open_zones=4" "--group_reporting=1")
check_written $((jobs * zone_size)) || return $?
Strict equality check but one thread can open all 4 zones and make
others quit.
> To fix the failure, modify the checks to target both job_max_open_zones
> and max_open_zones.
> --- a/zbd.c
> +++ b/zbd.c
> @@ -997,7 +997,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
>
> assert(is_valid_offset(f, io_u->offset));
>
> - if (td->o.job_max_open_zones) {
> + if (td->o.max_open_zones || td->o.job_max_open_zones) {
> /*
> * This statement accesses f->zbd_info->open_zones[] on purpose
> * without locking.
> @@ -1026,7 +1026,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
>
> zone_lock(td, f, z);
> pthread_mutex_lock(&f->zbd_info->mutex);
> - if (td->o.job_max_open_zones == 0)
> + if (td->o.max_open_zones == 0 && td->o.job_max_open_zones == 0)
> goto examine_zone;
> if (f->zbd_info->num_open_zones == 0) {
> pthread_mutex_unlock(&f->zbd_info->mutex);
> @@ -1082,7 +1082,7 @@ examine_zone:
> }
> dprint(FD_ZBD, "%s(%s): closing zone %d\n", __func__, f->file_name,
> zone_idx);
> - if (td->o.job_max_open_zones)
> + if (td->o.max_open_zones || td->o.job_max_open_zones)
> zbd_close_zone(td, f, open_zone_idx);
> pthread_mutex_unlock(&f->zbd_info->mutex);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] zbd: Fix max_open_zones checks
2020-05-27 8:34 ` Alexey Dobriyan
@ 2020-05-27 8:40 ` Damien Le Moal
0 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2020-05-27 8:40 UTC (permalink / raw)
To: Alexey Dobriyan, Shinichiro Kawasaki; +Cc: fio, Jens Axboe
On 2020/05/27 17:34, Alexey Dobriyan wrote:
> On Wed, May 27, 2020 at 10:20:13AM +0900, Shin'ichiro Kawasaki wrote:
>> Commit 219c662d3b12 ("zbd: introduce per job maximum open zones limit")
>> introduced job_max_open_zones option which limits the number of open
>> zones per job. It has similar role as max_open_zones option which limits
>> the number of open zones for all jobs. It was intended that these two
>> options both work, but the commit replaced some checks for max_open_zones
>> simply with checks for job_max_open_zones. Because of this, when
>> max_open_zones is set and job_max_open_zones is not set, fio fails to
>> limit the number of open zones. This resulted in test case #29 failure
>> of t/zbd/test-zbd-support script for regular null_blk devices.
>
> Isn't this test broken for the rationale behind "job_max_open_zones="?
>
> opts+=("--zonemode=zbd"...
> opts+=("--ioengine=psync" "--rw=randwrite" "--direct=1")
> opts+=("--max_open_zones=4" "--group_reporting=1")
> check_written $((jobs * zone_size)) || return $?
>
> Strict equality check but one thread can open all 4 zones and make
> others quit.
That is the legacy behavior that your patch allows changing with the new
job_max_open_zones option. Adding this option should not break existing scripts,
even though they are not "optimal".
Could you add some test cases to t/zbd/test-zbd-support for the
job_max_open_zones option and combinations of job_max_open_zones and
max_open_zones options ? That will facilitate regression testing going forward.
>
>> To fix the failure, modify the checks to target both job_max_open_zones
>> and max_open_zones.
>
>> --- a/zbd.c
>> +++ b/zbd.c
>> @@ -997,7 +997,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
>>
>> assert(is_valid_offset(f, io_u->offset));
>>
>> - if (td->o.job_max_open_zones) {
>> + if (td->o.max_open_zones || td->o.job_max_open_zones) {
>> /*
>> * This statement accesses f->zbd_info->open_zones[] on purpose
>> * without locking.
>> @@ -1026,7 +1026,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
>>
>> zone_lock(td, f, z);
>> pthread_mutex_lock(&f->zbd_info->mutex);
>> - if (td->o.job_max_open_zones == 0)
>> + if (td->o.max_open_zones == 0 && td->o.job_max_open_zones == 0)
>> goto examine_zone;
>> if (f->zbd_info->num_open_zones == 0) {
>> pthread_mutex_unlock(&f->zbd_info->mutex);
>> @@ -1082,7 +1082,7 @@ examine_zone:
>> }
>> dprint(FD_ZBD, "%s(%s): closing zone %d\n", __func__, f->file_name,
>> zone_idx);
>> - if (td->o.job_max_open_zones)
>> + if (td->o.max_open_zones || td->o.job_max_open_zones)
>> zbd_close_zone(td, f, open_zone_idx);
>> pthread_mutex_unlock(&f->zbd_info->mutex);
>>
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] zbd: Fix max_open_zones checks
2020-05-27 3:50 ` Damien Le Moal
@ 2020-06-04 1:43 ` Shinichiro Kawasaki
0 siblings, 0 replies; 6+ messages in thread
From: Shinichiro Kawasaki @ 2020-06-04 1:43 UTC (permalink / raw)
To: Damien Le Moal; +Cc: fio, Jens Axboe, Alexey Dobriyan
On May 27, 2020 / 03:50, Damien Le Moal wrote:
> On 2020/05/27 10:20, Shin'ichiro Kawasaki wrote:
> > Commit 219c662d3b12 ("zbd: introduce per job maximum open zones limit")
> > introduced job_max_open_zones option which limits the number of open
> > zones per job. It has similar role as max_open_zones option which limits
> > the number of open zones for all jobs. It was intended that these two
> > options both work, but the commit replaced some checks for max_open_zones
> > simply with checks for job_max_open_zones. Because of this, when
> > max_open_zones is set and job_max_open_zones is not set, fio fails to
> > limit the number of open zones. This resulted in test case #29 failure
> > of t/zbd/test-zbd-support script for regular null_blk devices.
> >
> > To fix the failure, modify the checks to target both job_max_open_zones
> > and max_open_zones.
> >
> > Fixes: 219c662d3b12 ("zbd: introduce per job maximum open zones limit")
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> > zbd.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/zbd.c b/zbd.c
> > index 72352db0..2a725c1f 100644
> > --- a/zbd.c
> > +++ b/zbd.c
> > @@ -997,7 +997,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
> >
> > assert(is_valid_offset(f, io_u->offset));
> >
> > - if (td->o.job_max_open_zones) {
> > + if (td->o.max_open_zones || td->o.job_max_open_zones) {
> > /*
> > * This statement accesses f->zbd_info->open_zones[] on purpose
> > * without locking.
> > @@ -1026,7 +1026,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
> >
> > zone_lock(td, f, z);
> > pthread_mutex_lock(&f->zbd_info->mutex);
> > - if (td->o.job_max_open_zones == 0)
> > + if (td->o.max_open_zones == 0 && td->o.job_max_open_zones == 0)
> > goto examine_zone;
> > if (f->zbd_info->num_open_zones == 0) {
> > pthread_mutex_unlock(&f->zbd_info->mutex);
> > @@ -1082,7 +1082,7 @@ examine_zone:
> > }
> > dprint(FD_ZBD, "%s(%s): closing zone %d\n", __func__, f->file_name,
> > zone_idx);
> > - if (td->o.job_max_open_zones)
> > + if (td->o.max_open_zones || td->o.job_max_open_zones)
> > zbd_close_zone(td, f, open_zone_idx);
> > pthread_mutex_unlock(&f->zbd_info->mutex);
> >
> >
>
> Looks good.
>
> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
Jens,
Could you consider to pick up this fix for upstream? I think it is good to keep
the max_open_zones option behavior same as before to avoid confusions.
--
Best Regards,
Shin'ichiro Kawasaki
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] zbd: Fix max_open_zones checks
2020-05-27 1:20 [PATCH] zbd: Fix max_open_zones checks Shin'ichiro Kawasaki
2020-05-27 3:50 ` Damien Le Moal
2020-05-27 8:34 ` Alexey Dobriyan
@ 2020-06-04 2:15 ` Jens Axboe
2 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2020-06-04 2:15 UTC (permalink / raw)
To: Shin'ichiro Kawasaki, fio; +Cc: Alexey Dobriyan, Damien Le Moal
On 5/26/20 7:20 PM, Shin'ichiro Kawasaki wrote:
> Commit 219c662d3b12 ("zbd: introduce per job maximum open zones limit")
> introduced job_max_open_zones option which limits the number of open
> zones per job. It has similar role as max_open_zones option which limits
> the number of open zones for all jobs. It was intended that these two
> options both work, but the commit replaced some checks for max_open_zones
> simply with checks for job_max_open_zones. Because of this, when
> max_open_zones is set and job_max_open_zones is not set, fio fails to
> limit the number of open zones. This resulted in test case #29 failure
> of t/zbd/test-zbd-support script for regular null_blk devices.
>
> To fix the failure, modify the checks to target both job_max_open_zones
> and max_open_zones.
Applied, thanks.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-04 2:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 1:20 [PATCH] zbd: Fix max_open_zones checks Shin'ichiro Kawasaki
2020-05-27 3:50 ` Damien Le Moal
2020-06-04 1:43 ` Shinichiro Kawasaki
2020-05-27 8:34 ` Alexey Dobriyan
2020-05-27 8:40 ` Damien Le Moal
2020-06-04 2:15 ` Jens Axboe
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.