[PATCH v2 0/4] improve zbd initialization

[PATCH v2 0/4] improve zbd initialization

[Niklas Cassel]

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

Hello there,

This series improves zbd initialization, by making it more robust.
Previously, zbd_create_zone_info() could return success at the same
time as f->zbd_info was never initialized. Remove ZBD_IGNORE to
avoid this combination from being possible.

Also, zonemode=zbd can now be used with regular files by emulating
zones inside the file. (Just like how we already support emulating
zones inside a regular block device).


Kind regards,
Niklas


Changes since v1:
-Picked up Damien's Reviewed-by tags on patch 1 and 4.
-Addressed Damien's review comments on patch 2 and 3.

Niklas Cassel (4):
  zbd: disallow pipes for zonemode=zbd
  zbd: allow zonemode=zbd with regular files by emulating zones
  zbd: remove zbd_zoned_model ZBD_IGNORE
  zbd: change some f->zbd_info conditionals to asserts

 engines/libzbc.c            |  6 ++----
 engines/skeleton_external.c |  1 -
 oslib/linux-blkzoned.c      |  6 ++----
 zbd.c                       | 37 +++++++++++++++++++++++++------------
 zbd_types.h                 |  7 +++----
 5 files changed, 32 insertions(+), 25 deletions(-)

-- 
2.31.1

[PATCH v2 2/4] zbd: allow zonemode=zbd with regular files by emulating zones

[Niklas Cassel]

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

Currently when using zonemode=zbd and running against a regular file,
fio will fail with:
fio: file hash not empty on exit

Treat regular files just like how we treat regular (non-zoned) block
devices: return ZBD_NONE and let zbd.c emulate zones inside the regular
file/block device.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 zbd.c       | 14 +++++++++++++-
 zbd_types.h |  2 +-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/zbd.c b/zbd.c
index 60325d28..d1db9adc 100644
--- a/zbd.c
+++ b/zbd.c
@@ -37,6 +37,12 @@ int zbd_get_zoned_model(struct thread_data *td, struct fio_file *f,
 		return -EINVAL;
 	}
 
+	/* If regular file, always emulate zones inside the file. */
+	if (f->filetype == FIO_TYPE_FILE) {
+		*model = ZBD_NONE;
+		return 0;
+	}
+
 	if (td->io_ops && td->io_ops->get_zoned_model)
 		ret = td->io_ops->get_zoned_model(td, f, model);
 	else
@@ -414,7 +420,7 @@ static int init_zone_info(struct thread_data *td, struct fio_file *f)
 	int i;
 
 	if (zone_size == 0) {
-		log_err("%s: Specifying the zone size is mandatory for regular block devices with --zonemode=zbd\n\n",
+		log_err("%s: Specifying the zone size is mandatory for regular file/block device with --zonemode=zbd\n\n",
 			f->file_name);
 		return 1;
 	}
@@ -435,6 +441,12 @@ static int init_zone_info(struct thread_data *td, struct fio_file *f)
 		return 1;
 	}
 
+	if (f->real_file_size < zone_size) {
+		log_err("%s: file/device size %"PRIu64" is smaller than zone size %"PRIu64"\n",
+			f->file_name, f->real_file_size, zone_size);
+		return -EINVAL;
+	}
+
 	nr_zones = (f->real_file_size + zone_size - 1) / zone_size;
 	zbd_info = scalloc(1, sizeof(*zbd_info) +
 			   (nr_zones + 1) * sizeof(zbd_info->zone_info[0]));
diff --git a/zbd_types.h b/zbd_types.h
index 5ed41aa0..d0f4c44e 100644
--- a/zbd_types.h
+++ b/zbd_types.h
@@ -15,7 +15,7 @@
  */
 enum zbd_zoned_model {
 	ZBD_IGNORE,		/* Ignore file */
-	ZBD_NONE,		/* Regular block device */
+	ZBD_NONE,		/* No zone support. Emulate zones. */
 	ZBD_HOST_AWARE,		/* Host-aware zoned block device */
 	ZBD_HOST_MANAGED,	/* Host-managed zoned block device */
 };
-- 
2.31.1

Re: [PATCH v2 2/4] zbd: allow zonemode=zbd with regular files by emulating zones

[Damien Le Moal]

On 2021/06/07 18:16, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Currently when using zonemode=zbd and running against a regular file,
> fio will fail with:
> fio: file hash not empty on exit
> 
> Treat regular files just like how we treat regular (non-zoned) block
> devices: return ZBD_NONE and let zbd.c emulate zones inside the regular
> file/block device.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  zbd.c       | 14 +++++++++++++-
>  zbd_types.h |  2 +-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/zbd.c b/zbd.c
> index 60325d28..d1db9adc 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -37,6 +37,12 @@ int zbd_get_zoned_model(struct thread_data *td, struct fio_file *f,
>  		return -EINVAL;
>  	}
>  
> +	/* If regular file, always emulate zones inside the file. */
> +	if (f->filetype == FIO_TYPE_FILE) {
> +		*model = ZBD_NONE;
> +		return 0;
> +	}
> +
>  	if (td->io_ops && td->io_ops->get_zoned_model)
>  		ret = td->io_ops->get_zoned_model(td, f, model);
>  	else
> @@ -414,7 +420,7 @@ static int init_zone_info(struct thread_data *td, struct fio_file *f)
>  	int i;
>  
>  	if (zone_size == 0) {
> -		log_err("%s: Specifying the zone size is mandatory for regular block devices with --zonemode=zbd\n\n",
> +		log_err("%s: Specifying the zone size is mandatory for regular file/block device with --zonemode=zbd\n\n",
>  			f->file_name);
>  		return 1;
>  	}
> @@ -435,6 +441,12 @@ static int init_zone_info(struct thread_data *td, struct fio_file *f)
>  		return 1;
>  	}
>  
> +	if (f->real_file_size < zone_size) {
> +		log_err("%s: file/device size %"PRIu64" is smaller than zone size %"PRIu64"\n",
> +			f->file_name, f->real_file_size, zone_size);
> +		return -EINVAL;
> +	}
> +
>  	nr_zones = (f->real_file_size + zone_size - 1) / zone_size;
>  	zbd_info = scalloc(1, sizeof(*zbd_info) +
>  			   (nr_zones + 1) * sizeof(zbd_info->zone_info[0]));
> diff --git a/zbd_types.h b/zbd_types.h
> index 5ed41aa0..d0f4c44e 100644
> --- a/zbd_types.h
> +++ b/zbd_types.h
> @@ -15,7 +15,7 @@
>   */
>  enum zbd_zoned_model {
>  	ZBD_IGNORE,		/* Ignore file */
> -	ZBD_NONE,		/* Regular block device */
> +	ZBD_NONE,		/* No zone support. Emulate zones. */
>  	ZBD_HOST_AWARE,		/* Host-aware zoned block device */
>  	ZBD_HOST_MANAGED,	/* Host-managed zoned block device */
>  };
> 

Looks good to me.

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

-- 
Damien Le Moal
Western Digital Research

[PATCH v2 1/4] zbd: disallow pipes for zonemode=zbd

[Niklas Cassel]

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

zoned block device support in fio cannot handle pipes,
so simply reject them and give a clear error message.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 zbd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/zbd.c b/zbd.c
index 5d9e331a..60325d28 100644
--- a/zbd.c
+++ b/zbd.c
@@ -32,6 +32,11 @@ int zbd_get_zoned_model(struct thread_data *td, struct fio_file *f,
 {
 	int ret;
 
+	if (f->filetype == FIO_TYPE_PIPE) {
+		log_err("zonemode=zbd does not support pipes\n");
+		return -EINVAL;
+	}
+
 	if (td->io_ops && td->io_ops->get_zoned_model)
 		ret = td->io_ops->get_zoned_model(td, f, model);
 	else
-- 
2.31.1

[PATCH v2 4/4] zbd: change some f->zbd_info conditionals to asserts

[Niklas Cassel]

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

Unfortunately, generic fio code calls some zbd_* functions unconditionally.
These functions will be called regardless if zonemode == ZONE_MODE_NONE,
ZONE_MODE_STRIDED or ZONE_MODE_ZBD, and cannot be optimized.

However, some functions are only called when zonemode == ZONE_MODE_ZBD.
Since f->zbd_info will always be non-NULL for a job with zonemode=zbd,
these functions can be optimized to not check if f->zbd_info is set.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 zbd.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/zbd.c b/zbd.c
index aab4d741..8e99eb95 100644
--- a/zbd.c
+++ b/zbd.c
@@ -808,8 +808,7 @@ int zbd_setup_files(struct thread_data *td)
 		struct fio_zone_info *z;
 		int zi;
 
-		if (!zbd)
-			continue;
+		assert(zbd);
 
 		f->min_zone = zbd_zone_idx(f, f->file_offset);
 		f->max_zone = zbd_zone_idx(f, f->file_offset + f->io_size);
@@ -1470,8 +1469,7 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 	uint32_t zone_idx;
 	uint64_t zone_end;
 
-	if (!zbd_info)
-		return;
+	assert(zbd_info);
 
 	zone_idx = zbd_zone_idx(f, io_u->offset);
 	assert(zone_idx < zbd_info->nr_zones);
@@ -1531,8 +1529,7 @@ static void zbd_put_io(struct thread_data *td, const struct io_u *io_u)
 	struct fio_zone_info *z;
 	uint32_t zone_idx;
 
-	if (!zbd_info)
-		return;
+	assert(zbd_info);
 
 	zone_idx = zbd_zone_idx(f, io_u->offset);
 	assert(zone_idx < zbd_info->nr_zones);
@@ -1588,6 +1585,7 @@ void setup_zbd_zone_mode(struct thread_data *td, struct io_u *io_u)
 
 	assert(td->o.zone_mode == ZONE_MODE_ZBD);
 	assert(td->o.zone_size);
+	assert(f->zbd_info);
 
 	zone_idx = zbd_zone_idx(f, f->last_pos[ddir]);
 	z = get_zone(f, zone_idx);
@@ -1662,6 +1660,7 @@ enum fio_ddir zbd_adjust_ddir(struct thread_data *td, struct io_u *io_u,
 	 * devices with all empty zones. Overwrite the first I/O direction as
 	 * write to make sure data to read exists.
 	 */
+	assert(io_u->file->zbd_info);
 	if (ddir != DDIR_READ || !td_rw(td))
 		return ddir;
 
@@ -1691,9 +1690,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	uint64_t new_len;
 	int64_t range;
 
-	if (!f->zbd_info)
-		return io_u_accept;
-
+	assert(f->zbd_info);
 	assert(min_bs);
 	assert(is_valid_offset(f, io_u->offset));
 	assert(io_u->buflen);
-- 
2.31.1

[PATCH v2 3/4] zbd: remove zbd_zoned_model ZBD_IGNORE

[Niklas Cassel]

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

For a job with zonemode=zbd, we do not want any file to be ignored.
Each file's file type in that job should be supported by either zbd.c
or the ioengine. If not, we should return an error.
This way, ZBD_IGNORE becomes redundant and can be removed.

By removing ZBD_IGNORE, we know that all files belonging to a job that
has zonemode=zbd set, will either be a zoned block device, or emulate
a zoned block device.

This means that for jobs that have zonemode=zbd, f->zbd_info will always
be non-NULL. This will make the zbd code slightly easier to reason about
and to maintain.

When removing zbd_zoned_model ZBD_IGNORE, define the new first enum value
as 0x1, so that we avoid potential ABI problems with existing binaries.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 engines/libzbc.c            | 6 ++----
 engines/skeleton_external.c | 1 -
 oslib/linux-blkzoned.c      | 6 ++----
 zbd.c                       | 3 +--
 zbd_types.h                 | 7 +++----
 5 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/engines/libzbc.c b/engines/libzbc.c
index 3dde93db..7f2bc431 100644
--- a/engines/libzbc.c
+++ b/engines/libzbc.c
@@ -180,10 +180,8 @@ static int libzbc_get_zoned_model(struct thread_data *td, struct fio_file *f,
 	struct libzbc_data *ld;
 	int ret;
 
-	if (f->filetype != FIO_TYPE_BLOCK && f->filetype != FIO_TYPE_CHAR) {
-		*model = ZBD_IGNORE;
-		return 0;
-	}
+	if (f->filetype != FIO_TYPE_BLOCK && f->filetype != FIO_TYPE_CHAR)
+		return -EINVAL;
 
 	ret = libzbc_open_dev(td, f, &ld);
 	if (ret)
diff --git a/engines/skeleton_external.c b/engines/skeleton_external.c
index c79b6f11..cff83a10 100644
--- a/engines/skeleton_external.c
+++ b/engines/skeleton_external.c
@@ -156,7 +156,6 @@ static int fio_skeleton_close(struct thread_data *td, struct fio_file *f)
 /*
  * Hook for getting the zoned model of a zoned block device for zonemode=zbd.
  * The zoned model can be one of (see zbd_types.h):
- * - ZBD_IGNORE: skip regular files
  * - ZBD_NONE: regular block device (zone emulation will be used)
  * - ZBD_HOST_AWARE: host aware zoned block device
  * - ZBD_HOST_MANAGED: host managed zoned block device
diff --git a/oslib/linux-blkzoned.c b/oslib/linux-blkzoned.c
index 6f89ec6f..4e441d29 100644
--- a/oslib/linux-blkzoned.c
+++ b/oslib/linux-blkzoned.c
@@ -140,10 +140,8 @@ int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
 {
 	char *model_str = NULL;
 
-	if (f->filetype != FIO_TYPE_BLOCK) {
-		*model = ZBD_IGNORE;
-		return 0;
-	}
+	if (f->filetype != FIO_TYPE_BLOCK)
+		return -EINVAL;
 
 	*model = ZBD_NONE;
 
diff --git a/zbd.c b/zbd.c
index d1db9adc..aab4d741 100644
--- a/zbd.c
+++ b/zbd.c
@@ -661,8 +661,6 @@ static int zbd_create_zone_info(struct thread_data *td, struct fio_file *f)
 		return ret;
 
 	switch (zbd_model) {
-	case ZBD_IGNORE:
-		return 0;
 	case ZBD_HOST_AWARE:
 	case ZBD_HOST_MANAGED:
 		ret = parse_zone_info(td, f);
@@ -680,6 +678,7 @@ static int zbd_create_zone_info(struct thread_data *td, struct fio_file *f)
 		return -EINVAL;
 	}
 
+	assert(f->zbd_info);
 	f->zbd_info->model = zbd_model;
 
 	ret = zbd_set_max_open_zones(td, f);
diff --git a/zbd_types.h b/zbd_types.h
index d0f4c44e..0a8630cb 100644
--- a/zbd_types.h
+++ b/zbd_types.h
@@ -14,10 +14,9 @@
  * Zoned block device models.
  */
 enum zbd_zoned_model {
-	ZBD_IGNORE,		/* Ignore file */
-	ZBD_NONE,		/* No zone support. Emulate zones. */
-	ZBD_HOST_AWARE,		/* Host-aware zoned block device */
-	ZBD_HOST_MANAGED,	/* Host-managed zoned block device */
+	ZBD_NONE		= 0x1,	/* No zone support. Emulate zones. */
+	ZBD_HOST_AWARE		= 0x2,	/* Host-aware zoned block device */
+	ZBD_HOST_MANAGED	= 0x3,	/* Host-managed zoned block device */
 };
 
 /*
-- 
2.31.1

Re: [PATCH v2 3/4] zbd: remove zbd_zoned_model ZBD_IGNORE

[Damien Le Moal]

On 2021/06/07 18:16, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> For a job with zonemode=zbd, we do not want any file to be ignored.
> Each file's file type in that job should be supported by either zbd.c
> or the ioengine. If not, we should return an error.
> This way, ZBD_IGNORE becomes redundant and can be removed.
> 
> By removing ZBD_IGNORE, we know that all files belonging to a job that
> has zonemode=zbd set, will either be a zoned block device, or emulate
> a zoned block device.
> 
> This means that for jobs that have zonemode=zbd, f->zbd_info will always
> be non-NULL. This will make the zbd code slightly easier to reason about
> and to maintain.
> 
> When removing zbd_zoned_model ZBD_IGNORE, define the new first enum value
> as 0x1, so that we avoid potential ABI problems with existing binaries.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  engines/libzbc.c            | 6 ++----
>  engines/skeleton_external.c | 1 -
>  oslib/linux-blkzoned.c      | 6 ++----
>  zbd.c                       | 3 +--
>  zbd_types.h                 | 7 +++----
>  5 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/engines/libzbc.c b/engines/libzbc.c
> index 3dde93db..7f2bc431 100644
> --- a/engines/libzbc.c
> +++ b/engines/libzbc.c
> @@ -180,10 +180,8 @@ static int libzbc_get_zoned_model(struct thread_data *td, struct fio_file *f,
>  	struct libzbc_data *ld;
>  	int ret;
>  
> -	if (f->filetype != FIO_TYPE_BLOCK && f->filetype != FIO_TYPE_CHAR) {
> -		*model = ZBD_IGNORE;
> -		return 0;
> -	}
> +	if (f->filetype != FIO_TYPE_BLOCK && f->filetype != FIO_TYPE_CHAR)
> +		return -EINVAL;
>  
>  	ret = libzbc_open_dev(td, f, &ld);
>  	if (ret)
> diff --git a/engines/skeleton_external.c b/engines/skeleton_external.c
> index c79b6f11..cff83a10 100644
> --- a/engines/skeleton_external.c
> +++ b/engines/skeleton_external.c
> @@ -156,7 +156,6 @@ static int fio_skeleton_close(struct thread_data *td, struct fio_file *f)
>  /*
>   * Hook for getting the zoned model of a zoned block device for zonemode=zbd.
>   * The zoned model can be one of (see zbd_types.h):
> - * - ZBD_IGNORE: skip regular files
>   * - ZBD_NONE: regular block device (zone emulation will be used)
>   * - ZBD_HOST_AWARE: host aware zoned block device
>   * - ZBD_HOST_MANAGED: host managed zoned block device
> diff --git a/oslib/linux-blkzoned.c b/oslib/linux-blkzoned.c
> index 6f89ec6f..4e441d29 100644
> --- a/oslib/linux-blkzoned.c
> +++ b/oslib/linux-blkzoned.c
> @@ -140,10 +140,8 @@ int blkzoned_get_zoned_model(struct thread_data *td, struct fio_file *f,
>  {
>  	char *model_str = NULL;
>  
> -	if (f->filetype != FIO_TYPE_BLOCK) {
> -		*model = ZBD_IGNORE;
> -		return 0;
> -	}
> +	if (f->filetype != FIO_TYPE_BLOCK)
> +		return -EINVAL;
>  
>  	*model = ZBD_NONE;
>  
> diff --git a/zbd.c b/zbd.c
> index d1db9adc..aab4d741 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -661,8 +661,6 @@ static int zbd_create_zone_info(struct thread_data *td, struct fio_file *f)
>  		return ret;
>  
>  	switch (zbd_model) {
> -	case ZBD_IGNORE:
> -		return 0;
>  	case ZBD_HOST_AWARE:
>  	case ZBD_HOST_MANAGED:
>  		ret = parse_zone_info(td, f);
> @@ -680,6 +678,7 @@ static int zbd_create_zone_info(struct thread_data *td, struct fio_file *f)
>  		return -EINVAL;
>  	}
>  
> +	assert(f->zbd_info);
>  	f->zbd_info->model = zbd_model;
>  
>  	ret = zbd_set_max_open_zones(td, f);
> diff --git a/zbd_types.h b/zbd_types.h
> index d0f4c44e..0a8630cb 100644
> --- a/zbd_types.h
> +++ b/zbd_types.h
> @@ -14,10 +14,9 @@
>   * Zoned block device models.
>   */
>  enum zbd_zoned_model {
> -	ZBD_IGNORE,		/* Ignore file */
> -	ZBD_NONE,		/* No zone support. Emulate zones. */
> -	ZBD_HOST_AWARE,		/* Host-aware zoned block device */
> -	ZBD_HOST_MANAGED,	/* Host-managed zoned block device */
> +	ZBD_NONE		= 0x1,	/* No zone support. Emulate zones. */
> +	ZBD_HOST_AWARE		= 0x2,	/* Host-aware zoned block device */
> +	ZBD_HOST_MANAGED	= 0x3,	/* Host-managed zoned block device */
>  };
>  
>  /*
> 

Looks good.

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

-- 
Damien Le Moal
Western Digital Research