Re: [PATCH 3/4] ioengines: add get_max_open_zones zoned block device operation

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On Fri, 2021-05-14 at 12:05 +0000, Niklas Cassel wrote:
> On Thu, May 13, 2021 at 12:23:59AM +0000, Damien Le Moal wrote:
> > On 2021/05/13 7:37, Niklas Cassel wrote:
> > > From: Niklas Cassel <niklas.cassel@wdc.com>
> > > 
> > > Define a new IO engine operation to get the maximum number of open zones.
> > > Like the existing IO engine operations: .get_zoned_model, .report_zones,
> > > and .reset_wp, this new IO engine operation is only valid for zoned block
> > > devices.
> > > 
> > > Similarly to the other zbd IO engine operations, also provide a default
> > > implementation inside oslib/linux-blkzoned.c that will be used if the
> > > ioengine does not override it.
> > > 
> > > The default Linux oslib implementation is implemented similarly to
> > > blkzoned_get_zoned_model(), i.e. it will return a successful error code
> > > even when the sysfs attribute does not exist.
> > > This is because the sysfs max_open_zones attribute was introduced first
> > > in Linux v5.9.
> > > All error handling is still there, so an ioengine that provides its own
> > > implementation will still have its error code respected properly.
> > > 
> > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > > ---
> > >  engines/skeleton_external.c | 13 ++++++
> > >  ioengines.h                 |  4 +-
> > >  oslib/blkzoned.h            |  7 +++
> > >  oslib/linux-blkzoned.c      | 22 ++++++++++
> > >  zbd.c                       | 87 ++++++++++++++++++++++++++++++++++---
> > >  5 files changed, 127 insertions(+), 6 deletions(-)
> > > 
> > 
> > 	if (!td->o.max_open_zones) {
> > 		/* User did not request a limit. Set limit to max supported */
> > 		zbd->max_open_zones = max_open_zones;
> > 	} else if (td->o.max_open_zones < max_open_zones) {
> > 		/* User requested a limit, limit is not too large */
> > 		zbd->max_open_zones = td->o.max_open_zones;
> > 	} else {
> > 		/* User requested a limit, but limit is too large */
> > 		...
> > 		return -EINVAL;
> > 	}
> > 
> > > +
> > > +out:
> > > +	/* Ensure that the limit is not larger than FIO's internal limit */
> > > +	zbd->max_open_zones = zbd->max_open_zones ?: ZBD_MAX_OPEN_ZONES;
> > > +	dprint(FD_ZBD, "%s: using max open zones limit: %"PRIu32"\n",
> > > +	       f->file_name, zbd->max_open_zones);
> > 
> > It may be good to have a log_info() too here, no ?
> 
> Hello Damien,
> 
> Thank you for your review!
> 
> I don't have any problem with any of your review comments, except this one.
> 
> I'm not a fan on unconditionally printing the amount of max open zones being
> used, so I think that keeping it for --debug=zbd is fine.

My apologies, I was not clear. My intent was not to suggest to print a message
unconditionally.

> 1) If the user did specify a limit, why print the same limit as they requested?

Agreed, no message is needed.

> 2) If they did specify a limit that is too high, fio will exit with a print.
> 
> 3) I guess we could print in the case where the user did not request a limit,
> and we implicitly set the limit to the device limit.

That was my suggestion.

> 
> However, right now we don't have any zbd specific prints when starting fio,
> e.g. when the user does not specify --zone_size, and we use zone size of
> the drive.
> 
> So I don't think that actually using the max open zones that the drive
> supports justifies a print, not even for case 3).

OK. That is fine.

That said, there is a refinement needed I think, which is to ignore the drive
advertised max_open_zones if max_active_zones is 0.

The reason is that for SMR drives, the max_open_zones limit is only meaningful
in the context of explicit zone open which fio does not do. For implicit zone
open as used in fio, there will be no IO error for a write workload that
simultaneously writes to more than max_open_zones since max_active_zones is
always 0 (no limit) with SMR.

Having the ability to run workloads that write to more than max_open_zones is
useful to measure the potential impact on performance of the drive implicit
zone close & implicit zone open triggered by such workload.

So I would suggest we change to something like this:

	if (!td->o.max_open_zones && f->zbd_info->max_active_zones) {
		/* User did not request a limit. Set limit to max supported
*/		zbd->max_open_zones = max_open_zones;
	} else if (td->o.max_open_zones < max_open_zones) {
		/* User requested a limit, limit is not too large */
		zbd->max_open_zones = td->o.max_open_zones;
	} else if (f->zbd_info->max_active_zones) {
		/* User requested a limit, but limit is too large */
		...
		return -EINVAL;
	}

Thoughts ?



-- 
Damien Le Moal
Western Digital Research