* [PATCH] zbd: fix sequential verify with max_open_zones=1
@ 2020-04-08 22:16 Alexey Dobriyan
2020-04-09 1:58 ` Damien Le Moal
0 siblings, 1 reply; 9+ messages in thread
From: Alexey Dobriyan @ 2020-04-08 22:16 UTC (permalink / raw)
To: axboe; +Cc: damien.lemoal, fio
Sequential write with max_open_zones=1 has interesting (read: buggy)
interaction with verify=.
If verify is off, then job runs correctly and IO is sequential,
and restarted from offset 0 and remains sequential.
If verify is on, then 1 full run is done and verified correctly.
At this point there is exactly 1 open zone which is the last zone.
Now IO restarts from offset 0 and pick_random_zone() picks opened zone
#0 which is the last zone because offset is 0. All IO is redirected
to the last zone, which is rewritten once triggering verify again.
IO pattern becomes: 1 full sequential rewrite followed by constant
sequential rewrites of the last zone.
[global]
filename=/dev/loop0
direct=1
zonemode=zbd
zonesize=1M
bs=512K
rw=write
verify=xxhash
[j]
max_open_zones=1
io_size=3G
Fix is to close everything knowing that "full reset" comes from verify.
max_open_zones=2 restarts from half of the device, etc.
Signed-off-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com>
---
zbd.c | 53 ++++++++++++++++++++++++++++++++++-------------------
1 file changed, 34 insertions(+), 19 deletions(-)
--- a/zbd.c
+++ b/zbd.c
@@ -710,6 +710,22 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
return zbd_reset_range(td, f, z->start, (z+1)->start - z->start);
}
+/* The caller must hold f->zbd_info->mutex */
+static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
+ unsigned int open_zone_idx)
+{
+ uint32_t zone_idx;
+
+ assert(open_zone_idx < f->zbd_info->num_open_zones);
+ zone_idx = f->zbd_info->open_zones[open_zone_idx];
+ memmove(f->zbd_info->open_zones + open_zone_idx,
+ f->zbd_info->open_zones + open_zone_idx + 1,
+ (ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
+ sizeof(f->zbd_info->open_zones[0]));
+ f->zbd_info->num_open_zones--;
+ f->zbd_info->zone_info[zone_idx].open = 0;
+}
+
/*
* Reset a range of zones. Returns 0 upon success and 1 upon failure.
* @td: fio thread data.
@@ -733,12 +749,27 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
dprint(FD_ZBD, "%s: examining zones %u .. %u\n", f->file_name,
zbd_zone_nr(f->zbd_info, zb), zbd_zone_nr(f->zbd_info, ze));
for (z = zb; z < ze; z++) {
+ uint32_t nz = z - f->zbd_info->zone_info;
+
if (!zbd_zone_swr(z))
continue;
zone_lock(td, z);
- reset_wp = all_zones ? z->wp != z->start :
- (td->o.td_ddir & TD_DDIR_WRITE) &&
- z->wp % min_bs != 0;
+ if (all_zones) {
+ unsigned int i;
+
+ pthread_mutex_lock(&f->zbd_info->mutex);
+ for (i = 0; i < f->zbd_info->num_open_zones; i++) {
+ if (f->zbd_info->open_zones[i] == nz) {
+ zbd_close_zone(td, f, i);
+ }
+ }
+ pthread_mutex_unlock(&f->zbd_info->mutex);
+
+ reset_wp = z->wp != z->start;
+ } else {
+ reset_wp = (td->o.td_ddir & TD_DDIR_WRITE) &&
+ z->wp % min_bs != 0;
+ }
if (reset_wp) {
dprint(FD_ZBD, "%s: resetting zone %u\n",
f->file_name,
@@ -928,22 +959,6 @@ out:
return res;
}
-/* The caller must hold f->zbd_info->mutex */
-static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
- unsigned int open_zone_idx)
-{
- uint32_t zone_idx;
-
- assert(open_zone_idx < f->zbd_info->num_open_zones);
- zone_idx = f->zbd_info->open_zones[open_zone_idx];
- memmove(f->zbd_info->open_zones + open_zone_idx,
- f->zbd_info->open_zones + open_zone_idx + 1,
- (ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
- sizeof(f->zbd_info->open_zones[0]));
- f->zbd_info->num_open_zones--;
- f->zbd_info->zone_info[zone_idx].open = 0;
-}
-
/* Anything goes as long as it is not a constant. */
static uint32_t pick_random_zone_idx(const struct fio_file *f,
const struct io_u *io_u)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] zbd: fix sequential verify with max_open_zones=1
2020-04-08 22:16 [PATCH] zbd: fix sequential verify with max_open_zones=1 Alexey Dobriyan
@ 2020-04-09 1:58 ` Damien Le Moal
2020-04-09 17:14 ` Alexey Dobriyan
0 siblings, 1 reply; 9+ messages in thread
From: Damien Le Moal @ 2020-04-09 1:58 UTC (permalink / raw)
To: Alexey Dobriyan, axboe@kernel.dk; +Cc: fio@vger.kernel.org
On 2020/04/09 7:16, Alexey Dobriyan wrote:
> Sequential write with max_open_zones=1 has interesting (read: buggy)
> interaction with verify=.
>
> If verify is off, then job runs correctly and IO is sequential,
> and restarted from offset 0 and remains sequential.
>
> If verify is on, then 1 full run is done and verified correctly.
> At this point there is exactly 1 open zone which is the last zone.
>
> Now IO restarts from offset 0 and pick_random_zone() picks opened zone
> #0 which is the last zone because offset is 0. All IO is redirected
> to the last zone, which is rewritten once triggering verify again.
>
> IO pattern becomes: 1 full sequential rewrite followed by constant
> sequential rewrites of the last zone.
>
>
> [global]
> filename=/dev/loop0
> direct=1
> zonemode=zbd
> zonesize=1M
> bs=512K
> rw=write
> verify=xxhash
> [j]
> max_open_zones=1
> io_size=3G
>
> Fix is to close everything knowing that "full reset" comes from verify.
>
> max_open_zones=2 restarts from half of the device, etc.
>
> Signed-off-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com>
> ---
>
> zbd.c | 53 ++++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 34 insertions(+), 19 deletions(-)
>
> --- a/zbd.c
> +++ b/zbd.c
> @@ -710,6 +710,22 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
> return zbd_reset_range(td, f, z->start, (z+1)->start - z->start);
> }
>
> +/* The caller must hold f->zbd_info->mutex */
> +static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
> + unsigned int open_zone_idx)
> +{
> + uint32_t zone_idx;
> +
> + assert(open_zone_idx < f->zbd_info->num_open_zones);
> + zone_idx = f->zbd_info->open_zones[open_zone_idx];
> + memmove(f->zbd_info->open_zones + open_zone_idx,
> + f->zbd_info->open_zones + open_zone_idx + 1,
> + (ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
> + sizeof(f->zbd_info->open_zones[0]));
> + f->zbd_info->num_open_zones--;
> + f->zbd_info->zone_info[zone_idx].open = 0;
> +}
> +
> /*
> * Reset a range of zones. Returns 0 upon success and 1 upon failure.
> * @td: fio thread data.
> @@ -733,12 +749,27 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
> dprint(FD_ZBD, "%s: examining zones %u .. %u\n", f->file_name,
> zbd_zone_nr(f->zbd_info, zb), zbd_zone_nr(f->zbd_info, ze));
> for (z = zb; z < ze; z++) {
> + uint32_t nz = z - f->zbd_info->zone_info;
> +
> if (!zbd_zone_swr(z))
> continue;
> zone_lock(td, z);
> - reset_wp = all_zones ? z->wp != z->start :
> - (td->o.td_ddir & TD_DDIR_WRITE) &&
> - z->wp % min_bs != 0;
> + if (all_zones) {
> + unsigned int i;
> +
> + pthread_mutex_lock(&f->zbd_info->mutex);
> + for (i = 0; i < f->zbd_info->num_open_zones; i++) {
> + if (f->zbd_info->open_zones[i] == nz) {
> + zbd_close_zone(td, f, i);
> + }
nit: you do not need the {} brackets for the if here.
> + }
> + pthread_mutex_unlock(&f->zbd_info->mutex);
> +
> + reset_wp = z->wp != z->start;
> + } else {
> + reset_wp = (td->o.td_ddir & TD_DDIR_WRITE) &&
> + z->wp % min_bs != 0;
> + }
> if (reset_wp) {
> dprint(FD_ZBD, "%s: resetting zone %u\n",
> f->file_name,
> @@ -928,22 +959,6 @@ out:
> return res;
> }
>
> -/* The caller must hold f->zbd_info->mutex */
> -static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
> - unsigned int open_zone_idx)
> -{
> - uint32_t zone_idx;
> -
> - assert(open_zone_idx < f->zbd_info->num_open_zones);
> - zone_idx = f->zbd_info->open_zones[open_zone_idx];
> - memmove(f->zbd_info->open_zones + open_zone_idx,
> - f->zbd_info->open_zones + open_zone_idx + 1,
> - (ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
> - sizeof(f->zbd_info->open_zones[0]));
> - f->zbd_info->num_open_zones--;
> - f->zbd_info->zone_info[zone_idx].open = 0;
> -}
> -
> /* Anything goes as long as it is not a constant. */
> static uint32_t pick_random_zone_idx(const struct fio_file *f,
> const struct io_u *io_u)
>
With the nits above fixed, this looks OK to me.
It would be good to add a test case for this to t/zbd/test-zbd-support. Can you
send something please ?
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] zbd: fix sequential verify with max_open_zones=1
2020-04-09 1:58 ` Damien Le Moal
@ 2020-04-09 17:14 ` Alexey Dobriyan
2020-04-09 22:41 ` Damien Le Moal
0 siblings, 1 reply; 9+ messages in thread
From: Alexey Dobriyan @ 2020-04-09 17:14 UTC (permalink / raw)
To: Damien Le Moal; +Cc: axboe@kernel.dk, fio@vger.kernel.org
On Thu, Apr 09, 2020 at 01:58:13AM +0000, Damien Le Moal wrote:
> On 2020/04/09 7:16, Alexey Dobriyan wrote:
> > Sequential write with max_open_zones=1 has interesting (read: buggy)
> > interaction with verify=.
> > + if (f->zbd_info->open_zones[i] == nz) {
> > + zbd_close_zone(td, f, i);
> > + }
>
> nit: you do not need the {} brackets for the if here.
I started to use {} everywhere, it is less thinking that way.
> With the nits above fixed, this looks OK to me.
>
> It would be good to add a test case for this to t/zbd/test-zbd-support. Can you
> send something please ?
I'll try. This needs a test with blktrace parsing and eveything.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] zbd: fix sequential verify with max_open_zones=1
2020-04-09 17:14 ` Alexey Dobriyan
@ 2020-04-09 22:41 ` Damien Le Moal
2020-04-10 18:46 ` Alexey Dobriyan
2020-04-11 20:18 ` Jens Axboe
0 siblings, 2 replies; 9+ messages in thread
From: Damien Le Moal @ 2020-04-09 22:41 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: axboe@kernel.dk, fio@vger.kernel.org
On 2020/04/10 2:14, Alexey Dobriyan wrote:
> On Thu, Apr 09, 2020 at 01:58:13AM +0000, Damien Le Moal wrote:
>> On 2020/04/09 7:16, Alexey Dobriyan wrote:
>>> Sequential write with max_open_zones=1 has interesting (read: buggy)
>>> interaction with verify=.
>
>>> + if (f->zbd_info->open_zones[i] == nz) {
>>> + zbd_close_zone(td, f, i);
>>> + }
>>
>> nit: you do not need the {} brackets for the if here.
>
> I started to use {} everywhere, it is less thinking that way.
Sure, but that is not the coding style normally used in fio and many other open
source C projects. I will let Jens decide here.
>
>> With the nits above fixed, this looks OK to me.
>>
>> It would be good to add a test case for this to t/zbd/test-zbd-support. Can you
>> send something please ?
>
> I'll try. This needs a test with blktrace parsing and eveything.
That sounds like an overkill. Can't we use the debug output (debug=zbd) for
checking ? If that does not work, let's not add a test that complicates the test
setup.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] zbd: fix sequential verify with max_open_zones=1
2020-04-09 22:41 ` Damien Le Moal
@ 2020-04-10 18:46 ` Alexey Dobriyan
2020-04-11 20:18 ` Jens Axboe
1 sibling, 0 replies; 9+ messages in thread
From: Alexey Dobriyan @ 2020-04-10 18:46 UTC (permalink / raw)
To: Damien Le Moal; +Cc: axboe@kernel.dk, fio@vger.kernel.org
On Thu, Apr 09, 2020 at 10:41:01PM +0000, Damien Le Moal wrote:
> On 2020/04/10 2:14, Alexey Dobriyan wrote:
> > On Thu, Apr 09, 2020 at 01:58:13AM +0000, Damien Le Moal wrote:
> >> It would be good to add a test case for this to t/zbd/test-zbd-support. Can you
> >> send something please ?
> >
> > I'll try. This needs a test with blktrace parsing and eveything.
>
> That sounds like an overkill.
I disagree. You want to see what is actually issued to the block device
in an independent way, not debug output of the same program.
> Can't we use the debug output (debug=zbd) for
> checking ? If that does not work, let's not add a test that complicates the test
> setup.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] zbd: fix sequential verify with max_open_zones=1
2020-04-09 22:41 ` Damien Le Moal
2020-04-10 18:46 ` Alexey Dobriyan
@ 2020-04-11 20:18 ` Jens Axboe
2020-04-13 18:51 ` [PATCH v2] zbd: fix sequential write pattern with verify= and max_open_zones= Alexey Dobriyan
1 sibling, 1 reply; 9+ messages in thread
From: Jens Axboe @ 2020-04-11 20:18 UTC (permalink / raw)
To: Damien Le Moal, Alexey Dobriyan; +Cc: fio@vger.kernel.org
On 4/9/20 4:41 PM, Damien Le Moal wrote:
> On 2020/04/10 2:14, Alexey Dobriyan wrote:
>> On Thu, Apr 09, 2020 at 01:58:13AM +0000, Damien Le Moal wrote:
>>> On 2020/04/09 7:16, Alexey Dobriyan wrote:
>>>> Sequential write with max_open_zones=1 has interesting (read: buggy)
>>>> interaction with verify=.
>>
>>>> + if (f->zbd_info->open_zones[i] == nz) {
>>>> + zbd_close_zone(td, f, i);
>>>> + }
>>>
>>> nit: you do not need the {} brackets for the if here.
>>
>> I started to use {} everywhere, it is less thinking that way.
>
> Sure, but that is not the coding style normally used in fio and many other open
> source C projects. I will let Jens decide here.
I prefer no braces for single lines. The exception is if/else, where if one
has braces, the rest should too. Fio isn't really consistent in the latter
I think, but definitely the former.
--
Jens Axboe
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] zbd: fix sequential write pattern with verify= and max_open_zones=
2020-04-11 20:18 ` Jens Axboe
@ 2020-04-13 18:51 ` Alexey Dobriyan
2020-04-13 23:13 ` Damien Le Moal
2020-04-13 23:18 ` Jens Axboe
0 siblings, 2 replies; 9+ messages in thread
From: Alexey Dobriyan @ 2020-04-13 18:51 UTC (permalink / raw)
To: Jens Axboe; +Cc: Damien Le Moal, fio@vger.kernel.org
Sequential write with max_open_zones=1 has interesting (read: buggy)
interaction with verify=.
If verify is off, then job runs correctly and IO is sequential,
and restarted from offset 0 and remains sequential.
If verify is on, then 1 full run is done and verified correctly.
At this point there is exactly 1 open zone which is the last zone.
Now IO restarts from offset 0 and pick_random_zone() picks opened zone
#0 which is the last zone because offset is 0. All IO is redirected
to the last zone, which is rewritten once triggering verify again.
IO pattern becomes: 1 full sequential rewrite followed by constant
sequential rewrites of the last zone.
[global]
filename=/dev/loop0
direct=1
zonemode=zbd
zonesize=1M
bs=512K
rw=write
verify=xxhash
[j]
max_open_zones=1
io_size=3G
Fix is to close every zone given that verification acts as a barrier
between jobs.
max_open_zones=2 can restart from half of the device, etc.
Signed-off-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com>
---
zbd.c | 52 +++++++++++++++++++++++++++++++++-------------------
1 file changed, 33 insertions(+), 19 deletions(-)
--- a/zbd.c
+++ b/zbd.c
@@ -687,6 +687,22 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
return zbd_reset_range(td, f, z->start, (z+1)->start - z->start);
}
+/* The caller must hold f->zbd_info->mutex */
+static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
+ unsigned int open_zone_idx)
+{
+ uint32_t zone_idx;
+
+ assert(open_zone_idx < f->zbd_info->num_open_zones);
+ zone_idx = f->zbd_info->open_zones[open_zone_idx];
+ memmove(f->zbd_info->open_zones + open_zone_idx,
+ f->zbd_info->open_zones + open_zone_idx + 1,
+ (ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
+ sizeof(f->zbd_info->open_zones[0]));
+ f->zbd_info->num_open_zones--;
+ f->zbd_info->zone_info[zone_idx].open = 0;
+}
+
/*
* Reset a range of zones. Returns 0 upon success and 1 upon failure.
* @td: fio thread data.
@@ -710,12 +726,26 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
dprint(FD_ZBD, "%s: examining zones %u .. %u\n", f->file_name,
zbd_zone_nr(f->zbd_info, zb), zbd_zone_nr(f->zbd_info, ze));
for (z = zb; z < ze; z++) {
+ uint32_t nz = z - f->zbd_info->zone_info;
+
if (!zbd_zone_swr(z))
continue;
zone_lock(td, z);
- reset_wp = all_zones ? z->wp != z->start :
- (td->o.td_ddir & TD_DDIR_WRITE) &&
- z->wp % min_bs != 0;
+ if (all_zones) {
+ unsigned int i;
+
+ pthread_mutex_lock(&f->zbd_info->mutex);
+ for (i = 0; i < f->zbd_info->num_open_zones; i++) {
+ if (f->zbd_info->open_zones[i] == nz)
+ zbd_close_zone(td, f, i);
+ }
+ pthread_mutex_unlock(&f->zbd_info->mutex);
+
+ reset_wp = z->wp != z->start;
+ } else {
+ reset_wp = (td->o.td_ddir & TD_DDIR_WRITE) &&
+ z->wp % min_bs != 0;
+ }
if (reset_wp) {
dprint(FD_ZBD, "%s: resetting zone %u\n",
f->file_name,
@@ -905,22 +935,6 @@ out:
return res;
}
-/* The caller must hold f->zbd_info->mutex */
-static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
- unsigned int open_zone_idx)
-{
- uint32_t zone_idx;
-
- assert(open_zone_idx < f->zbd_info->num_open_zones);
- zone_idx = f->zbd_info->open_zones[open_zone_idx];
- memmove(f->zbd_info->open_zones + open_zone_idx,
- f->zbd_info->open_zones + open_zone_idx + 1,
- (ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
- sizeof(f->zbd_info->open_zones[0]));
- f->zbd_info->num_open_zones--;
- f->zbd_info->zone_info[zone_idx].open = 0;
-}
-
/* Anything goes as long as it is not a constant. */
static uint32_t pick_random_zone_idx(const struct fio_file *f,
const struct io_u *io_u)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] zbd: fix sequential write pattern with verify= and max_open_zones=
2020-04-13 18:51 ` [PATCH v2] zbd: fix sequential write pattern with verify= and max_open_zones= Alexey Dobriyan
@ 2020-04-13 23:13 ` Damien Le Moal
2020-04-13 23:18 ` Jens Axboe
1 sibling, 0 replies; 9+ messages in thread
From: Damien Le Moal @ 2020-04-13 23:13 UTC (permalink / raw)
To: Alexey Dobriyan, Jens Axboe; +Cc: fio@vger.kernel.org
On 2020/04/14 3:52, Alexey Dobriyan wrote:
> Sequential write with max_open_zones=1 has interesting (read: buggy)
> interaction with verify=.
>
> If verify is off, then job runs correctly and IO is sequential,
> and restarted from offset 0 and remains sequential.
>
> If verify is on, then 1 full run is done and verified correctly.
> At this point there is exactly 1 open zone which is the last zone.
>
> Now IO restarts from offset 0 and pick_random_zone() picks opened zone
> #0 which is the last zone because offset is 0. All IO is redirected
> to the last zone, which is rewritten once triggering verify again.
>
> IO pattern becomes: 1 full sequential rewrite followed by constant
> sequential rewrites of the last zone.
>
>
> [global]
> filename=/dev/loop0
> direct=1
> zonemode=zbd
> zonesize=1M
> bs=512K
> rw=write
> verify=xxhash
> [j]
> max_open_zones=1
> io_size=3G
>
> Fix is to close every zone given that verification acts as a barrier
> between jobs.
>
> max_open_zones=2 can restart from half of the device, etc.
>
> Signed-off-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com>
Looks good to me.
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>
> zbd.c | 52 +++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 33 insertions(+), 19 deletions(-)
>
> --- a/zbd.c
> +++ b/zbd.c
> @@ -687,6 +687,22 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
> return zbd_reset_range(td, f, z->start, (z+1)->start - z->start);
> }
>
> +/* The caller must hold f->zbd_info->mutex */
> +static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
> + unsigned int open_zone_idx)
> +{
> + uint32_t zone_idx;
> +
> + assert(open_zone_idx < f->zbd_info->num_open_zones);
> + zone_idx = f->zbd_info->open_zones[open_zone_idx];
> + memmove(f->zbd_info->open_zones + open_zone_idx,
> + f->zbd_info->open_zones + open_zone_idx + 1,
> + (ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
> + sizeof(f->zbd_info->open_zones[0]));
> + f->zbd_info->num_open_zones--;
> + f->zbd_info->zone_info[zone_idx].open = 0;
> +}
> +
> /*
> * Reset a range of zones. Returns 0 upon success and 1 upon failure.
> * @td: fio thread data.
> @@ -710,12 +726,26 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
> dprint(FD_ZBD, "%s: examining zones %u .. %u\n", f->file_name,
> zbd_zone_nr(f->zbd_info, zb), zbd_zone_nr(f->zbd_info, ze));
> for (z = zb; z < ze; z++) {
> + uint32_t nz = z - f->zbd_info->zone_info;
> +
> if (!zbd_zone_swr(z))
> continue;
> zone_lock(td, z);
> - reset_wp = all_zones ? z->wp != z->start :
> - (td->o.td_ddir & TD_DDIR_WRITE) &&
> - z->wp % min_bs != 0;
> + if (all_zones) {
> + unsigned int i;
> +
> + pthread_mutex_lock(&f->zbd_info->mutex);
> + for (i = 0; i < f->zbd_info->num_open_zones; i++) {
> + if (f->zbd_info->open_zones[i] == nz)
> + zbd_close_zone(td, f, i);
> + }
> + pthread_mutex_unlock(&f->zbd_info->mutex);
> +
> + reset_wp = z->wp != z->start;
> + } else {
> + reset_wp = (td->o.td_ddir & TD_DDIR_WRITE) &&
> + z->wp % min_bs != 0;
> + }
> if (reset_wp) {
> dprint(FD_ZBD, "%s: resetting zone %u\n",
> f->file_name,
> @@ -905,22 +935,6 @@ out:
> return res;
> }
>
> -/* The caller must hold f->zbd_info->mutex */
> -static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
> - unsigned int open_zone_idx)
> -{
> - uint32_t zone_idx;
> -
> - assert(open_zone_idx < f->zbd_info->num_open_zones);
> - zone_idx = f->zbd_info->open_zones[open_zone_idx];
> - memmove(f->zbd_info->open_zones + open_zone_idx,
> - f->zbd_info->open_zones + open_zone_idx + 1,
> - (ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
> - sizeof(f->zbd_info->open_zones[0]));
> - f->zbd_info->num_open_zones--;
> - f->zbd_info->zone_info[zone_idx].open = 0;
> -}
> -
> /* Anything goes as long as it is not a constant. */
> static uint32_t pick_random_zone_idx(const struct fio_file *f,
> const struct io_u *io_u)
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] zbd: fix sequential write pattern with verify= and max_open_zones=
2020-04-13 18:51 ` [PATCH v2] zbd: fix sequential write pattern with verify= and max_open_zones= Alexey Dobriyan
2020-04-13 23:13 ` Damien Le Moal
@ 2020-04-13 23:18 ` Jens Axboe
1 sibling, 0 replies; 9+ messages in thread
From: Jens Axboe @ 2020-04-13 23:18 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: Damien Le Moal, fio@vger.kernel.org
On 4/13/20 12:51 PM, Alexey Dobriyan wrote:
> Sequential write with max_open_zones=1 has interesting (read: buggy)
> interaction with verify=.
>
> If verify is off, then job runs correctly and IO is sequential,
> and restarted from offset 0 and remains sequential.
>
> If verify is on, then 1 full run is done and verified correctly.
> At this point there is exactly 1 open zone which is the last zone.
>
> Now IO restarts from offset 0 and pick_random_zone() picks opened zone
> #0 which is the last zone because offset is 0. All IO is redirected
> to the last zone, which is rewritten once triggering verify again.
>
> IO pattern becomes: 1 full sequential rewrite followed by constant
> sequential rewrites of the last zone.
>
>
> [global]
> filename=/dev/loop0
> direct=1
> zonemode=zbd
> zonesize=1M
> bs=512K
> rw=write
> verify=xxhash
> [j]
> max_open_zones=1
> io_size=3G
>
> Fix is to close every zone given that verification acts as a barrier
> between jobs.
>
> max_open_zones=2 can restart from half of the device, etc.
Applied, thanks.
--
Jens Axboe
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-04-13 23:18 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-08 22:16 [PATCH] zbd: fix sequential verify with max_open_zones=1 Alexey Dobriyan
2020-04-09 1:58 ` Damien Le Moal
2020-04-09 17:14 ` Alexey Dobriyan
2020-04-09 22:41 ` Damien Le Moal
2020-04-10 18:46 ` Alexey Dobriyan
2020-04-11 20:18 ` Jens Axboe
2020-04-13 18:51 ` [PATCH v2] zbd: fix sequential write pattern with verify= and max_open_zones= Alexey Dobriyan
2020-04-13 23:13 ` Damien Le Moal
2020-04-13 23:18 ` 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.