Re: [PATCH] zbd: remove reset_zone flag from fio_zone_info

Re: [PATCH] zbd: remove reset_zone flag from fio_zone_info

[Alexey Dobriyan]

> The reset_zone flag that is defined in fio_zone_info structure is
> only referenced in zbd_adjust_block() function. Convert this flag
> to a local variable and save some room in zbd_info array which can
> be pretty large when running fio against high capacity zoned devices.

This flag should be kept.

Test can crash or be interrupted leaving hw in indeterminate state,
it must be restarted from clean state. Or test can be precondition and
zone should not be reset.

Can't distinguish these two cases from write pointers alone.

Given that precondition and real workload can be in different fio
instances, this information must be somehow given to fio process
and either force or not force zone reset (config option obviously)

> @@ -41,7 +40,6 @@ struct fio_zone_info {
>  	enum zbd_zone_type	type:2;
>  	enum zbd_zone_cond	cond:4;
>  	unsigned int		open:1;
> -	unsigned int		reset_zone:1;

Does it save space? Bitfields stick together after all.

RE: [PATCH] zbd: remove reset_zone flag from fio_zone_info

[Dmitry Fomichev]

> -----Original Message-----
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Sent: Wednesday, August 19, 2020 8:56 AM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> Cc: fio@vger.kernel.org
> Subject: Re: [PATCH] zbd: remove reset_zone flag from fio_zone_info
> 
> > The reset_zone flag that is defined in fio_zone_info structure is
> > only referenced in zbd_adjust_block() function. Convert this flag
> > to a local variable and save some room in zbd_info array which can
> > be pretty large when running fio against high capacity zoned devices.
> 
> This flag should be kept.
> 
> Test can crash or be interrupted leaving hw in indeterminate state,
> it must be restarted from clean state. Or test can be precondition and
> zone should not be reset.

This is beyond the scope of fio. Any preconditioning of zones is done
by external scripts that have no access to this flag. Tools like blkzone,
libzbc or nvme-cli are typically used to precondition zones before
testing and to analyze zone state after test runs.

> 
> Can't distinguish these two cases from write pointers alone.
> 
> Given that precondition and real workload can be in different fio
> instances, this information must be somehow given to fio process
> and either force or not force zone reset (config option obviously)

The functionality you are describing in not a part of the current fio code.
Perhaps you are talking about some proprietary code additions that you
are developing. Please submit a patch that will implement what you are
talking about and if it has merit you will have a solid case to reintroduce
this flag.

For now, having this flag only makes the code less straightforward.

> 
> > @@ -41,7 +40,6 @@ struct fio_zone_info {
> >  	enum zbd_zone_type	type:2;
> >  	enum zbd_zone_cond	cond:4;
> >  	unsigned int		open:1;
> > -	unsigned int		reset_zone:1;
> 
> Does it save space? Bitfields stick together after all.

It saves room for other stuff that is going be added to this struct in the
future. Some new members will likely be added there to support zone
append and there is some other development in the works that will likely
add more bits there. Keep in mind that the number of zones on a 20TB
DHSMR drive may exceed 130K and these numbers will only grow
over time. Every bit counts in this structure :)

Re: [PATCH] zbd: remove reset_zone flag from fio_zone_info

[Alexey Dobriyan]

On Wed, Aug 19, 2020 at 07:35:03PM +0000, Dmitry Fomichev wrote:
> 
> 
> > -----Original Message-----
> > From: Alexey Dobriyan <adobriyan@gmail.com>
> > Sent: Wednesday, August 19, 2020 8:56 AM
> > To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> > Cc: fio@vger.kernel.org
> > Subject: Re: [PATCH] zbd: remove reset_zone flag from fio_zone_info
> > 
> > > The reset_zone flag that is defined in fio_zone_info structure is
> > > only referenced in zbd_adjust_block() function. Convert this flag
> > > to a local variable and save some room in zbd_info array which can
> > > be pretty large when running fio against high capacity zoned devices.
> > 
> > This flag should be kept.
> > 
> > Test can crash or be interrupted leaving hw in indeterminate state,
> > it must be restarted from clean state. Or test can be precondition and
> > zone should not be reset.
> 
> This is beyond the scope of fio. Any preconditioning of zones is done
> by external scripts that have no access to this flag. Tools like blkzone,
> libzbc or nvme-cli are typically used to precondition zones before
> testing and to analyze zone state after test runs.

If it is a matter of policy that fio doesn't do preconditioning,
then the patch is OK.

We do preconditioning in fio, it is neat:
* don't need external programs
* everything inside one job file
* not a lot of code to add

But to do this, flag must live in "struct fio_zone_info".

	[zra]
	zone_reset_all=1
	zone_finish_all=1
	rw=write

	[pre]
	stonewall
	zone_reset_all=0
	zone_finish_all=0
	job_max_open_zones=...
	rw=write
		...

	[j]
	stonewall
	zone_reset_all=0
	zone_finish_all=0
	rw=randread
		...

RE: [PATCH] zbd: remove reset_zone flag from fio_zone_info

[Dmitry Fomichev]

> -----Original Message-----
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Sent: Friday, August 21, 2020 1:07 PM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> Cc: fio@vger.kernel.org
> Subject: Re: [PATCH] zbd: remove reset_zone flag from fio_zone_info
> 
> On Wed, Aug 19, 2020 at 07:35:03PM +0000, Dmitry Fomichev wrote:
> >
> >
> > > -----Original Message-----
> > > From: Alexey Dobriyan <adobriyan@gmail.com>
> > > Sent: Wednesday, August 19, 2020 8:56 AM
> > > To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> > > Cc: fio@vger.kernel.org
> > > Subject: Re: [PATCH] zbd: remove reset_zone flag from fio_zone_info
> > >
> > > > The reset_zone flag that is defined in fio_zone_info structure is
> > > > only referenced in zbd_adjust_block() function. Convert this flag
> > > > to a local variable and save some room in zbd_info array which can
> > > > be pretty large when running fio against high capacity zoned devices.
> > >
> > > This flag should be kept.
> > >
> > > Test can crash or be interrupted leaving hw in indeterminate state,
> > > it must be restarted from clean state. Or test can be precondition and
> > > zone should not be reset.
> >
> > This is beyond the scope of fio. Any preconditioning of zones is done
> > by external scripts that have no access to this flag. Tools like blkzone,
> > libzbc or nvme-cli are typically used to precondition zones before
> > testing and to analyze zone state after test runs.
> 
> If it is a matter of policy that fio doesn't do preconditioning,
> then the patch is OK.
> 
> We do preconditioning in fio, it is neat:
> * don't need external programs
> * everything inside one job file
> * not a lot of code to add
> 
> But to do this, flag must live in "struct fio_zone_info".
> 
> 	[zra]
> 	zone_reset_all=1

The way I understand this, this option makes ZBD code to set the reset_zone
flag for all zones during init. This will force zone reset before the first write
to a zone. This indeed is a neat idea and this feature looks to be pretty 
lightweight as you said.

> 	zone_finish_all=1

How does this option work? Seems like you have added a new
finish_zone flag and you finish every zone for which the flag is set
instead of performing the first write to the zone...

> 	rw=write
> 
> 	[pre]
> 	stonewall
> 	zone_reset_all=0
> 	zone_finish_all=0
> 	job_max_open_zones=...
> 	rw=write
> 		...
> 
> 	[j]
> 	stonewall
> 	zone_reset_all=0
> 	zone_finish_all=0
> 	rw=randread
> 		...

I guess we can hold off with removing the reset_zone flag if you are planning
to send the patch for this.

Dmitry

Re: [PATCH] zbd: remove reset_zone flag from fio_zone_info

[Alexey Dobriyan]

On Thu, Aug 27, 2020 at 07:18:07PM +0000, Dmitry Fomichev wrote:
> 
> 
> > -----Original Message-----
> > From: Alexey Dobriyan <adobriyan@gmail.com>
> > Sent: Friday, August 21, 2020 1:07 PM
> > To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> > Cc: fio@vger.kernel.org
> > Subject: Re: [PATCH] zbd: remove reset_zone flag from fio_zone_info
> > 
> > On Wed, Aug 19, 2020 at 07:35:03PM +0000, Dmitry Fomichev wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Alexey Dobriyan <adobriyan@gmail.com>
> > > > Sent: Wednesday, August 19, 2020 8:56 AM
> > > > To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> > > > Cc: fio@vger.kernel.org
> > > > Subject: Re: [PATCH] zbd: remove reset_zone flag from fio_zone_info
> > > >
> > > > > The reset_zone flag that is defined in fio_zone_info structure is
> > > > > only referenced in zbd_adjust_block() function. Convert this flag
> > > > > to a local variable and save some room in zbd_info array which can
> > > > > be pretty large when running fio against high capacity zoned devices.
> > > >
> > > > This flag should be kept.
> > > >
> > > > Test can crash or be interrupted leaving hw in indeterminate state,
> > > > it must be restarted from clean state. Or test can be precondition and
> > > > zone should not be reset.
> > >
> > > This is beyond the scope of fio. Any preconditioning of zones is done
> > > by external scripts that have no access to this flag. Tools like blkzone,
> > > libzbc or nvme-cli are typically used to precondition zones before
> > > testing and to analyze zone state after test runs.
> > 
> > If it is a matter of policy that fio doesn't do preconditioning,
> > then the patch is OK.
> > 
> > We do preconditioning in fio, it is neat:
> > * don't need external programs
> > * everything inside one job file
> > * not a lot of code to add
> > 
> > But to do this, flag must live in "struct fio_zone_info".
> > 
> > 	[zra]
> > 	zone_reset_all=1
> 
> The way I understand this, this option makes ZBD code to set the reset_zone
> flag for all zones during init. This will force zone reset before the first write
> to a zone. This indeed is a neat idea and this feature looks to be pretty 
> lightweight as you said.
> 
> > 	zone_finish_all=1
> 
> How does this option work? Seems like you have added a new
> finish_zone flag and you finish every zone for which the flag is set
> instead of performing the first write to the zone...

Sorry for delay.

ZRA marks every zone in [->min_zone, ->max_zone) for reset,
forcing Zone Reset before first write.

ZFA sends Finish Zone with "all" flag set synchronously.

Some job orchestration is required but once it is done, jobs run reliably.

> > 	rw=write
> > 
> > 	[pre]
> > 	stonewall
> > 	zone_reset_all=0
> > 	zone_finish_all=0
> > 	job_max_open_zones=...
> > 	rw=write
> > 		...
> > 
> > 	[j]
> > 	stonewall
> > 	zone_reset_all=0
> > 	zone_finish_all=0
> > 	rw=randread
> > 		...
> 
> I guess we can hold off with removing the reset_zone flag if you are planning
> to send the patch for this.

Thanks, I'll try to extract something mainlineable.

[PATCH] zbd: remove reset_zone flag from fio_zone_info

[Dmitry Fomichev]

The reset_zone flag that is defined in fio_zone_info structure is
only referenced in zbd_adjust_block() function. Convert this flag
to a local variable and save some room in zbd_info array which can
be pretty large when running fio against high capacity zoned devices.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 7 ++++---
 zbd.h | 2 --
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/zbd.c b/zbd.c
index 5af8af4a..a7a48d27 100644
--- a/zbd.c
+++ b/zbd.c
@@ -1438,6 +1438,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	uint32_t min_bs = td->o.min_bs[io_u->ddir];
 	uint64_t new_len;
 	int64_t range;
+	bool reset_zone;
 
 	if (!f->zbd_info)
 		return io_u_accept;
@@ -1536,15 +1537,16 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			zone_idx_b = zb - f->zbd_info->zone_info;
 		}
 		/* Check whether the zone reset threshold has been exceeded */
+		reset_zone = false;
 		if (td->o.zrf.u.f) {
 			if (f->zbd_info->sectors_with_data >=
 			    f->io_size * td->o.zrt.u.f &&
 			    zbd_dec_and_reset_write_cnt(td, f)) {
-				zb->reset_zone = 1;
+				reset_zone = true;
 			}
 		}
 		/* Reset the zone pointer if necessary */
-		if (zb->reset_zone || zbd_zone_full(f, zb, min_bs)) {
+		if (reset_zone || zbd_zone_full(f, zb, min_bs)) {
 			assert(td->o.verify == VERIFY_NONE);
 			/*
 			 * Since previous write requests may have been submitted
@@ -1554,7 +1556,6 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			 * zone reset.
 			 */
 			io_u_quiesce(td);
-			zb->reset_zone = 0;
 			if (zbd_reset_zone(td, f, zb) < 0)
 				goto eof;
 
diff --git a/zbd.h b/zbd.h
index 021174c1..fe04d4ad 100644
--- a/zbd.h
+++ b/zbd.h
@@ -30,7 +30,6 @@ enum io_u_action {
  * @cond: zone state (BLK_ZONE_COND_*)
  * @open: whether or not this zone is currently open. Only relevant if
  *		max_open_zones > 0.
- * @reset_zone: whether or not this zone should be reset before writing to it
  */
 struct fio_zone_info {
 	pthread_mutex_t		mutex;
@@ -41,7 +40,6 @@ struct fio_zone_info {
 	enum zbd_zone_type	type:2;
 	enum zbd_zone_cond	cond:4;
 	unsigned int		open:1;
-	unsigned int		reset_zone:1;
 };
 
 /**
-- 
2.21.0

Re: [PATCH] zbd: remove reset_zone flag from fio_zone_info

[Damien Le Moal]

On 2020/08/18 9:02, Dmitry Fomichev wrote:
> The reset_zone flag that is defined in fio_zone_info structure is
> only referenced in zbd_adjust_block() function. Convert this flag
> to a local variable and save some room in zbd_info array which can
> be pretty large when running fio against high capacity zoned devices.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  zbd.c | 7 ++++---
>  zbd.h | 2 --
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/zbd.c b/zbd.c
> index 5af8af4a..a7a48d27 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -1438,6 +1438,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
>  	uint32_t min_bs = td->o.min_bs[io_u->ddir];
>  	uint64_t new_len;
>  	int64_t range;
> +	bool reset_zone;
>  
>  	if (!f->zbd_info)
>  		return io_u_accept;
> @@ -1536,15 +1537,16 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
>  			zone_idx_b = zb - f->zbd_info->zone_info;
>  		}
>  		/* Check whether the zone reset threshold has been exceeded */
> +		reset_zone = false;
>  		if (td->o.zrf.u.f) {
>  			if (f->zbd_info->sectors_with_data >=
>  			    f->io_size * td->o.zrt.u.f &&
>  			    zbd_dec_and_reset_write_cnt(td, f)) {
> -				zb->reset_zone = 1;
> +				reset_zone = true;
>  			}
>  		}
>  		/* Reset the zone pointer if necessary */
> -		if (zb->reset_zone || zbd_zone_full(f, zb, min_bs)) {
> +		if (reset_zone || zbd_zone_full(f, zb, min_bs)) {
>  			assert(td->o.verify == VERIFY_NONE);
>  			/*
>  			 * Since previous write requests may have been submitted
> @@ -1554,7 +1556,6 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
>  			 * zone reset.
>  			 */
>  			io_u_quiesce(td);
> -			zb->reset_zone = 0;
>  			if (zbd_reset_zone(td, f, zb) < 0)
>  				goto eof;
>  
> diff --git a/zbd.h b/zbd.h
> index 021174c1..fe04d4ad 100644
> --- a/zbd.h
> +++ b/zbd.h
> @@ -30,7 +30,6 @@ enum io_u_action {
>   * @cond: zone state (BLK_ZONE_COND_*)
>   * @open: whether or not this zone is currently open. Only relevant if
>   *		max_open_zones > 0.
> - * @reset_zone: whether or not this zone should be reset before writing to it
>   */
>  struct fio_zone_info {
>  	pthread_mutex_t		mutex;
> @@ -41,7 +40,6 @@ struct fio_zone_info {
>  	enum zbd_zone_type	type:2;
>  	enum zbd_zone_cond	cond:4;
>  	unsigned int		open:1;
> -	unsigned int		reset_zone:1;
>  };
>  
>  /**
> 

Looks good to me.

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


-- 
Damien Le Moal
Western Digital Research