[PATCH 0/4] Improve fio max open zones handling

[PATCH 0/4] Improve fio max open zones handling

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Improve max open zones handling by introducing a new IO engine operation,
which can be implemented by ioengines.

Also provide a default implementation for Linux, which parses sysfs for
the max open zones value.

Update libzbc ioengine to utilize the new callback.

Having this callback has the two following advantages:

1) If the user forgets to specify a --max_open_zones value, we will
automatically use the maximum the drive supports.
(Instead of getting errors because fio tries to open too many zones.)

2) If the user specifies a --max_open_zones value that is too big,
we will return an error and fio will exit with a proper error message.
(Instead of getting errors because fio tries to open too many zones.)


Kind regards,
Niklas

Niklas Cassel (4):
  zbd: only put an upper limit on max open zones once
  oslib/linux-blkzoned: move sysfs reading into its own function
  ioengines: add get_max_open_zones zoned block device operation
  engines/libzbc: add support for the get_max_open_zones io op

 engines/libzbc.c            | 21 +++++++++
 engines/skeleton_external.c | 13 ++++++
 ioengines.h                 |  4 +-
 oslib/blkzoned.h            |  7 +++
 oslib/linux-blkzoned.c      | 82 ++++++++++++++++++++++++----------
 zbd.c                       | 88 ++++++++++++++++++++++++++++++++++---
 6 files changed, 186 insertions(+), 29 deletions(-)

-- 
2.25.1

[PATCH 1/4] zbd: only put an upper limit on max open zones once

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

There is an upper limit that is checked for each td, and for each file,
even though a file has a pointer to a zoned_block_device_info that has
already been created. Multiple files, from the same or from another td
can point to the same zoned_block_device_info.
All zoned_block_device_info:s have already been created earlier in the
call chain.

Simplify this by only checking the upper limit on max open zones when a
zoned_block_device_info is created.

This way, max_open_zones is handled from a single location, instead of
potentially being reassigned from a completely different location.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 zbd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/zbd.c b/zbd.c
index eed796b3..8ed8f195 100644
--- a/zbd.c
+++ b/zbd.c
@@ -588,7 +588,8 @@ static int zbd_create_zone_info(struct thread_data *td, struct fio_file *f)
 
 	if (ret == 0) {
 		f->zbd_info->model = zbd_model;
-		f->zbd_info->max_open_zones = td->o.max_open_zones;
+		f->zbd_info->max_open_zones =
+			td->o.max_open_zones ?: ZBD_MAX_OPEN_ZONES;
 	}
 	return ret;
 }
@@ -726,8 +727,6 @@ int zbd_setup_files(struct thread_data *td)
 		if (zbd_is_seq_job(f))
 			assert(f->min_zone < f->max_zone);
 
-		zbd->max_open_zones = zbd->max_open_zones ?: ZBD_MAX_OPEN_ZONES;
-
 		if (td->o.max_open_zones > 0 &&
 		    zbd->max_open_zones != td->o.max_open_zones) {
 			log_err("Different 'max_open_zones' values\n");
-- 
2.25.1

[PATCH 2/4] oslib/linux-blkzoned: move sysfs reading into its own function

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Move the sysfs reading into its own function so that it can be reused.
This new function will be reused in an succeeding patch.

No functional change intended.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 oslib/linux-blkzoned.c | 60 ++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/oslib/linux-blkzoned.c b/oslib/linux-blkzoned.c
index 81e4e7f0..127069ca 100644
--- a/oslib/linux-blkzoned.c
+++ b/oslib/linux-blkzoned.c
@@ -74,12 +74,14 @@ static char *read_file(const char *path)
 	return strdup(line);
 }
 
-int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
-			     enum zbd_zoned_model *model)
+/*
+ * Returns NULL on failure.
+ * Returns a pointer to a string on success.
+ * The caller is responsible for freeing the memory.
+ */
+static char *blkzoned_get_sysfs_attr(const char *file_name, const char *attr)
 {
-	const char *file_name = f->file_name;
-	char *zoned_attr_path = NULL;
-	char *model_str = NULL;
+	char *attr_path = NULL;
 	struct stat statbuf;
 	char *sys_devno_path = NULL;
 	char *part_attr_path = NULL;
@@ -87,13 +89,7 @@ int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
 	char sys_path[PATH_MAX];
 	ssize_t sz;
 	char *delim = NULL;
-
-	if (f->filetype != FIO_TYPE_BLOCK) {
-		*model = ZBD_IGNORE;
-		return 0;
-	}
-
-	*model = ZBD_NONE;
+	char *ret = NULL;
 
 	if (stat(file_name, &statbuf) < 0)
 		goto out;
@@ -123,24 +119,44 @@ int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
 		*delim = '\0';
 	}
 
-	if (asprintf(&zoned_attr_path,
-		     "/sys/dev/block/%s/queue/zoned", sys_path) < 0)
+	if (asprintf(&attr_path,
+		     "/sys/dev/block/%s/%s", sys_path, attr) < 0)
 		goto out;
 
-	model_str = read_file(zoned_attr_path);
+	ret = read_file(attr_path);
+out:
+	free(attr_path);
+	free(part_str);
+	free(part_attr_path);
+	free(sys_devno_path);
+
+	return ret;
+}
+
+int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
+			     enum zbd_zoned_model *model)
+{
+	char *model_str = NULL;
+
+	if (f->filetype != FIO_TYPE_BLOCK) {
+		*model = ZBD_IGNORE;
+		return 0;
+	}
+
+	*model = ZBD_NONE;
+
+	model_str = blkzoned_get_sysfs_attr(f->file_name, "queue/zoned");
 	if (!model_str)
-		goto out;
-	dprint(FD_ZBD, "%s: zbd model string: %s\n", file_name, model_str);
+		return 0;
+
+	dprint(FD_ZBD, "%s: zbd model string: %s\n", f->file_name, model_str);
 	if (strcmp(model_str, "host-aware") == 0)
 		*model = ZBD_HOST_AWARE;
 	else if (strcmp(model_str, "host-managed") == 0)
 		*model = ZBD_HOST_MANAGED;
-out:
+
 	free(model_str);
-	free(zoned_attr_path);
-	free(part_str);
-	free(part_attr_path);
-	free(sys_devno_path);
+
 	return 0;
 }
 
-- 
2.25.1

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

[Niklas Cassel]

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

diff --git a/engines/skeleton_external.c b/engines/skeleton_external.c
index 7f3e4cb3..c79b6f11 100644
--- a/engines/skeleton_external.c
+++ b/engines/skeleton_external.c
@@ -193,6 +193,18 @@ static int fio_skeleton_reset_wp(struct thread_data *td, struct fio_file *f,
 	return 0;
 }
 
+/*
+ * Hook called for getting the maximum number of open zones for a
+ * ZBD_HOST_MANAGED zoned block device.
+ * A @max_open_zones value set to zero means no limit.
+ */
+static int fio_skeleton_get_max_open_zones(struct thread_data *td,
+					   struct fio_file *f,
+					   unsigned int *max_open_zones)
+{
+	return 0;
+}
+
 /*
  * Note that the structure is exported, so that fio can get it via
  * dlsym(..., "ioengine"); for (and only for) external engines.
@@ -212,6 +224,7 @@ struct ioengine_ops ioengine = {
 	.get_zoned_model = fio_skeleton_get_zoned_model,
 	.report_zones	= fio_skeleton_report_zones,
 	.reset_wp	= fio_skeleton_reset_wp,
+	.get_max_open_zones = fio_skeleton_get_max_open_zones,
 	.options	= options,
 	.option_struct_size	= sizeof(struct fio_skeleton_options),
 };
diff --git a/ioengines.h b/ioengines.h
index 1d01ab0a..b3f755b4 100644
--- a/ioengines.h
+++ b/ioengines.h
@@ -8,7 +8,7 @@
 #include "io_u.h"
 #include "zbd_types.h"
 
-#define FIO_IOOPS_VERSION	29
+#define FIO_IOOPS_VERSION	30
 
 #ifndef CONFIG_DYNAMIC_ENGINES
 #define FIO_STATIC	static
@@ -59,6 +59,8 @@ struct ioengine_ops {
 			    uint64_t, struct zbd_zone *, unsigned int);
 	int (*reset_wp)(struct thread_data *, struct fio_file *,
 			uint64_t, uint64_t);
+	int (*get_max_open_zones)(struct thread_data *, struct fio_file *,
+				  unsigned int *);
 	int option_struct_size;
 	struct fio_option *options;
 };
diff --git a/oslib/blkzoned.h b/oslib/blkzoned.h
index 4cc071dc..719b041d 100644
--- a/oslib/blkzoned.h
+++ b/oslib/blkzoned.h
@@ -16,6 +16,8 @@ extern int blkzoned_report_zones(struct thread_data *td,
 				struct zbd_zone *zones, unsigned int nr_zones);
 extern int blkzoned_reset_wp(struct thread_data *td, struct fio_file *f,
 				uint64_t offset, uint64_t length);
+extern int blkzoned_get_max_open_zones(struct thread_data *td, struct fio_file *f,
+				       unsigned int *max_open_zones);
 #else
 /*
  * Define stubs for systems that do not have zoned block device support.
@@ -44,6 +46,11 @@ static inline int blkzoned_reset_wp(struct thread_data *td, struct fio_file *f,
 {
 	return -EIO;
 }
+static inline int blkzoned_get_max_open_zones(struct thread_data *td, struct fio_file *f,
+					      unsigned int *max_open_zones)
+{
+	return -EIO;
+}
 #endif
 
 #endif /* FIO_BLKZONED_H */
diff --git a/oslib/linux-blkzoned.c b/oslib/linux-blkzoned.c
index 127069ca..82b662a2 100644
--- a/oslib/linux-blkzoned.c
+++ b/oslib/linux-blkzoned.c
@@ -160,6 +160,28 @@ int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
 	return 0;
 }
 
+int blkzoned_get_max_open_zones(struct thread_data *td, struct fio_file *f,
+				unsigned int *max_open_zones)
+{
+	char *max_open_str;
+
+	if (f->filetype != FIO_TYPE_BLOCK) {
+		return -EIO;
+	}
+
+	max_open_str = blkzoned_get_sysfs_attr(f->file_name, "queue/max_open_zones");
+	if (!max_open_str)
+		return 0;
+
+	dprint(FD_ZBD, "%s: max open zones supported by device: %s\n",
+	       f->file_name, max_open_str);
+	*max_open_zones = atoll(max_open_str);
+
+	free(max_open_str);
+
+	return 0;
+}
+
 static uint64_t zone_capacity(struct blk_zone_report *hdr,
 			      struct blk_zone *blkz)
 {
diff --git a/zbd.c b/zbd.c
index 8ed8f195..f23bbbac 100644
--- a/zbd.c
+++ b/zbd.c
@@ -113,6 +113,34 @@ int zbd_reset_wp(struct thread_data *td, struct fio_file *f,
 	return ret;
 }
 
+/**
+ * zbd_get_max_open_zones - Get the maximum number of open zones
+ * @td: FIO thread data
+ * @f: FIO file for which to get max open zones
+ * @max_open_zones: Upon success, result will be stored here.
+ *
+ * A @max_open_zones value set to zero means no limit.
+ *
+ * Returns 0 upon success and a negative error code upon failure.
+ */
+int zbd_get_max_open_zones(struct thread_data *td, struct fio_file *f,
+			   unsigned int *max_open_zones)
+{
+	int ret;
+
+	if (td->io_ops && td->io_ops->get_max_open_zones)
+		ret = td->io_ops->get_max_open_zones(td, f, max_open_zones);
+	else
+		ret = blkzoned_get_max_open_zones(td, f, max_open_zones);
+	if (ret < 0) {
+		td_verror(td, errno, "get max open zones failed");
+		log_err("%s: get max open zones failed (%d).\n",
+			f->file_name, errno);
+	}
+
+	return ret;
+}
+
 /**
  * zbd_zone_idx - convert an offset into a zone number
  * @f: file pointer.
@@ -554,6 +582,48 @@ out:
 	return ret;
 }
 
+static int zbd_set_max_open_zones(struct thread_data *td, struct fio_file *f)
+{
+	struct zoned_block_device_info *zbd = f->zbd_info;
+	unsigned int max_open_zones;
+	int ret;
+
+	if (zbd->model != ZBD_HOST_MANAGED) {
+		/* Only host-managed devices have a max open limit */
+		zbd->max_open_zones = td->o.max_open_zones;
+		goto out;
+	}
+
+	/* If host-managed, get the max open limit */
+	ret = zbd_get_max_open_zones(td, f, &max_open_zones);
+	if (ret)
+		return ret;
+
+	if (td->o.max_open_zones > 0 && max_open_zones > 0 &&
+	    td->o.max_open_zones > max_open_zones) {
+		/* User requested a limit, but limit is too large */
+		td_verror(td, EINVAL,
+			  "Specified --max_open_zones is too large");
+		log_err("Specified --max_open_zones (%d) is larger than max (%u)\n",
+			td->o.max_open_zones, max_open_zones);
+		return -EINVAL;
+	} else if (td->o.max_open_zones > 0) {
+		/* User requested a limit, limit is not too large */
+		zbd->max_open_zones = td->o.max_open_zones;
+	} else {
+		/* User did not request a limit. Set limit to max supported */
+		zbd->max_open_zones = max_open_zones;
+	}
+
+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);
+
+	return 0;
+}
+
 /*
  * Allocate zone information and store it into f->zbd_info if zonemode=zbd.
  *
@@ -576,9 +646,13 @@ static int zbd_create_zone_info(struct thread_data *td, struct fio_file *f)
 	case ZBD_HOST_AWARE:
 	case ZBD_HOST_MANAGED:
 		ret = parse_zone_info(td, f);
+		if (ret)
+			return ret;
 		break;
 	case ZBD_NONE:
 		ret = init_zone_info(td, f);
+		if (ret)
+			return ret;
 		break;
 	default:
 		td_verror(td, EINVAL, "Unsupported zoned model");
@@ -586,12 +660,15 @@ static int zbd_create_zone_info(struct thread_data *td, struct fio_file *f)
 		return -EINVAL;
 	}
 
-	if (ret == 0) {
-		f->zbd_info->model = zbd_model;
-		f->zbd_info->max_open_zones =
-			td->o.max_open_zones ?: ZBD_MAX_OPEN_ZONES;
+	f->zbd_info->model = zbd_model;
+
+	ret = zbd_set_max_open_zones(td, f);
+	if (ret) {
+		zbd_free_zone_info(f);
+		return ret;
 	}
-	return ret;
+
+	return 0;
 }
 
 void zbd_free_zone_info(struct fio_file *f)
-- 
2.25.1

[PATCH 4/4] engines/libzbc: add support for the get_max_open_zones io op

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Add support for the new .get_max_open_zones io operation.

zbc.c will only ever call this callback for host-managed devices.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 engines/libzbc.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/engines/libzbc.c b/engines/libzbc.c
index 2aacf7bb..3dde93db 100644
--- a/engines/libzbc.c
+++ b/engines/libzbc.c
@@ -19,6 +19,7 @@ struct libzbc_data {
 	struct zbc_device	*zdev;
 	enum zbc_dev_model	model;
 	uint64_t		nr_sectors;
+	uint32_t		max_open_seq_req;
 };
 
 static int libzbc_get_dev_info(struct libzbc_data *ld, struct fio_file *f)
@@ -32,6 +33,7 @@ static int libzbc_get_dev_info(struct libzbc_data *ld, struct fio_file *f)
 	zbc_get_device_info(ld->zdev, zinfo);
 	ld->model = zinfo->zbd_model;
 	ld->nr_sectors = zinfo->zbd_sectors;
+	ld->max_open_seq_req = zinfo->zbd_max_nr_open_seq_req;
 
 	dprint(FD_ZBD, "%s: vendor_id:%s, type: %s, model: %s\n",
 	       f->file_name, zinfo->zbd_vendor_id,
@@ -335,6 +337,24 @@ err:
 	return -ret;
 }
 
+static int libzbc_get_max_open_zones(struct thread_data *td, struct fio_file *f,
+				     unsigned int *max_open_zones)
+{
+	struct libzbc_data *ld;
+	int ret;
+
+	ret = libzbc_open_dev(td, f, &ld);
+	if (ret)
+		return ret;
+
+	if (ld->max_open_seq_req == ZBC_NO_LIMIT)
+		*max_open_zones = 0;
+	else
+		*max_open_zones = ld->max_open_seq_req;
+
+	return 0;
+}
+
 ssize_t libzbc_rw(struct thread_data *td, struct io_u *io_u)
 {
 	struct libzbc_data *ld = td->io_ops_data;
@@ -414,6 +434,7 @@ FIO_STATIC struct ioengine_ops ioengine = {
 	.get_zoned_model	= libzbc_get_zoned_model,
 	.report_zones		= libzbc_report_zones,
 	.reset_wp		= libzbc_reset_wp,
+	.get_max_open_zones	= libzbc_get_max_open_zones,
 	.queue			= libzbc_queue,
 	.flags			= FIO_SYNCIO | FIO_NOEXTEND | FIO_RAWIO,
 };
-- 
2.25.1

Re: [PATCH 1/4] zbd: only put an upper limit on max open zones once

[Damien Le Moal]

On 2021/05/13 7:36, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> There is an upper limit that is checked for each td, and for each file,
> even though a file has a pointer to a zoned_block_device_info that has
> already been created. Multiple files, from the same or from another td
> can point to the same zoned_block_device_info.
> All zoned_block_device_info:s have already been created earlier in the
> call chain.
> 
> Simplify this by only checking the upper limit on max open zones when a
> zoned_block_device_info is created.
> 
> This way, max_open_zones is handled from a single location, instead of
> potentially being reassigned from a completely different location.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  zbd.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/zbd.c b/zbd.c
> index eed796b3..8ed8f195 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -588,7 +588,8 @@ static int zbd_create_zone_info(struct thread_data *td, struct fio_file *f)
>  
>  	if (ret == 0) {
>  		f->zbd_info->model = zbd_model;
> -		f->zbd_info->max_open_zones = td->o.max_open_zones;
> +		f->zbd_info->max_open_zones =
> +			td->o.max_open_zones ?: ZBD_MAX_OPEN_ZONES;

		f->zbd_info->max_open_zones =
			min_not_zero(td->o.max_open_zones, ZBD_MAX_OPEN_ZONES);

is more readable (easier to understand), no ?

>  	}
>  	return ret;
>  }
> @@ -726,8 +727,6 @@ int zbd_setup_files(struct thread_data *td)
>  		if (zbd_is_seq_job(f))
>  			assert(f->min_zone < f->max_zone);
>  
> -		zbd->max_open_zones = zbd->max_open_zones ?: ZBD_MAX_OPEN_ZONES;
> -
>  		if (td->o.max_open_zones > 0 &&
>  		    zbd->max_open_zones != td->o.max_open_zones) {
>  			log_err("Different 'max_open_zones' values\n");
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/4] oslib/linux-blkzoned: move sysfs reading into its own function

[Damien Le Moal]

On 2021/05/13 7:36, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Move the sysfs reading into its own function so that it can be reused.
> This new function will be reused in an succeeding patch.

s/succeeding/following (or subsequent)

> 
> No functional change intended.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  oslib/linux-blkzoned.c | 60 ++++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/oslib/linux-blkzoned.c b/oslib/linux-blkzoned.c
> index 81e4e7f0..127069ca 100644
> --- a/oslib/linux-blkzoned.c
> +++ b/oslib/linux-blkzoned.c
> @@ -74,12 +74,14 @@ static char *read_file(const char *path)
>  	return strdup(line);
>  }
>  
> -int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
> -			     enum zbd_zoned_model *model)
> +/*

Please add a simple description of what the function does (ok, it is clear from
the function name, but that comes after the comment so it becomes weird to read).

> + * Returns NULL on failure.
> + * Returns a pointer to a string on success.
> + * The caller is responsible for freeing the memory.
> + */
> +static char *blkzoned_get_sysfs_attr(const char *file_name, const char *attr)
>  {
> -	const char *file_name = f->file_name;
> -	char *zoned_attr_path = NULL;
> -	char *model_str = NULL;
> +	char *attr_path = NULL;
>  	struct stat statbuf;
>  	char *sys_devno_path = NULL;
>  	char *part_attr_path = NULL;
> @@ -87,13 +89,7 @@ int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
>  	char sys_path[PATH_MAX];
>  	ssize_t sz;
>  	char *delim = NULL;
> -
> -	if (f->filetype != FIO_TYPE_BLOCK) {
> -		*model = ZBD_IGNORE;
> -		return 0;
> -	}
> -
> -	*model = ZBD_NONE;
> +	char *ret = NULL;

calling this ret is unusual. Generally, an int is assumed. Can you change that
to something like attr_str or val_str ?

>  
>  	if (stat(file_name, &statbuf) < 0)
>  		goto out;
> @@ -123,24 +119,44 @@ int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
>  		*delim = '\0';
>  	}
>  
> -	if (asprintf(&zoned_attr_path,
> -		     "/sys/dev/block/%s/queue/zoned", sys_path) < 0)
> +	if (asprintf(&attr_path,
> +		     "/sys/dev/block/%s/%s", sys_path, attr) < 0)
>  		goto out;
>  
> -	model_str = read_file(zoned_attr_path);
> +	ret = read_file(attr_path);
> +out:
> +	free(attr_path);
> +	free(part_str);
> +	free(part_attr_path);
> +	free(sys_devno_path);
> +
> +	return ret;
> +}
> +
> +int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
> +			     enum zbd_zoned_model *model)
> +{
> +	char *model_str = NULL;
> +
> +	if (f->filetype != FIO_TYPE_BLOCK) {
> +		*model = ZBD_IGNORE;
> +		return 0;
> +	}
> +
> +	*model = ZBD_NONE;
> +
> +	model_str = blkzoned_get_sysfs_attr(f->file_name, "queue/zoned");
>  	if (!model_str)
> -		goto out;
> -	dprint(FD_ZBD, "%s: zbd model string: %s\n", file_name, model_str);
> +		return 0;
> +
> +	dprint(FD_ZBD, "%s: zbd model string: %s\n", f->file_name, model_str);
>  	if (strcmp(model_str, "host-aware") == 0)
>  		*model = ZBD_HOST_AWARE;
>  	else if (strcmp(model_str, "host-managed") == 0)
>  		*model = ZBD_HOST_MANAGED;
> -out:
> +
>  	free(model_str);
> -	free(zoned_attr_path);
> -	free(part_str);
> -	free(part_attr_path);
> -	free(sys_devno_path);
> +
>  	return 0;
>  }
>  
> 


-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

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(-)
> 
> diff --git a/engines/skeleton_external.c b/engines/skeleton_external.c
> index 7f3e4cb3..c79b6f11 100644
> --- a/engines/skeleton_external.c
> +++ b/engines/skeleton_external.c
> @@ -193,6 +193,18 @@ static int fio_skeleton_reset_wp(struct thread_data *td, struct fio_file *f,
>  	return 0;
>  }
>  
> +/*
> + * Hook called for getting the maximum number of open zones for a
> + * ZBD_HOST_MANAGED zoned block device.
> + * A @max_open_zones value set to zero means no limit.
> + */
> +static int fio_skeleton_get_max_open_zones(struct thread_data *td,
> +					   struct fio_file *f,
> +					   unsigned int *max_open_zones)
> +{
> +	return 0;
> +}
> +
>  /*
>   * Note that the structure is exported, so that fio can get it via
>   * dlsym(..., "ioengine"); for (and only for) external engines.
> @@ -212,6 +224,7 @@ struct ioengine_ops ioengine = {
>  	.get_zoned_model = fio_skeleton_get_zoned_model,
>  	.report_zones	= fio_skeleton_report_zones,
>  	.reset_wp	= fio_skeleton_reset_wp,
> +	.get_max_open_zones = fio_skeleton_get_max_open_zones,
>  	.options	= options,
>  	.option_struct_size	= sizeof(struct fio_skeleton_options),
>  };
> diff --git a/ioengines.h b/ioengines.h
> index 1d01ab0a..b3f755b4 100644
> --- a/ioengines.h
> +++ b/ioengines.h
> @@ -8,7 +8,7 @@
>  #include "io_u.h"
>  #include "zbd_types.h"
>  
> -#define FIO_IOOPS_VERSION	29
> +#define FIO_IOOPS_VERSION	30
>  
>  #ifndef CONFIG_DYNAMIC_ENGINES
>  #define FIO_STATIC	static
> @@ -59,6 +59,8 @@ struct ioengine_ops {
>  			    uint64_t, struct zbd_zone *, unsigned int);
>  	int (*reset_wp)(struct thread_data *, struct fio_file *,
>  			uint64_t, uint64_t);
> +	int (*get_max_open_zones)(struct thread_data *, struct fio_file *,
> +				  unsigned int *);
>  	int option_struct_size;
>  	struct fio_option *options;
>  };
> diff --git a/oslib/blkzoned.h b/oslib/blkzoned.h
> index 4cc071dc..719b041d 100644
> --- a/oslib/blkzoned.h
> +++ b/oslib/blkzoned.h
> @@ -16,6 +16,8 @@ extern int blkzoned_report_zones(struct thread_data *td,
>  				struct zbd_zone *zones, unsigned int nr_zones);
>  extern int blkzoned_reset_wp(struct thread_data *td, struct fio_file *f,
>  				uint64_t offset, uint64_t length);
> +extern int blkzoned_get_max_open_zones(struct thread_data *td, struct fio_file *f,
> +				       unsigned int *max_open_zones);
>  #else
>  /*
>   * Define stubs for systems that do not have zoned block device support.
> @@ -44,6 +46,11 @@ static inline int blkzoned_reset_wp(struct thread_data *td, struct fio_file *f,
>  {
>  	return -EIO;
>  }
> +static inline int blkzoned_get_max_open_zones(struct thread_data *td, struct fio_file *f,
> +					      unsigned int *max_open_zones)
> +{
> +	return -EIO;
> +}
>  #endif
>  
>  #endif /* FIO_BLKZONED_H */
> diff --git a/oslib/linux-blkzoned.c b/oslib/linux-blkzoned.c
> index 127069ca..82b662a2 100644
> --- a/oslib/linux-blkzoned.c
> +++ b/oslib/linux-blkzoned.c
> @@ -160,6 +160,28 @@ int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
>  	return 0;
>  }
>  
> +int blkzoned_get_max_open_zones(struct thread_data *td, struct fio_file *f,
> +				unsigned int *max_open_zones)
> +{
> +	char *max_open_str;
> +
> +	if (f->filetype != FIO_TYPE_BLOCK) {
> +		return -EIO;
> +	}

No need for the curly brackets.

> +
> +	max_open_str = blkzoned_get_sysfs_attr(f->file_name, "queue/max_open_zones");
> +	if (!max_open_str)
> +		return 0;
> +
> +	dprint(FD_ZBD, "%s: max open zones supported by device: %s\n",
> +	       f->file_name, max_open_str);
> +	*max_open_zones = atoll(max_open_str);
> +
> +	free(max_open_str);
> +
> +	return 0;
> +}
> +
>  static uint64_t zone_capacity(struct blk_zone_report *hdr,
>  			      struct blk_zone *blkz)
>  {
> diff --git a/zbd.c b/zbd.c
> index 8ed8f195..f23bbbac 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -113,6 +113,34 @@ int zbd_reset_wp(struct thread_data *td, struct fio_file *f,
>  	return ret;
>  }
>  
> +/**
> + * zbd_get_max_open_zones - Get the maximum number of open zones
> + * @td: FIO thread data
> + * @f: FIO file for which to get max open zones
> + * @max_open_zones: Upon success, result will be stored here.
> + *
> + * A @max_open_zones value set to zero means no limit.
> + *
> + * Returns 0 upon success and a negative error code upon failure.
> + */
> +int zbd_get_max_open_zones(struct thread_data *td, struct fio_file *f,
> +			   unsigned int *max_open_zones)
> +{
> +	int ret;
> +
> +	if (td->io_ops && td->io_ops->get_max_open_zones)
> +		ret = td->io_ops->get_max_open_zones(td, f, max_open_zones);
> +	else
> +		ret = blkzoned_get_max_open_zones(td, f, max_open_zones);
> +	if (ret < 0) {
> +		td_verror(td, errno, "get max open zones failed");
> +		log_err("%s: get max open zones failed (%d).\n",
> +			f->file_name, errno);
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * zbd_zone_idx - convert an offset into a zone number
>   * @f: file pointer.
> @@ -554,6 +582,48 @@ out:
>  	return ret;
>  }
>  
> +static int zbd_set_max_open_zones(struct thread_data *td, struct fio_file *f)
> +{
> +	struct zoned_block_device_info *zbd = f->zbd_info;
> +	unsigned int max_open_zones;
> +	int ret;
> +
> +	if (zbd->model != ZBD_HOST_MANAGED) {
> +		/* Only host-managed devices have a max open limit */
> +		zbd->max_open_zones = td->o.max_open_zones;
> +		goto out;
> +	}
> +
> +	/* If host-managed, get the max open limit */
> +	ret = zbd_get_max_open_zones(td, f, &max_open_zones);
> +	if (ret)
> +		return ret;
> +
> +	if (td->o.max_open_zones > 0 && max_open_zones > 0 &&
> +	    td->o.max_open_zones > max_open_zones) {
> +		/* User requested a limit, but limit is too large */
> +		td_verror(td, EINVAL,
> +			  "Specified --max_open_zones is too large");
> +		log_err("Specified --max_open_zones (%d) is larger than max (%u)\n",
> +			td->o.max_open_zones, max_open_zones);
> +		return -EINVAL;
> +	} else if (td->o.max_open_zones > 0) {
> +		/* User requested a limit, limit is not too large */
> +		zbd->max_open_zones = td->o.max_open_zones;
> +	} else {
> +		/* User did not request a limit. Set limit to max supported */
> +		zbd->max_open_zones = max_open_zones;
> +	}

Here, you could reverse the order of the if/else if/else. That would simplify
the conditions:

	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 ?

> +
> +	return 0;
> +}
> +
>  /*
>   * Allocate zone information and store it into f->zbd_info if zonemode=zbd.
>   *
> @@ -576,9 +646,13 @@ static int zbd_create_zone_info(struct thread_data *td, struct fio_file *f)
>  	case ZBD_HOST_AWARE:
>  	case ZBD_HOST_MANAGED:
>  		ret = parse_zone_info(td, f);
> +		if (ret)
> +			return ret;
>  		break;
>  	case ZBD_NONE:
>  		ret = init_zone_info(td, f);
> +		if (ret)
> +			return ret;
>  		break;
>  	default:
>  		td_verror(td, EINVAL, "Unsupported zoned model");
> @@ -586,12 +660,15 @@ static int zbd_create_zone_info(struct thread_data *td, struct fio_file *f)
>  		return -EINVAL;
>  	}
>  
> -	if (ret == 0) {
> -		f->zbd_info->model = zbd_model;
> -		f->zbd_info->max_open_zones =
> -			td->o.max_open_zones ?: ZBD_MAX_OPEN_ZONES;
> +	f->zbd_info->model = zbd_model;
> +
> +	ret = zbd_set_max_open_zones(td, f);
> +	if (ret) {
> +		zbd_free_zone_info(f);
> +		return ret;
>  	}
> -	return ret;
> +
> +	return 0;
>  }
>  
>  void zbd_free_zone_info(struct fio_file *f)
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] engines/libzbc: add support for the get_max_open_zones io op

[Damien Le Moal]

On 2021/05/13 7:37, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Add support for the new .get_max_open_zones io operation.
> 
> zbc.c will only ever call this callback for host-managed devices.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  engines/libzbc.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/engines/libzbc.c b/engines/libzbc.c
> index 2aacf7bb..3dde93db 100644
> --- a/engines/libzbc.c
> +++ b/engines/libzbc.c
> @@ -19,6 +19,7 @@ struct libzbc_data {
>  	struct zbc_device	*zdev;
>  	enum zbc_dev_model	model;
>  	uint64_t		nr_sectors;
> +	uint32_t		max_open_seq_req;
>  };
>  
>  static int libzbc_get_dev_info(struct libzbc_data *ld, struct fio_file *f)
> @@ -32,6 +33,7 @@ static int libzbc_get_dev_info(struct libzbc_data *ld, struct fio_file *f)
>  	zbc_get_device_info(ld->zdev, zinfo);
>  	ld->model = zinfo->zbd_model;
>  	ld->nr_sectors = zinfo->zbd_sectors;
> +	ld->max_open_seq_req = zinfo->zbd_max_nr_open_seq_req;
>  
>  	dprint(FD_ZBD, "%s: vendor_id:%s, type: %s, model: %s\n",
>  	       f->file_name, zinfo->zbd_vendor_id,
> @@ -335,6 +337,24 @@ err:
>  	return -ret;
>  }
>  
> +static int libzbc_get_max_open_zones(struct thread_data *td, struct fio_file *f,
> +				     unsigned int *max_open_zones)
> +{
> +	struct libzbc_data *ld;
> +	int ret;
> +
> +	ret = libzbc_open_dev(td, f, &ld);
> +	if (ret)
> +		return ret;
> +
> +	if (ld->max_open_seq_req == ZBC_NO_LIMIT)
> +		*max_open_zones = 0;
> +	else
> +		*max_open_zones = ld->max_open_seq_req;
> +
> +	return 0;
> +}
> +
>  ssize_t libzbc_rw(struct thread_data *td, struct io_u *io_u)
>  {
>  	struct libzbc_data *ld = td->io_ops_data;
> @@ -414,6 +434,7 @@ FIO_STATIC struct ioengine_ops ioengine = {
>  	.get_zoned_model	= libzbc_get_zoned_model,
>  	.report_zones		= libzbc_report_zones,
>  	.reset_wp		= libzbc_reset_wp,
> +	.get_max_open_zones	= libzbc_get_max_open_zones,
>  	.queue			= libzbc_queue,
>  	.flags			= FIO_SYNCIO | FIO_NOEXTEND | FIO_RAWIO,
>  };
> 

Looks good.

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

-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

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

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

[Damien Le Moal]

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

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

[Niklas Cassel]

On Sat, May 15, 2021 at 01:16:00AM +0000, Damien Le Moal wrote:
> 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>
> > > > ---

(snip)

> 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 ?

Even on a zoned block device with max active zones == 0, with a max open
zones limit != 0, writing to more zones than supported will, in addition
to the regular I/O, cause implicit open + implicit closed operations.

These operations will of course take time, time that would otherwise be
spent on I/O, meaning that the results you get would not be representative
of a drive's performance.

While I do agree that this test scenario could be benificial, I do think
that it is a very special type of test.

These patches have been merged already, but I don't see why you can't make
a patch that e.g. adds an ioengine that just opens (imp/exp) + closes zones.
(We had a filedelete ioengine merged recently, which tests how quickly
files than be unlinked.)
Or, if it is easier, you could add a new option to zbd.c --ignore_zbd_limits,
so that you can specify an max open zones limit that is greater than what
the drive supports, in order to facilitate your test scenario.



Reasoning for my suggestion:
1) As you know, fio currently has no accounting of active zones.
It seems a bit awkward to parse max active zones from sysfs, when zbd.c
itself currently has no concept of active zones.
So it seems easier to e.g. add a new parameter that just ignores the device
limits, than to implement full support for active zones.

2) You are using detailed knowledge of how fio handles zones.
It does happen that fio currently uses writes without first doing an explicit
open zone, but should you really take that for granted?
If fio adds support for active zones, perhaps that implementation will chose
to do implement it using explicit zone open, so that if the zone could be
opened, it will also be possible to write to that zone without I/O errors.
(As we have implemented in e.g. zonefs.)


Kind regards,
Niklas