[PATCH 00/15] ZBD fixes and improvements

[PATCH 00/15] ZBD fixes and improvements

[Dmitry Fomichev]

This patch series contains bug fixes and refactoring changes
related to support for Zoned Block Devices (ZBD) in fio.
The highlights:

 - fix several errors related to running workloads that span
   a mix of conventional zones and write pointer zones.
 - improve counting of sectors with data (SWD).
 - remove dependencies on particular zone types in the code.
 - add code to gracefully handle offline zones.

Aravind Ramesh (1):
  zbd: initialize sectors with data at start time

Dmitry Fomichev (10):
  zbd: return ENOMEM if zone buffer allocation fails
  zbd: use zbd_zone_nr() more actively in the code
  zbd: add get_zone() helper function
  zbd: introduce zone_unlock()
  zbd: engines/libzbc: don't fail on assert for offline zones
  zbd: remove dependency on zone type during i/o
  zbd: skip offline zones in zbd_convert_to_open_zone()
  zbd: avoid zone buffer overrun
  zbd: don't unlock zone mutex after verify replay
  zbd: use zone_lock() in zbd_process_swd()

Shin'ichiro Kawasaki (4):
  zbd: do not lock conventional zones on I/O adjustment
  zbd: do not set zbd handlers for conventional zones
  zbd: count sectors with data for write pointer zones
  zbd: initialize min_zone and max_zone for all zone types

 engines/libzbc.c       |   3 +-
 oslib/linux-blkzoned.c |   3 +-
 zbd.c                  | 265 ++++++++++++++++++++++++-----------------
 zbd.h                  |   5 +
 4 files changed, 164 insertions(+), 112 deletions(-)

-- 
2.21.0

[PATCH 01/15] zbd: return ENOMEM if zone buffer allocation fails

[Dmitry Fomichev]

parse_zone_info() function tries to allocate a buffer of
ZBD_REPORT_MAX_ZONES zone descriptors and exits if this allocation
fails. The problem is that it returns 0 error code in this case and
the caller may interpret this as the success.

Just return ENOMEM if we can't allocate that buffer.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/zbd.c b/zbd.c
index 9327816a..ad1ad1a9 100644
--- a/zbd.c
+++ b/zbd.c
@@ -443,7 +443,7 @@ static int parse_zone_info(struct thread_data *td, struct fio_file *f)
 	struct fio_zone_info *p;
 	uint64_t zone_size, offset;
 	struct zoned_block_device_info *zbd_info = NULL;
-	int i, j, ret = 0;
+	int i, j, ret = -ENOMEM;
 
 	zones = calloc(ZBD_REPORT_MAX_ZONES, sizeof(struct zbd_zone));
 	if (!zones)
@@ -475,7 +475,6 @@ static int parse_zone_info(struct thread_data *td, struct fio_file *f)
 
 	zbd_info = scalloc(1, sizeof(*zbd_info) +
 			   (nr_zones + 1) * sizeof(zbd_info->zone_info[0]));
-	ret = -ENOMEM;
 	if (!zbd_info)
 		goto out;
 	mutex_init_pshared(&zbd_info->mutex);
-- 
2.21.0

[PATCH 02/15] zbd: use zbd_zone_nr() more actively in the code

[Dmitry Fomichev]

The function zbd_zone_nr() is always called with the first argument
being f->zbd_info. If "f" is made the first argument instead, calls
of this function become more compact end easier to read.

Besides this change, convert several places in the code where the same
zone number calculation is open coded to zbd_zone_nr() calls.
This is a refactoring patch, no change in functionality.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/zbd.c b/zbd.c
index ad1ad1a9..841427e7 100644
--- a/zbd.c
+++ b/zbd.c
@@ -694,10 +694,10 @@ int zbd_setup_files(struct thread_data *td)
 	return 0;
 }
 
-static unsigned int zbd_zone_nr(struct zoned_block_device_info *zbd_info,
-				struct fio_zone_info *zone)
+static inline unsigned int zbd_zone_nr(const struct fio_file *f,
+				       struct fio_zone_info *zone)
 {
-	return zone - zbd_info->zone_info;
+	return zone - f->zbd_info->zone_info;
 }
 
 /**
@@ -723,7 +723,7 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
 	assert(is_valid_offset(f, offset + length - 1));
 
 	dprint(FD_ZBD, "%s: resetting wp of zone %u.\n", f->file_name,
-		zbd_zone_nr(f->zbd_info, z));
+		zbd_zone_nr(f, z));
 	switch (f->zbd_info->model) {
 	case ZBD_HOST_AWARE:
 	case ZBD_HOST_MANAGED:
@@ -793,9 +793,9 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
 	assert(min_bs);
 
 	dprint(FD_ZBD, "%s: examining zones %u .. %u\n", f->file_name,
-		zbd_zone_nr(f->zbd_info, zb), zbd_zone_nr(f->zbd_info, ze));
+		zbd_zone_nr(f, zb), zbd_zone_nr(f, ze));
 	for (z = zb; z < ze; z++) {
-		uint32_t nz = z - f->zbd_info->zone_info;
+		uint32_t nz = zbd_zone_nr(f, z);
 
 		if (!zbd_zone_swr(z))
 			continue;
@@ -811,8 +811,7 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
 		}
 		if (reset_wp) {
 			dprint(FD_ZBD, "%s: resetting zone %u\n",
-			       f->file_name,
-			       zbd_zone_nr(f->zbd_info, z));
+			       f->file_name, zbd_zone_nr(f, z));
 			if (zbd_reset_zone(td, f, z) < 0)
 				res = 1;
 		}
@@ -1197,7 +1196,7 @@ static struct fio_zone_info *zbd_replay_write_order(struct thread_data *td,
 	const struct fio_file *f = io_u->file;
 	const uint32_t min_bs = td->o.min_bs[DDIR_WRITE];
 
-	if (!zbd_open_zone(td, f, z - f->zbd_info->zone_info)) {
+	if (!zbd_open_zone(td, f, zbd_zone_nr(f, z))) {
 		pthread_mutex_unlock(&z->mutex);
 		z = zbd_convert_to_open_zone(td, io_u);
 		assert(z);
@@ -1271,7 +1270,7 @@ static void zbd_end_zone_io(struct thread_data *td, const struct io_u *io_u,
 	if (io_u->ddir == DDIR_WRITE &&
 	    io_u->offset + io_u->buflen >= zbd_zone_capacity_end(z)) {
 		pthread_mutex_lock(&f->zbd_info->mutex);
-		zbd_close_zone(td, f, z - f->zbd_info->zone_info);
+		zbd_close_zone(td, f, zbd_zone_nr(f, z));
 		pthread_mutex_unlock(&f->zbd_info->mutex);
 	}
 }
@@ -1430,8 +1429,7 @@ void setup_zbd_zone_mode(struct thread_data *td, struct io_u *io_u)
 		       "%s: Jump from zone capacity limit to zone end:"
 		       " (%llu -> %llu) for zone %u (%llu)\n",
 		       f->file_name, (unsigned long long) f->last_pos[ddir],
-		       (unsigned long long) zbd_zone_end(z),
-		       zbd_zone_nr(f->zbd_info, z),
+		       (unsigned long long) zbd_zone_end(z), zone_idx,
 		       (unsigned long long) z->capacity);
 		td->io_skip_bytes += zbd_zone_end(z) - f->last_pos[ddir];
 		f->last_pos[ddir] = zbd_zone_end(z);
@@ -1612,7 +1610,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			zb = zbd_convert_to_open_zone(td, io_u);
 			if (!zb)
 				goto eof;
-			zone_idx_b = zb - f->zbd_info->zone_info;
+			zone_idx_b = zbd_zone_nr(f, zb);
 		}
 		/* Check whether the zone reset threshold has been exceeded */
 		if (td->o.zrf.u.f) {
-- 
2.21.0

[PATCH 03/15] zbd: add get_zone() helper function

[Dmitry Fomichev]

The following pattern is widely used in zbd.c -

zone = &f->zbd_info->zone_info[zone_idx] .

For the sake of code clarity, wrap this construct into an inline
helper. No change in functionality.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/zbd.c b/zbd.c
index 841427e7..be3b554d 100644
--- a/zbd.c
+++ b/zbd.c
@@ -204,6 +204,12 @@ static bool is_valid_offset(const struct fio_file *f, uint64_t offset)
 	return (uint64_t)(offset - f->file_offset) < f->io_size;
 }
 
+static inline struct fio_zone_info *get_zone(const struct fio_file *f,
+					     unsigned int zone_nr)
+{
+	return &f->zbd_info->zone_info[zone_nr];
+}
+
 /* Verify whether direct I/O is used for all host-managed zoned drives. */
 static bool zbd_using_direct_io(void)
 {
@@ -286,7 +292,7 @@ static bool zbd_verify_sizes(void)
 			}
 
 			zone_idx = zbd_zone_idx(f, f->file_offset);
-			z = &f->zbd_info->zone_info[zone_idx];
+			z = get_zone(f, zone_idx);
 			if ((f->file_offset != z->start) &&
 			    (td->o.td_ddir != TD_DDIR_READ)) {
 				new_offset = zbd_zone_end(z);
@@ -302,7 +308,7 @@ static bool zbd_verify_sizes(void)
 				f->file_offset = new_offset;
 			}
 			zone_idx = zbd_zone_idx(f, f->file_offset + f->io_size);
-			z = &f->zbd_info->zone_info[zone_idx];
+			z = get_zone(f, zone_idx);
 			new_end = z->start;
 			if ((td->o.td_ddir != TD_DDIR_READ) &&
 			    (f->file_offset + f->io_size != new_end)) {
@@ -769,7 +775,7 @@ static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
 		sizeof(f->zbd_info->open_zones[0]));
 	f->zbd_info->num_open_zones--;
 	td->num_open_zones--;
-	f->zbd_info->zone_info[zone_idx].open = 0;
+	get_zone(f, zone_idx)->open = 0;
 }
 
 /*
@@ -869,8 +875,8 @@ static uint64_t zbd_process_swd(const struct fio_file *f, enum swd_action a)
 	struct fio_zone_info *zb, *ze, *z;
 	uint64_t swd = 0;
 
-	zb = &f->zbd_info->zone_info[f->min_zone];
-	ze = &f->zbd_info->zone_info[f->max_zone];
+	zb = get_zone(f, f->min_zone);
+	ze = get_zone(f, f->max_zone);
 	for (z = zb; z < ze; z++) {
 		pthread_mutex_lock(&z->mutex);
 		swd += z->wp - z->start;
@@ -925,8 +931,8 @@ void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 	if (!f->zbd_info || !td_write(td))
 		return;
 
-	zb = &f->zbd_info->zone_info[f->min_zone];
-	ze = &f->zbd_info->zone_info[f->max_zone];
+	zb = get_zone(f, f->min_zone);
+	ze = get_zone(f, f->max_zone);
 	zbd_init_swd(f);
 	/*
 	 * If data verification is enabled reset the affected zones before
@@ -966,7 +972,7 @@ static bool zbd_open_zone(struct thread_data *td, const struct fio_file *f,
 			  uint32_t zone_idx)
 {
 	const uint32_t min_bs = td->o.min_bs[DDIR_WRITE];
-	struct fio_zone_info *z = &f->zbd_info->zone_info[zone_idx];
+	struct fio_zone_info *z = get_zone(f, zone_idx);
 	bool res = true;
 
 	if (z->cond == ZBD_ZONE_COND_OFFLINE)
@@ -1059,7 +1065,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
 	for (;;) {
 		uint32_t tmp_idx;
 
-		z = &f->zbd_info->zone_info[zone_idx];
+		z = get_zone(f, zone_idx);
 
 		zone_lock(td, f, z);
 		pthread_mutex_lock(&f->zbd_info->mutex);
@@ -1147,7 +1153,7 @@ open_other_zone:
 		if (!is_valid_offset(f, z->start)) {
 			/* Wrap-around. */
 			zone_idx = f->min_zone;
-			z = &f->zbd_info->zone_info[zone_idx];
+			z = get_zone(f, zone_idx);
 		}
 		assert(is_valid_offset(f, z->start));
 		zone_lock(td, f, z);
@@ -1168,7 +1174,7 @@ open_other_zone:
 		pthread_mutex_unlock(&f->zbd_info->mutex);
 		pthread_mutex_unlock(&z->mutex);
 
-		z = &f->zbd_info->zone_info[zone_idx];
+		z = get_zone(f, zone_idx);
 
 		zone_lock(td, f, z);
 		if (z->wp + min_bs <= zbd_zone_capacity_end(z))
@@ -1224,8 +1230,7 @@ zbd_find_zone(struct thread_data *td, struct io_u *io_u,
 	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 =
-		&f->zbd_info->zone_info[f->min_zone];
+	const struct fio_zone_info *const zf = get_zone(f, f->min_zone);
 
 	/*
 	 * Skip to the next non-empty zone in case of sequential I/O and to
@@ -1298,7 +1303,7 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 
 	zone_idx = zbd_zone_idx(f, io_u->offset);
 	assert(zone_idx < zbd_info->nr_zones);
-	z = &zbd_info->zone_info[zone_idx];
+	z = get_zone(f, zone_idx);
 
 	if (!zbd_zone_swr(z))
 		return;
@@ -1359,7 +1364,7 @@ static void zbd_put_io(struct thread_data *td, const struct io_u *io_u)
 
 	zone_idx = zbd_zone_idx(f, io_u->offset);
 	assert(zone_idx < zbd_info->nr_zones);
-	z = &zbd_info->zone_info[zone_idx];
+	z = get_zone(f, zone_idx);
 
 	if (!zbd_zone_swr(z))
 		return;
@@ -1415,7 +1420,7 @@ void setup_zbd_zone_mode(struct thread_data *td, struct io_u *io_u)
 	assert(td->o.zone_size);
 
 	zone_idx = zbd_zone_idx(f, f->last_pos[ddir]);
-	z = &f->zbd_info->zone_info[zone_idx];
+	z = get_zone(f, zone_idx);
 
 	/*
 	 * When the zone capacity is smaller than the zone size and the I/O is
@@ -1523,7 +1528,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	assert(is_valid_offset(f, io_u->offset));
 	assert(io_u->buflen);
 	zone_idx_b = zbd_zone_idx(f, io_u->offset);
-	zb = &f->zbd_info->zone_info[zone_idx_b];
+	zb = get_zone(f, zone_idx_b);
 	orig_zb = zb;
 
 	/* Accept the I/O offset for conventional zones. */
@@ -1559,7 +1564,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 		if (range < min_bs ||
 		    ((!td_random(td)) && (io_u->offset + min_bs > zb->wp))) {
 			pthread_mutex_unlock(&zb->mutex);
-			zl = &f->zbd_info->zone_info[f->max_zone];
+			zl = get_zone(f, f->max_zone);
 			zb = zbd_find_zone(td, io_u, zb, zl);
 			if (!zb) {
 				dprint(FD_ZBD,
-- 
2.21.0

[PATCH 04/15] zbd: introduce zone_unlock()

[Dmitry Fomichev]

ZBD code already defines a helper function to lock a device zone,
zone_lock(). There is no zone_unlock() function though.

Wrap zone mutex unlock to zone_unlock() helper along with an assert
to make sure that the unlock is successful, i.e. that the function
is being called with the pointer to a locked zone.

Suggested-by: Niklas Cassel <niklas.cassel@wdc.com>
Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/zbd.c b/zbd.c
index be3b554d..2aa83e4a 100644
--- a/zbd.c
+++ b/zbd.c
@@ -199,6 +199,14 @@ static void zone_lock(struct thread_data *td, struct fio_file *f, struct fio_zon
 	}
 }
 
+static inline void zone_unlock(struct fio_zone_info *z)
+{
+	int ret;
+
+	ret = pthread_mutex_unlock(&z->mutex);
+	assert(!ret);
+}
+
 static bool is_valid_offset(const struct fio_file *f, uint64_t offset)
 {
 	return (uint64_t)(offset - f->file_offset) < f->io_size;
@@ -821,7 +829,7 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
 			if (zbd_reset_zone(td, f, z) < 0)
 				res = 1;
 		}
-		pthread_mutex_unlock(&z->mutex);
+		zone_unlock(z);
 	}
 
 	return res;
@@ -892,7 +900,7 @@ static uint64_t zbd_process_swd(const struct fio_file *f, enum swd_action a)
 	}
 	pthread_mutex_unlock(&f->zbd_info->mutex);
 	for (z = zb; z < ze; z++)
-		pthread_mutex_unlock(&z->mutex);
+		zone_unlock(z);
 
 	return swd;
 }
@@ -1102,7 +1110,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
 		dprint(FD_ZBD, "%s(%s): no candidate zone\n",
 			__func__, f->file_name);
 		pthread_mutex_unlock(&f->zbd_info->mutex);
-		pthread_mutex_unlock(&z->mutex);
+		zone_unlock(z);
 		return NULL;
 
 found_candidate_zone:
@@ -1111,7 +1119,7 @@ found_candidate_zone:
 			break;
 		zone_idx = new_zone_idx;
 		pthread_mutex_unlock(&f->zbd_info->mutex);
-		pthread_mutex_unlock(&z->mutex);
+		zone_unlock(z);
 	}
 
 	/* Both z->mutex and f->zbd_info->mutex are held. */
@@ -1148,7 +1156,7 @@ open_other_zone:
 	/* Zone 'z' is full, so try to open a new zone. */
 	for (i = f->io_size / f->zbd_info->zone_size; i > 0; i--) {
 		zone_idx++;
-		pthread_mutex_unlock(&z->mutex);
+		zone_unlock(z);
 		z++;
 		if (!is_valid_offset(f, z->start)) {
 			/* Wrap-around. */
@@ -1172,7 +1180,7 @@ open_other_zone:
 		if (zone_idx < f->min_zone || zone_idx >= f->max_zone)
 			continue;
 		pthread_mutex_unlock(&f->zbd_info->mutex);
-		pthread_mutex_unlock(&z->mutex);
+		zone_unlock(z);
 
 		z = get_zone(f, zone_idx);
 
@@ -1182,7 +1190,7 @@ open_other_zone:
 		pthread_mutex_lock(&f->zbd_info->mutex);
 	}
 	pthread_mutex_unlock(&f->zbd_info->mutex);
-	pthread_mutex_unlock(&z->mutex);
+	zone_unlock(z);
 	dprint(FD_ZBD, "%s(%s): did not open another zone\n", __func__,
 	       f->file_name);
 	return NULL;
@@ -1203,7 +1211,7 @@ static struct fio_zone_info *zbd_replay_write_order(struct thread_data *td,
 	const uint32_t min_bs = td->o.min_bs[DDIR_WRITE];
 
 	if (!zbd_open_zone(td, f, zbd_zone_nr(f, z))) {
-		pthread_mutex_unlock(&z->mutex);
+		zone_unlock(z);
 		z = zbd_convert_to_open_zone(td, io_u);
 		assert(z);
 	}
@@ -1241,7 +1249,7 @@ zbd_find_zone(struct thread_data *td, struct io_u *io_u,
 			zone_lock(td, f, z1);
 			if (z1->start + min_bs <= z1->wp)
 				return z1;
-			pthread_mutex_unlock(&z1->mutex);
+			zone_unlock(z1);
 		} else if (!td_random(td)) {
 			break;
 		}
@@ -1250,7 +1258,7 @@ zbd_find_zone(struct thread_data *td, struct io_u *io_u,
 			zone_lock(td, f, z2);
 			if (z2->start + min_bs <= z2->wp)
 				return z2;
-			pthread_mutex_unlock(&z2->mutex);
+			zone_unlock(z2);
 		}
 	}
 	dprint(FD_ZBD, "%s: adjusting random read offset failed\n",
@@ -1342,7 +1350,7 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 unlock:
 	if (!success || q != FIO_Q_QUEUED) {
 		/* BUSY or COMPLETED: unlock the zone */
-		pthread_mutex_unlock(&z->mutex);
+		zone_unlock(z);
 		io_u->zbd_put_io = NULL;
 	}
 }
@@ -1357,7 +1365,6 @@ static void zbd_put_io(struct thread_data *td, const struct io_u *io_u)
 	struct zoned_block_device_info *zbd_info = f->zbd_info;
 	struct fio_zone_info *z;
 	uint32_t zone_idx;
-	int ret;
 
 	if (!zbd_info)
 		return;
@@ -1375,8 +1382,7 @@ static void zbd_put_io(struct thread_data *td, const struct io_u *io_u)
 
 	zbd_end_zone_io(td, io_u, z);
 
-	ret = pthread_mutex_unlock(&z->mutex);
-	assert(ret == 0);
+	zone_unlock(z);
 	zbd_check_swd(f);
 }
 
@@ -1551,7 +1557,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	case DDIR_READ:
 		if (td->runstate == TD_VERIFYING && td_write(td)) {
 			zb = zbd_replay_write_order(td, io_u, zb);
-			pthread_mutex_unlock(&zb->mutex);
+			zone_unlock(zb);
 			goto accept;
 		}
 		/*
@@ -1563,7 +1569,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			zb->wp - zb->start : 0;
 		if (range < min_bs ||
 		    ((!td_random(td)) && (io_u->offset + min_bs > zb->wp))) {
-			pthread_mutex_unlock(&zb->mutex);
+			zone_unlock(zb);
 			zl = get_zone(f, f->max_zone);
 			zb = zbd_find_zone(td, io_u, zb, zl);
 			if (!zb) {
@@ -1611,7 +1617,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 		if (io_u->buflen > f->zbd_info->zone_size)
 			goto eof;
 		if (!zbd_open_zone(td, f, zone_idx_b)) {
-			pthread_mutex_unlock(&zb->mutex);
+			zone_unlock(zb);
 			zb = zbd_convert_to_open_zone(td, io_u);
 			if (!zb)
 				goto eof;
@@ -1699,7 +1705,7 @@ accept:
 
 eof:
 	if (zb)
-		pthread_mutex_unlock(&zb->mutex);
+		zone_unlock(zb);
 	return io_u_eof;
 }
 
-- 
2.21.0

[PATCH 05/15] zbd: engines/libzbc: don't fail on assert for offline zones

[Dmitry Fomichev]

If fio is run against a zoned device that has any zones in OFFLINE
condition, the following assertion is raised -

fio: zbd.c:473: parse_zone_info: Assertion `z->wp <= z->start + zone_size' failed.

This happens because offline zones have no valid write pointer and
it is reported by libzbc and blkzoned as (uint64_t)(-1). To avoid
violating this assertion, set the write pointer in all offline zones
to point at the start of the zone.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 engines/libzbc.c       | 3 ++-
 oslib/linux-blkzoned.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/engines/libzbc.c b/engines/libzbc.c
index 4b900233..1b362ca0 100644
--- a/engines/libzbc.c
+++ b/engines/libzbc.c
@@ -283,7 +283,8 @@ static int libzbc_report_zones(struct thread_data *td, struct fio_file *f,
 		default:
 			/* Treat all these conditions as offline (don't use!) */
 			zbdz->cond = ZBD_ZONE_COND_OFFLINE;
-			break;
+			zbdz->wp = zbdz->start;
+			continue;
 		}
 	}
 
diff --git a/oslib/linux-blkzoned.c b/oslib/linux-blkzoned.c
index 0a8a577a..eee327dd 100644
--- a/oslib/linux-blkzoned.c
+++ b/oslib/linux-blkzoned.c
@@ -203,7 +203,8 @@ int blkzoned_report_zones(struct thread_data *td, struct fio_file *f,
 		default:
 			/* Treat all these conditions as offline (don't use!) */
 			z->cond = ZBD_ZONE_COND_OFFLINE;
-			break;
+			z->wp = z->start;
+			continue;
 		}
 	}
 
-- 
2.21.0

[PATCH 06/15] zbd: remove dependency on zone type during i/o

[Dmitry Fomichev]

Two different type of zones have a write pointer: Sequential Write
Required (SWR) and Sequential Write Preferred (SWP). Introduce the
zone flag "has_wp" in struct zbd_zone_info and set it to 1 for these
zone types upon initialization, thus avoiding the necessity to check
multiple zone types in core zbd code. This flag replaces zbd_zone_swr()
function and lays the groundwork for supporting additional write
pointer zone types in the future.

The overall functionality stays the same after this commit.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 31 ++++++++++++++++---------------
 zbd.h |  2 ++
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/zbd.c b/zbd.c
index 2aa83e4a..b9aafed3 100644
--- a/zbd.c
+++ b/zbd.c
@@ -131,15 +131,6 @@ static uint32_t zbd_zone_idx(const struct fio_file *f, uint64_t offset)
 	return min(zone_idx, f->zbd_info->nr_zones);
 }
 
-/**
- * zbd_zone_swr - Test whether a zone requires sequential writes
- * @z: zone info pointer.
- */
-static inline bool zbd_zone_swr(struct fio_zone_info *z)
-{
-	return z->type == ZBD_ZONE_TYPE_SWR;
-}
-
 /**
  * zbd_zone_end - Return zone end location
  * @z: zone info pointer.
@@ -171,7 +162,7 @@ static bool zbd_zone_full(const struct fio_file *f, struct fio_zone_info *z,
 {
 	assert((required & 511) == 0);
 
-	return zbd_zone_swr(z) &&
+	return z->has_wp &&
 		z->wp + required > zbd_zone_capacity_end(z);
 }
 
@@ -249,7 +240,7 @@ static bool zbd_is_seq_job(struct fio_file *f)
 	zone_idx_b = zbd_zone_idx(f, f->file_offset);
 	zone_idx_e = zbd_zone_idx(f, f->file_offset + f->io_size - 1);
 	for (zone_idx = zone_idx_b; zone_idx <= zone_idx_e; zone_idx++)
-		if (zbd_zone_swr(&f->zbd_info->zone_info[zone_idx]))
+		if (f->zbd_info->zone_info[zone_idx].has_wp)
 			return true;
 
 	return false;
@@ -429,6 +420,7 @@ static int init_zone_info(struct thread_data *td, struct fio_file *f)
 		p->type = ZBD_ZONE_TYPE_SWR;
 		p->cond = ZBD_ZONE_COND_EMPTY;
 		p->capacity = zone_capacity;
+		p->has_wp = 1;
 	}
 	/* a sentinel */
 	p->start = nr_zones * zone_size;
@@ -512,8 +504,17 @@ static int parse_zone_info(struct thread_data *td, struct fio_file *f)
 				p->wp = z->wp;
 				break;
 			}
+
+			switch (z->type) {
+			case ZBD_ZONE_TYPE_SWR:
+				p->has_wp = 1;
+				break;
+			default:
+				p->has_wp = 0;
+			}
 			p->type = z->type;
 			p->cond = z->cond;
+
 			if (j > 0 && p->start != p[-1].start + zone_size) {
 				log_info("%s: invalid zone data\n",
 					 f->file_name);
@@ -811,7 +812,7 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
 	for (z = zb; z < ze; z++) {
 		uint32_t nz = zbd_zone_nr(f, z);
 
-		if (!zbd_zone_swr(z))
+		if (!z->has_wp)
 			continue;
 		zone_lock(td, f, z);
 		if (all_zones) {
@@ -1313,7 +1314,7 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 	assert(zone_idx < zbd_info->nr_zones);
 	z = get_zone(f, zone_idx);
 
-	if (!zbd_zone_swr(z))
+	if (!z->has_wp)
 		return;
 
 	if (!success)
@@ -1373,7 +1374,7 @@ static void zbd_put_io(struct thread_data *td, const struct io_u *io_u)
 	assert(zone_idx < zbd_info->nr_zones);
 	z = get_zone(f, zone_idx);
 
-	if (!zbd_zone_swr(z))
+	if (!z->has_wp)
 		return;
 
 	dprint(FD_ZBD,
@@ -1538,7 +1539,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	orig_zb = zb;
 
 	/* Accept the I/O offset for conventional zones. */
-	if (!zbd_zone_swr(zb))
+	if (!zb->has_wp)
 		return io_u_accept;
 
 	/*
diff --git a/zbd.h b/zbd.h
index bff55f99..059a9f9e 100644
--- a/zbd.h
+++ b/zbd.h
@@ -28,6 +28,7 @@ enum io_u_action {
  * @mutex: protects the modifiable members in this structure
  * @type: zone type (BLK_ZONE_TYPE_*)
  * @cond: zone state (BLK_ZONE_COND_*)
+ * @has_wp: whether or not this zone can have a valid write pointer
  * @open: whether or not this zone is currently open. Only relevant if
  *		max_open_zones > 0.
  * @reset_zone: whether or not this zone should be reset before writing to it
@@ -40,6 +41,7 @@ struct fio_zone_info {
 	uint32_t		verify_block;
 	enum zbd_zone_type	type:2;
 	enum zbd_zone_cond	cond:4;
+	unsigned int		has_wp:1;
 	unsigned int		open:1;
 	unsigned int		reset_zone:1;
 };
-- 
2.21.0

[PATCH 07/15] zbd: skip offline zones in zbd_convert_to_open_zone()

[Dmitry Fomichev]

Since all I/Os to an offline zone will fail, add a check in
zbd_convert_to_open_zone() to ignore zones that have this condition.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/zbd.c b/zbd.c
index b9aafed3..048fdbd0 100644
--- a/zbd.c
+++ b/zbd.c
@@ -1078,7 +1078,8 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
 
 		zone_lock(td, f, z);
 		pthread_mutex_lock(&f->zbd_info->mutex);
-		if (td->o.max_open_zones == 0 && td->o.job_max_open_zones == 0)
+		if (z->cond != ZBD_ZONE_COND_OFFLINE &&
+		    td->o.max_open_zones == 0 && td->o.job_max_open_zones == 0)
 			goto examine_zone;
 		if (f->zbd_info->num_open_zones == 0) {
 			dprint(FD_ZBD, "%s(%s): no zones are open\n",
@@ -1200,6 +1201,7 @@ out:
 	dprint(FD_ZBD, "%s(%s): returning zone %d\n", __func__, f->file_name,
 	       zone_idx);
 	io_u->offset = z->start;
+	assert(z->cond != ZBD_ZONE_COND_OFFLINE);
 	return z;
 }
 
-- 
2.21.0

[PATCH 08/15] zbd: avoid zone buffer overrun

[Dmitry Fomichev]

If the total number of zones on a drive is calculated to a value that
is less than the number of zones it can actually report, zone info
buffer can be overrun. This may happen not only due to drive firmware
problems, but also because of underlying software incorrectly
reporting zoned device capacity.

Fix this by more carefully setting zone report size.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/zbd.c b/zbd.c
index 048fdbd0..52cd7b8b 100644
--- a/zbd.c
+++ b/zbd.c
@@ -526,8 +526,9 @@ static int parse_zone_info(struct thread_data *td, struct fio_file *f)
 		offset = z->start + z->len;
 		if (j >= nr_zones)
 			break;
-		nrz = zbd_report_zones(td, f, offset,
-					    zones, ZBD_REPORT_MAX_ZONES);
+		nrz = zbd_report_zones(td, f, offset, zones,
+				       min((uint32_t)(nr_zones - j),
+					   ZBD_REPORT_MAX_ZONES));
 		if (nrz < 0) {
 			ret = nrz;
 			log_info("fio: report zones (offset %llu) failed for %s (%d).\n",
-- 
2.21.0

[PATCH 09/15] zbd: don't unlock zone mutex after verify replay

[Dmitry Fomichev]

zbd_adjust_block() always returns with the zone locked if the i/o is
accepted. The corresponding unlock happens in zbd_put_io(). The
function description says -

 * Locking strategy: returns with z->mutex locked if and only if z refers
 * to a sequential zone and if io_u_accept is returned. z is the zone that
 * corresponds to io_u->offset at the end of this function.

Remove the recently added unlock after zbd_replay_write_order() call.
Add a Coverity annotation to mark the absence of unlock as intentional.

Fixes: b2726d53bb5d ("zbd: Add a missing pthread_mutex_unlock() call")
Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/zbd.c b/zbd.c
index 52cd7b8b..eaf2137f 100644
--- a/zbd.c
+++ b/zbd.c
@@ -1561,7 +1561,12 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	case DDIR_READ:
 		if (td->runstate == TD_VERIFYING && td_write(td)) {
 			zb = zbd_replay_write_order(td, io_u, zb);
-			zone_unlock(zb);
+			/*
+			 * Since we return with the zone lock still held,
+			 * add an annotation to let Coverity know that it
+			 * is intentional.
+			 */
+			/* coverity[missing_unlock] */
 			goto accept;
 		}
 		/*
-- 
2.21.0

[PATCH 10/15] zbd: do not lock conventional zones on I/O adjustment

[Dmitry Fomichev]

From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

When a random workload runs against write pointer zones, I/Os are
adjusted to meet write pointer restrictions. During read, I/O offsets
are adjusted to point to zones with data to read and during write, I/O
offsets are adjusted to be at write pointers of open zones.

However, when a random workload runs in a range that contains both
write pointer zones and conventional zones, I/Os to write pointer
zones can potentially be adjusted to conventional zones. The functions
zbd_find_zone() and zbd_convert_to_open_zone() search for zones
regardless of their type, and therefore they may return conventional
zones. These functions lock the found zone to guard its open status
and write pointer position, but this lock is meaningless for
conventional zones. This unwanted lock of conventional zones has been
observed to cause a deadlock.

Furthermore, zbd_convert_to_open_zone() may add the found conventional
zone to the array of open zones. However, conventional zones should
never be added to the array of open zones as conventional zones never
take the "implicit open" condition and not counted as part of the
device open zone management.

To avoid the deadlock, modify zbd_find_zone() not to lock zone when it
checks conventional zone without write pointer. To avoid the deadlock
and the conventional zone open, modify zbd_convert_to_open_zone() to
ignore conventional zones.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/zbd.c b/zbd.c
index eaf2137f..3473be8d 100644
--- a/zbd.c
+++ b/zbd.c
@@ -1033,7 +1033,8 @@ static uint32_t pick_random_zone_idx(const struct fio_file *f,
 /*
  * Modify the offset of an I/O unit that does not refer to an open zone such
  * that it refers to an open zone. Close an open zone and open a new zone if
- * necessary. This algorithm can only work correctly if all write pointers are
+ * necessary. The open zone is searched across sequential zones.
+ * This algorithm can only work correctly if all write pointers are
  * a multiple of the fio block size. The caller must neither hold z->mutex
  * nor f->zbd_info->mutex. Returns with z->mutex held upon success.
  */
@@ -1159,7 +1160,8 @@ open_other_zone:
 	/* Zone 'z' is full, so try to open a new zone. */
 	for (i = f->io_size / f->zbd_info->zone_size; i > 0; i--) {
 		zone_idx++;
-		zone_unlock(z);
+		if (z->has_wp)
+			zone_unlock(z);
 		z++;
 		if (!is_valid_offset(f, z->start)) {
 			/* Wrap-around. */
@@ -1167,6 +1169,8 @@ open_other_zone:
 			z = get_zone(f, zone_idx);
 		}
 		assert(is_valid_offset(f, z->start));
+		if (!z->has_wp)
+			continue;
 		zone_lock(td, f, z);
 		if (z->open)
 			continue;
@@ -1202,6 +1206,7 @@ out:
 	dprint(FD_ZBD, "%s(%s): returning zone %d\n", __func__, f->file_name,
 	       zone_idx);
 	io_u->offset = z->start;
+	assert(z->has_wp);
 	assert(z->cond != ZBD_ZONE_COND_OFFLINE);
 	return z;
 }
@@ -1228,12 +1233,12 @@ static struct fio_zone_info *zbd_replay_write_order(struct thread_data *td,
 }
 
 /*
- * Find another zone for which @io_u fits below the write pointer. Start
- * searching in zones @zb + 1 .. @zl and continue searching in zones
- * @zf .. @zb - 1.
+ * 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.
  *
- * Either returns NULL or returns a zone pointer and holds the mutex for that
- * zone.
+ * 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,
@@ -1250,19 +1255,23 @@ zbd_find_zone(struct thread_data *td, struct io_u *io_u,
 	 */
 	for (z1 = zb + 1, z2 = zb - 1; z1 < zl || z2 >= zf; z1++, z2--) {
 		if (z1 < zl && z1->cond != ZBD_ZONE_COND_OFFLINE) {
-			zone_lock(td, f, z1);
+			if (z1->has_wp)
+				zone_lock(td, f, z1);
 			if (z1->start + min_bs <= z1->wp)
 				return z1;
-			zone_unlock(z1);
+			if (z1->has_wp)
+				zone_unlock(z1);
 		} else if (!td_random(td)) {
 			break;
 		}
 		if (td_random(td) && z2 >= zf &&
 		    z2->cond != ZBD_ZONE_COND_OFFLINE) {
-			zone_lock(td, f, z2);
+			if (z2->has_wp)
+				zone_lock(td, f, z2);
 			if (z2->start + min_bs <= z2->wp)
 				return z2;
-			zone_unlock(z2);
+			if (z2->has_wp)
+				zone_unlock(z2);
 		}
 	}
 	dprint(FD_ZBD, "%s: adjusting random read offset failed\n",
-- 
2.21.0

[PATCH 11/15] zbd: do not set zbd handlers for conventional zones

[Dmitry Fomichev]

From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

When zbd_adjust_block() modifies io_u to satisfy write pointer
restrictions, it may change the zone for the io_u. The function sets
pointers to zbd_queue_io() and zbd_put_io() handlers to io_u to
further process write pointer zones. However, when the I/O is
redirected to a conventional zone, these handlers should not
be set in io_u.

Skip setting the handlers when this function returns a conventional
zone. When zbd_adjust_block() can not find a zone to fit the I/O,
the existing code unlocks the zone pointer 'zb' used in the function.
This unlock should not be performed if 'zb' points to a conventional
zone upon return, skip it in this case.

These changes make the assert for 'zb' pointer near 'accept' label in
zbd_adjust_block() unnecessary. Replace it with assert for zb->has_wp,
since the zone at the step shall have write pointer.

Since zone locking functions (zone_lock(), zbd_queue_io() and
zbd_put_io()) are supposed to be called only for write pointer zones,
add assertions to zone_lock() and zone_unlock() to make sure this is
the case. This allows us to convert a few existing conditional checks
to assertions and therefore make zone type validation more strict.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/zbd.c b/zbd.c
index 3473be8d..a99b6b2f 100644
--- a/zbd.c
+++ b/zbd.c
@@ -174,6 +174,8 @@ static void zone_lock(struct thread_data *td, struct fio_file *f, struct fio_zon
 	/* A thread should never lock zones outside its working area. */
 	assert(f->min_zone <= nz && nz < f->max_zone);
 
+	assert(z->has_wp);
+
 	/*
 	 * Lock the io_u target zone. The zone will be unlocked if io_u offset
 	 * is changed or when io_u completes and zbd_put_io() executed.
@@ -194,6 +196,7 @@ static inline void zone_unlock(struct fio_zone_info *z)
 {
 	int ret;
 
+	assert(z->has_wp);
 	ret = pthread_mutex_unlock(&z->mutex);
 	assert(!ret);
 }
@@ -1326,8 +1329,7 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 	assert(zone_idx < zbd_info->nr_zones);
 	z = get_zone(f, zone_idx);
 
-	if (!z->has_wp)
-		return;
+	assert(z->has_wp);
 
 	if (!success)
 		goto unlock;
@@ -1386,8 +1388,7 @@ static void zbd_put_io(struct thread_data *td, const struct io_u *io_u)
 	assert(zone_idx < zbd_info->nr_zones);
 	z = get_zone(f, zone_idx);
 
-	if (!z->has_wp)
-		return;
+	assert(z->has_wp);
 
 	dprint(FD_ZBD,
 	       "%s: terminate I/O (%lld, %llu) for zone %u\n",
@@ -1617,6 +1618,12 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			io_u->offset = zb->start +
 				((io_u->offset - orig_zb->start) %
 				 (range - io_u->buflen)) / min_bs * min_bs;
+		/*
+		 * When zbd_find_zone() returns a conventional zone,
+		 * we can simply accept the new i/o offset here.
+		 */
+		if (!zb->has_wp)
+			return io_u_accept;
 		/*
 		 * Make sure the I/O does not cross over the zone wp position.
 		 */
@@ -1713,7 +1720,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	assert(false);
 
 accept:
-	assert(zb);
+	assert(zb->has_wp);
 	assert(zb->cond != ZBD_ZONE_COND_OFFLINE);
 	assert(!io_u->zbd_queue_io);
 	assert(!io_u->zbd_put_io);
@@ -1722,7 +1729,7 @@ accept:
 	return io_u_accept;
 
 eof:
-	if (zb)
+	if (zb && zb->has_wp)
 		zone_unlock(zb);
 	return io_u_eof;
 }
-- 
2.21.0

[PATCH 12/15] zbd: count sectors with data for write pointer zones

[Dmitry Fomichev]

From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

ZBD fio code tracks 'sectors with data' for two different purposes.
The first one is to process zone_reset_threshold. When the ratio of
sectors with data in zones with write pointer goes beyond the specified
number, zone reset is triggered. The second purpose is to control the
direction of the first I/O of random mixed read/write workloads. If all
write pointer zones in the I/O range are reset at the beginning of such
a workload, fio has no data to read and will immediately end the run of
the test section. To avoid this, fio checks 'sectors with data' and if
it is zero (i.e. it is the very first I/O), it modifies the direction
of that I/O from read to write.

Currently, when the workload range includes both conventional and
sequential zones, all sectors in conventional zones are counted as
'sectors with data' along with sectors in sequential zones.
This leads to incorrect handling  of 'zone_reset_threshold' option -
zone reset timing of sequential zones is affected by the amount of
data read from or written to conventional zones. To avoid this,
conventional zones should be excluded from 'sectors with data'
calculation.

On the other hand, if the sectors of conventional zones were excluded
from the sectors with data, it could result in the wrong initial I/O
direction for random workloads. When the zones in I/O region are all
conventional, 'sectors with data' would always be zero. Because of
this, read operations are always changed to writes and reads are never
performed.

To avoid this contradiction, introduce another counter,
'wp_sector_with_data'. It works similar to the existing
'sectors_with_data', but it counts data sectors only in write pointer
zones. Use this newly introduced count for zone_reset_threshold checks
and keep on using the original count for the initial random I/O
direction determination.

When counting sectors with data, lock only write pointer zones, no need
to lock conventional zones.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 zbd.c | 25 ++++++++++++++++++-------
 zbd.h |  3 +++
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/zbd.c b/zbd.c
index a99b6b2f..f513295a 100644
--- a/zbd.c
+++ b/zbd.c
@@ -734,9 +734,10 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
 {
 	uint64_t offset = z->start;
 	uint64_t length = (z+1)->start - offset;
+	uint64_t data_in_zone = z->wp - z->start;
 	int ret = 0;
 
-	if (z->wp == z->start)
+	if (!data_in_zone)
 		return 0;
 
 	assert(is_valid_offset(f, offset + length - 1));
@@ -755,7 +756,8 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
 	}
 
 	pthread_mutex_lock(&f->zbd_info->mutex);
-	f->zbd_info->sectors_with_data -= z->wp - z->start;
+	f->zbd_info->sectors_with_data -= data_in_zone;
+	f->zbd_info->wp_sectors_with_data -= data_in_zone;
 	pthread_mutex_unlock(&f->zbd_info->mutex);
 	z->wp = z->start;
 	z->verify_block = 0;
@@ -887,25 +889,32 @@ static uint64_t zbd_process_swd(const struct fio_file *f, enum swd_action a)
 {
 	struct fio_zone_info *zb, *ze, *z;
 	uint64_t swd = 0;
+	uint64_t wp_swd = 0;
 
 	zb = get_zone(f, f->min_zone);
 	ze = get_zone(f, f->max_zone);
 	for (z = zb; z < ze; z++) {
-		pthread_mutex_lock(&z->mutex);
+		if (z->has_wp) {
+			pthread_mutex_lock(&z->mutex);
+			wp_swd += z->wp - z->start;
+		}
 		swd += z->wp - z->start;
 	}
 	pthread_mutex_lock(&f->zbd_info->mutex);
 	switch (a) {
 	case CHECK_SWD:
 		assert(f->zbd_info->sectors_with_data == swd);
+		assert(f->zbd_info->wp_sectors_with_data == wp_swd);
 		break;
 	case SET_SWD:
 		f->zbd_info->sectors_with_data = swd;
+		f->zbd_info->wp_sectors_with_data = wp_swd;
 		break;
 	}
 	pthread_mutex_unlock(&f->zbd_info->mutex);
 	for (z = zb; z < ze; z++)
-		zone_unlock(z);
+		if (z->has_wp)
+			zone_unlock(z);
 
 	return swd;
 }
@@ -916,7 +925,7 @@ static uint64_t zbd_process_swd(const struct fio_file *f, enum swd_action a)
  */
 static const bool enable_check_swd = false;
 
-/* Check whether the value of zbd_info.sectors_with_data is correct. */
+/* Check whether the values of zbd_info.*sectors_with_data are correct. */
 static void zbd_check_swd(const struct fio_file *f)
 {
 	if (!enable_check_swd)
@@ -1347,8 +1356,10 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 		 * z->wp > zone_end means that one or more I/O errors
 		 * have occurred.
 		 */
-		if (z->wp <= zone_end)
+		if (z->wp <= zone_end) {
 			zbd_info->sectors_with_data += zone_end - z->wp;
+			zbd_info->wp_sectors_with_data += zone_end - z->wp;
+		}
 		pthread_mutex_unlock(&zbd_info->mutex);
 		z->wp = zone_end;
 		break;
@@ -1650,7 +1661,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 		}
 		/* Check whether the zone reset threshold has been exceeded */
 		if (td->o.zrf.u.f) {
-			if (f->zbd_info->sectors_with_data >=
+			if (f->zbd_info->wp_sectors_with_data >=
 			    f->io_size * td->o.zrt.u.f &&
 			    zbd_dec_and_reset_write_cnt(td, f)) {
 				zb->reset_zone = 1;
diff --git a/zbd.h b/zbd.h
index 059a9f9e..cc3ab624 100644
--- a/zbd.h
+++ b/zbd.h
@@ -55,6 +55,8 @@ struct fio_zone_info {
  *		num_open_zones).
  * @zone_size: size of a single zone in bytes.
  * @sectors_with_data: total size of data in all zones in units of 512 bytes
+ * @wp_sectors_with_data: total size of data in zones with write pointers in
+ *                        units of 512 bytes
  * @zone_size_log2: log2 of the zone size in bytes if it is a power of 2 or 0
  *		if the zone size is not a power of 2.
  * @nr_zones: number of zones
@@ -75,6 +77,7 @@ struct zoned_block_device_info {
 	pthread_mutex_t		mutex;
 	uint64_t		zone_size;
 	uint64_t		sectors_with_data;
+	uint64_t		wp_sectors_with_data;
 	uint32_t		zone_size_log2;
 	uint32_t		nr_zones;
 	uint32_t		refcount;
-- 
2.21.0

[PATCH 13/15] zbd: initialize min_zone and max_zone for all zone types

[Dmitry Fomichev]

From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

The function zbd_verify_sizes() checks if the given I/O range includes
write pointer zones. When all zones in the I/O range are conventional,
it skips checks for size options and leaves min_zone and max_zone in
struct fio_file with zero values. These uninitialized min_zone and
max_zone fields trigger unexpected behaviors such as unset
sectors_with_data.

Fix this by moving min_zone and max_zone set up from zbd_verify_sizes()
to zbd_setup_files(). This allows for setting up the values regardless
of zone types in I/O range.

Bypass the assertion to ensure that max_zone is larger than min_zone if
all zones in the I/O range are conventional. In this case, io_size
can be smaller than zone size and, consequently, min_zone may become
the same as max_zone.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/zbd.c b/zbd.c
index f513295a..c364e87c 100644
--- a/zbd.c
+++ b/zbd.c
@@ -324,10 +324,6 @@ static bool zbd_verify_sizes(void)
 					 (unsigned long long) new_end - f->file_offset);
 				f->io_size = new_end - f->file_offset;
 			}
-
-			f->min_zone = zbd_zone_idx(f, f->file_offset);
-			f->max_zone = zbd_zone_idx(f, f->file_offset + f->io_size);
-			assert(f->min_zone < f->max_zone);
 		}
 	}
 
@@ -680,6 +676,18 @@ int zbd_setup_files(struct thread_data *td)
 		if (!zbd)
 			continue;
 
+		f->min_zone = zbd_zone_idx(f, f->file_offset);
+		f->max_zone = zbd_zone_idx(f, f->file_offset + f->io_size);
+
+		/*
+		 * When all zones in the I/O range are conventional, io_size
+		 * can be smaller than zone size, making min_zone the same
+		 * as max_zone. This is why the assert below needs to be made
+		 * conditional.
+		 */
+		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 &&
-- 
2.21.0

[PATCH 14/15] zbd: initialize sectors with data at start time

[Dmitry Fomichev]

From: Aravind Ramesh <aravind.ramesh@wdc.com>

Based on the flag enable_check_swd, which is false by default, fio
does not initialize the swd value at startup, initializing the swd
value to be zero, even if some zones have sectors with data. This can
result in fio reflecting less than actual swd after a few writes are
completed. In workloads where verify is enabled, fio resets all the
zones and while resetting, it decrements the swd counter with the
actual number of swds in that zone(swd-count - swd-in-zone),
since swd-count is initialized to 0, it results in overflow of the
variable causing unpredictable issues.

So, initialize the swd to the correct value.

Fixes: 409a4f291e7f ("zbd: avoid initializing swd when unnecessary")
Signed-off-by: Aravind Ramesh <aravind.ramesh@wdc.com>
Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/zbd.c b/zbd.c
index c364e87c..6a58115d 100644
--- a/zbd.c
+++ b/zbd.c
@@ -946,9 +946,6 @@ static void zbd_init_swd(struct fio_file *f)
 {
 	uint64_t swd;
 
-	if (!enable_check_swd)
-		return;
-
 	swd = zbd_process_swd(f, SET_SWD);
 	dprint(FD_ZBD, "%s(%s): swd = %" PRIu64 "\n", __func__, f->file_name,
 	       swd);
-- 
2.21.0

[PATCH 15/15] zbd: use zone_lock() in zbd_process_swd()

[Dmitry Fomichev]

Most of ZBD code in fio uses zone_lock() to lock write pointer zones.
This wrapper, besides doing the obvious pthread mutex lock, quiesce
the outstanding i/o when running via asynchronous ioengines. This is
necessary to avoid deadlocks. The function zbd_process_swd(), however,
still uses the naked pthread mutex to lock zones and this leads to a
deadlock when running ZBD test #48 against regular nullb devices.

The fix added in the same patch series that introduced test #48 was to
NOT initialize SWD at all, but this solution is found to create
problems with verify. As the proper fix, modify zbd_process_swd()
to use zone_lock(). This makes the test #48 pass even when SWD counter
is initialized.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/zbd.c b/zbd.c
index 6a58115d..cdb4dc17 100644
--- a/zbd.c
+++ b/zbd.c
@@ -166,7 +166,8 @@ static bool zbd_zone_full(const struct fio_file *f, struct fio_zone_info *z,
 		z->wp + required > zbd_zone_capacity_end(z);
 }
 
-static void zone_lock(struct thread_data *td, struct fio_file *f, struct fio_zone_info *z)
+static void zone_lock(struct thread_data *td, const struct fio_file *f,
+		      struct fio_zone_info *z)
 {
 	struct zoned_block_device_info *zbd = f->zbd_info;
 	uint32_t nz = z - zbd->zone_info;
@@ -893,7 +894,8 @@ enum swd_action {
 };
 
 /* Calculate the number of sectors with data (swd) and perform action 'a' */
-static uint64_t zbd_process_swd(const struct fio_file *f, enum swd_action a)
+static uint64_t zbd_process_swd(struct thread_data *td,
+				const struct fio_file *f, enum swd_action a)
 {
 	struct fio_zone_info *zb, *ze, *z;
 	uint64_t swd = 0;
@@ -903,7 +905,7 @@ static uint64_t zbd_process_swd(const struct fio_file *f, enum swd_action a)
 	ze = get_zone(f, f->max_zone);
 	for (z = zb; z < ze; z++) {
 		if (z->has_wp) {
-			pthread_mutex_lock(&z->mutex);
+			zone_lock(td, f, z);
 			wp_swd += z->wp - z->start;
 		}
 		swd += z->wp - z->start;
@@ -934,33 +936,27 @@ static uint64_t zbd_process_swd(const struct fio_file *f, enum swd_action a)
 static const bool enable_check_swd = false;
 
 /* Check whether the values of zbd_info.*sectors_with_data are correct. */
-static void zbd_check_swd(const struct fio_file *f)
+static void zbd_check_swd(struct thread_data *td, const struct fio_file *f)
 {
 	if (!enable_check_swd)
 		return;
 
-	zbd_process_swd(f, CHECK_SWD);
-}
-
-static void zbd_init_swd(struct fio_file *f)
-{
-	uint64_t swd;
-
-	swd = zbd_process_swd(f, SET_SWD);
-	dprint(FD_ZBD, "%s(%s): swd = %" PRIu64 "\n", __func__, f->file_name,
-	       swd);
+	zbd_process_swd(td, f, CHECK_SWD);
 }
 
 void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 {
 	struct fio_zone_info *zb, *ze;
+	uint64_t swd;
 
 	if (!f->zbd_info || !td_write(td))
 		return;
 
 	zb = get_zone(f, f->min_zone);
 	ze = get_zone(f, f->max_zone);
-	zbd_init_swd(f);
+	swd = zbd_process_swd(td, f, SET_SWD);
+	dprint(FD_ZBD, "%s(%s): swd = %" PRIu64 "\n", __func__, f->file_name,
+	       swd);
 	/*
 	 * If data verification is enabled reset the affected zones before
 	 * writing any data to avoid that a zone reset has to be issued while
@@ -1413,7 +1409,7 @@ static void zbd_put_io(struct thread_data *td, const struct io_u *io_u)
 	zbd_end_zone_io(td, io_u, z);
 
 	zone_unlock(z);
-	zbd_check_swd(f);
+	zbd_check_swd(td, f);
 }
 
 /*
@@ -1579,7 +1575,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	    io_u->ddir == DDIR_READ && td->o.read_beyond_wp)
 		return io_u_accept;
 
-	zbd_check_swd(f);
+	zbd_check_swd(td, f);
 
 	zone_lock(td, f, zb);
 
-- 
2.21.0

Re: [PATCH 00/15] ZBD fixes and improvements

[Bart Van Assche]

On 11/20/20 6:45 PM, Dmitry Fomichev wrote:
> This patch series contains bug fixes and refactoring changes
> related to support for Zoned Block Devices (ZBD) in fio.
> The highlights:
> 
>  - fix several errors related to running workloads that span
>    a mix of conventional zones and write pointer zones.
>  - improve counting of sectors with data (SWD).
>  - remove dependencies on particular zone types in the code.
>  - add code to gracefully handle offline zones.

Hi Dmitry,

This patch series looks interesting. Out of curiosity, do you perhaps
know how much of the modified code is covered by the tests in t/zbd?

Thanks,

Bart.

RE: [PATCH 00/15] ZBD fixes and improvements

[Dmitry Fomichev]

> -----Original Message-----
> From: Bart Van Assche <bvanassche@acm.org>
> Sent: Friday, November 20, 2020 11:01 PM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
> <axboe@kernel.dk>; fio@vger.kernel.org; Aravind Ramesh
> <Aravind.Ramesh@wdc.com>; Naohiro Aota <Naohiro.Aota@wdc.com>;
> Niklas Cassel <Niklas.Cassel@wdc.com>
> Cc: Damien Le Moal <Damien.LeMoal@wdc.com>; Shinichiro Kawasaki
> <shinichiro.kawasaki@wdc.com>
> Subject: Re: [PATCH 00/15] ZBD fixes and improvements
> 
> On 11/20/20 6:45 PM, Dmitry Fomichev wrote:
> > This patch series contains bug fixes and refactoring changes
> > related to support for Zoned Block Devices (ZBD) in fio.
> > The highlights:
> >
> >  - fix several errors related to running workloads that span
> >    a mix of conventional zones and write pointer zones.
> >  - improve counting of sectors with data (SWD).
> >  - remove dependencies on particular zone types in the code.
> >  - add code to gracefully handle offline zones.
> 
> Hi Dmitry,
> 
> This patch series looks interesting. Out of curiosity, do you perhaps
> know how much of the modified code is covered by the tests in t/zbd?

Hi Bart,

All tests in t/zbd are passing with this series in place. This gives us confidence
that the patches don't break anything in terms of the existing functionality.
Most of the bugs that are fixed in this patchset were uncovered by running fio in
environments that go beyond the scope of t/zbd tests, such as ZNS, XMR, some
specific MaxOpen values, etc. and the fixes have been verified in these same
conditions. Having said that, the newer test #48 has become a very handy tool
for identifying zone deadlocks.

One caveat about the paragraph above - some test script modifications are
needed to fully cover support for offline zones and I have some patches in the
works to add such functionality. I am planning to send these in in the near future.
The current tests do pass in the case of individually injected offline zones on a
drive and this level of testing should suffice for the time being.

One thing that I thought about while writing this email - maybe we could add
a script to run t/zbd tests on a mixed null_blk with a good amount of conventional
zones? Damien, Shinichiro, do you think that such an addition would improve the
test coverage?

Best regards,
Dmitry

> 
> Thanks,
> 
> Bart.

Re: [PATCH 00/15] ZBD fixes and improvements

[Damien Le Moal]

On 2020/11/24 4:18, Dmitry Fomichev wrote:
>> -----Original Message-----
>> From: Bart Van Assche <bvanassche@acm.org>
>> Sent: Friday, November 20, 2020 11:01 PM
>> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
>> <axboe@kernel.dk>; fio@vger.kernel.org; Aravind Ramesh
>> <Aravind.Ramesh@wdc.com>; Naohiro Aota <Naohiro.Aota@wdc.com>;
>> Niklas Cassel <Niklas.Cassel@wdc.com>
>> Cc: Damien Le Moal <Damien.LeMoal@wdc.com>; Shinichiro Kawasaki
>> <shinichiro.kawasaki@wdc.com>
>> Subject: Re: [PATCH 00/15] ZBD fixes and improvements
>>
>> On 11/20/20 6:45 PM, Dmitry Fomichev wrote:
>>> This patch series contains bug fixes and refactoring changes
>>> related to support for Zoned Block Devices (ZBD) in fio.
>>> The highlights:
>>>
>>>  - fix several errors related to running workloads that span
>>>    a mix of conventional zones and write pointer zones.
>>>  - improve counting of sectors with data (SWD).
>>>  - remove dependencies on particular zone types in the code.
>>>  - add code to gracefully handle offline zones.
>>
>> Hi Dmitry,
>>
>> This patch series looks interesting. Out of curiosity, do you perhaps
>> know how much of the modified code is covered by the tests in t/zbd?
> 
> Hi Bart,
> 
> All tests in t/zbd are passing with this series in place. This gives us confidence
> that the patches don't break anything in terms of the existing functionality.
> Most of the bugs that are fixed in this patchset were uncovered by running fio in
> environments that go beyond the scope of t/zbd tests, such as ZNS, XMR, some
> specific MaxOpen values, etc. and the fixes have been verified in these same
> conditions. Having said that, the newer test #48 has become a very handy tool
> for identifying zone deadlocks.
> 
> One caveat about the paragraph above - some test script modifications are
> needed to fully cover support for offline zones and I have some patches in the
> works to add such functionality. I am planning to send these in in the near future.
> The current tests do pass in the case of individually injected offline zones on a
> drive and this level of testing should suffice for the time being.
> 
> One thing that I thought about while writing this email - maybe we could add
> a script to run t/zbd tests on a mixed null_blk with a good amount of conventional
> zones? Damien, Shinichiro, do you think that such an addition would improve the
> test coverage?

Yes, we should add that to run-tests-against-zoned-nullb to avoid future
possible regressions with conventional zones. And we should make sure that one
test case runs over a range of mixed conv/seq zones.

Going further, I think that we should merge run-tests-against-zoned-nullb and
run-tests-against-zoned-nullb into a new script run-tests-against-nullb which
would test all device configurations: regular nullb, zoned nullb with
conventional zones (SMR disk like device), zoned nullb with seq zones only and
zone capacity < zone size (ZNS like device). With that, test coverage would be
improved to include all recent changes.

> 
> Best regards,
> Dmitry
> 
>>
>> Thanks,
>>
>> Bart.
> 


-- 
Damien Le Moal
Western Digital Research

RE: [PATCH 00/15] ZBD fixes and improvements

[Dmitry Fomichev]

> -----Original Message-----
> From: Damien Le Moal <Damien.LeMoal@wdc.com>
> Sent: Monday, November 23, 2020 7:07 PM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Bart Van Assche
> <bvanassche@acm.org>; Jens Axboe <axboe@kernel.dk>;
> fio@vger.kernel.org; Aravind Ramesh <Aravind.Ramesh@wdc.com>;
> Naohiro Aota <Naohiro.Aota@wdc.com>; Niklas Cassel
> <Niklas.Cassel@wdc.com>
> Cc: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Subject: Re: [PATCH 00/15] ZBD fixes and improvements
> 
> On 2020/11/24 4:18, Dmitry Fomichev wrote:
> >> -----Original Message-----
> >> From: Bart Van Assche <bvanassche@acm.org>
> >> Sent: Friday, November 20, 2020 11:01 PM
> >> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
> >> <axboe@kernel.dk>; fio@vger.kernel.org; Aravind Ramesh
> >> <Aravind.Ramesh@wdc.com>; Naohiro Aota <Naohiro.Aota@wdc.com>;
> >> Niklas Cassel <Niklas.Cassel@wdc.com>
> >> Cc: Damien Le Moal <Damien.LeMoal@wdc.com>; Shinichiro Kawasaki
> >> <shinichiro.kawasaki@wdc.com>
> >> Subject: Re: [PATCH 00/15] ZBD fixes and improvements
> >>
> >> On 11/20/20 6:45 PM, Dmitry Fomichev wrote:
> >>> This patch series contains bug fixes and refactoring changes
> >>> related to support for Zoned Block Devices (ZBD) in fio.
> >>> The highlights:
> >>>
> >>>  - fix several errors related to running workloads that span
> >>>    a mix of conventional zones and write pointer zones.
> >>>  - improve counting of sectors with data (SWD).
> >>>  - remove dependencies on particular zone types in the code.
> >>>  - add code to gracefully handle offline zones.
> >>
> >> Hi Dmitry,
> >>
> >> This patch series looks interesting. Out of curiosity, do you perhaps
> >> know how much of the modified code is covered by the tests in t/zbd?
> >
> > Hi Bart,
> >
> > All tests in t/zbd are passing with this series in place. This gives us
> confidence
> > that the patches don't break anything in terms of the existing functionality.
> > Most of the bugs that are fixed in this patchset were uncovered by running
> fio in
> > environments that go beyond the scope of t/zbd tests, such as ZNS, XMR,
> some
> > specific MaxOpen values, etc. and the fixes have been verified in these
> same
> > conditions. Having said that, the newer test #48 has become a very handy
> tool
> > for identifying zone deadlocks.
> >
> > One caveat about the paragraph above - some test script modifications are
> > needed to fully cover support for offline zones and I have some patches in
> the
> > works to add such functionality. I am planning to send these in in the near
> future.
> > The current tests do pass in the case of individually injected offline zones
> on a
> > drive and this level of testing should suffice for the time being.
> >
> > One thing that I thought about while writing this email - maybe we could
> add
> > a script to run t/zbd tests on a mixed null_blk with a good amount of
> conventional
> > zones? Damien, Shinichiro, do you think that such an addition would
> improve the
> > test coverage?
> 
> Yes, we should add that to run-tests-against-zoned-nullb to avoid future
> possible regressions with conventional zones. And we should make sure that
> one
> test case runs over a range of mixed conv/seq zones.
> 
> Going further, I think that we should merge run-tests-against-zoned-nullb
> and
> run-tests-against-zoned-nullb into a new script run-tests-against-nullb which
> would test all device configurations: regular nullb, zoned nullb with
> conventional zones (SMR disk like device), zoned nullb with seq zones only
> and
> zone capacity < zone size (ZNS like device). With that, test coverage would be
> improved to include all recent changes.

I was thinking about adding a meta-script that would invoke the existing two
and some new scripts, but we could just merge all of them like you suggested.
The entire test would consist of several sections and each of them would run
the entire test-zbd-support script against different null_blk configurations.
I'll try to come up with something like that... This leaves the question - should
such a script be posted separately or be included in this series. IMO, it should
be a separate patch(set).

> 
> >
> > Best regards,
> > Dmitry
> >
> >>
> >> Thanks,
> >>
> >> Bart.
> >
> 
> 
> --
> Damien Le Moal
> Western Digital Research

Re: [PATCH 00/15] ZBD fixes and improvements

[Damien Le Moal]

On 2020/11/24 10:12, Dmitry Fomichev wrote:
>> -----Original Message-----
>> From: Damien Le Moal <Damien.LeMoal@wdc.com>
>> Sent: Monday, November 23, 2020 7:07 PM
>> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Bart Van Assche
>> <bvanassche@acm.org>; Jens Axboe <axboe@kernel.dk>;
>> fio@vger.kernel.org; Aravind Ramesh <Aravind.Ramesh@wdc.com>;
>> Naohiro Aota <Naohiro.Aota@wdc.com>; Niklas Cassel
>> <Niklas.Cassel@wdc.com>
>> Cc: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>> Subject: Re: [PATCH 00/15] ZBD fixes and improvements
>>
>> On 2020/11/24 4:18, Dmitry Fomichev wrote:
>>>> -----Original Message-----
>>>> From: Bart Van Assche <bvanassche@acm.org>
>>>> Sent: Friday, November 20, 2020 11:01 PM
>>>> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
>>>> <axboe@kernel.dk>; fio@vger.kernel.org; Aravind Ramesh
>>>> <Aravind.Ramesh@wdc.com>; Naohiro Aota <Naohiro.Aota@wdc.com>;
>>>> Niklas Cassel <Niklas.Cassel@wdc.com>
>>>> Cc: Damien Le Moal <Damien.LeMoal@wdc.com>; Shinichiro Kawasaki
>>>> <shinichiro.kawasaki@wdc.com>
>>>> Subject: Re: [PATCH 00/15] ZBD fixes and improvements
>>>>
>>>> On 11/20/20 6:45 PM, Dmitry Fomichev wrote:
>>>>> This patch series contains bug fixes and refactoring changes
>>>>> related to support for Zoned Block Devices (ZBD) in fio.
>>>>> The highlights:
>>>>>
>>>>>  - fix several errors related to running workloads that span
>>>>>    a mix of conventional zones and write pointer zones.
>>>>>  - improve counting of sectors with data (SWD).
>>>>>  - remove dependencies on particular zone types in the code.
>>>>>  - add code to gracefully handle offline zones.
>>>>
>>>> Hi Dmitry,
>>>>
>>>> This patch series looks interesting. Out of curiosity, do you perhaps
>>>> know how much of the modified code is covered by the tests in t/zbd?
>>>
>>> Hi Bart,
>>>
>>> All tests in t/zbd are passing with this series in place. This gives us
>> confidence
>>> that the patches don't break anything in terms of the existing functionality.
>>> Most of the bugs that are fixed in this patchset were uncovered by running
>> fio in
>>> environments that go beyond the scope of t/zbd tests, such as ZNS, XMR,
>> some
>>> specific MaxOpen values, etc. and the fixes have been verified in these
>> same
>>> conditions. Having said that, the newer test #48 has become a very handy
>> tool
>>> for identifying zone deadlocks.
>>>
>>> One caveat about the paragraph above - some test script modifications are
>>> needed to fully cover support for offline zones and I have some patches in
>> the
>>> works to add such functionality. I am planning to send these in in the near
>> future.
>>> The current tests do pass in the case of individually injected offline zones
>> on a
>>> drive and this level of testing should suffice for the time being.
>>>
>>> One thing that I thought about while writing this email - maybe we could
>> add
>>> a script to run t/zbd tests on a mixed null_blk with a good amount of
>> conventional
>>> zones? Damien, Shinichiro, do you think that such an addition would
>> improve the
>>> test coverage?
>>
>> Yes, we should add that to run-tests-against-zoned-nullb to avoid future
>> possible regressions with conventional zones. And we should make sure that
>> one
>> test case runs over a range of mixed conv/seq zones.
>>
>> Going further, I think that we should merge run-tests-against-zoned-nullb
>> and
>> run-tests-against-zoned-nullb into a new script run-tests-against-nullb which
>> would test all device configurations: regular nullb, zoned nullb with
>> conventional zones (SMR disk like device), zoned nullb with seq zones only
>> and
>> zone capacity < zone size (ZNS like device). With that, test coverage would be
>> improved to include all recent changes.
> 
> I was thinking about adding a meta-script that would invoke the existing two
> and some new scripts, but we could just merge all of them like you suggested.
> The entire test would consist of several sections and each of them would run
> the entire test-zbd-support script against different null_blk configurations.
> I'll try to come up with something like that... This leaves the question - should
> such a script be posted separately or be included in this series. IMO, it should
> be a separate patch(set).

Merging the scripts is I think better than adding yet another one: it becomes
easy to understand which one to run :)

If Jens is OK with it, I think sending the test changes as follow-up patches is
fine so that we do not delay merging this series.

> 
>>
>>>
>>> Best regards,
>>> Dmitry
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Bart.
>>>
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 00/15] ZBD fixes and improvements

[Shinichiro Kawasaki]

On Nov 24, 2020 / 00:07, Damien Le Moal wrote:
> On 2020/11/24 4:18, Dmitry Fomichev wrote:
> >> -----Original Message-----
> >> From: Bart Van Assche <bvanassche@acm.org>
> >> Sent: Friday, November 20, 2020 11:01 PM
> >> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
> >> <axboe@kernel.dk>; fio@vger.kernel.org; Aravind Ramesh
> >> <Aravind.Ramesh@wdc.com>; Naohiro Aota <Naohiro.Aota@wdc.com>;
> >> Niklas Cassel <Niklas.Cassel@wdc.com>
> >> Cc: Damien Le Moal <Damien.LeMoal@wdc.com>; Shinichiro Kawasaki
> >> <shinichiro.kawasaki@wdc.com>
> >> Subject: Re: [PATCH 00/15] ZBD fixes and improvements
> >>
> >> On 11/20/20 6:45 PM, Dmitry Fomichev wrote:
> >>> This patch series contains bug fixes and refactoring changes
> >>> related to support for Zoned Block Devices (ZBD) in fio.
> >>> The highlights:
> >>>
> >>>  - fix several errors related to running workloads that span
> >>>    a mix of conventional zones and write pointer zones.
> >>>  - improve counting of sectors with data (SWD).
> >>>  - remove dependencies on particular zone types in the code.
> >>>  - add code to gracefully handle offline zones.
> >>
> >> Hi Dmitry,
> >>
> >> This patch series looks interesting. Out of curiosity, do you perhaps
> >> know how much of the modified code is covered by the tests in t/zbd?
> > 
> > Hi Bart,
> > 
> > All tests in t/zbd are passing with this series in place. This gives us confidence
> > that the patches don't break anything in terms of the existing functionality.
> > Most of the bugs that are fixed in this patchset were uncovered by running fio in
> > environments that go beyond the scope of t/zbd tests, such as ZNS, XMR, some
> > specific MaxOpen values, etc. and the fixes have been verified in these same
> > conditions. Having said that, the newer test #48 has become a very handy tool
> > for identifying zone deadlocks.
> > 
> > One caveat about the paragraph above - some test script modifications are
> > needed to fully cover support for offline zones and I have some patches in the
> > works to add such functionality. I am planning to send these in in the near future.
> > The current tests do pass in the case of individually injected offline zones on a
> > drive and this level of testing should suffice for the time being.
> > 
> > One thing that I thought about while writing this email - maybe we could add
> > a script to run t/zbd tests on a mixed null_blk with a good amount of conventional
> > zones? Damien, Shinichiro, do you think that such an addition would improve the
> > test coverage?
> 
> Yes, we should add that to run-tests-against-zoned-nullb to avoid future
> possible regressions with conventional zones. And we should make sure that one
> test case runs over a range of mixed conv/seq zones.

Agreed. I will work to add the test case to cover conv/seq mixed range.

-- 
Best Regards,
Shin'ichiro Kawasaki