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

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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.


1) If the user did specify a limit, why print the same limit as they requested?
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.

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).


Kind regards,
Niklas