[PATCH 0/4] zbd: Support zone reset by trim

[PATCH 0/4] zbd: Support zone reset by trim

[Shin'ichiro Kawasaki]

Let me repost this series to refresh its status. Reviews on the list will be
appreciated. I confirmed it can be applied to the current fio master branch tip.

Trim workload is currently not supported for zonemode=zbd. This patch series
enables it by resetting zones for trim I/Os. This allows fio to measure
performance of zoned block devices using more realistic workloads which mixes
write and trim.

The first patch prepares for trim support by modifying the helper function
zbd_find_zone(). The second patch adds zone reset for trim. The third patch
enables trim workload for libzbc I/O engine. The fourth patch adds a test
case that exercises trim operation with zonemode=zbd.

Shin'ichiro Kawasaki (4):
  zbd: Add min_bytes argument to zbd_find_zone()
  zbd: Support zone reset by trim
  engines/libzbc: Enable trim for libzbc I/O engine
  t/zbd: Add test #58 to test zone reset by trim workload

 HOWTO                  |  3 ++
 engines/libzbc.c       | 16 +++++---
 fio.1                  |  2 +
 io_u.c                 |  9 +++++
 t/zbd/test-zbd-support | 26 ++++++++++++
 zbd.c                  | 89 +++++++++++++++++++++++++++++++++++-------
 zbd.h                  |  2 +
 7 files changed, 127 insertions(+), 20 deletions(-)

-- 
2.31.1

[PATCH 1/4] zbd: Add min_bytes argument to zbd_find_zone()

[Shin'ichiro Kawasaki]

The helper function zbd_find_zone() finds a zone with at least
min_bs[DDIR_READ] bytes readable before the zone write pointer. This
patch generalizes this function to allow finding a non-empty zone. To do
so, add the min_bytes argument to specify the minimum readable data of a
zone to filter the search. Specifying 1 to min_bytes then become
equivalent to finding a non-empty zone.

This change will allow to reuse this function to find a suitable zone
for trim I/O.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 zbd.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/zbd.c b/zbd.c
index 04c68dea..f10b3267 100644
--- a/zbd.c
+++ b/zbd.c
@@ -1413,18 +1413,16 @@ static struct fio_zone_info *zbd_replay_write_order(struct thread_data *td,
 }
 
 /*
- * Find another zone for which @io_u fits in the readable data in the zone.
- * Search in zones @zb + 1 .. @zl. For random workload, also search in zones
- * @zb - 1 .. @zf.
+ * Find another zone which has @min_bytes readable data. Search in zones
+ * @zb + 1 .. @zl. For random workload, also search in zones @zb - 1 .. @zf.
  *
  * Either returns NULL or returns a zone pointer. When the zone has write
  * pointer, hold the mutex for the zone.
  */
 static struct fio_zone_info *
-zbd_find_zone(struct thread_data *td, struct io_u *io_u,
+zbd_find_zone(struct thread_data *td, struct io_u *io_u, uint32_t min_bytes,
 	      struct fio_zone_info *zb, struct fio_zone_info *zl)
 {
-	const uint32_t min_bs = td->o.min_bs[io_u->ddir];
 	struct fio_file *f = io_u->file;
 	struct fio_zone_info *z1, *z2;
 	const struct fio_zone_info *const zf = get_zone(f, f->min_zone);
@@ -1437,7 +1435,7 @@ zbd_find_zone(struct thread_data *td, struct io_u *io_u,
 		if (z1 < zl && z1->cond != ZBD_ZONE_COND_OFFLINE) {
 			if (z1->has_wp)
 				zone_lock(td, f, z1);
-			if (z1->start + min_bs <= z1->wp)
+			if (z1->start + min_bytes <= z1->wp)
 				return z1;
 			if (z1->has_wp)
 				zone_unlock(z1);
@@ -1448,14 +1446,14 @@ zbd_find_zone(struct thread_data *td, struct io_u *io_u,
 		    z2->cond != ZBD_ZONE_COND_OFFLINE) {
 			if (z2->has_wp)
 				zone_lock(td, f, z2);
-			if (z2->start + min_bs <= z2->wp)
+			if (z2->start + min_bytes <= z2->wp)
 				return z2;
 			if (z2->has_wp)
 				zone_unlock(z2);
 		}
 	}
-	dprint(FD_ZBD, "%s: adjusting random read offset failed\n",
-	       f->file_name);
+	dprint(FD_ZBD, "%s: no zone has %d bytes readable data\n",
+	       f->file_name, min_bytes);
 	return NULL;
 }
 
@@ -1784,7 +1782,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 		    ((!td_random(td)) && (io_u->offset + min_bs > zb->wp))) {
 			zone_unlock(zb);
 			zl = get_zone(f, f->max_zone);
-			zb = zbd_find_zone(td, io_u, zb, zl);
+			zb = zbd_find_zone(td, io_u, min_bs, zb, zl);
 			if (!zb) {
 				dprint(FD_ZBD,
 				       "%s: zbd_find_zone(%lld, %llu) failed\n",
-- 
2.31.1

[PATCH 2/4] zbd: Support zone reset by trim

[Shin'ichiro Kawasaki]

Enable trim workload for zonemode=zbd by modifying do_io_u_trim() to
call zoned block device unique function zbd_do_io_u_trim() which resets
target zone. This allows fio to emulate workloads which mix data
read/write and zone resets with zonemode=zbd.

To call reset zones, the trim I/O shall have offset aligned to zone
start and block size same as zone size. Reset zone is called only to
sequential write required zones and sequential write preferred zones.
Conventional zones are handled in same manner as regular block devices
by calling os_trim() function.

When zones are reset with random trim workload, choose only non-empty
zones as trim target. This avoids meaningless trim to empty zones and
makes the workload more realistic. To find the non-empty zones, utilize
zbd_find_zone() helper function which is already used for read workload,
specifying 1 byte as the minimum valid data size.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 HOWTO  |  3 +++
 fio.1  |  2 ++
 io_u.c |  9 ++++++++
 zbd.c  | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 zbd.h  |  2 ++
 5 files changed, 83 insertions(+), 4 deletions(-)

diff --git a/HOWTO b/HOWTO
index 59c7f1ff..f1fd2045 100644
--- a/HOWTO
+++ b/HOWTO
@@ -992,6 +992,9 @@ Target file/device
 				single zone. The :option:`zoneskip` parameter
 				is ignored. :option:`zonerange` and
 				:option:`zonesize` must be identical.
+				Trim is handled using a zone reset operation.
+				Trim only considers non-empty sequential write
+				required and sequential write preferred zones.
 
 .. option:: zonerange=int
 
diff --git a/fio.1 b/fio.1
index 6cc82542..ef319062 100644
--- a/fio.1
+++ b/fio.1
@@ -766,6 +766,8 @@ starts. The \fBzonecapacity\fR parameter is ignored.
 Zoned block device mode. I/O happens sequentially in each zone, even if random
 I/O has been selected. Random I/O happens across all zones instead of being
 restricted to a single zone.
+Trim is handled using a zone reset operation. Trim only considers non-empty
+sequential write required and sequential write preferred zones.
 .RE
 .RE
 .TP
diff --git a/io_u.c b/io_u.c
index 9a1cd547..696d25cd 100644
--- a/io_u.c
+++ b/io_u.c
@@ -2317,10 +2317,19 @@ int do_io_u_trim(const struct thread_data *td, struct io_u *io_u)
 	struct fio_file *f = io_u->file;
 	int ret;
 
+	if (td->o.zone_mode == ZONE_MODE_ZBD) {
+		ret = zbd_do_io_u_trim(td, io_u);
+		if (ret == io_u_completed)
+			return io_u->xfer_buflen;
+		if (ret)
+			goto err;
+	}
+
 	ret = os_trim(f, io_u->offset, io_u->xfer_buflen);
 	if (!ret)
 		return io_u->xfer_buflen;
 
+err:
 	io_u->error = ret;
 	return 0;
 #endif
diff --git a/zbd.c b/zbd.c
index f10b3267..39060ecd 100644
--- a/zbd.c
+++ b/zbd.c
@@ -375,12 +375,24 @@ static bool zbd_verify_bs(void)
 	int i, j, k;
 
 	for_each_td(td, i) {
+		if (td_trim(td) &&
+		    (td->o.min_bs[DDIR_TRIM] != td->o.max_bs[DDIR_TRIM] ||
+		     td->o.bssplit_nr[DDIR_TRIM])) {
+			log_info("bsrange and bssplit is not allowed for trim with zonemode=zbd\n");
+			return false;
+		}
 		for_each_file(td, f, j) {
 			uint64_t zone_size;
 
 			if (!f->zbd_info)
 				continue;
 			zone_size = f->zbd_info->zone_size;
+			if (td_trim(td) && td->o.bs[DDIR_TRIM] != zone_size) {
+				log_info("%s: trim block size %llu is not the zone size %llu\n",
+					 f->file_name, td->o.bs[DDIR_TRIM],
+					 (unsigned long long)zone_size);
+				return false;
+			}
 			for (k = 0; k < FIO_ARRAY_SIZE(td->o.bs); k++) {
 				if (td->o.verify != VERIFY_NONE &&
 				    zone_size % td->o.bs[k] != 0) {
@@ -1528,9 +1540,6 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 		pthread_mutex_unlock(&zbd_info->mutex);
 		z->wp = zone_end;
 		break;
-	case DDIR_TRIM:
-		assert(z->wp == z->start);
-		break;
 	default:
 		break;
 	}
@@ -1910,8 +1919,23 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			(zbd_zone_capacity_end(zb) - io_u->offset), min_bs);
 		goto eof;
 	case DDIR_TRIM:
-		/* fall-through */
+		/* Check random trim targets a non-empty zone */
+		if (!td_random(td) || zb->wp > zb->start)
+			goto accept;
+
+		/* Find out a non-empty zone to trim */
+		zone_unlock(zb);
+		zl = get_zone(f, f->max_zone);
+		zb = zbd_find_zone(td, io_u, 1, zb, zl);
+		if (zb) {
+			io_u->offset = zb->start;
+			dprint(FD_ZBD, "%s: found new zone(%lld) for trim\n",
+			       f->file_name, io_u->offset);
+			goto accept;
+		}
+		goto eof;
 	case DDIR_SYNC:
+		/* fall-through */
 	case DDIR_DATASYNC:
 	case DDIR_SYNC_FILE_RANGE:
 	case DDIR_WAIT:
@@ -1952,3 +1976,42 @@ char *zbd_write_status(const struct thread_stat *ts)
 		return NULL;
 	return res;
 }
+
+/**
+ * zbd_do_io_u_trim - If reset zone is applicable, do reset zone instead of trim
+ *
+ * @td: FIO thread data.
+ * @io_u: FIO I/O unit.
+ *
+ * It is assumed that z->mutex is already locked.
+ * Return io_u_completed when reset zone succeeds. Return 0 when the target zone
+ * does not have write pointer. On error, return negative errno.
+ */
+int zbd_do_io_u_trim(const struct thread_data *td, struct io_u *io_u)
+{
+	struct fio_file *f = io_u->file;
+	struct fio_zone_info *z;
+	uint32_t zone_idx;
+	int ret;
+
+	zone_idx = zbd_zone_idx(f, io_u->offset);
+	z = get_zone(f, zone_idx);
+
+	if (!z->has_wp)
+		return 0;
+
+	if (io_u->offset != z->start) {
+		log_err("Trim offset not at zone start (%lld)\n", io_u->offset);
+		return -EINVAL;
+	}
+
+	/*
+	 * Cast td to drop const modifier so that zbd_reset_zone() can change td
+	 * members.
+	 */
+	ret = zbd_reset_zone((struct thread_data *)td, f, z);
+	if (ret < 0)
+		return ret;
+
+	return io_u_completed;
+}
diff --git a/zbd.h b/zbd.h
index 39dc45e3..0a73b41d 100644
--- a/zbd.h
+++ b/zbd.h
@@ -17,6 +17,7 @@ struct fio_file;
 enum io_u_action {
 	io_u_accept	= 0,
 	io_u_eof	= 1,
+	io_u_completed  = 2,
 };
 
 /**
@@ -99,6 +100,7 @@ enum fio_ddir zbd_adjust_ddir(struct thread_data *td, struct io_u *io_u,
 			      enum fio_ddir ddir);
 enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u);
 char *zbd_write_status(const struct thread_stat *ts);
+int zbd_do_io_u_trim(const struct thread_data *td, struct io_u *io_u);
 
 static inline void zbd_close_file(struct fio_file *f)
 {
-- 
2.31.1

[PATCH 3/4] engines/libzbc: Enable trim for libzbc I/O engine

[Shin'ichiro Kawasaki]

The trim workload to zoned block devices is supported as zone reset, and
this feature is available for I/O engines which support both zoned
devices and trim workload. Libzbc I/O engine supports zoned but lacks
trim workload support. To enable trim support with libzbc I/O engine,
remove the check which inhibited trim from requests to libzbc I/O
engine. Also set file open flags for trim same as write, and call
zbd_do_io_u_trim() for trim I/O.

Of note is that libzbc I/O engine now can support trim to sequential
write required zones, but still can not support os_trim() call and
BLKDISCARD ioctl for the conventional zones. The trim I/Os to
conventional zones are reported as an error.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 engines/libzbc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/engines/libzbc.c b/engines/libzbc.c
index 7f2bc431..5b4c5e8e 100644
--- a/engines/libzbc.c
+++ b/engines/libzbc.c
@@ -14,6 +14,7 @@
 #include "fio.h"
 #include "err.h"
 #include "zbd_types.h"
+#include "zbd.h"
 
 struct libzbc_data {
 	struct zbc_device	*zdev;
@@ -63,7 +64,7 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
 		return -EINVAL;
 	}
 
-	if (td_write(td)) {
+	if (td_write(td) || td_trim(td)) {
 		if (!read_only)
 			flags |= O_RDWR;
 	} else if (td_read(td)) {
@@ -71,10 +72,6 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
 			flags |= O_RDWR;
 		else
 			flags |= O_RDONLY;
-	} else if (td_trim(td)) {
-		td_verror(td, EINVAL, "libzbc does not support trim");
-		log_err("%s: libzbc does not support trim\n", f->file_name);
-		return -EINVAL;
 	}
 
 	if (td->o.oatomic) {
@@ -411,7 +408,14 @@ static enum fio_q_status libzbc_queue(struct thread_data *td, struct io_u *io_u)
 		ret = zbc_flush(ld->zdev);
 		if (ret)
 			log_err("zbc_flush error %zd\n", ret);
-	} else if (io_u->ddir != DDIR_TRIM) {
+	} else if (io_u->ddir == DDIR_TRIM) {
+		ret = zbd_do_io_u_trim(td, io_u);
+		if (!ret) {
+			log_err("libzbc does not support trim to "
+				"conventional zones\n");
+			ret = EINVAL;
+		}
+	} else {
 		log_err("Unsupported operation %u\n", io_u->ddir);
 		ret = -EINVAL;
 	}
-- 
2.31.1

[PATCH 4/4] t/zbd: Add test #58 to test zone reset by trim workload

[Shin'ichiro Kawasaki]

To exercise zone reset by trim workload, add the test case #58. As the
precondition, it fills several zones. After that, a trim job and a write
job run in parallel for 30 seconds. The ratio of trim commands and write
commands is controlled by --flow option.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 t/zbd/test-zbd-support | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index 57e6d05e..5103c406 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -1215,6 +1215,32 @@ test57() {
 		>> "${logfile}.${test_number}" 2>&1 || return $?
 }
 
+# Random writes and random trims to sequential write required zones for 30s.
+test58() {
+    local off size bs
+
+    require_seq_zones 128 || return $SKIP_TESTCASE
+
+    size=$((zone_size * 128))
+    bs="$(max $((zone_size / 128)) "$logical_block_size")"
+    prep_write
+    off=$((first_sequential_zone_sector * 512))
+    run_fio --zonemode=zbd --direct=1 --zonesize="${zone_size}" --thread=1 \
+	    --filename="${dev}" --norandommap=1 \
+            --name="precondition"  --rw=write "$(ioengine "psync")" \
+            --offset="${off}" --size=$((zone_size * 16)) --bs="${bs}" \
+	    "${job_var_opts[@]}" \
+	    --name=wjob --wait_for="precondition" --rw=randwrite \
+	    "$(ioengine "libaio")" --iodepth=8 \
+	    --offset="${off}" --size="${size}" --bs="${bs}" \
+	    --time_based --runtime=30s --flow=128 "${job_var_opts[@]}" \
+	    --name=trimjob --wait_for="precondition" --rw=randtrim \
+	    "$(ioengine "psync")" \
+	    --offset="${off}" --size="${size}" --bs="${zone_size}" \
+	    --time_based --runtime=30s --flow=1 "${job_var_opts[@]}" \
+	    >>"${logfile}.${test_number}" 2>&1
+}
+
 SECONDS=0
 tests=()
 dynamic_analyzer=()
-- 
2.31.1

Re: [PATCH 1/4] zbd: Add min_bytes argument to zbd_find_zone()

[Dmitry Fomichev]

On Wed, 2021-07-28 at 19:47 +0900, Shin'ichiro Kawasaki wrote:
> The helper function zbd_find_zone() finds a zone with at least
> min_bs[DDIR_READ] bytes readable before the zone write pointer. This
> patch generalizes this function to allow finding a non-empty zone. To
> do
> so, add the min_bytes argument to specify the minimum readable data of
> a
> zone to filter the search. Specifying 1 to min_bytes then become
> equivalent to finding a non-empty zone.
> 
> This change will allow to reuse this function to find a suitable zone
> for trim I/O.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

With a nit below,
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>

> diff --git a/zbd.c b/zbd.c
> index 04c68dea..f10b3267 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -1413,18 +1413,16 @@ static struct fio_zone_info
> *zbd_replay_write_order(struct thread_data *td,
>  }
>  
>  /*
> - * Find another zone for which @io_u fits in the readable data in the
> zone.
> - * Search in zones @zb + 1 .. @zl. For random workload, also search in
> zones
> - * @zb - 1 .. @zf.
> + * Find another zone which has @min_bytes readable data. Search in
> zones
> + * @zb + 1 .. @zl. For random workload, also search in zones @zb - 1
> .. @zf.
>   *
>   * Either returns NULL or returns a zone pointer. When the zone has
> write
>   * pointer, hold the mutex for the zone.
>   */
>  static struct fio_zone_info *
> -zbd_find_zone(struct thread_data *td, struct io_u *io_u,
> +zbd_find_zone(struct thread_data *td, struct io_u *io_u, uint32_t
> min_bytes,
>               struct fio_zone_info *zb, struct fio_zone_info *zl)
>  {
> -       const uint32_t min_bs = td->o.min_bs[io_u->ddir];
>         struct fio_file *f = io_u->file;
>         struct fio_zone_info *z1, *z2;
>         const struct fio_zone_info *const zf = get_zone(f, f-
> >min_zone);
> @@ -1437,7 +1435,7 @@ zbd_find_zone(struct thread_data *td, struct io_u
> *io_u,
>                 if (z1 < zl && z1->cond != ZBD_ZONE_COND_OFFLINE) {
>                         if (z1->has_wp)
>                                 zone_lock(td, f, z1);
> -                       if (z1->start + min_bs <= z1->wp)
> +                       if (z1->start + min_bytes <= z1->wp)
>                                 return z1;
>                         if (z1->has_wp)
>                                 zone_unlock(z1);
> @@ -1448,14 +1446,14 @@ zbd_find_zone(struct thread_data *td, struct
> io_u *io_u,
>                     z2->cond != ZBD_ZONE_COND_OFFLINE) {
>                         if (z2->has_wp)
>                                 zone_lock(td, f, z2);
> -                       if (z2->start + min_bs <= z2->wp)
> +                       if (z2->start + min_bytes <= z2->wp)
>                                 return z2;
>                         if (z2->has_wp)
>                                 zone_unlock(z2);
>                 }
>         }
> -       dprint(FD_ZBD, "%s: adjusting random read offset failed\n",
> -              f->file_name);
> +       dprint(FD_ZBD, "%s: no zone has %d bytes readable data\n",
> +              f->file_name, min_bytes);

This message is more clear than the old version, but you could say
"bytes of readable data" here instead of "bytes readable data". The
same wording change can be done in the patch annotation.

>         return NULL;
>  }
>  
> @@ -1784,7 +1782,7 @@ enum io_u_action zbd_adjust_block(struct
> thread_data *td, struct io_u *io_u)
>                     ((!td_random(td)) && (io_u->offset + min_bs > zb-
> >wp))) {
>                         zone_unlock(zb);
>                         zl = get_zone(f, f->max_zone);
> -                       zb = zbd_find_zone(td, io_u, zb, zl);
> +                       zb = zbd_find_zone(td, io_u, min_bs, zb, zl);
>                         if (!zb) {
>                                 dprint(FD_ZBD,
>                                        "%s: zbd_find_zone(%lld, %llu)
> failed\n",

Re: [PATCH 2/4] zbd: Support zone reset by trim

[Dmitry Fomichev]

On Wed, 2021-07-28 at 19:47 +0900, Shin'ichiro Kawasaki wrote:
> Enable trim workload for zonemode=zbd by modifying do_io_u_trim() to
> call zoned block device unique function zbd_do_io_u_trim() which resets
> target zone. This allows fio to emulate workloads which mix data
> read/write and zone resets with zonemode=zbd.
> 
> To call reset zones, the trim I/O shall have offset aligned to zone
> start and block size same as zone size. Reset zone is called only to
> sequential write required zones and sequential write preferred zones.
> Conventional zones are handled in same manner as regular block devices
> by calling os_trim() function.
> 
> When zones are reset with random trim workload, choose only non-empty
> zones as trim target. This avoids meaningless trim to empty zones and
> makes the workload more realistic. To find the non-empty zones, utilize
> zbd_find_zone() helper function which is already used for read
> workload,
> specifying 1 byte as the minimum valid data size.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  HOWTO  |  3 +++
>  fio.1  |  2 ++
>  io_u.c |  9 ++++++++
>  zbd.c  | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  zbd.h  |  2 ++
>  5 files changed, 83 insertions(+), 4 deletions(-)
> 
> diff --git a/HOWTO b/HOWTO
> index 59c7f1ff..f1fd2045 100644
> --- a/HOWTO
> +++ b/HOWTO
> @@ -992,6 +992,9 @@ Target file/device
>                                 single zone. The :option:`zoneskip`
> parameter
>                                 is ignored. :option:`zonerange` and
>                                 :option:`zonesize` must be identical.
> +                               Trim is handled using a zone reset
> operation.
> +                               Trim only considers non-empty
> sequential write
> +                               required and sequential write preferred
> zones.

The documentation changes could be moved to a separate patch. While
looking at HOWTO file, I notice that libzbc is absent from the list of
i/o engines there. Perhaps, since you are making changes to this file,
you could add a small description of libzbc there and mention that it
supports read, write and trim workloads when run against zoned block
devices.

>  
>  .. option:: zonerange=int
>  
> diff --git a/fio.1 b/fio.1
> index 6cc82542..ef319062 100644
> --- a/fio.1
> +++ b/fio.1
> @@ -766,6 +766,8 @@ starts. The \fBzonecapacity\fR parameter is
> ignored.
>  Zoned block device mode. I/O happens sequentially in each zone, even
> if random32ad4373
>  I/O has been selected. Random I/O happens across all zones instead of
> being
>  restricted to a single zone.
> +Trim is handled using a zone reset operation. Trim only considers non-
> empty
> +sequential write required and sequential write preferred zones.
>  .RE
>  .RE
>  .TP
> diff --git a/io_u.c b/io_u.c
> index 9a1cd547..696d25cd 100644
> --- a/io_u.c
> +++ b/io_u.c
> @@ -2317,10 +2317,19 @@ int do_io_u_trim(const struct thread_data *td,
> struct io_u *io_u)
>         struct fio_file *f = io_u->file;
>         int ret;
>  
> +       if (td->o.zone_mode == ZONE_MODE_ZBD) {
> +               ret = zbd_do_io_u_trim(td, io_u);
> +               if (ret == io_u_completed)
> +                       return io_u->xfer_buflen;
> +               if (ret)
> +                       goto err;
> +       }
> +
>         ret = os_trim(f, io_u->offset, io_u->xfer_buflen);
>         if (!ret)
>                 return io_u->xfer_buflen;
>  
> +err:
>         io_u->error = ret;
>         return 0;
>  #endif
> diff --git a/zbd.c b/zbd.c
> index f10b3267..39060ecd 100644
> --- a/zbd.cpassing in an offset
        modifier with a value of 8. If the suffix is used with a
sequential I/O

> +++ b/zbd.c
> @@ -375,12 +375,24 @@ static bool zbd_verify_bs(void)
>         int i, j, k;
>  
>         for_each_td(td, i) {
> +               if (td_trim(td) &&
> +                   (td->o.min_bs[DDIR_TRIM] != td-
> >o.max_bs[DDIR_TRIM] ||
> +                    td->o.bssplit_nr[DDIR_TRIM])) {
> +                       log_info("bsrange and bssplit is not allowed
> for trim with zonemode=zbd\n");
> +                       return false;
> +               }
>                 for_each_file(td, f, j) {
>                         uint64_t zone_size;
>  
>                         if (!f->zbd_info)
>                                 continue;
>                         zone_size = f->zbd_info->zone_size;
> +                       if (td_trim(td) && td->o.bs[DDIR_TRIM] !=
> zone_size) {
> +                               log_info("%s: trim block size %llu is
> not the zone size %llu\n",
> +                                        f->file_name, td-
> >o.bs[DDIR_TRIM],
> +                                        (unsigned long
> long)zone_size);
> +                               return false;
> +                       }
>                         for (k = 0; k < FIO_ARRAY_SIZE(td->o.bs);
> k++) {
>                                 if (td->o.verify != VERIFY_NONE &&
>                                     zone_size % td->o.bs[k] != 0) {
> @@ -1528,9 +1540,6 @@ static void zbd_queue_io(struct thread_data
> *td, struct io_u *io_u, int q,
>                 pthread_mutex_unlock(&zbd_info->mutex);
>                 z->wp = zone_end;
>                 break;
> -       case DDIR_TRIM:
> -               assert(z->wp == z->start);
> -               break;
>         default:
>                 break;
>         }
> @@ -1910,8 +1919,23 @@ enum io_u_action zbd_adjust_block(struct
> thread_data *td, struct io_u *io_u)
>                         (zbd_zone_capacity_end(zb) - io_u->offset),
> min_bs);
>                 goto eof;
>         case DDIR_TRIM:
> -               /* fall-through */
> +               /* Check random trim targets a non-empty zone */
> +               if (!td_random(td) || zb->wp > zb->start)
> +                       goto accept;
> +
> +               /* Find out a non-empty zone to trim */
> +               zone_unlock(zb);
> +               zl = get_zone(f, f->max_zone);
> +               zb = zbd_find_zone(td, io_u, 1, zb, zl);
> +               if (zb) {
> +                       io_u->offset = zb->start;
> +                       dprint(FD_ZBD, "%s: found new zone(%lld) for
> trim\n",
> +                              f->file_name, io_u->offset);
> +                       goto accept;
> +               }
> +               goto eof;
>         case DDIR_SYNC:
> +               /* fall-through */
>         case DDIR_DATASYNC:
>         case DDIR_SYNC_FILE_RANGE:
>         case DDIR_WAIT:
> @@ -1952,3 +1976,42 @@ char *zbd_write_status(const struct
> thread_stat *ts)
>                 return NULL;
>         return res;
>  }
> +
> +/**
> + * zbd_do_io_u_trim - If reset zone is applicable, do reset zone
> instead of trim
> + *
> + * @td: FIO thread data.
> + * @io_u: FIO I/O unit.
> + *
> + * It is assumed that z->mutex is already locked.
> + * Return io_u_completed when reset zone succeeds. Return 0 when the
> target zone
> + * does not have write pointer. On error, return negative errno.
> + */
> +int zbd_do_io_u_trim(const struct thread_data *td, struct io_u
> *io_u)
> +{
> +       struct fio_file *f = io_u->file;
> +       struct fio_zone_info *z;
> +       uint32_t zone_idx;
> +       int ret;
> +
> +       zone_idx = zbd_zone_idx(f, io_u->offset);
> +       z = get_zone(f, zone_idx);
> +
> +       if (!z->has_wp)
> +               return 0;
> +
> +       if (io_u->offset != z->start) {
> +               log_err("Trim offset not at zone start (%lld)\n",
> io_u->offset);
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * Cast td to drop const modifier so that zbd_reset_zone()
> can change td
> +        * members.
> +        */

This comment is not really necessary.

> +       ret = zbd_reset_zone((struct thread_data *)td, f, z);
> +       if (ret < 0)
> +               return ret;
> +
> +       return io_u_completed;
> +}
> diff --git a/zbd.h b/zbd.h
> index 39dc45e3..0a73b41d 100644
> --- a/zbd.h
> +++ b/zbd.h
> @@ -17,6 +17,7 @@ struct fio_file;
>  enum io_u_action {
>         io_u_accept     = 0,
>         io_u_eof        = 1,
> +       io_u_completed  = 2,
>  };
>  
>  /**
> @@ -99,6 +100,7 @@ enum fio_ddir zbd_adjust_ddir(struct thread_data
> *td, struct io_u *io_u,
>                               enum fio_ddir ddir);
>  enum io_u_action zbd_adjust_block(struct thread_data *td, struct
> io_u *io_u);
>  char *zbd_write_status(const struct thread_stat *ts);
> +int zbd_do_io_u_trim(const struct thread_data *td, struct io_u
> *io_u);
>  
>  static inline void zbd_close_file(struct fio_file *f)
>  {

Re: [PATCH 3/4] engines/libzbc: Enable trim for libzbc I/O engine

[Dmitry Fomichev]

On Wed, 2021-07-28 at 19:47 +0900, Shin'ichiro Kawasaki wrote:
> The trim workload to zoned block devices is supported as zone reset,
> and
> this feature is available for I/O engines which support both zoned
> devices and trim workload. Libzbc I/O engine supports zoned but lacks
> trim workload support. To enable trim support with libzbc I/O engine,
> remove the check which inhibited trim from requests to libzbc I/O
> engine. Also set file open flags for trim same as write, and call
> zbd_do_io_u_trim() for trim I/O.
> 
> Of note is that libzbc I/O engine now can support trim to sequential
> write required zones, but still can not support os_trim() call and
> BLKDISCARD ioctl for the conventional zones. The trim I/Os to
> conventional zones are reported as an error.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  engines/libzbc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/engines/libzbc.c b/engines/libzbc.c
> index 7f2bc431..5b4c5e8e 100644
> --- a/engines/libzbc.c
> +++ b/engines/libzbc.c
> @@ -14,6 +14,7 @@
>  #include "fio.h"
>  #include "err.h"
>  #include "zbd_types.h"
> +#include "zbd.h"
>  
>  struct libzbc_data {
>         struct zbc_device       *zdev;
> @@ -63,7 +64,7 @@ static int libzbc_open_dev(struct thread_data *td,
> struct fio_file *f,
>                 return -EINVAL;
>         }
>  
> -       if (td_write(td)) {
> +       if (td_write(td) || td_trim(td)) {
>                 if (!read_only)
>                         flags |= O_RDWR;
>         } else if (td_read(td)) {
> @@ -71,10 +72,6 @@ static int libzbc_open_dev(struct thread_data *td,
> struct fio_file *f,
>                         flags |= O_RDWR;
>                 else
>                         flags |= O_RDONLY;
> -       } else if (td_trim(td)) {
> -               td_verror(td, EINVAL, "libzbc does not support
> trim");
> -               log_err("%s: libzbc does not support trim\n", f-
> >file_name);
> -               return -EINVAL;
>         }
>  
>         if (td->o.oatomic) {
> @@ -411,7 +408,14 @@ static enum fio_q_status libzbc_queue(struct
> thread_data *td, struct io_u *io_u)
>                 ret = zbc_flush(ld->zdev);
>                 if (ret)
>                         log_err("zbc_flush error %zd\n", ret);
> -       } else if (io_u->ddir != DDIR_TRIM) {
> +       } else if (io_u->ddir == DDIR_TRIM) {
> +               ret = zbd_do_io_u_trim(td, io_u);
> +               if (!ret) {
> +                       log_err("libzbc does not support trim to "
> +                               "conventional zones\n");
> +                       ret = EINVAL;

Wouldn't that be more appropriate to just call os_trim() here and avoid
this error? This way, any ZBD trim workload that succeeds without using
libzbc would also succeed with using this engine... not the case with
the code above.

> +               }
> +       } else {
>                 log_err("Unsupported operation %u\n", io_u->ddir);
>                 ret = -EINVAL;
>         }

Re: [PATCH 4/4] t/zbd: Add test #58 to test zone reset by trim workload

[Dmitry Fomichev]

On Wed, 2021-07-28 at 19:47 +0900, Shin'ichiro Kawasaki wrote:
> To exercise zone reset by trim workload, add the test case #58. As the
> precondition, it fills several zones. After that, a trim job and a
> write
> job run in parallel for 30 seconds. The ratio of trim commands and
> write
> commands is controlled by --flow option.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

Tested-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  t/zbd/test-zbd-support | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
> index 57e6d05e..5103c406 100755
> --- a/t/zbd/test-zbd-support
> +++ b/t/zbd/test-zbd-support
> @@ -1215,6 +1215,32 @@ test57() {
>                 >> "${logfile}.${test_number}" 2>&1 || return $?
>  }
>  
> +# Random writes and random trims to sequential write required zones
> for 30s.
> +test58() {
> +    local off size bs
> +
> +    require_seq_zones 128 || return $SKIP_TESTCASE
> +
> +    size=$((zone_size * 128))
> +    bs="$(max $((zone_size / 128)) "$logical_block_size")"
> +    prep_write
> +    off=$((first_sequential_zone_sector * 512))
> +    run_fio --zonemode=zbd --direct=1 --zonesize="${zone_size}" --
> thread=1 \
> +           --filename="${dev}" --norandommap=1 \
> +            --name="precondition"  --rw=write "$(ioengine "psync")" \
> +            --offset="${off}" --size=$((zone_size * 16)) --bs="${bs}"
> \
> +           "${job_var_opts[@]}" \
> +           --name=wjob --wait_for="precondition" --rw=randwrite \
> +           "$(ioengine "libaio")" --iodepth=8 \
> +           --offset="${off}" --size="${size}" --bs="${bs}" \
> +           --time_based --runtime=30s --flow=128 "${job_var_opts[@]}"
> \
> +           --name=trimjob --wait_for="precondition" --rw=randtrim \
> +           "$(ioengine "psync")" \
> +           --offset="${off}" --size="${size}" --bs="${zone_size}" \
> +           --time_based --runtime=30s --flow=1 "${job_var_opts[@]}" \
> +           >>"${logfile}.${test_number}" 2>&1
> +}
> +
>  SECONDS=0
>  tests=()
>  dynamic_analyzer=()

Re: [PATCH 2/4] zbd: Support zone reset by trim

[Shinichiro Kawasaki]

Dmitry, thank you for the review comments.

On Aug 03, 2021 / 19:36, Dmitry Fomichev wrote:
> On Wed, 2021-07-28 at 19:47 +0900, Shin'ichiro Kawasaki wrote:
> > Enable trim workload for zonemode=zbd by modifying do_io_u_trim() to
> > call zoned block device unique function zbd_do_io_u_trim() which resets
> > target zone. This allows fio to emulate workloads which mix data
> > read/write and zone resets with zonemode=zbd.
> > 
> > To call reset zones, the trim I/O shall have offset aligned to zone
> > start and block size same as zone size. Reset zone is called only to
> > sequential write required zones and sequential write preferred zones.
> > Conventional zones are handled in same manner as regular block devices
> > by calling os_trim() function.
> > 
> > When zones are reset with random trim workload, choose only non-empty
> > zones as trim target. This avoids meaningless trim to empty zones and
> > makes the workload more realistic. To find the non-empty zones, utilize
> > zbd_find_zone() helper function which is already used for read
> > workload,
> > specifying 1 byte as the minimum valid data size.
> > 
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >  HOWTO  |  3 +++
> >  fio.1  |  2 ++
> >  io_u.c |  9 ++++++++
> >  zbd.c  | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> >  zbd.h  |  2 ++
> >  5 files changed, 83 insertions(+), 4 deletions(-)
> > 
> > diff --git a/HOWTO b/HOWTO
> > index 59c7f1ff..f1fd2045 100644
> > --- a/HOWTO
> > +++ b/HOWTO
> > @@ -992,6 +992,9 @@ Target file/device
> >                                 single zone. The :option:`zoneskip`
> > parameter
> >                                 is ignored. :option:`zonerange` and
> >                                 :option:`zonesize` must be identical.
> > +                               Trim is handled using a zone reset
> > operation.
> > +                               Trim only considers non-empty
> > sequential write
> > +                               required and sequential write preferred
> > zones.
> 
> The documentation changes could be moved to a separate patch. While
> looking at HOWTO file, I notice that libzbc is absent from the list of
> i/o engines there. Perhaps, since you are making changes to this file,
> you could add a small description of libzbc there and mention that it
> supports read, write and trim workloads when run against zoned block
> devices.

Okay, I will separate out changes in HOWTO and fio.1 into a patch. Will
add the description of libzbc i/o engine also.

> 
> >  
> >  .. option:: zonerange=int
> >  
> > diff --git a/fio.1 b/fio.1
> > index 6cc82542..ef319062 100644
> > --- a/fio.1
> > +++ b/fio.1
> > @@ -766,6 +766,8 @@ starts. The \fBzonecapacity\fR parameter is
> > ignored.
> >  Zoned block device mode. I/O happens sequentially in each zone, even
> > if random32ad4373
> >  I/O has been selected. Random I/O happens across all zones instead of
> > being
> >  restricted to a single zone.
> > +Trim is handled using a zone reset operation. Trim only considers non-
> > empty
> > +sequential write required and sequential write preferred zones.
> >  .RE
> >  .RE
> >  .TP
> > diff --git a/io_u.c b/io_u.c
> > index 9a1cd547..696d25cd 100644
> > --- a/io_u.c
> > +++ b/io_u.c
> > @@ -2317,10 +2317,19 @@ int do_io_u_trim(const struct thread_data *td,
> > struct io_u *io_u)
> >         struct fio_file *f = io_u->file;
> >         int ret;
> >  
> > +       if (td->o.zone_mode == ZONE_MODE_ZBD) {
> > +               ret = zbd_do_io_u_trim(td, io_u);
> > +               if (ret == io_u_completed)
> > +                       return io_u->xfer_buflen;
> > +               if (ret)
> > +                       goto err;
> > +       }
> > +
> >         ret = os_trim(f, io_u->offset, io_u->xfer_buflen);
> >         if (!ret)
> >                 return io_u->xfer_buflen;
> >  
> > +err:
> >         io_u->error = ret;
> >         return 0;
> >  #endif
> > diff --git a/zbd.c b/zbd.c
> > index f10b3267..39060ecd 100644
> > --- a/zbd.cpassing in an offset
>         modifier with a value of 8. If the suffix is used with a
> sequential I/O

I did not catch this part. May I ask to rephrase?

> 
> > +++ b/zbd.c
> > @@ -375,12 +375,24 @@ static bool zbd_verify_bs(void)
> >         int i, j, k;
> >  
> >         for_each_td(td, i) {
> > +               if (td_trim(td) &&
> > +                   (td->o.min_bs[DDIR_TRIM] != td-
> > >o.max_bs[DDIR_TRIM] ||
> > +                    td->o.bssplit_nr[DDIR_TRIM])) {
> > +                       log_info("bsrange and bssplit is not allowed
> > for trim with zonemode=zbd\n");
> > +                       return false;
> > +               }
> >                 for_each_file(td, f, j) {
> >                         uint64_t zone_size;
> >  
> >                         if (!f->zbd_info)
> >                                 continue;
> >                         zone_size = f->zbd_info->zone_size;
> > +                       if (td_trim(td) && td->o.bs[DDIR_TRIM] !=
> > zone_size) {
> > +                               log_info("%s: trim block size %llu is
> > not the zone size %llu\n",
> > +                                        f->file_name, td-
> > >o.bs[DDIR_TRIM],
> > +                                        (unsigned long
> > long)zone_size);
> > +                               return false;
> > +                       }
> >                         for (k = 0; k < FIO_ARRAY_SIZE(td->o.bs);
> > k++) {
> >                                 if (td->o.verify != VERIFY_NONE &&
> >                                     zone_size % td->o.bs[k] != 0) {
> > @@ -1528,9 +1540,6 @@ static void zbd_queue_io(struct thread_data
> > *td, struct io_u *io_u, int q,
> >                 pthread_mutex_unlock(&zbd_info->mutex);
> >                 z->wp = zone_end;
> >                 break;
> > -       case DDIR_TRIM:
> > -               assert(z->wp == z->start);
> > -               break;
> >         default:
> >                 break;
> >         }
> > @@ -1910,8 +1919,23 @@ enum io_u_action zbd_adjust_block(struct
> > thread_data *td, struct io_u *io_u)
> >                         (zbd_zone_capacity_end(zb) - io_u->offset),
> > min_bs);
> >                 goto eof;
> >         case DDIR_TRIM:
> > -               /* fall-through */
> > +               /* Check random trim targets a non-empty zone */
> > +               if (!td_random(td) || zb->wp > zb->start)
> > +                       goto accept;
> > +
> > +               /* Find out a non-empty zone to trim */
> > +               zone_unlock(zb);
> > +               zl = get_zone(f, f->max_zone);
> > +               zb = zbd_find_zone(td, io_u, 1, zb, zl);
> > +               if (zb) {
> > +                       io_u->offset = zb->start;
> > +                       dprint(FD_ZBD, "%s: found new zone(%lld) for
> > trim\n",
> > +                              f->file_name, io_u->offset);
> > +                       goto accept;
> > +               }
> > +               goto eof;
> >         case DDIR_SYNC:
> > +               /* fall-through */
> >         case DDIR_DATASYNC:
> >         case DDIR_SYNC_FILE_RANGE:
> >         case DDIR_WAIT:
> > @@ -1952,3 +1976,42 @@ char *zbd_write_status(const struct
> > thread_stat *ts)
> >                 return NULL;
> >         return res;
> >  }
> > +
> > +/**
> > + * zbd_do_io_u_trim - If reset zone is applicable, do reset zone
> > instead of trim
> > + *
> > + * @td: FIO thread data.
> > + * @io_u: FIO I/O unit.
> > + *
> > + * It is assumed that z->mutex is already locked.
> > + * Return io_u_completed when reset zone succeeds. Return 0 when the
> > target zone
> > + * does not have write pointer. On error, return negative errno.
> > + */
> > +int zbd_do_io_u_trim(const struct thread_data *td, struct io_u
> > *io_u)
> > +{
> > +       struct fio_file *f = io_u->file;
> > +       struct fio_zone_info *z;
> > +       uint32_t zone_idx;
> > +       int ret;
> > +
> > +       zone_idx = zbd_zone_idx(f, io_u->offset);
> > +       z = get_zone(f, zone_idx);
> > +
> > +       if (!z->has_wp)
> > +               return 0;
> > +
> > +       if (io_u->offset != z->start) {
> > +               log_err("Trim offset not at zone start (%lld)\n",
> > io_u->offset);
> > +               return -EINVAL;
> > +       }
> > +
> > +       /*
> > +        * Cast td to drop const modifier so that zbd_reset_zone()
> > can change td
> > +        * members.
> > +        */
> 
> This comment is not really necessary.

Will remove it. Thanks.

-- 
Best Regards,
Shin'ichiro Kawasaki

> 
> > +       ret = zbd_reset_zone((struct thread_data *)td, f, z);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       return io_u_completed;
> > +}
> > diff --git a/zbd.h b/zbd.h
> > index 39dc45e3..0a73b41d 100644
> > --- a/zbd.h
> > +++ b/zbd.h
> > @@ -17,6 +17,7 @@ struct fio_file;
> >  enum io_u_action {
> >         io_u_accept     = 0,
> >         io_u_eof        = 1,
> > +       io_u_completed  = 2,
> >  };
> >  
> >  /**
> > @@ -99,6 +100,7 @@ enum fio_ddir zbd_adjust_ddir(struct thread_data
> > *td, struct io_u *io_u,
> >                               enum fio_ddir ddir);
> >  enum io_u_action zbd_adjust_block(struct thread_data *td, struct
> > io_u *io_u);
> >  char *zbd_write_status(const struct thread_stat *ts);
> > +int zbd_do_io_u_trim(const struct thread_data *td, struct io_u
> > *io_u);
> >  
> >  static inline void zbd_close_file(struct fio_file *f)
> >  {
> 

Re: [PATCH 3/4] engines/libzbc: Enable trim for libzbc I/O engine

[Shinichiro Kawasaki]

On Aug 03, 2021 / 19:36, Dmitry Fomichev wrote:
> On Wed, 2021-07-28 at 19:47 +0900, Shin'ichiro Kawasaki wrote:
> > The trim workload to zoned block devices is supported as zone reset,
> > and
> > this feature is available for I/O engines which support both zoned
> > devices and trim workload. Libzbc I/O engine supports zoned but lacks
> > trim workload support. To enable trim support with libzbc I/O engine,
> > remove the check which inhibited trim from requests to libzbc I/O
> > engine. Also set file open flags for trim same as write, and call
> > zbd_do_io_u_trim() for trim I/O.
> > 
> > Of note is that libzbc I/O engine now can support trim to sequential
> > write required zones, but still can not support os_trim() call and
> > BLKDISCARD ioctl for the conventional zones. The trim I/Os to
> > conventional zones are reported as an error.
> > 
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >  engines/libzbc.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/engines/libzbc.c b/engines/libzbc.c
> > index 7f2bc431..5b4c5e8e 100644
> > --- a/engines/libzbc.c
> > +++ b/engines/libzbc.c
> > @@ -14,6 +14,7 @@
> >  #include "fio.h"
> >  #include "err.h"
> >  #include "zbd_types.h"
> > +#include "zbd.h"
> >  
> >  struct libzbc_data {
> >         struct zbc_device       *zdev;
> > @@ -63,7 +64,7 @@ static int libzbc_open_dev(struct thread_data *td,
> > struct fio_file *f,
> >                 return -EINVAL;
> >         }
> >  
> > -       if (td_write(td)) {
> > +       if (td_write(td) || td_trim(td)) {
> >                 if (!read_only)
> >                         flags |= O_RDWR;
> >         } else if (td_read(td)) {
> > @@ -71,10 +72,6 @@ static int libzbc_open_dev(struct thread_data *td,
> > struct fio_file *f,
> >                         flags |= O_RDWR;
> >                 else
> >                         flags |= O_RDONLY;
> > -       } else if (td_trim(td)) {
> > -               td_verror(td, EINVAL, "libzbc does not support
> > trim");
> > -               log_err("%s: libzbc does not support trim\n", f-
> > >file_name);
> > -               return -EINVAL;
> >         }
> >  
> >         if (td->o.oatomic) {
> > @@ -411,7 +408,14 @@ static enum fio_q_status libzbc_queue(struct
> > thread_data *td, struct io_u *io_u)
> >                 ret = zbc_flush(ld->zdev);
> >                 if (ret)
> >                         log_err("zbc_flush error %zd\n", ret);
> > -       } else if (io_u->ddir != DDIR_TRIM) {
> > +       } else if (io_u->ddir == DDIR_TRIM) {
> > +               ret = zbd_do_io_u_trim(td, io_u);
> > +               if (!ret) {
> > +                       log_err("libzbc does not support trim to "
> > +                               "conventional zones\n");
> > +                       ret = EINVAL;
> 
> Wouldn't that be more appropriate to just call os_trim() here and avoid
> this error? This way, any ZBD trim workload that succeeds without using
> libzbc would also succeed with using this engine... not the case with
> the code above.

os_trim() for Linux calls BLKDISCARD ioctl, which does not work for SG node.
Then, os_trim() call is not appropriate here.

One point to improve is the log_err() message I added. "Trim" and
"Conventional zone" does not have relation, then the message is confusing.
The error message is too much, probably. I will remove it and just return
EINVAL.

> 
> > +               }
> > +       } else {
> >                 log_err("Unsupported operation %u\n", io_u->ddir);
> >                 ret = -EINVAL;
> >         }
> 

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [PATCH 2/4] zbd: Support zone reset by trim

[Dmitry Fomichev]

On Wed, 2021-08-04 at 06:32 +0000, Shinichiro Kawasaki wrote:
> Dmitry, thank you for the review comments.
> 
> On Aug 03, 2021 / 19:36, Dmitry Fomichev wrote:
> > On Wed, 2021-07-28 at 19:47 +0900, Shin'ichiro Kawasaki wrote:
> > > Enable trim workload for zonemode=zbd by modifying do_io_u_trim()
> > > to
> > > call zoned block device unique function zbd_do_io_u_trim() which
> > > resets
> > > target zone. This allows fio to emulate workloads which mix data
> > > read/write and zone resets with zonemode=zbd.
> > > 
> > > To call reset zones, the trim I/O shall have offset aligned to
> > > zone
> > > start and block size same as zone size. Reset zone is called only
> > > to
> > > sequential write required zones and sequential write preferred
> > > zones.
> > > Conventional zones are handled in same manner as regular block
> > > devices
> > > by calling os_trim() function.
> > > 
> > > When zones are reset with random trim workload, choose only non-
> > > empty
> > > zones as trim target. This avoids meaningless trim to empty zones
> > > and
> > > makes the workload more realistic. To find the non-empty zones,
> > > utilize
> > > zbd_find_zone() helper function which is already used for read
> > > workload,
> > > specifying 1 byte as the minimum valid data size.
> > > 
> > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > ---
> > >  HOWTO  |  3 +++
> > >  fio.1  |  2 ++
> > >  io_u.c |  9 ++++++++
> > >  zbd.c  | 71
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> > >  zbd.h  |  2 ++
> > >  5 files changed, 83 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/HOWTO b/HOWTO
> > > index 59c7f1ff..f1fd2045 100644
> > > --- a/HOWTO
> > > +++ b/HOWTO
> > > @@ -992,6 +992,9 @@ Target file/device
> > >                                 single zone. The
> > > :option:`zoneskip`
> > > parameter
> > >                                 is ignored. :option:`zonerange`
> > > and
> > >                                 :option:`zonesize` must be
> > > identical.
> > > +                               Trim is handled using a zone
> > > reset
> > > operation.
> > > +                               Trim only considers non-empty
> > > sequential write
> > > +                               required and sequential write
> > > preferred
> > > zones.
> > 
> > The documentation changes could be moved to a separate patch. While
> > looking at HOWTO file, I notice that libzbc is absent from the list
> > of
> > i/o engines there. Perhaps, since you are making changes to this
> > file,
> > you could add a small description of libzbc there and mention that
> > it
> > supports read, write and trim workloads when run against zoned
> > block
> > devices.
> 
> Okay, I will separate out changes in HOWTO and fio.1 into a patch.
> Will
> add the description of libzbc i/o engine also.
> 
> > 
> > >  
> > >  .. option:: zonerange=int
> > >  
> > > diff --git a/fio.1 b/fio.1
> > > index 6cc82542..ef319062 100644
> > > --- a/fio.1
> > > +++ b/fio.1
> > > @@ -766,6 +766,8 @@ starts. The \fBzonecapacity\fR parameter is
> > > ignored.
> > >  Zoned block device mode. I/O happens sequentially in each zone,
> > > even
> > > if random32ad4373
> > >  I/O has been selected. Random I/O happens across all zones
> > > instead of
> > > being
> > >  restricted to a single zone.
> > > +Trim is handled using a zone reset operation. Trim only
> > > considers non-
> > > empty
> > > +sequential write required and sequential write preferred zones.
> > >  .RE
> > >  .RE
> > >  .TP
> > > diff --git a/io_u.c b/io_u.c
> > > index 9a1cd547..696d25cd 100644
> > > --- a/io_u.c
> > > +++ b/io_u.c
> > > @@ -2317,10 +2317,19 @@ int do_io_u_trim(const struct thread_data
> > > *td,
> > > struct io_u *io_u)
> > >         struct fio_file *f = io_u->file;
> > >         int ret;
> > >  
> > > +       if (td->o.zone_mode == ZONE_MODE_ZBD) {
> > > +               ret = zbd_do_io_u_trim(td, io_u);
> > > +               if (ret == io_u_completed)
> > > +                       return io_u->xfer_buflen;
> > > +               if (ret)
> > > +                       goto err;
> > > +       }
> > > +
> > >         ret = os_trim(f, io_u->offset, io_u->xfer_buflen);
> > >         if (!ret)
> > >                 return io_u->xfer_buflen;
> > >  
> > > +err:
> > >         io_u->error = ret;
> > >         return 0;
> > >  #endif
> > > diff --git a/zbd.c b/zbd.c
> > > index f10b3267..39060ecd 100644
> > > --- a/zbd.cpassing in an offset
> >         modifier with a value of 8. If the suffix is used with a
> > sequential I/O
> 
> I did not catch this part. May I ask to rephrase?

Please disregard this, I think my email client (Evolution) had a little
glitch while formatting the reply...
> 

Re: [PATCH 3/4] engines/libzbc: Enable trim for libzbc I/O engine

[Dmitry Fomichev]

On Wed, 2021-08-04 at 06:39 +0000, Shinichiro Kawasaki wrote:
> On Aug 03, 2021 / 19:36, Dmitry Fomichev wrote:
> > On Wed, 2021-07-28 at 19:47 +0900, Shin'ichiro Kawasaki wrote:
> > > The trim workload to zoned block devices is supported as zone
> > > reset,
> > > and
> > > this feature is available for I/O engines which support both
> > > zoned
> > > devices and trim workload. Libzbc I/O engine supports zoned but
> > > lacks
> > > trim workload support. To enable trim support with libzbc I/O
> > > engine,
> > > remove the check which inhibited trim from requests to libzbc I/O
> > > engine. Also set file open flags for trim same as write, and call
> > > zbd_do_io_u_trim() for trim I/O.
> > > 
> > > Of note is that libzbc I/O engine now can support trim to
> > > sequential
> > > write required zones, but still can not support os_trim() call
> > > and
> > > BLKDISCARD ioctl for the conventional zones. The trim I/Os to
> > > conventional zones are reported as an error.
> > > 
> > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > ---
> > >  engines/libzbc.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/engines/libzbc.c b/engines/libzbc.c
> > > index 7f2bc431..5b4c5e8e 100644
> > > --- a/engines/libzbc.c
> > > +++ b/engines/libzbc.c
> > > @@ -14,6 +14,7 @@
> > >  #include "fio.h"
> > >  #include "err.h"
> > >  #include "zbd_types.h"
> > > +#include "zbd.h"
> > >  
> > >  struct libzbc_data {
> > >         struct zbc_device       *zdev;
> > > @@ -63,7 +64,7 @@ static int libzbc_open_dev(struct thread_data
> > > *td,
> > > struct fio_file *f,
> > >                 return -EINVAL;
> > >         }
> > >  
> > > -       if (td_write(td)) {
> > > +       if (td_write(td) || td_trim(td)) {
> > >                 if (!read_only)
> > >                         flags |= O_RDWR;
> > >         } else if (td_read(td)) {
> > > @@ -71,10 +72,6 @@ static int libzbc_open_dev(struct thread_data
> > > *td,
> > > struct fio_file *f,
> > >                         flags |= O_RDWR;
> > >                 else
> > >                         flags |= O_RDONLY;
> > > -       } else if (td_trim(td)) {
> > > -               td_verror(td, EINVAL, "libzbc does not support
> > > trim");
> > > -               log_err("%s: libzbc does not support trim\n", f-
> > > > file_name);
> > > -               return -EINVAL;
> > >         }
> > >  
> > >         if (td->o.oatomic) {
> > > @@ -411,7 +408,14 @@ static enum fio_q_status libzbc_queue(struct
> > > thread_data *td, struct io_u *io_u)
> > >                 ret = zbc_flush(ld->zdev);
> > >                 if (ret)
> > >                         log_err("zbc_flush error %zd\n", ret);
> > > -       } else if (io_u->ddir != DDIR_TRIM) {
> > > +       } else if (io_u->ddir == DDIR_TRIM) {
> > > +               ret = zbd_do_io_u_trim(td, io_u);
> > > +               if (!ret) {
> > > +                       log_err("libzbc does not support trim to
> > > "
> > > +                               "conventional zones\n");
> > > +                       ret = EINVAL;
> > 
> > Wouldn't that be more appropriate to just call os_trim() here and
> > avoid
> > this error? This way, any ZBD trim workload that succeeds without
> > using
> > libzbc would also succeed with using this engine... not the case
> > with
> > the code above.
> 
> os_trim() for Linux calls BLKDISCARD ioctl, which does not work for
> SG node.
> Then, os_trim() call is not appropriate here.
> 
> One point to improve is the log_err() message I added. "Trim" and
> "Conventional zone" does not have relation, then the message is
> confusing.
> The error message is too much, probably. I will remove it and just
> return
> EINVAL.

I see... since we don't have the block FD readily available, a
WRITE_SAME with UNMAP bit over SG_IO could do the trick. But doing this
would add a lot of complexity just to cover this relatively narrow use
case. Erroring out with no message should be fine here.

> 
> > 
> > > +               }
> > > +       } else {
> > >                 log_err("Unsupported operation %u\n", io_u-
> > > >ddir);
> > >                 ret = -EINVAL;
> > >         }
> > 
> 

Re: [PATCH 3/4] engines/libzbc: Enable trim for libzbc I/O engine

[Shinichiro Kawasaki]

On Aug 04, 2021 / 19:29, Dmitry Fomichev wrote:
> On Wed, 2021-08-04 at 06:39 +0000, Shinichiro Kawasaki wrote:
> > On Aug 03, 2021 / 19:36, Dmitry Fomichev wrote:
> > > On Wed, 2021-07-28 at 19:47 +0900, Shin'ichiro Kawasaki wrote:
> > > > The trim workload to zoned block devices is supported as zone
> > > > reset,
> > > > and
> > > > this feature is available for I/O engines which support both
> > > > zoned
> > > > devices and trim workload. Libzbc I/O engine supports zoned but
> > > > lacks
> > > > trim workload support. To enable trim support with libzbc I/O
> > > > engine,
> > > > remove the check which inhibited trim from requests to libzbc I/O
> > > > engine. Also set file open flags for trim same as write, and call
> > > > zbd_do_io_u_trim() for trim I/O.
> > > > 
> > > > Of note is that libzbc I/O engine now can support trim to
> > > > sequential
> > > > write required zones, but still can not support os_trim() call
> > > > and
> > > > BLKDISCARD ioctl for the conventional zones. The trim I/Os to
> > > > conventional zones are reported as an error.
> > > > 
> > > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > ---
> > > >  engines/libzbc.c | 16 ++++++++++------
> > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/engines/libzbc.c b/engines/libzbc.c
> > > > index 7f2bc431..5b4c5e8e 100644
> > > > --- a/engines/libzbc.c
> > > > +++ b/engines/libzbc.c
> > > > @@ -14,6 +14,7 @@
> > > >  #include "fio.h"
> > > >  #include "err.h"
> > > >  #include "zbd_types.h"
> > > > +#include "zbd.h"
> > > >  
> > > >  struct libzbc_data {
> > > >         struct zbc_device       *zdev;
> > > > @@ -63,7 +64,7 @@ static int libzbc_open_dev(struct thread_data
> > > > *td,
> > > > struct fio_file *f,
> > > >                 return -EINVAL;
> > > >         }
> > > >  
> > > > -       if (td_write(td)) {
> > > > +       if (td_write(td) || td_trim(td)) {
> > > >                 if (!read_only)
> > > >                         flags |= O_RDWR;
> > > >         } else if (td_read(td)) {
> > > > @@ -71,10 +72,6 @@ static int libzbc_open_dev(struct thread_data
> > > > *td,
> > > > struct fio_file *f,
> > > >                         flags |= O_RDWR;
> > > >                 else
> > > >                         flags |= O_RDONLY;
> > > > -       } else if (td_trim(td)) {
> > > > -               td_verror(td, EINVAL, "libzbc does not support
> > > > trim");
> > > > -               log_err("%s: libzbc does not support trim\n", f-
> > > > > file_name);
> > > > -               return -EINVAL;
> > > >         }
> > > >  
> > > >         if (td->o.oatomic) {
> > > > @@ -411,7 +408,14 @@ static enum fio_q_status libzbc_queue(struct
> > > > thread_data *td, struct io_u *io_u)
> > > >                 ret = zbc_flush(ld->zdev);
> > > >                 if (ret)
> > > >                         log_err("zbc_flush error %zd\n", ret);
> > > > -       } else if (io_u->ddir != DDIR_TRIM) {
> > > > +       } else if (io_u->ddir == DDIR_TRIM) {
> > > > +               ret = zbd_do_io_u_trim(td, io_u);
> > > > +               if (!ret) {
> > > > +                       log_err("libzbc does not support trim to
> > > > "
> > > > +                               "conventional zones\n");
> > > > +                       ret = EINVAL;
> > > 
> > > Wouldn't that be more appropriate to just call os_trim() here and
> > > avoid
> > > this error? This way, any ZBD trim workload that succeeds without
> > > using
> > > libzbc would also succeed with using this engine... not the case
> > > with
> > > the code above.
> > 
> > os_trim() for Linux calls BLKDISCARD ioctl, which does not work for
> > SG node.
> > Then, os_trim() call is not appropriate here.
> > 
> > One point to improve is the log_err() message I added. "Trim" and
> > "Conventional zone" does not have relation, then the message is
> > confusing.
> > The error message is too much, probably. I will remove it and just
> > return
> > EINVAL.
> 
> I see... since we don't have the block FD readily available, a
> WRITE_SAME with UNMAP bit over SG_IO could do the trick. But doing this
> would add a lot of complexity just to cover this relatively narrow use
> case. Erroring out with no message should be fine here.

That WRITE_SAME with UNMAP bit is an idea. It sounds deserved as the
future work item.

Anyway, thanks for your review on this series. I have sent out v2. Your
check on the v2 will be appreciated.

-- 
Best Regards,
Shin'ichiro Kawasaki

[PATCH 3/4] engines/libzbc: Enable trim for libzbc I/O engine

[Shin'ichiro Kawasaki]

The trim workload to zoned block devices is supported as zone reset, and
this feature is available for I/O engines which support both zoned
devices and trim workload. Libzbc I/O engine supports zoned but lacks
trim workload support. To enable trim support with libzbc I/O engine,
remove the check which inhibited trim from requests to libzbc I/O
engine. Also set file open flags for trim same as write, and call
zbd_do_io_u_trim() for trim I/O.

Of note is that libzbc I/O engine now can support trim to sequential
write required zones, but still can not support os_trim() call and
BLKDISCARD ioctl for the conventional zones. The trim I/Os to
conventional zones are reported as an error.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 engines/libzbc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/engines/libzbc.c b/engines/libzbc.c
index 7f2bc431..5b4c5e8e 100644
--- a/engines/libzbc.c
+++ b/engines/libzbc.c
@@ -14,6 +14,7 @@
 #include "fio.h"
 #include "err.h"
 #include "zbd_types.h"
+#include "zbd.h"
 
 struct libzbc_data {
 	struct zbc_device	*zdev;
@@ -63,7 +64,7 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
 		return -EINVAL;
 	}
 
-	if (td_write(td)) {
+	if (td_write(td) || td_trim(td)) {
 		if (!read_only)
 			flags |= O_RDWR;
 	} else if (td_read(td)) {
@@ -71,10 +72,6 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
 			flags |= O_RDWR;
 		else
 			flags |= O_RDONLY;
-	} else if (td_trim(td)) {
-		td_verror(td, EINVAL, "libzbc does not support trim");
-		log_err("%s: libzbc does not support trim\n", f->file_name);
-		return -EINVAL;
 	}
 
 	if (td->o.oatomic) {
@@ -411,7 +408,14 @@ static enum fio_q_status libzbc_queue(struct thread_data *td, struct io_u *io_u)
 		ret = zbc_flush(ld->zdev);
 		if (ret)
 			log_err("zbc_flush error %zd\n", ret);
-	} else if (io_u->ddir != DDIR_TRIM) {
+	} else if (io_u->ddir == DDIR_TRIM) {
+		ret = zbd_do_io_u_trim(td, io_u);
+		if (!ret) {
+			log_err("libzbc does not support trim to "
+				"conventional zones\n");
+			ret = EINVAL;
+		}
+	} else {
 		log_err("Unsupported operation %u\n", io_u->ddir);
 		ret = -EINVAL;
 	}
-- 
2.31.1