All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.