[PATCH 00/11] Cleanup zbd code

[PATCH 00/11] Cleanup zbd code

[Damien Le Moal]

The patch series cleans up zbd.c code, improving code readability.      
Overall, no functional change is introduced.

Damien Le Moal (11):
  fio: Improve documentation of ignore_zone_limits option
  zbd: define local functions as static
  zbd: move and cleanup code
  zbd: remove is_zone_open() helper
  zbd: introduce zbd_verify_file_sizes()
  zbd: fix code style issues
  zbd: simplify zbd_close_zone()
  zbd: simplify zbd_open_zone()
  zbd: rename zbd_zone_idx() and zbd_zone_nr()
  zbd: rename get_zone()
  zbd: introduce zbd_get_zone_from_offset() helper

 HOWTO |   6 +
 fio.1 |   6 +-
 zbd.c | 960 +++++++++++++++++++++++++++++++---------------------------
 3 files changed, 524 insertions(+), 448 deletions(-)

-- 
2.31.1

[PATCH 01/11] fio: Improve documentation of ignore_zone_limits option

[Damien Le Moal]

In the manual pages, change the description of the option
ignore_zone_limits to its action when set, instead of the confusing text
describing what happens when it is not set. Also add the description
of this option in the HOWTO file as it is missing.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 HOWTO | 6 ++++++
 fio.1 | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/HOWTO b/HOWTO
index 8c9e4135..2956e50d 100644
--- a/HOWTO
+++ b/HOWTO
@@ -1063,6 +1063,12 @@ Target file/device
 	Limit on the number of simultaneously opened zones per single
 	thread/process.
 
+.. option:: ignore_zone_limits=bool
+	If this option is used, fio will ignore the maximum number of open
+	zones limit of the zoned block device in use, thus allowing the
+	option :option:`max_open_zones` value to be larger than the device
+	reported limit. Default: false.
+
 .. option:: zone_reset_threshold=float
 
 	A number between zero and one that indicates the ratio of logical
diff --git a/fio.1 b/fio.1
index a3ebb67d..e0458c22 100644
--- a/fio.1
+++ b/fio.1
@@ -838,9 +838,9 @@ threads/processes.
 Limit on the number of simultaneously opened zones per single thread/process.
 .TP
 .BI ignore_zone_limits \fR=\fPbool
-If this isn't set, fio will query the max open zones limit from the zoned block
-device, and exit if the specified \fBmax_open_zones\fR value is larger than the
-limit reported by the device. Default: false.
+If this option is used, fio will ignore the maximum number of open zones limit
+of the zoned block device in use, thus allowing the option \fBmax_open_zones\fR
+value to be larger than the device reported limit. Default: false.
 .TP
 .BI zone_reset_threshold \fR=\fPfloat
 A number between zero and one that indicates the ratio of logical blocks with
-- 
2.31.1

Re: [PATCH 01/11] fio: Improve documentation of ignore_zone_limits option

[Niklas Cassel]

On Fri, Dec 10, 2021 at 11:15:31AM +0900, Damien Le Moal wrote:
> In the manual pages, change the description of the option
> ignore_zone_limits to its action when set, instead of the confusing text
> describing what happens when it is not set. Also add the description
> of this option in the HOWTO file as it is missing.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

[PATCH 02/11] zbd: define local functions as static

[Damien Le Moal]

Define zbd_get_zoned_model(), zbd_report_zones(), zbd_reset_wp() and
zbd_get_max_open_zones() as static since these functions are used
locally only.

No functional changes.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 zbd.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/zbd.c b/zbd.c
index c18998c4..44e77227 100644
--- a/zbd.c
+++ b/zbd.c
@@ -27,8 +27,8 @@
  * @td: FIO thread data
  * @f: FIO file for which to get model information
  */
-int zbd_get_zoned_model(struct thread_data *td, struct fio_file *f,
-			enum zbd_zoned_model *model)
+static int zbd_get_zoned_model(struct thread_data *td, struct fio_file *f,
+			       enum zbd_zoned_model *model)
 {
 	int ret;
 
@@ -71,9 +71,9 @@ int zbd_get_zoned_model(struct thread_data *td, struct fio_file *f,
  * upon failure. If the zone report is empty, always assume an error (device
  * problem) and return -EIO.
  */
-int zbd_report_zones(struct thread_data *td, struct fio_file *f,
-		     uint64_t offset, struct zbd_zone *zones,
-		     unsigned int nr_zones)
+static int zbd_report_zones(struct thread_data *td, struct fio_file *f,
+			    uint64_t offset, struct zbd_zone *zones,
+			    unsigned int nr_zones)
 {
 	int ret;
 
@@ -105,8 +105,8 @@ int zbd_report_zones(struct thread_data *td, struct fio_file *f,
  * Reset the write pointer of all zones in the range @offset...@offset+@length.
  * Returns 0 upon success and a negative error code upon failure.
  */
-int zbd_reset_wp(struct thread_data *td, struct fio_file *f,
-		 uint64_t offset, uint64_t length)
+static int zbd_reset_wp(struct thread_data *td, struct fio_file *f,
+			uint64_t offset, uint64_t length)
 {
 	int ret;
 
@@ -133,8 +133,8 @@ int zbd_reset_wp(struct thread_data *td, struct fio_file *f,
  *
  * Returns 0 upon success and a negative error code upon failure.
  */
-int zbd_get_max_open_zones(struct thread_data *td, struct fio_file *f,
-			   unsigned int *max_open_zones)
+static int zbd_get_max_open_zones(struct thread_data *td, struct fio_file *f,
+				  unsigned int *max_open_zones)
 {
 	int ret;
 
-- 
2.31.1

Re: [PATCH 02/11] zbd: define local functions as static

[Niklas Cassel]

On Fri, Dec 10, 2021 at 11:15:32AM +0900, Damien Le Moal wrote:
> Define zbd_get_zoned_model(), zbd_report_zones(), zbd_reset_wp() and
> zbd_get_max_open_zones() as static since these functions are used
> locally only.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

[PATCH 03/11] zbd: move and cleanup code

[Damien Le Moal]

Move zone manipulation helper functions at the beginning of the zbd.c
file to avoid forward declarations and to group these functions
together apart from the IO manipulation functions.
Also fix function comments.

No functional changes.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 zbd.c | 582 ++++++++++++++++++++++++++++++----------------------------
 1 file changed, 301 insertions(+), 281 deletions(-)

diff --git a/zbd.c b/zbd.c
index 44e77227..20d53b15 100644
--- a/zbd.c
+++ b/zbd.c
@@ -22,6 +22,112 @@
 #include "pshared.h"
 #include "zbd.h"
 
+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 unsigned int zbd_zone_nr(const struct fio_file *f,
+				       struct fio_zone_info *zone)
+{
+	return zone - f->zbd_info->zone_info;
+}
+
+/**
+ * zbd_zone_idx - convert an offset into a zone number
+ * @f: file pointer.
+ * @offset: offset in bytes. If this offset is in the first zone_size bytes
+ *	    past the disk size then the index of the sentinel is returned.
+ */
+static uint32_t zbd_zone_idx(const struct fio_file *f, uint64_t offset)
+{
+	uint32_t zone_idx;
+
+	if (f->zbd_info->zone_size_log2 > 0)
+		zone_idx = offset >> f->zbd_info->zone_size_log2;
+	else
+		zone_idx = offset / f->zbd_info->zone_size;
+
+	return min(zone_idx, f->zbd_info->nr_zones);
+}
+
+/**
+ * zbd_zone_end - Return zone end location
+ * @z: zone info pointer.
+ */
+static inline uint64_t zbd_zone_end(const struct fio_zone_info *z)
+{
+	return (z+1)->start;
+}
+
+/**
+ * zbd_zone_capacity_end - Return zone capacity limit end location
+ * @z: zone info pointer.
+ */
+static inline uint64_t zbd_zone_capacity_end(const struct fio_zone_info *z)
+{
+	return z->start + z->capacity;
+}
+
+/**
+ * zbd_zone_full - verify whether a minimum number of bytes remain in a zone
+ * @f: file pointer.
+ * @z: zone info pointer.
+ * @required: minimum number of bytes that must remain in a zone.
+ *
+ * The caller must hold z->mutex.
+ */
+static bool zbd_zone_full(const struct fio_file *f, struct fio_zone_info *z,
+			  uint64_t required)
+{
+	assert((required & 511) == 0);
+
+	return z->has_wp &&
+		z->wp + required > zbd_zone_capacity_end(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;
+
+	/* 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.
+	 * To avoid multiple jobs doing asynchronous I/Os from deadlocking each
+	 * other waiting for zone locks when building an io_u batch, first
+	 * only trylock the zone. If the zone is already locked by another job,
+	 * process the currently queued I/Os so that I/O progress is made and
+	 * zones unlocked.
+	 */
+	if (pthread_mutex_trylock(&z->mutex) != 0) {
+		if (!td_ioengine_flagged(td, FIO_SYNCIO))
+			io_u_quiesce(td);
+		pthread_mutex_lock(&z->mutex);
+	}
+}
+
+static inline void zone_unlock(struct fio_zone_info *z)
+{
+	int ret;
+
+	assert(z->has_wp);
+	ret = pthread_mutex_unlock(&z->mutex);
+	assert(!ret);
+}
+
+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];
+}
+
 /**
  * zbd_get_zoned_model - Get a device zoned model
  * @td: FIO thread data
@@ -123,6 +229,126 @@ static int zbd_reset_wp(struct thread_data *td, struct fio_file *f,
 	return ret;
 }
 
+/**
+ * zbd_reset_zone - reset the write pointer of a single zone
+ * @td: FIO thread data.
+ * @f: FIO file associated with the disk for which to reset a write pointer.
+ * @z: Zone to reset.
+ *
+ * Returns 0 upon success and a negative error code upon failure.
+ *
+ * The caller must hold z->mutex.
+ */
+static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
+			  struct fio_zone_info *z)
+{
+	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 (!data_in_zone)
+		return 0;
+
+	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, z));
+	switch (f->zbd_info->model) {
+	case ZBD_HOST_AWARE:
+	case ZBD_HOST_MANAGED:
+		ret = zbd_reset_wp(td, f, offset, length);
+		if (ret < 0)
+			return ret;
+		break;
+	default:
+		break;
+	}
+
+	pthread_mutex_lock(&f->zbd_info->mutex);
+	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;
+
+	td->ts.nr_zone_resets++;
+
+	return ret;
+}
+
+/**
+ * zbd_close_zone - Remove a zone from the open zones array.
+ * @td: FIO thread data.
+ * @f: FIO file associated with the disk for which to reset a write pointer.
+ * @zone_idx: Index of the zone to remove.
+ *
+ * The caller must hold f->zbd_info->mutex.
+ */
+static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
+			   unsigned int zone_idx)
+{
+	uint32_t open_zone_idx = 0;
+
+	for (; open_zone_idx < f->zbd_info->num_open_zones; open_zone_idx++) {
+		if (f->zbd_info->open_zones[open_zone_idx] == zone_idx)
+			break;
+	}
+	if (open_zone_idx == f->zbd_info->num_open_zones)
+		return;
+
+	dprint(FD_ZBD, "%s: closing zone %d\n", f->file_name, zone_idx);
+	memmove(f->zbd_info->open_zones + open_zone_idx,
+		f->zbd_info->open_zones + open_zone_idx + 1,
+		(ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
+		sizeof(f->zbd_info->open_zones[0]));
+	f->zbd_info->num_open_zones--;
+	td->num_open_zones--;
+	get_zone(f, zone_idx)->open = 0;
+}
+
+/**
+ * zbd_reset_zones - Reset a range of zones.
+ * @td: fio thread data.
+ * @f: fio file for which to reset zones
+ * @zb: first zone to reset.
+ * @ze: first zone not to reset.
+ *
+ * Returns 0 upon success and 1 upon failure.
+ */
+static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
+			   struct fio_zone_info *const zb,
+			   struct fio_zone_info *const ze)
+{
+	struct fio_zone_info *z;
+	const uint64_t min_bs = td->o.min_bs[DDIR_WRITE];
+	int res = 0;
+
+	assert(min_bs);
+
+	dprint(FD_ZBD, "%s: examining zones %u .. %u\n", f->file_name,
+		zbd_zone_nr(f, zb), zbd_zone_nr(f, ze));
+	for (z = zb; z < ze; z++) {
+		uint32_t nz = zbd_zone_nr(f, z);
+
+		if (!z->has_wp)
+			continue;
+		zone_lock(td, f, z);
+		pthread_mutex_lock(&f->zbd_info->mutex);
+		zbd_close_zone(td, f, nz);
+		pthread_mutex_unlock(&f->zbd_info->mutex);
+		if (z->wp != z->start) {
+			dprint(FD_ZBD, "%s: resetting zone %u\n",
+			       f->file_name, zbd_zone_nr(f, z));
+			if (zbd_reset_zone(td, f, z) < 0)
+				res = 1;
+		}
+		zone_unlock(z);
+	}
+
+	return res;
+}
+
 /**
  * zbd_get_max_open_zones - Get the maximum number of open zones
  * @td: FIO thread data
@@ -152,103 +378,99 @@ static int zbd_get_max_open_zones(struct thread_data *td, struct fio_file *f,
 }
 
 /**
- * zbd_zone_idx - convert an offset into a zone number
- * @f: file pointer.
- * @offset: offset in bytes. If this offset is in the first zone_size bytes
- *	    past the disk size then the index of the sentinel is returned.
+ * is_zone_open - Test if a zone is already in the array of open zones.
+ * @td: fio thread data.
+ * @f: fio file for which to test zones.
+ * @zone_idx: Index of the zone to check.
+ *
+ * The caller must hold f->zbd_info->mutex.
  */
-static uint32_t zbd_zone_idx(const struct fio_file *f, uint64_t offset)
+static bool is_zone_open(const struct thread_data *td, const struct fio_file *f,
+			 unsigned int zone_idx)
 {
-	uint32_t zone_idx;
-
-	if (f->zbd_info->zone_size_log2 > 0)
-		zone_idx = offset >> f->zbd_info->zone_size_log2;
-	else
-		zone_idx = offset / f->zbd_info->zone_size;
+	struct zoned_block_device_info *zbdi = f->zbd_info;
+	int i;
 
-	return min(zone_idx, f->zbd_info->nr_zones);
-}
+	/*
+	 * This function should never be called when zbdi->max_open_zones == 0.
+	 */
+	assert(zbdi->max_open_zones);
+	assert(td->o.job_max_open_zones == 0 ||
+	       td->num_open_zones <= td->o.job_max_open_zones);
+	assert(td->o.job_max_open_zones <= zbdi->max_open_zones);
+	assert(zbdi->num_open_zones <= zbdi->max_open_zones);
 
-/**
- * zbd_zone_end - Return zone end location
- * @z: zone info pointer.
- */
-static inline uint64_t zbd_zone_end(const struct fio_zone_info *z)
-{
-	return (z+1)->start;
-}
+	for (i = 0; i < zbdi->num_open_zones; i++)
+		if (zbdi->open_zones[i] == zone_idx)
+			return true;
 
-/**
- * zbd_zone_capacity_end - Return zone capacity limit end location
- * @z: zone info pointer.
- */
-static inline uint64_t zbd_zone_capacity_end(const struct fio_zone_info *z)
-{
-	return z->start + z->capacity;
+	return false;
 }
 
 /**
- * zbd_zone_full - verify whether a minimum number of bytes remain in a zone
- * @f: file pointer.
- * @z: zone info pointer.
- * @required: minimum number of bytes that must remain in a zone.
+ * zbd_open_zone - Add a zone to the array of open zones.
+ * @td: fio thread data.
+ * @f: fio file that has the open zones to add.
+ * @zone_idx: Index of the zone to add.
  *
- * The caller must hold z->mutex.
+ * Open a ZBD zone if it is not already open. Returns true if either the zone
+ * was already open or if the zone was successfully added to the array of open
+ * zones without exceeding the maximum number of open zones. Returns false if
+ * the zone was not already open and opening the zone would cause the zone limit
+ * to be exceeded.
  */
-static bool zbd_zone_full(const struct fio_file *f, struct fio_zone_info *z,
-			  uint64_t required)
-{
-	assert((required & 511) == 0);
-
-	return z->has_wp &&
-		z->wp + required > zbd_zone_capacity_end(z);
-}
-
-static void zone_lock(struct thread_data *td, const struct fio_file *f,
-		      struct fio_zone_info *z)
+static bool zbd_open_zone(struct thread_data *td, const struct fio_file *f,
+			  uint32_t zone_idx)
 {
-	struct zoned_block_device_info *zbd = f->zbd_info;
-	uint32_t nz = z - zbd->zone_info;
-
-	/* A thread should never lock zones outside its working area. */
-	assert(f->min_zone <= nz && nz < f->max_zone);
+	const uint64_t min_bs = td->o.min_bs[DDIR_WRITE];
+	struct zoned_block_device_info *zbdi = f->zbd_info;
+	struct fio_zone_info *z = get_zone(f, zone_idx);
+	bool res = true;
 
-	assert(z->has_wp);
+	if (z->cond == ZBD_ZONE_COND_OFFLINE)
+		return false;
 
 	/*
-	 * 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.
-	 * To avoid multiple jobs doing asynchronous I/Os from deadlocking each
-	 * other waiting for zone locks when building an io_u batch, first
-	 * only trylock the zone. If the zone is already locked by another job,
-	 * process the currently queued I/Os so that I/O progress is made and
-	 * zones unlocked.
+	 * Skip full zones with data verification enabled because resetting a
+	 * zone causes data loss and hence causes verification to fail.
 	 */
-	if (pthread_mutex_trylock(&z->mutex) != 0) {
-		if (!td_ioengine_flagged(td, FIO_SYNCIO))
-			io_u_quiesce(td);
-		pthread_mutex_lock(&z->mutex);
-	}
-}
-
-static inline void zone_unlock(struct fio_zone_info *z)
-{
-	int ret;
+	if (td->o.verify != VERIFY_NONE && zbd_zone_full(f, z, min_bs))
+		return false;
 
-	assert(z->has_wp);
-	ret = pthread_mutex_unlock(&z->mutex);
-	assert(!ret);
-}
+	/*
+	 * zbdi->max_open_zones == 0 means that there is no limit on the maximum
+	 * number of open zones. In this case, do no track open zones in
+	 * zbdi->open_zones array.
+	 */
+	if (!zbdi->max_open_zones)
+		return true;
 
-static bool is_valid_offset(const struct fio_file *f, uint64_t offset)
-{
-	return (uint64_t)(offset - f->file_offset) < f->io_size;
-}
+	pthread_mutex_lock(&zbdi->mutex);
+	if (is_zone_open(td, f, zone_idx)) {
+		/*
+		 * If the zone is already open and going to be full by writes
+		 * in-flight, handle it as a full zone instead of an open zone.
+		 */
+		if (z->wp >= zbd_zone_capacity_end(z))
+			res = false;
+		goto out;
+	}
+	res = false;
+	/* Zero means no limit */
+	if (td->o.job_max_open_zones > 0 &&
+	    td->num_open_zones >= td->o.job_max_open_zones)
+		goto out;
+	if (zbdi->num_open_zones >= zbdi->max_open_zones)
+		goto out;
+	dprint(FD_ZBD, "%s: opening zone %d\n", f->file_name, zone_idx);
+	zbdi->open_zones[zbdi->num_open_zones++] = zone_idx;
+	td->num_open_zones++;
+	z->open = 1;
+	res = true;
 
-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];
+out:
+	pthread_mutex_unlock(&zbdi->mutex);
+	return res;
 }
 
 /* Verify whether direct I/O is used for all host-managed zoned drives. */
@@ -751,11 +973,6 @@ static int zbd_init_zone_info(struct thread_data *td, struct fio_file *file)
 	return ret;
 }
 
-static bool zbd_open_zone(struct thread_data *td, const struct fio_file *f,
-			  uint32_t zone_idx);
-static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
-			  struct fio_zone_info *z);
-
 int zbd_init_files(struct thread_data *td)
 {
 	struct fio_file *f;
@@ -879,123 +1096,6 @@ int zbd_setup_files(struct thread_data *td)
 	return 0;
 }
 
-static inline unsigned int zbd_zone_nr(const struct fio_file *f,
-				       struct fio_zone_info *zone)
-{
-	return zone - f->zbd_info->zone_info;
-}
-
-/**
- * zbd_reset_zone - reset the write pointer of a single zone
- * @td: FIO thread data.
- * @f: FIO file associated with the disk for which to reset a write pointer.
- * @z: Zone to reset.
- *
- * Returns 0 upon success and a negative error code upon failure.
- *
- * The caller must hold z->mutex.
- */
-static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
-			  struct fio_zone_info *z)
-{
-	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 (!data_in_zone)
-		return 0;
-
-	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, z));
-	switch (f->zbd_info->model) {
-	case ZBD_HOST_AWARE:
-	case ZBD_HOST_MANAGED:
-		ret = zbd_reset_wp(td, f, offset, length);
-		if (ret < 0)
-			return ret;
-		break;
-	default:
-		break;
-	}
-
-	pthread_mutex_lock(&f->zbd_info->mutex);
-	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;
-
-	td->ts.nr_zone_resets++;
-
-	return ret;
-}
-
-/* The caller must hold f->zbd_info->mutex */
-static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
-			   unsigned int zone_idx)
-{
-	uint32_t open_zone_idx = 0;
-
-	for (; open_zone_idx < f->zbd_info->num_open_zones; open_zone_idx++) {
-		if (f->zbd_info->open_zones[open_zone_idx] == zone_idx)
-			break;
-	}
-	if (open_zone_idx == f->zbd_info->num_open_zones)
-		return;
-
-	dprint(FD_ZBD, "%s: closing zone %d\n", f->file_name, zone_idx);
-	memmove(f->zbd_info->open_zones + open_zone_idx,
-		f->zbd_info->open_zones + open_zone_idx + 1,
-		(ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
-		sizeof(f->zbd_info->open_zones[0]));
-	f->zbd_info->num_open_zones--;
-	td->num_open_zones--;
-	get_zone(f, zone_idx)->open = 0;
-}
-
-/*
- * Reset a range of zones. Returns 0 upon success and 1 upon failure.
- * @td: fio thread data.
- * @f: fio file for which to reset zones
- * @zb: first zone to reset.
- * @ze: first zone not to reset.
- */
-static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
-			   struct fio_zone_info *const zb,
-			   struct fio_zone_info *const ze)
-{
-	struct fio_zone_info *z;
-	const uint64_t min_bs = td->o.min_bs[DDIR_WRITE];
-	int res = 0;
-
-	assert(min_bs);
-
-	dprint(FD_ZBD, "%s: examining zones %u .. %u\n", f->file_name,
-		zbd_zone_nr(f, zb), zbd_zone_nr(f, ze));
-	for (z = zb; z < ze; z++) {
-		uint32_t nz = zbd_zone_nr(f, z);
-
-		if (!z->has_wp)
-			continue;
-		zone_lock(td, f, z);
-		pthread_mutex_lock(&f->zbd_info->mutex);
-		zbd_close_zone(td, f, nz);
-		pthread_mutex_unlock(&f->zbd_info->mutex);
-		if (z->wp != z->start) {
-			dprint(FD_ZBD, "%s: resetting zone %u\n",
-			       f->file_name, zbd_zone_nr(f, z));
-			if (zbd_reset_zone(td, f, z) < 0)
-				res = 1;
-		}
-		zone_unlock(z);
-	}
-
-	return res;
-}
-
 /*
  * Reset zbd_info.write_cnt, the counter that counts down towards the next
  * zone reset.
@@ -1112,86 +1212,6 @@ void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 	zbd_reset_write_cnt(td, f);
 }
 
-/* The caller must hold f->zbd_info->mutex. */
-static bool is_zone_open(const struct thread_data *td, const struct fio_file *f,
-			 unsigned int zone_idx)
-{
-	struct zoned_block_device_info *zbdi = f->zbd_info;
-	int i;
-
-	/* This function should never be called when zbdi->max_open_zones == 0 */
-	assert(zbdi->max_open_zones);
-	assert(td->o.job_max_open_zones == 0 || td->num_open_zones <= td->o.job_max_open_zones);
-	assert(td->o.job_max_open_zones <= zbdi->max_open_zones);
-	assert(zbdi->num_open_zones <= zbdi->max_open_zones);
-
-	for (i = 0; i < zbdi->num_open_zones; i++)
-		if (zbdi->open_zones[i] == zone_idx)
-			return true;
-
-	return false;
-}
-
-/*
- * Open a ZBD zone if it was not yet open. Returns true if either the zone was
- * already open or if opening a new zone is allowed. Returns false if the zone
- * was not yet open and opening a new zone would cause the zone limit to be
- * exceeded.
- */
-static bool zbd_open_zone(struct thread_data *td, const struct fio_file *f,
-			  uint32_t zone_idx)
-{
-	const uint64_t min_bs = td->o.min_bs[DDIR_WRITE];
-	struct zoned_block_device_info *zbdi = f->zbd_info;
-	struct fio_zone_info *z = get_zone(f, zone_idx);
-	bool res = true;
-
-	if (z->cond == ZBD_ZONE_COND_OFFLINE)
-		return false;
-
-	/*
-	 * Skip full zones with data verification enabled because resetting a
-	 * zone causes data loss and hence causes verification to fail.
-	 */
-	if (td->o.verify != VERIFY_NONE && zbd_zone_full(f, z, min_bs))
-		return false;
-
-	/*
-	 * zbdi->max_open_zones == 0 means that there is no limit on the maximum
-	 * number of open zones. In this case, do no track open zones in
-	 * zbdi->open_zones array.
-	 */
-	if (!zbdi->max_open_zones)
-		return true;
-
-	pthread_mutex_lock(&zbdi->mutex);
-	if (is_zone_open(td, f, zone_idx)) {
-		/*
-		 * If the zone is already open and going to be full by writes
-		 * in-flight, handle it as a full zone instead of an open zone.
-		 */
-		if (z->wp >= zbd_zone_capacity_end(z))
-			res = false;
-		goto out;
-	}
-	res = false;
-	/* Zero means no limit */
-	if (td->o.job_max_open_zones > 0 &&
-	    td->num_open_zones >= td->o.job_max_open_zones)
-		goto out;
-	if (zbdi->num_open_zones >= zbdi->max_open_zones)
-		goto out;
-	dprint(FD_ZBD, "%s: opening zone %d\n", f->file_name, zone_idx);
-	zbdi->open_zones[zbdi->num_open_zones++] = zone_idx;
-	td->num_open_zones++;
-	z->open = 1;
-	res = true;
-
-out:
-	pthread_mutex_unlock(&zbdi->mutex);
-	return res;
-}
-
 /* Return random zone index for one of the open zones. */
 static uint32_t pick_random_zone_idx(const struct fio_file *f,
 				     const struct io_u *io_u)
-- 
2.31.1

Re: [PATCH 03/11] zbd: move and cleanup code

[Niklas Cassel]

On Fri, Dec 10, 2021 at 11:15:33AM +0900, Damien Le Moal wrote:
> Move zone manipulation helper functions at the beginning of the zbd.c
> file to avoid forward declarations and to group these functions
> together apart from the IO manipulation functions.
> Also fix function comments.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---

Would have been much easier to review this if the function comment
changes were in a separate patch.

Regardless:

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

[PATCH 04/11] zbd: remove is_zone_open() helper

[Damien Le Moal]

The helper function is_zone_open() is useless as a each zone has an open
flag indicating if it is part of the array of open zones. Remove this
function code and use the zone open flag in zbd_open_zone().

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 zbd.c | 38 +++++---------------------------------
 1 file changed, 5 insertions(+), 33 deletions(-)

diff --git a/zbd.c b/zbd.c
index 20d53b15..70afdd82 100644
--- a/zbd.c
+++ b/zbd.c
@@ -377,36 +377,6 @@ static int zbd_get_max_open_zones(struct thread_data *td, struct fio_file *f,
 	return ret;
 }
 
-/**
- * is_zone_open - Test if a zone is already in the array of open zones.
- * @td: fio thread data.
- * @f: fio file for which to test zones.
- * @zone_idx: Index of the zone to check.
- *
- * The caller must hold f->zbd_info->mutex.
- */
-static bool is_zone_open(const struct thread_data *td, const struct fio_file *f,
-			 unsigned int zone_idx)
-{
-	struct zoned_block_device_info *zbdi = f->zbd_info;
-	int i;
-
-	/*
-	 * This function should never be called when zbdi->max_open_zones == 0.
-	 */
-	assert(zbdi->max_open_zones);
-	assert(td->o.job_max_open_zones == 0 ||
-	       td->num_open_zones <= td->o.job_max_open_zones);
-	assert(td->o.job_max_open_zones <= zbdi->max_open_zones);
-	assert(zbdi->num_open_zones <= zbdi->max_open_zones);
-
-	for (i = 0; i < zbdi->num_open_zones; i++)
-		if (zbdi->open_zones[i] == zone_idx)
-			return true;
-
-	return false;
-}
-
 /**
  * zbd_open_zone - Add a zone to the array of open zones.
  * @td: fio thread data.
@@ -446,10 +416,12 @@ static bool zbd_open_zone(struct thread_data *td, const struct fio_file *f,
 		return true;
 
 	pthread_mutex_lock(&zbdi->mutex);
-	if (is_zone_open(td, f, zone_idx)) {
+
+	if (z->open) {
 		/*
-		 * If the zone is already open and going to be full by writes
-		 * in-flight, handle it as a full zone instead of an open zone.
+		 * If the zone is going to be completely filled by writes
+		 * already in-flight, handle it as a full zone instead of an
+		 * open zone.
 		 */
 		if (z->wp >= zbd_zone_capacity_end(z))
 			res = false;
-- 
2.31.1

Re: [PATCH 04/11] zbd: remove is_zone_open() helper

[Niklas Cassel]

On Fri, Dec 10, 2021 at 11:15:34AM +0900, Damien Le Moal wrote:
> The helper function is_zone_open() is useless as a each zone has an open
> flag indicating if it is part of the array of open zones. Remove this
> function code and use the zone open flag in zbd_open_zone().
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

[PATCH 05/11] zbd: introduce zbd_verify_file_sizes()

[Damien Le Moal]

Introduce the helper function zbd_verify_file_sizes() to check whether
the offset and size options are aligned with zone boundaries for a
job/file combination. This avoids large indentation of the code in
zbd_verify_sizes() and makes the code easier to read.

No functional changes.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 zbd.c | 135 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 74 insertions(+), 61 deletions(-)

diff --git a/zbd.c b/zbd.c
index 70afdd82..70073461 100644
--- a/zbd.c
+++ b/zbd.c
@@ -485,76 +485,89 @@ static bool zbd_is_seq_job(struct fio_file *f)
 /*
  * Verify whether offset and size parameters are aligned with zone boundaries.
  */
-static bool zbd_verify_sizes(void)
+static bool zbd_verify_file_sizes(struct thread_data *td, struct fio_file *f)
 {
 	const struct fio_zone_info *z;
-	struct thread_data *td;
-	struct fio_file *f;
 	uint64_t new_offset, new_end;
 	uint32_t zone_idx;
+
+	if (!f->zbd_info)
+		return true;
+	if (f->file_offset >= f->real_file_size)
+		return true;
+	if (!zbd_is_seq_job(f))
+		return true;
+
+	if (!td->o.zone_size) {
+		td->o.zone_size = f->zbd_info->zone_size;
+		if (!td->o.zone_size) {
+			log_err("%s: invalid 0 zone size\n",
+				f->file_name);
+			return false;
+		}
+	} else if (td->o.zone_size != f->zbd_info->zone_size) {
+		log_err("%s: zonesize %llu does not match the device zone size %"PRIu64".\n",
+			f->file_name, td->o.zone_size,
+			f->zbd_info->zone_size);
+		return false;
+	}
+
+	if (td->o.zone_skip % td->o.zone_size) {
+		log_err("%s: zoneskip %llu is not a multiple of the device zone size %llu.\n",
+			f->file_name, td->o.zone_skip,
+			td->o.zone_size);
+		return false;
+	}
+
+	zone_idx = zbd_zone_idx(f, f->file_offset);
+	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);
+		if (new_offset >= f->file_offset + f->io_size) {
+			log_info("%s: io_size must be at least one zone\n",
+				 f->file_name);
+			return false;
+		}
+		log_info("%s: rounded up offset from %"PRIu64" to %"PRIu64"\n",
+			 f->file_name, f->file_offset,
+			 new_offset);
+		f->io_size -= (new_offset - f->file_offset);
+		f->file_offset = new_offset;
+	}
+
+	zone_idx = zbd_zone_idx(f, f->file_offset + f->io_size);
+	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)) {
+		if (new_end <= f->file_offset) {
+			log_info("%s: io_size must be at least one zone\n",
+				 f->file_name);
+			return false;
+		}
+		log_info("%s: rounded down io_size from %"PRIu64" to %"PRIu64"\n",
+			 f->file_name, f->io_size,
+			 new_end - f->file_offset);
+		f->io_size = new_end - f->file_offset;
+	}
+
+	return true;
+}
+
+/*
+ * Verify whether offset and size parameters are aligned with zone boundaries.
+ */
+static bool zbd_verify_sizes(void)
+{
+	struct thread_data *td;
+	struct fio_file *f;
 	int i, j;
 
 	for_each_td(td, i) {
 		for_each_file(td, f, j) {
-			if (!f->zbd_info)
-				continue;
-			if (f->file_offset >= f->real_file_size)
-				continue;
-			if (!zbd_is_seq_job(f))
-				continue;
-
-			if (!td->o.zone_size) {
-				td->o.zone_size = f->zbd_info->zone_size;
-				if (!td->o.zone_size) {
-					log_err("%s: invalid 0 zone size\n",
-						f->file_name);
-					return false;
-				}
-			} else if (td->o.zone_size != f->zbd_info->zone_size) {
-				log_err("%s: job parameter zonesize %llu does not match disk zone size %"PRIu64".\n",
-					f->file_name, td->o.zone_size,
-					f->zbd_info->zone_size);
-				return false;
-			}
-
-			if (td->o.zone_skip % td->o.zone_size) {
-				log_err("%s: zoneskip %llu is not a multiple of the device zone size %llu.\n",
-					f->file_name, td->o.zone_skip,
-					td->o.zone_size);
+			if (!zbd_verify_file_sizes(td, f))
 				return false;
-			}
-
-			zone_idx = zbd_zone_idx(f, f->file_offset);
-			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);
-				if (new_offset >= f->file_offset + f->io_size) {
-					log_info("%s: io_size must be at least one zone\n",
-						 f->file_name);
-					return false;
-				}
-				log_info("%s: rounded up offset from %"PRIu64" to %"PRIu64"\n",
-					 f->file_name, f->file_offset,
-					 new_offset);
-				f->io_size -= (new_offset - f->file_offset);
-				f->file_offset = new_offset;
-			}
-			zone_idx = zbd_zone_idx(f, f->file_offset + f->io_size);
-			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)) {
-				if (new_end <= f->file_offset) {
-					log_info("%s: io_size must be at least one zone\n",
-						 f->file_name);
-					return false;
-				}
-				log_info("%s: rounded down io_size from %"PRIu64" to %"PRIu64"\n",
-					 f->file_name, f->io_size,
-					 new_end - f->file_offset);
-				f->io_size = new_end - f->file_offset;
-			}
 		}
 	}
 
-- 
2.31.1

Re: [PATCH 05/11] zbd: introduce zbd_verify_file_sizes()

[Niklas Cassel]

On Fri, Dec 10, 2021 at 11:15:35AM +0900, Damien Le Moal wrote:
> Introduce the helper function zbd_verify_file_sizes() to check whether
> the offset and size options are aligned with zone boundaries for a
> job/file combination. This avoids large indentation of the code in
> zbd_verify_sizes() and makes the code easier to read.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  zbd.c | 135 ++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 74 insertions(+), 61 deletions(-)
> 
> diff --git a/zbd.c b/zbd.c
> index 70afdd82..70073461 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -485,76 +485,89 @@ static bool zbd_is_seq_job(struct fio_file *f)
>  /*
>   * Verify whether offset and size parameters are aligned with zone boundaries.
>   */
> -static bool zbd_verify_sizes(void)
> +static bool zbd_verify_file_sizes(struct thread_data *td, struct fio_file *f)
>  {
>  	const struct fio_zone_info *z;
> -	struct thread_data *td;
> -	struct fio_file *f;
>  	uint64_t new_offset, new_end;
>  	uint32_t zone_idx;
> +
> +	if (!f->zbd_info)
> +		return true;
> +	if (f->file_offset >= f->real_file_size)
> +		return true;
> +	if (!zbd_is_seq_job(f))
> +		return true;
> +
> +	if (!td->o.zone_size) {
> +		td->o.zone_size = f->zbd_info->zone_size;
> +		if (!td->o.zone_size) {
> +			log_err("%s: invalid 0 zone size\n",
> +				f->file_name);
> +			return false;
> +		}
> +	} else if (td->o.zone_size != f->zbd_info->zone_size) {
> +		log_err("%s: zonesize %llu does not match the device zone size %"PRIu64".\n",
> +			f->file_name, td->o.zone_size,
> +			f->zbd_info->zone_size);
> +		return false;
> +	}
> +
> +	if (td->o.zone_skip % td->o.zone_size) {
> +		log_err("%s: zoneskip %llu is not a multiple of the device zone size %llu.\n",
> +			f->file_name, td->o.zone_skip,
> +			td->o.zone_size);
> +		return false;
> +	}
> +
> +	zone_idx = zbd_zone_idx(f, f->file_offset);
> +	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);
> +		if (new_offset >= f->file_offset + f->io_size) {
> +			log_info("%s: io_size must be at least one zone\n",
> +				 f->file_name);
> +			return false;
> +		}
> +		log_info("%s: rounded up offset from %"PRIu64" to %"PRIu64"\n",
> +			 f->file_name, f->file_offset,
> +			 new_offset);
> +		f->io_size -= (new_offset - f->file_offset);
> +		f->file_offset = new_offset;
> +	}
> +
> +	zone_idx = zbd_zone_idx(f, f->file_offset + f->io_size);
> +	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)) {
> +		if (new_end <= f->file_offset) {
> +			log_info("%s: io_size must be at least one zone\n",
> +				 f->file_name);
> +			return false;
> +		}
> +		log_info("%s: rounded down io_size from %"PRIu64" to %"PRIu64"\n",
> +			 f->file_name, f->io_size,
> +			 new_end - f->file_offset);
> +		f->io_size = new_end - f->file_offset;
> +	}
> +
> +	return true;
> +}
> +
> +/*
> + * Verify whether offset and size parameters are aligned with zone boundaries.
> + */
> +static bool zbd_verify_sizes(void)
> +{
> +	struct thread_data *td;
> +	struct fio_file *f;
>  	int i, j;
>  
>  	for_each_td(td, i) {
>  		for_each_file(td, f, j) {
> -			if (!f->zbd_info)
> -				continue;
> -			if (f->file_offset >= f->real_file_size)
> -				continue;
> -			if (!zbd_is_seq_job(f))
> -				continue;
> -
> -			if (!td->o.zone_size) {
> -				td->o.zone_size = f->zbd_info->zone_size;
> -				if (!td->o.zone_size) {
> -					log_err("%s: invalid 0 zone size\n",
> -						f->file_name);
> -					return false;
> -				}
> -			} else if (td->o.zone_size != f->zbd_info->zone_size) {
> -				log_err("%s: job parameter zonesize %llu does not match disk zone size %"PRIu64".\n",
> -					f->file_name, td->o.zone_size,
> -					f->zbd_info->zone_size);
> -				return false;
> -			}
> -
> -			if (td->o.zone_skip % td->o.zone_size) {
> -				log_err("%s: zoneskip %llu is not a multiple of the device zone size %llu.\n",
> -					f->file_name, td->o.zone_skip,
> -					td->o.zone_size);
> +			if (!zbd_verify_file_sizes(td, f))

Just looking at this, I would assume that zbd_verify_file_sizes() returns 0 on
success.

Perhaps rename zbd_verify_file_sizes to something like:
zbd_file_is_zone_aligned()

then it is more obvious from the name that it returns true on success.


Kind regards,
Niklas

[PATCH 06/11] zbd: fix code style issues

[Damien Le Moal]

Avoid overly long lines, remove unnecessary curly brackets and add blank
lines to make the code more readable.

No functional changes.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 zbd.c | 177 +++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 125 insertions(+), 52 deletions(-)

diff --git a/zbd.c b/zbd.c
index 70073461..6a26269d 100644
--- a/zbd.c
+++ b/zbd.c
@@ -252,8 +252,9 @@ 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, z));
+	dprint(FD_ZBD, "%s: resetting wp of zone %u.\n",
+	       f->file_name, zbd_zone_nr(f, z));
+
 	switch (f->zbd_info->model) {
 	case ZBD_HOST_AWARE:
 	case ZBD_HOST_MANAGED:
@@ -269,6 +270,7 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
 	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;
 
@@ -297,11 +299,14 @@ static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
 	if (open_zone_idx == f->zbd_info->num_open_zones)
 		return;
 
-	dprint(FD_ZBD, "%s: closing zone %d\n", f->file_name, zone_idx);
+	dprint(FD_ZBD, "%s: closing zone %d\n",
+	       f->file_name, zone_idx);
+
 	memmove(f->zbd_info->open_zones + open_zone_idx,
 		f->zbd_info->open_zones + open_zone_idx + 1,
 		(ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
 		sizeof(f->zbd_info->open_zones[0]));
+
 	f->zbd_info->num_open_zones--;
 	td->num_open_zones--;
 	get_zone(f, zone_idx)->open = 0;
@@ -326,23 +331,27 @@ 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, zb), zbd_zone_nr(f, ze));
+	dprint(FD_ZBD, "%s: examining zones %u .. %u\n",
+	       f->file_name, zbd_zone_nr(f, zb), zbd_zone_nr(f, ze));
+
 	for (z = zb; z < ze; z++) {
 		uint32_t nz = zbd_zone_nr(f, z);
 
 		if (!z->has_wp)
 			continue;
+
 		zone_lock(td, f, z);
 		pthread_mutex_lock(&f->zbd_info->mutex);
 		zbd_close_zone(td, f, nz);
 		pthread_mutex_unlock(&f->zbd_info->mutex);
+
 		if (z->wp != z->start) {
 			dprint(FD_ZBD, "%s: resetting zone %u\n",
 			       f->file_name, zbd_zone_nr(f, z));
 			if (zbd_reset_zone(td, f, z) < 0)
 				res = 1;
 		}
+
 		zone_unlock(z);
 	}
 
@@ -427,6 +436,7 @@ static bool zbd_open_zone(struct thread_data *td, const struct fio_file *f,
 			res = false;
 		goto out;
 	}
+
 	res = false;
 	/* Zero means no limit */
 	if (td->o.job_max_open_zones > 0 &&
@@ -434,7 +444,10 @@ static bool zbd_open_zone(struct thread_data *td, const struct fio_file *f,
 		goto out;
 	if (zbdi->num_open_zones >= zbdi->max_open_zones)
 		goto out;
-	dprint(FD_ZBD, "%s: opening zone %d\n", f->file_name, zone_idx);
+
+	dprint(FD_ZBD, "%s: opening zone %d\n",
+	       f->file_name, zone_idx);
+
 	zbdi->open_zones[zbdi->num_open_zones++] = zone_idx;
 	td->num_open_zones++;
 	z->open = 1;
@@ -471,8 +484,10 @@ static bool zbd_is_seq_job(struct fio_file *f)
 	uint32_t zone_idx, zone_idx_b, zone_idx_e;
 
 	assert(f->zbd_info);
+
 	if (f->io_size == 0)
 		return false;
+
 	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++)
@@ -592,6 +607,7 @@ static bool zbd_verify_bs(void)
 
 			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 %"PRIu64"\n",
@@ -736,8 +752,8 @@ static int parse_zone_info(struct thread_data *td, struct fio_file *f)
 		goto out;
 	}
 
-	dprint(FD_ZBD, "Device %s has %d zones of size %"PRIu64" KB\n", f->file_name,
-	       nr_zones, zone_size / 1024);
+	dprint(FD_ZBD, "Device %s has %d zones of size %"PRIu64" KB\n",
+	       f->file_name, nr_zones, zone_size / 1024);
 
 	zbd_info = scalloc(1, sizeof(*zbd_info) +
 			   (nr_zones + 1) * sizeof(zbd_info->zone_info[0]));
@@ -753,6 +769,7 @@ static int parse_zone_info(struct thread_data *td, struct fio_file *f)
 						     PTHREAD_MUTEX_RECURSIVE);
 			p->start = z->start;
 			p->capacity = z->capacity;
+
 			switch (z->cond) {
 			case ZBD_ZONE_COND_NOT_WP:
 			case ZBD_ZONE_COND_FULL:
@@ -786,6 +803,7 @@ 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,
 				       min((uint32_t)(nr_zones - j),
 					   ZBD_REPORT_MAX_ZONES));
@@ -853,7 +871,8 @@ out:
 	/* Ensure that the limit is not larger than FIO's internal limit */
 	if (zbd->max_open_zones > ZBD_MAX_OPEN_ZONES) {
 		td_verror(td, EINVAL, "'max_open_zones' value is too large");
-		log_err("'max_open_zones' value is larger than %u\n", ZBD_MAX_OPEN_ZONES);
+		log_err("'max_open_zones' value is larger than %u\n",
+			ZBD_MAX_OPEN_ZONES);
 		return -EINVAL;
 	}
 
@@ -955,6 +974,7 @@ static int zbd_init_zone_info(struct thread_data *td, struct fio_file *file)
 	ret = zbd_create_zone_info(td, file);
 	if (ret < 0)
 		td_verror(td, -ret, "zbd_create_zone_info() failed");
+
 	return ret;
 }
 
@@ -967,6 +987,7 @@ int zbd_init_files(struct thread_data *td)
 		if (zbd_init_zone_info(td, f))
 			return 1;
 	}
+
 	return 0;
 }
 
@@ -977,27 +998,24 @@ void zbd_recalc_options_with_zone_granularity(struct thread_data *td)
 
 	for_each_file(td, f, i) {
 		struct zoned_block_device_info *zbd = f->zbd_info;
-		// zonemode=strided doesn't get per-file zone size.
-		uint64_t zone_size = zbd ? zbd->zone_size : td->o.zone_size;
+		uint64_t zone_size;
 
+		/* zonemode=strided doesn't get per-file zone size. */
+		zone_size = zbd ? zbd->zone_size : td->o.zone_size;
 		if (zone_size == 0)
 			continue;
 
-		if (td->o.size_nz > 0) {
+		if (td->o.size_nz > 0)
 			td->o.size = td->o.size_nz * zone_size;
-		}
-		if (td->o.io_size_nz > 0) {
+		if (td->o.io_size_nz > 0)
 			td->o.io_size = td->o.io_size_nz * zone_size;
-		}
-		if (td->o.start_offset_nz > 0) {
+		if (td->o.start_offset_nz > 0)
 			td->o.start_offset = td->o.start_offset_nz * zone_size;
-		}
-		if (td->o.offset_increment_nz > 0) {
-			td->o.offset_increment = td->o.offset_increment_nz * zone_size;
-		}
-		if (td->o.zone_skip_nz > 0) {
+		if (td->o.offset_increment_nz > 0)
+			td->o.offset_increment =
+				td->o.offset_increment_nz * zone_size;
+		if (td->o.zone_skip_nz > 0)
 			td->o.zone_skip = td->o.zone_skip_nz * zone_size;
-		}
 	}
 }
 
@@ -1140,6 +1158,7 @@ static uint64_t zbd_process_swd(struct thread_data *td,
 		}
 		swd += z->wp - z->start;
 	}
+
 	pthread_mutex_lock(&f->zbd_info->mutex);
 	switch (a) {
 	case CHECK_SWD:
@@ -1152,6 +1171,7 @@ static uint64_t zbd_process_swd(struct thread_data *td,
 		break;
 	}
 	pthread_mutex_unlock(&f->zbd_info->mutex);
+
 	for (z = zb; z < ze; z++)
 		if (z->has_wp)
 			zone_unlock(z);
@@ -1185,8 +1205,10 @@ void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 	zb = get_zone(f, f->min_zone);
 	ze = get_zone(f, f->max_zone);
 	swd = zbd_process_swd(td, f, SET_SWD);
-	dprint(FD_ZBD, "%s(%s): swd = %" PRIu64 "\n", __func__, f->file_name,
-	       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
@@ -1201,8 +1223,8 @@ void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 static uint32_t pick_random_zone_idx(const struct fio_file *f,
 				     const struct io_u *io_u)
 {
-	return (io_u->offset - f->file_offset) * f->zbd_info->num_open_zones /
-		f->io_size;
+	return (io_u->offset - f->file_offset) *
+		f->zbd_info->num_open_zones / f->io_size;
 }
 
 static bool any_io_in_flight(void)
@@ -1255,7 +1277,9 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
 		zone_idx = f->min_zone;
 	else if (zone_idx >= f->max_zone)
 		zone_idx = f->max_zone - 1;
-	dprint(FD_ZBD, "%s(%s): starting from zone %d (offset %lld, buflen %lld)\n",
+
+	dprint(FD_ZBD,
+	       "%s(%s): starting from zone %d (offset %lld, buflen %lld)\n",
 	       __func__, f->file_name, zone_idx, io_u->offset, io_u->buflen);
 
 	/*
@@ -1270,10 +1294,13 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
 		z = get_zone(f, zone_idx);
 		if (z->has_wp)
 			zone_lock(td, f, z);
+
 		pthread_mutex_lock(&zbdi->mutex);
+
 		if (z->has_wp) {
 			if (z->cond != ZBD_ZONE_COND_OFFLINE &&
-			    zbdi->max_open_zones == 0 && td->o.job_max_open_zones == 0)
+			    zbdi->max_open_zones == 0 &&
+			    td->o.job_max_open_zones == 0)
 				goto examine_zone;
 			if (zbdi->num_open_zones == 0) {
 				dprint(FD_ZBD, "%s(%s): no zones are open\n",
@@ -1283,14 +1310,15 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
 		}
 
 		/*
-		 * List of opened zones is per-device, shared across all threads.
-		 * Start with quasi-random candidate zone.
-		 * Ignore zones which don't belong to thread's offset/size area.
+		 * List of opened zones is per-device, shared across all
+		 * threads. Start with quasi-random candidate zone. Ignore
+		 * zones which don't belong to thread's offset/size area.
 		 */
 		open_zone_idx = pick_random_zone_idx(f, io_u);
 		assert(!open_zone_idx ||
 		       open_zone_idx < zbdi->num_open_zones);
 		tmp_idx = open_zone_idx;
+
 		for (i = 0; i < zbdi->num_open_zones; i++) {
 			uint32_t tmpz;
 
@@ -1307,9 +1335,12 @@ 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(&zbdi->mutex);
+
 		if (z->has_wp)
 			zone_unlock(z);
+
 		return NULL;
 
 found_candidate_zone:
@@ -1317,7 +1348,9 @@ found_candidate_zone:
 		if (new_zone_idx == zone_idx)
 			break;
 		zone_idx = new_zone_idx;
+
 		pthread_mutex_unlock(&zbdi->mutex);
+
 		if (z->has_wp)
 			zone_unlock(z);
 	}
@@ -1348,7 +1381,8 @@ open_other_zone:
 	 * zone close before opening a new zone.
 	 */
 	if (wait_zone_close) {
-		dprint(FD_ZBD, "%s(%s): quiesce to allow open zones to close\n",
+		dprint(FD_ZBD,
+		       "%s(%s): quiesce to allow open zones to close\n",
 		       __func__, f->file_name);
 		io_u_quiesce(td);
 	}
@@ -1401,7 +1435,8 @@ retry:
 	 */
 	in_flight = any_io_in_flight();
 	if (in_flight || should_retry) {
-		dprint(FD_ZBD, "%s(%s): wait zone close and retry open zones\n",
+		dprint(FD_ZBD,
+		       "%s(%s): wait zone close and retry open zones\n",
 		       __func__, f->file_name);
 		pthread_mutex_unlock(&zbdi->mutex);
 		zone_unlock(z);
@@ -1412,17 +1447,22 @@ retry:
 	}
 
 	pthread_mutex_unlock(&zbdi->mutex);
+
 	zone_unlock(z);
-	dprint(FD_ZBD, "%s(%s): did not open another zone\n", __func__,
-	       f->file_name);
+
+	dprint(FD_ZBD, "%s(%s): did not open another zone\n",
+	       __func__, f->file_name);
+
 	return NULL;
 
 out:
-	dprint(FD_ZBD, "%s(%s): returning zone %d\n", __func__, f->file_name,
-	       zone_idx);
+	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;
 }
 
@@ -1441,18 +1481,20 @@ static struct fio_zone_info *zbd_replay_write_order(struct thread_data *td,
 	}
 
 	if (z->verify_block * min_bs >= z->capacity) {
-		log_err("%s: %d * %"PRIu64" >= %"PRIu64"\n", f->file_name, z->verify_block,
-			min_bs, z->capacity);
+		log_err("%s: %d * %"PRIu64" >= %"PRIu64"\n",
+			f->file_name, z->verify_block, min_bs, z->capacity);
 		/*
 		 * If the assertion below fails during a test run, adding
 		 * "--experimental_verify=1" to the command line may help.
 		 */
 		assert(false);
 	}
+
 	io_u->offset = z->start + z->verify_block * min_bs;
 	if (io_u->offset + io_u->buflen >= zbd_zone_capacity_end(z)) {
-		log_err("%s: %llu + %llu >= %"PRIu64"\n", f->file_name, io_u->offset,
-			io_u->buflen, zbd_zone_capacity_end(z));
+		log_err("%s: %llu + %llu >= %"PRIu64"\n",
+			f->file_name, io_u->offset, io_u->buflen,
+			zbd_zone_capacity_end(z));
 		assert(false);
 	}
 	z->verify_block += io_u->buflen / min_bs;
@@ -1490,6 +1532,7 @@ zbd_find_zone(struct thread_data *td, struct io_u *io_u, uint64_t min_bytes,
 		} else if (!td_random(td)) {
 			break;
 		}
+
 		if (td_random(td) && z2 >= zf &&
 		    z2->cond != ZBD_ZONE_COND_OFFLINE) {
 			if (z2->has_wp)
@@ -1500,8 +1543,11 @@ zbd_find_zone(struct thread_data *td, struct io_u *io_u, uint64_t min_bytes,
 				zone_unlock(z2);
 		}
 	}
-	dprint(FD_ZBD, "%s: no zone has %"PRIu64" bytes of readable data\n",
+
+	dprint(FD_ZBD,
+	       "%s: no zone has %"PRIu64" bytes of readable data\n",
 	       f->file_name, min_bytes);
+
 	return NULL;
 }
 
@@ -1564,11 +1610,12 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 	case DDIR_WRITE:
 		zone_end = min((uint64_t)(io_u->offset + io_u->buflen),
 			       zbd_zone_capacity_end(z));
-		pthread_mutex_lock(&zbd_info->mutex);
+
 		/*
 		 * z->wp > zone_end means that one or more I/O errors
 		 * have occurred.
 		 */
+		pthread_mutex_lock(&zbd_info->mutex);
 		if (z->wp <= zone_end) {
 			zbd_info->sectors_with_data += zone_end - z->wp;
 			zbd_info->wp_sectors_with_data += zone_end - z->wp;
@@ -1668,8 +1715,8 @@ void setup_zbd_zone_mode(struct thread_data *td, struct io_u *io_u)
 	 * sequential write, skip to zone end if the latest position is at the
 	 * zone capacity limit.
 	 */
-	if (z->capacity < f->zbd_info->zone_size && !td_random(td) &&
-	    ddir == DDIR_WRITE &&
+	if (z->capacity < f->zbd_info->zone_size &&
+	    !td_random(td) && ddir == DDIR_WRITE &&
 	    f->last_pos[ddir] >= zbd_zone_capacity_end(z)) {
 		dprint(FD_ZBD,
 		       "%s: Jump from zone capacity limit to zone end:"
@@ -1767,6 +1814,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	assert(min_bs);
 	assert(is_valid_offset(f, io_u->offset));
 	assert(io_u->buflen);
+
 	zone_idx_b = zbd_zone_idx(f, io_u->offset);
 	zb = get_zone(f, zone_idx_b);
 	orig_zb = zb;
@@ -1775,6 +1823,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 		/* Accept non-write I/Os for conventional zones. */
 		if (io_u->ddir != DDIR_WRITE)
 			return io_u_accept;
+
 		/*
 		 * Make sure that writes to conventional zones
 		 * don't cross over to any sequential zones.
@@ -1788,12 +1837,16 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			       "%s: off=%llu + min_bs=%"PRIu64" > next zone %"PRIu64"\n",
 			       f->file_name, io_u->offset,
 			       min_bs, (zb + 1)->start);
-			io_u->offset = zb->start + (zb + 1)->start - io_u->offset;
-			new_len = min(io_u->buflen, (zb + 1)->start - io_u->offset);
+			io_u->offset =
+				zb->start + (zb + 1)->start - io_u->offset;
+			new_len = min(io_u->buflen,
+				      (zb + 1)->start - io_u->offset);
 		} else {
 			new_len = (zb + 1)->start - io_u->offset;
 		}
+
 		io_u->buflen = new_len / min_bs * min_bs;
+
 		return io_u_accept;
 	}
 
@@ -1815,6 +1868,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			zb = zbd_replay_write_order(td, io_u, zb);
 			goto accept;
 		}
+
 		/*
 		 * Check that there is enough written data in the zone to do an
 		 * I/O of at least min_bs B. If there isn't, find a new zone for
@@ -1844,6 +1898,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			if (!td_random(td))
 				io_u->offset = zb->start;
 		}
+
 		/*
 		 * Make sure the I/O is within the zone valid data range while
 		 * maximizing the I/O size and preserving randomness.
@@ -1854,12 +1909,14 @@ 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.
 		 */
@@ -1871,9 +1928,12 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			dprint(FD_IO, "Changed length from %u into %llu\n",
 			       orig_len, io_u->buflen);
 		}
+
 		assert(zb->start <= io_u->offset);
 		assert(io_u->offset + io_u->buflen <= zb->wp);
+
 		goto accept;
+
 	case DDIR_WRITE:
 		if (io_u->buflen > zbdi->zone_size) {
 			td_verror(td, EINVAL, "I/O buflen exceeds zone size");
@@ -1882,6 +1942,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			       f->file_name, io_u->buflen, zbdi->zone_size);
 			goto eof;
 		}
+
 		if (!zbd_open_zone(td, f, zone_idx_b)) {
 			zone_unlock(zb);
 			zb = zbd_convert_to_open_zone(td, io_u);
@@ -1891,14 +1952,14 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 				goto eof;
 			}
 		}
+
 		/* Check whether the zone reset threshold has been exceeded */
 		if (td->o.zrf.u.f) {
-			if (zbdi->wp_sectors_with_data >=
-			    f->io_size * td->o.zrt.u.f &&
-			    zbd_dec_and_reset_write_cnt(td, f)) {
+			if (zbdi->wp_sectors_with_data >= f->io_size * td->o.zrt.u.f &&
+			    zbd_dec_and_reset_write_cnt(td, f))
 				zb->reset_zone = 1;
-			}
 		}
+
 		/* Reset the zone pointer if necessary */
 		if (zb->reset_zone || zbd_zone_full(f, zb, min_bs)) {
 			assert(td->o.verify == VERIFY_NONE);
@@ -1921,6 +1982,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 				goto eof;
 			}
 		}
+
 		/* Make writes occur at the write pointer */
 		assert(!zbd_zone_full(f, zb, min_bs));
 		io_u->offset = zb->wp;
@@ -1930,6 +1992,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			       f->file_name, io_u->offset);
 			goto eof;
 		}
+
 		/*
 		 * Make sure that the buflen is a multiple of the minimal
 		 * block size. Give up if shrinking would make the request too
@@ -1946,10 +2009,13 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			       orig_len, io_u->buflen);
 			goto accept;
 		}
+
 		td_verror(td, EIO, "zone remainder too small");
 		log_err("zone remainder %lld smaller than min block size %"PRIu64"\n",
 			(zbd_zone_capacity_end(zb) - io_u->offset), min_bs);
+
 		goto eof;
+
 	case DDIR_TRIM:
 		/* Check random trim targets a non-empty zone */
 		if (!td_random(td) || zb->wp > zb->start)
@@ -1965,7 +2031,9 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			       f->file_name, io_u->offset);
 			goto accept;
 		}
+
 		goto eof;
+
 	case DDIR_SYNC:
 		/* fall-through */
 	case DDIR_DATASYNC:
@@ -1983,19 +2051,23 @@ accept:
 	assert(zb->cond != ZBD_ZONE_COND_OFFLINE);
 	assert(!io_u->zbd_queue_io);
 	assert(!io_u->zbd_put_io);
+
 	io_u->zbd_queue_io = zbd_queue_io;
 	io_u->zbd_put_io = zbd_put_io;
+
 	/*
 	 * Since we return with the zone lock still held,
 	 * add an annotation to let Coverity know that it
 	 * is intentional.
 	 */
 	/* coverity[missing_unlock] */
+
 	return io_u_accept;
 
 eof:
 	if (zb && zb->has_wp)
 		zone_unlock(zb);
+
 	return io_u_eof;
 }
 
@@ -2033,7 +2105,8 @@ int zbd_do_io_u_trim(const struct thread_data *td, struct io_u *io_u)
 		return 0;
 
 	if (io_u->offset != z->start) {
-		log_err("Trim offset not at zone start (%lld)\n", io_u->offset);
+		log_err("Trim offset not at zone start (%lld)\n",
+			io_u->offset);
 		return -EINVAL;
 	}
 
-- 
2.31.1

Re: [PATCH 06/11] zbd: fix code style issues

[Niklas Cassel]

On Fri, Dec 10, 2021 at 11:15:36AM +0900, Damien Le Moal wrote:
> Avoid overly long lines, remove unnecessary curly brackets and add blank
> lines to make the code more readable.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---

Personally, I think that line lengths > 80 is fine, as long as they are <= 100.

Regardless, the blank lines alone makes the code more readable, so:

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

[PATCH 07/11] zbd: simplify zbd_close_zone()

[Damien Le Moal]

Change the interface of zbd_close_zone() to directly use a pointer to a
zone information structure as all callers already have this information.
Also do nothing for zones that are not marked as open instead of
figuring out this fact by searching the array of open zones.

No functional changes.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 zbd.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/zbd.c b/zbd.c
index 6a26269d..c0f43f21 100644
--- a/zbd.c
+++ b/zbd.c
@@ -288,28 +288,31 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
  * The caller must hold f->zbd_info->mutex.
  */
 static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
-			   unsigned int zone_idx)
+			   struct fio_zone_info *z)
 {
-	uint32_t open_zone_idx = 0;
+	uint32_t ozi;
 
-	for (; open_zone_idx < f->zbd_info->num_open_zones; open_zone_idx++) {
-		if (f->zbd_info->open_zones[open_zone_idx] == zone_idx)
+	if (!z->open)
+		return;
+
+	for (ozi = 0; ozi < f->zbd_info->num_open_zones; ozi++) {
+		if (get_zone(f, f->zbd_info->open_zones[ozi]) == z)
 			break;
 	}
-	if (open_zone_idx == f->zbd_info->num_open_zones)
+	if (ozi == f->zbd_info->num_open_zones)
 		return;
 
-	dprint(FD_ZBD, "%s: closing zone %d\n",
-	       f->file_name, zone_idx);
+	dprint(FD_ZBD, "%s: closing zone %u\n",
+	       f->file_name, zbd_zone_nr(f, z));
 
-	memmove(f->zbd_info->open_zones + open_zone_idx,
-		f->zbd_info->open_zones + open_zone_idx + 1,
-		(ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
+	memmove(f->zbd_info->open_zones + ozi,
+		f->zbd_info->open_zones + ozi + 1,
+		(ZBD_MAX_OPEN_ZONES - (ozi + 1)) *
 		sizeof(f->zbd_info->open_zones[0]));
 
 	f->zbd_info->num_open_zones--;
 	td->num_open_zones--;
-	get_zone(f, zone_idx)->open = 0;
+	z->open = 0;
 }
 
 /**
@@ -335,14 +338,12 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
 	       f->file_name, zbd_zone_nr(f, zb), zbd_zone_nr(f, ze));
 
 	for (z = zb; z < ze; z++) {
-		uint32_t nz = zbd_zone_nr(f, z);
-
 		if (!z->has_wp)
 			continue;
 
 		zone_lock(td, f, z);
 		pthread_mutex_lock(&f->zbd_info->mutex);
-		zbd_close_zone(td, f, nz);
+		zbd_close_zone(td, f, z);
 		pthread_mutex_unlock(&f->zbd_info->mutex);
 
 		if (z->wp != z->start) {
@@ -1568,7 +1569,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, zbd_zone_nr(f, z));
+		zbd_close_zone(td, f, z);
 		pthread_mutex_unlock(&f->zbd_info->mutex);
 	}
 }
-- 
2.31.1

Re: [PATCH 07/11] zbd: simplify zbd_close_zone()

[Niklas Cassel]

On Fri, Dec 10, 2021 at 11:15:37AM +0900, Damien Le Moal wrote:
> Change the interface of zbd_close_zone() to directly use a pointer to a
> zone information structure as all callers already have this information.
> Also do nothing for zones that are not marked as open instead of
> figuring out this fact by searching the array of open zones.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---

Nice cleanup!

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

[PATCH 08/11] zbd: simplify zbd_open_zone()

[Damien Le Moal]

Similarly to zbd_close_zone(), directly pass a pointer to a zone
information structure to zbd_open_zone() instead of a zone number.

No functional changes.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 zbd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/zbd.c b/zbd.c
index c0f43f21..49aee9b8 100644
--- a/zbd.c
+++ b/zbd.c
@@ -400,11 +400,11 @@ static int zbd_get_max_open_zones(struct thread_data *td, struct fio_file *f,
  * to be exceeded.
  */
 static bool zbd_open_zone(struct thread_data *td, const struct fio_file *f,
-			  uint32_t zone_idx)
+			  struct fio_zone_info *z)
 {
 	const uint64_t min_bs = td->o.min_bs[DDIR_WRITE];
 	struct zoned_block_device_info *zbdi = f->zbd_info;
-	struct fio_zone_info *z = get_zone(f, zone_idx);
+	uint32_t zone_idx = zbd_zone_nr(f, z);
 	bool res = true;
 
 	if (z->cond == ZBD_ZONE_COND_OFFLINE)
@@ -446,7 +446,7 @@ static bool zbd_open_zone(struct thread_data *td, const struct fio_file *f,
 	if (zbdi->num_open_zones >= zbdi->max_open_zones)
 		goto out;
 
-	dprint(FD_ZBD, "%s: opening zone %d\n",
+	dprint(FD_ZBD, "%s: opening zone %u\n",
 	       f->file_name, zone_idx);
 
 	zbdi->open_zones[zbdi->num_open_zones++] = zone_idx;
@@ -1084,7 +1084,7 @@ int zbd_setup_files(struct thread_data *td)
 			if (z->cond != ZBD_ZONE_COND_IMP_OPEN &&
 			    z->cond != ZBD_ZONE_COND_EXP_OPEN)
 				continue;
-			if (zbd_open_zone(td, f, zi))
+			if (zbd_open_zone(td, f, z))
 				continue;
 			/*
 			 * If the number of open zones exceeds specified limits,
@@ -1406,7 +1406,7 @@ retry:
 		zone_lock(td, f, z);
 		if (z->open)
 			continue;
-		if (zbd_open_zone(td, f, zone_idx))
+		if (zbd_open_zone(td, f, z))
 			goto out;
 	}
 
@@ -1475,7 +1475,7 @@ static struct fio_zone_info *zbd_replay_write_order(struct thread_data *td,
 	const struct fio_file *f = io_u->file;
 	const uint64_t min_bs = td->o.min_bs[DDIR_WRITE];
 
-	if (!zbd_open_zone(td, f, zbd_zone_nr(f, z))) {
+	if (!zbd_open_zone(td, f, z)) {
 		zone_unlock(z);
 		z = zbd_convert_to_open_zone(td, io_u);
 		assert(z);
@@ -1944,7 +1944,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 			goto eof;
 		}
 
-		if (!zbd_open_zone(td, f, zone_idx_b)) {
+		if (!zbd_open_zone(td, f, zb)) {
 			zone_unlock(zb);
 			zb = zbd_convert_to_open_zone(td, io_u);
 			if (!zb) {
-- 
2.31.1

Re: [PATCH 08/11] zbd: simplify zbd_open_zone()

[Niklas Cassel]

On Fri, Dec 10, 2021 at 11:15:38AM +0900, Damien Le Moal wrote:
> Similarly to zbd_close_zone(), directly pass a pointer to a zone
> information structure to zbd_open_zone() instead of a zone number.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---

Nice cleanup!

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

[PATCH 09/11] zbd: rename zbd_zone_idx() and zbd_zone_nr()

[Damien Le Moal]

Rename zbd_zone_idx() to zbd_zone_idx_from_offset() to make it clear
that the argument determining the zone is a file offset. To be
consistent, rename zbd_zone_nr() to zbd_zone_idx() to avoid confusion
with a number of zones.

No functional changes.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 zbd.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/zbd.c b/zbd.c
index 49aee9b8..7204f68f 100644
--- a/zbd.c
+++ b/zbd.c
@@ -27,19 +27,20 @@ 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 unsigned int zbd_zone_nr(const struct fio_file *f,
-				       struct fio_zone_info *zone)
+static inline unsigned int zbd_zone_idx(const struct fio_file *f,
+					struct fio_zone_info *zone)
 {
 	return zone - f->zbd_info->zone_info;
 }
 
 /**
- * zbd_zone_idx - convert an offset into a zone number
+ * zbd_zone_idx_from_offset - convert an offset into a zone number
  * @f: file pointer.
  * @offset: offset in bytes. If this offset is in the first zone_size bytes
  *	    past the disk size then the index of the sentinel is returned.
  */
-static uint32_t zbd_zone_idx(const struct fio_file *f, uint64_t offset)
+static uint32_t zbd_zone_idx_from_offset(const struct fio_file *f,
+					 uint64_t offset)
 {
 	uint32_t zone_idx;
 
@@ -253,7 +254,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, z));
+	       f->file_name, zbd_zone_idx(f, z));
 
 	switch (f->zbd_info->model) {
 	case ZBD_HOST_AWARE:
@@ -303,7 +304,7 @@ static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
 		return;
 
 	dprint(FD_ZBD, "%s: closing zone %u\n",
-	       f->file_name, zbd_zone_nr(f, z));
+	       f->file_name, zbd_zone_idx(f, z));
 
 	memmove(f->zbd_info->open_zones + ozi,
 		f->zbd_info->open_zones + ozi + 1,
@@ -335,7 +336,7 @@ 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, zb), zbd_zone_nr(f, ze));
+	       f->file_name, zbd_zone_idx(f, zb), zbd_zone_idx(f, ze));
 
 	for (z = zb; z < ze; z++) {
 		if (!z->has_wp)
@@ -348,7 +349,7 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
 
 		if (z->wp != z->start) {
 			dprint(FD_ZBD, "%s: resetting zone %u\n",
-			       f->file_name, zbd_zone_nr(f, z));
+			       f->file_name, zbd_zone_idx(f, z));
 			if (zbd_reset_zone(td, f, z) < 0)
 				res = 1;
 		}
@@ -404,7 +405,7 @@ static bool zbd_open_zone(struct thread_data *td, const struct fio_file *f,
 {
 	const uint64_t min_bs = td->o.min_bs[DDIR_WRITE];
 	struct zoned_block_device_info *zbdi = f->zbd_info;
-	uint32_t zone_idx = zbd_zone_nr(f, z);
+	uint32_t zone_idx = zbd_zone_idx(f, z);
 	bool res = true;
 
 	if (z->cond == ZBD_ZONE_COND_OFFLINE)
@@ -489,8 +490,9 @@ static bool zbd_is_seq_job(struct fio_file *f)
 	if (f->io_size == 0)
 		return false;
 
-	zone_idx_b = zbd_zone_idx(f, f->file_offset);
-	zone_idx_e = zbd_zone_idx(f, f->file_offset + f->io_size - 1);
+	zone_idx_b = zbd_zone_idx_from_offset(f, f->file_offset);
+	zone_idx_e =
+		zbd_zone_idx_from_offset(f, f->file_offset + f->io_size - 1);
 	for (zone_idx = zone_idx_b; zone_idx <= zone_idx_e; zone_idx++)
 		if (get_zone(f, zone_idx)->has_wp)
 			return true;
@@ -535,7 +537,7 @@ static bool zbd_verify_file_sizes(struct thread_data *td, struct fio_file *f)
 		return false;
 	}
 
-	zone_idx = zbd_zone_idx(f, f->file_offset);
+	zone_idx = zbd_zone_idx_from_offset(f, f->file_offset);
 	z = get_zone(f, zone_idx);
 	if ((f->file_offset != z->start) &&
 	    (td->o.td_ddir != TD_DDIR_READ)) {
@@ -552,7 +554,7 @@ static bool zbd_verify_file_sizes(struct thread_data *td, struct fio_file *f)
 		f->file_offset = new_offset;
 	}
 
-	zone_idx = zbd_zone_idx(f, f->file_offset + f->io_size);
+	zone_idx = zbd_zone_idx_from_offset(f, f->file_offset + f->io_size);
 	z = get_zone(f, zone_idx);
 	new_end = z->start;
 	if ((td->o.td_ddir != TD_DDIR_READ) &&
@@ -1043,8 +1045,9 @@ int zbd_setup_files(struct thread_data *td)
 
 		assert(zbd);
 
-		f->min_zone = zbd_zone_idx(f, f->file_offset);
-		f->max_zone = zbd_zone_idx(f, f->file_offset + f->io_size);
+		f->min_zone = zbd_zone_idx_from_offset(f, f->file_offset);
+		f->max_zone =
+			zbd_zone_idx_from_offset(f, f->file_offset + f->io_size);
 
 		/*
 		 * When all zones in the I/O range are conventional, io_size
@@ -1272,7 +1275,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
 		 */
 		zone_idx = zbdi->open_zones[pick_random_zone_idx(f, io_u)];
 	} else {
-		zone_idx = zbd_zone_idx(f, io_u->offset);
+		zone_idx = zbd_zone_idx_from_offset(f, io_u->offset);
 	}
 	if (zone_idx < f->min_zone)
 		zone_idx = f->min_zone;
@@ -1594,7 +1597,7 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 
 	assert(zbd_info);
 
-	zone_idx = zbd_zone_idx(f, io_u->offset);
+	zone_idx = zbd_zone_idx_from_offset(f, io_u->offset);
 	assert(zone_idx < zbd_info->nr_zones);
 	z = get_zone(f, zone_idx);
 
@@ -1652,7 +1655,7 @@ static void zbd_put_io(struct thread_data *td, const struct io_u *io_u)
 
 	assert(zbd_info);
 
-	zone_idx = zbd_zone_idx(f, io_u->offset);
+	zone_idx = zbd_zone_idx_from_offset(f, io_u->offset);
 	assert(zone_idx < zbd_info->nr_zones);
 	z = get_zone(f, zone_idx);
 
@@ -1708,7 +1711,7 @@ void setup_zbd_zone_mode(struct thread_data *td, struct io_u *io_u)
 	assert(td->o.zone_size);
 	assert(f->zbd_info);
 
-	zone_idx = zbd_zone_idx(f, f->last_pos[ddir]);
+	zone_idx = zbd_zone_idx_from_offset(f, f->last_pos[ddir]);
 	z = get_zone(f, zone_idx);
 
 	/*
@@ -1816,7 +1819,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);
+	zone_idx_b = zbd_zone_idx_from_offset(f, io_u->offset);
 	zb = get_zone(f, zone_idx_b);
 	orig_zb = zb;
 
@@ -2099,7 +2102,7 @@ int zbd_do_io_u_trim(const struct thread_data *td, struct io_u *io_u)
 	uint32_t zone_idx;
 	int ret;
 
-	zone_idx = zbd_zone_idx(f, io_u->offset);
+	zone_idx = zbd_zone_idx_from_offset(f, io_u->offset);
 	z = get_zone(f, zone_idx);
 
 	if (!z->has_wp)
-- 
2.31.1

Re: [PATCH 09/11] zbd: rename zbd_zone_idx() and zbd_zone_nr()

[Niklas Cassel]

On Fri, Dec 10, 2021 at 11:15:39AM +0900, Damien Le Moal wrote:
> Rename zbd_zone_idx() to zbd_zone_idx_from_offset() to make it clear
> that the argument determining the zone is a file offset. To be
> consistent, rename zbd_zone_nr() to zbd_zone_idx() to avoid confusion
> with a number of zones.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  zbd.c | 45 ++++++++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/zbd.c b/zbd.c
> index 49aee9b8..7204f68f 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -27,19 +27,20 @@ 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 unsigned int zbd_zone_nr(const struct fio_file *f,
> -				       struct fio_zone_info *zone)
> +static inline unsigned int zbd_zone_idx(const struct fio_file *f,
> +					struct fio_zone_info *zone)
>  {
>  	return zone - f->zbd_info->zone_info;
>  }
>  
>  /**
> - * zbd_zone_idx - convert an offset into a zone number
> + * zbd_zone_idx_from_offset - convert an offset into a zone number
>   * @f: file pointer.
>   * @offset: offset in bytes. If this offset is in the first zone_size bytes
>   *	    past the disk size then the index of the sentinel is returned.
>   */
> -static uint32_t zbd_zone_idx(const struct fio_file *f, uint64_t offset)
> +static uint32_t zbd_zone_idx_from_offset(const struct fio_file *f,
> +					 uint64_t offset)

Perhaps instead of having _from_ in the name, use _to_ ?
(Like virt_to_phys())

e.g. zbd_offset_to_zone_idx()

What do you think?


It does seem weird that one function that gets the zone_idx returns a
unsigned int, while the function that get the zone_idx returns a uint32_t.


Keeping the _from_ name would be fine by me as well, if you think
that having a similar prefix is more important than the function
name length.


Kind regards,
Niklas

[PATCH 10/11] zbd: rename get_zone()

[Damien Le Moal]

Rename get_zone() to zbd_get_zone() to be consistent with the naming
pattern of most zbd functions.

No functional changes.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 zbd.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/zbd.c b/zbd.c
index 7204f68f..5c0fe00b 100644
--- a/zbd.c
+++ b/zbd.c
@@ -123,10 +123,10 @@ static inline void zone_unlock(struct fio_zone_info *z)
 	assert(!ret);
 }
 
-static inline struct fio_zone_info *get_zone(const struct fio_file *f,
-					     unsigned int zone_nr)
+static inline struct fio_zone_info *zbd_get_zone(const struct fio_file *f,
+						 unsigned int zone_idx)
 {
-	return &f->zbd_info->zone_info[zone_nr];
+	return &f->zbd_info->zone_info[zone_idx];
 }
 
 /**
@@ -297,7 +297,7 @@ static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
 		return;
 
 	for (ozi = 0; ozi < f->zbd_info->num_open_zones; ozi++) {
-		if (get_zone(f, f->zbd_info->open_zones[ozi]) == z)
+		if (zbd_get_zone(f, f->zbd_info->open_zones[ozi]) == z)
 			break;
 	}
 	if (ozi == f->zbd_info->num_open_zones)
@@ -494,7 +494,7 @@ static bool zbd_is_seq_job(struct fio_file *f)
 	zone_idx_e =
 		zbd_zone_idx_from_offset(f, f->file_offset + f->io_size - 1);
 	for (zone_idx = zone_idx_b; zone_idx <= zone_idx_e; zone_idx++)
-		if (get_zone(f, zone_idx)->has_wp)
+		if (zbd_get_zone(f, zone_idx)->has_wp)
 			return true;
 
 	return false;
@@ -538,7 +538,7 @@ static bool zbd_verify_file_sizes(struct thread_data *td, struct fio_file *f)
 	}
 
 	zone_idx = zbd_zone_idx_from_offset(f, f->file_offset);
-	z = get_zone(f, zone_idx);
+	z = zbd_get_zone(f, zone_idx);
 	if ((f->file_offset != z->start) &&
 	    (td->o.td_ddir != TD_DDIR_READ)) {
 		new_offset = zbd_zone_end(z);
@@ -555,7 +555,7 @@ static bool zbd_verify_file_sizes(struct thread_data *td, struct fio_file *f)
 	}
 
 	zone_idx = zbd_zone_idx_from_offset(f, f->file_offset + f->io_size);
-	z = get_zone(f, zone_idx);
+	z = zbd_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)) {
@@ -1153,8 +1153,8 @@ static uint64_t zbd_process_swd(struct thread_data *td,
 	uint64_t swd = 0;
 	uint64_t wp_swd = 0;
 
-	zb = get_zone(f, f->min_zone);
-	ze = get_zone(f, f->max_zone);
+	zb = zbd_get_zone(f, f->min_zone);
+	ze = zbd_get_zone(f, f->max_zone);
 	for (z = zb; z < ze; z++) {
 		if (z->has_wp) {
 			zone_lock(td, f, z);
@@ -1206,8 +1206,8 @@ void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 	if (!f->zbd_info || !td_write(td))
 		return;
 
-	zb = get_zone(f, f->min_zone);
-	ze = get_zone(f, f->max_zone);
+	zb = zbd_get_zone(f, f->min_zone);
+	ze = zbd_get_zone(f, f->max_zone);
 	swd = zbd_process_swd(td, f, SET_SWD);
 
 	dprint(FD_ZBD, "%s(%s): swd = %" PRIu64 "\n",
@@ -1295,7 +1295,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
 	for (;;) {
 		uint32_t tmp_idx;
 
-		z = get_zone(f, zone_idx);
+		z = zbd_get_zone(f, zone_idx);
 		if (z->has_wp)
 			zone_lock(td, f, z);
 
@@ -1401,7 +1401,7 @@ retry:
 		if (!is_valid_offset(f, z->start)) {
 			/* Wrap-around. */
 			zone_idx = f->min_zone;
-			z = get_zone(f, zone_idx);
+			z = zbd_get_zone(f, zone_idx);
 		}
 		assert(is_valid_offset(f, z->start));
 		if (!z->has_wp)
@@ -1424,7 +1424,7 @@ retry:
 		pthread_mutex_unlock(&zbdi->mutex);
 		zone_unlock(z);
 
-		z = get_zone(f, zone_idx);
+		z = zbd_get_zone(f, zone_idx);
 
 		zone_lock(td, f, z);
 		if (z->wp + min_bs <= zbd_zone_capacity_end(z))
@@ -1519,7 +1519,7 @@ zbd_find_zone(struct thread_data *td, struct io_u *io_u, uint64_t min_bytes,
 {
 	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);
+	const struct fio_zone_info *const zf = zbd_get_zone(f, f->min_zone);
 
 	/*
 	 * Skip to the next non-empty zone in case of sequential I/O and to
@@ -1599,7 +1599,7 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 
 	zone_idx = zbd_zone_idx_from_offset(f, io_u->offset);
 	assert(zone_idx < zbd_info->nr_zones);
-	z = get_zone(f, zone_idx);
+	z = zbd_get_zone(f, zone_idx);
 
 	assert(z->has_wp);
 
@@ -1657,7 +1657,7 @@ static void zbd_put_io(struct thread_data *td, const struct io_u *io_u)
 
 	zone_idx = zbd_zone_idx_from_offset(f, io_u->offset);
 	assert(zone_idx < zbd_info->nr_zones);
-	z = get_zone(f, zone_idx);
+	z = zbd_get_zone(f, zone_idx);
 
 	assert(z->has_wp);
 
@@ -1712,7 +1712,7 @@ void setup_zbd_zone_mode(struct thread_data *td, struct io_u *io_u)
 	assert(f->zbd_info);
 
 	zone_idx = zbd_zone_idx_from_offset(f, f->last_pos[ddir]);
-	z = get_zone(f, zone_idx);
+	z = zbd_get_zone(f, zone_idx);
 
 	/*
 	 * When the zone capacity is smaller than the zone size and the I/O is
@@ -1820,7 +1820,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	assert(io_u->buflen);
 
 	zone_idx_b = zbd_zone_idx_from_offset(f, io_u->offset);
-	zb = get_zone(f, zone_idx_b);
+	zb = zbd_get_zone(f, zone_idx_b);
 	orig_zb = zb;
 
 	if (!zb->has_wp) {
@@ -1883,7 +1883,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))) {
 			zone_unlock(zb);
-			zl = get_zone(f, f->max_zone);
+			zl = zbd_get_zone(f, f->max_zone);
 			zb = zbd_find_zone(td, io_u, min_bs, zb, zl);
 			if (!zb) {
 				dprint(FD_ZBD,
@@ -2027,7 +2027,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 
 		/* Find out a non-empty zone to trim */
 		zone_unlock(zb);
-		zl = get_zone(f, f->max_zone);
+		zl = zbd_get_zone(f, f->max_zone);
 		zb = zbd_find_zone(td, io_u, 1, zb, zl);
 		if (zb) {
 			io_u->offset = zb->start;
@@ -2103,7 +2103,7 @@ int zbd_do_io_u_trim(const struct thread_data *td, struct io_u *io_u)
 	int ret;
 
 	zone_idx = zbd_zone_idx_from_offset(f, io_u->offset);
-	z = get_zone(f, zone_idx);
+	z = zbd_get_zone(f, zone_idx);
 
 	if (!z->has_wp)
 		return 0;
-- 
2.31.1

Re: [PATCH 10/11] zbd: rename get_zone()

[Niklas Cassel]

On Fri, Dec 10, 2021 at 11:15:40AM +0900, Damien Le Moal wrote:
> Rename get_zone() to zbd_get_zone() to be consistent with the naming
> pattern of most zbd functions.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

[PATCH 11/11] zbd: introduce zbd_get_zone_from_offset() helper

[Damien Le Moal]

Introduce the helper function zbd_get_zone_from_offset() to get a zone
structure using a file offset. This replaces the common code pattern:

	zone_idx = zbd_zone_idx_from_offset(f, _offset);
	z = zbd_get_zone(f, zone_idx);

with a single function call in many functions.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 zbd.c | 44 ++++++++++++++++----------------------------
 1 file changed, 16 insertions(+), 28 deletions(-)

diff --git a/zbd.c b/zbd.c
index 5c0fe00b..ba583c9f 100644
--- a/zbd.c
+++ b/zbd.c
@@ -129,6 +129,12 @@ static inline struct fio_zone_info *zbd_get_zone(const struct fio_file *f,
 	return &f->zbd_info->zone_info[zone_idx];
 }
 
+static inline struct fio_zone_info *
+zbd_get_zone_from_offset(const struct fio_file *f,  uint64_t offset)
+{
+	return zbd_get_zone(f, zbd_zone_idx_from_offset(f, offset));
+}
+
 /**
  * zbd_get_zoned_model - Get a device zoned model
  * @td: FIO thread data
@@ -507,7 +513,6 @@ static bool zbd_verify_file_sizes(struct thread_data *td, struct fio_file *f)
 {
 	const struct fio_zone_info *z;
 	uint64_t new_offset, new_end;
-	uint32_t zone_idx;
 
 	if (!f->zbd_info)
 		return true;
@@ -537,8 +542,7 @@ static bool zbd_verify_file_sizes(struct thread_data *td, struct fio_file *f)
 		return false;
 	}
 
-	zone_idx = zbd_zone_idx_from_offset(f, f->file_offset);
-	z = zbd_get_zone(f, zone_idx);
+	z = zbd_get_zone_from_offset(f, f->file_offset);
 	if ((f->file_offset != z->start) &&
 	    (td->o.td_ddir != TD_DDIR_READ)) {
 		new_offset = zbd_zone_end(z);
@@ -554,8 +558,7 @@ static bool zbd_verify_file_sizes(struct thread_data *td, struct fio_file *f)
 		f->file_offset = new_offset;
 	}
 
-	zone_idx = zbd_zone_idx_from_offset(f, f->file_offset + f->io_size);
-	z = zbd_get_zone(f, zone_idx);
+	z = zbd_get_zone_from_offset(f, f->file_offset + f->io_size);
 	new_end = z->start;
 	if ((td->o.td_ddir != TD_DDIR_READ) &&
 	    (f->file_offset + f->io_size != new_end)) {
@@ -1592,15 +1595,11 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 	const struct fio_file *f = io_u->file;
 	struct zoned_block_device_info *zbd_info = f->zbd_info;
 	struct fio_zone_info *z;
-	uint32_t zone_idx;
 	uint64_t zone_end;
 
 	assert(zbd_info);
 
-	zone_idx = zbd_zone_idx_from_offset(f, io_u->offset);
-	assert(zone_idx < zbd_info->nr_zones);
-	z = zbd_get_zone(f, zone_idx);
-
+	z = zbd_get_zone_from_offset(f, io_u->offset);
 	assert(z->has_wp);
 
 	if (!success)
@@ -1608,7 +1607,7 @@ static void zbd_queue_io(struct thread_data *td, struct io_u *io_u, int q,
 
 	dprint(FD_ZBD,
 	       "%s: queued I/O (%lld, %llu) for zone %u\n",
-	       f->file_name, io_u->offset, io_u->buflen, zone_idx);
+	       f->file_name, io_u->offset, io_u->buflen, zbd_zone_idx(f, z));
 
 	switch (io_u->ddir) {
 	case DDIR_WRITE:
@@ -1651,19 +1650,15 @@ static void zbd_put_io(struct thread_data *td, const struct io_u *io_u)
 	const struct fio_file *f = io_u->file;
 	struct zoned_block_device_info *zbd_info = f->zbd_info;
 	struct fio_zone_info *z;
-	uint32_t zone_idx;
 
 	assert(zbd_info);
 
-	zone_idx = zbd_zone_idx_from_offset(f, io_u->offset);
-	assert(zone_idx < zbd_info->nr_zones);
-	z = zbd_get_zone(f, zone_idx);
-
+	z = zbd_get_zone_from_offset(f, io_u->offset);
 	assert(z->has_wp);
 
 	dprint(FD_ZBD,
 	       "%s: terminate I/O (%lld, %llu) for zone %u\n",
-	       f->file_name, io_u->offset, io_u->buflen, zone_idx);
+	       f->file_name, io_u->offset, io_u->buflen, zbd_zone_idx(f, z));
 
 	zbd_end_zone_io(td, io_u, z);
 
@@ -1705,14 +1700,12 @@ void setup_zbd_zone_mode(struct thread_data *td, struct io_u *io_u)
 	struct fio_file *f = io_u->file;
 	enum fio_ddir ddir = io_u->ddir;
 	struct fio_zone_info *z;
-	uint32_t zone_idx;
 
 	assert(td->o.zone_mode == ZONE_MODE_ZBD);
 	assert(td->o.zone_size);
 	assert(f->zbd_info);
 
-	zone_idx = zbd_zone_idx_from_offset(f, f->last_pos[ddir]);
-	z = zbd_get_zone(f, zone_idx);
+	z = zbd_get_zone_from_offset(f, f->last_pos[ddir]);
 
 	/*
 	 * When the zone capacity is smaller than the zone size and the I/O is
@@ -1726,7 +1719,7 @@ void setup_zbd_zone_mode(struct thread_data *td, struct io_u *io_u)
 		       "%s: Jump from zone capacity limit to zone end:"
 		       " (%"PRIu64" -> %"PRIu64") for zone %u (%"PRIu64")\n",
 		       f->file_name, f->last_pos[ddir],
-		       zbd_zone_end(z), zone_idx, z->capacity);
+		       zbd_zone_end(z), zbd_zone_idx(f, z), z->capacity);
 		td->io_skip_bytes += zbd_zone_end(z) - f->last_pos[ddir];
 		f->last_pos[ddir] = zbd_zone_end(z);
 	}
@@ -1807,7 +1800,6 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 {
 	struct fio_file *f = io_u->file;
 	struct zoned_block_device_info *zbdi = f->zbd_info;
-	uint32_t zone_idx_b;
 	struct fio_zone_info *zb, *zl, *orig_zb;
 	uint32_t orig_len = io_u->buflen;
 	uint64_t min_bs = td->o.min_bs[io_u->ddir];
@@ -1819,8 +1811,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_from_offset(f, io_u->offset);
-	zb = zbd_get_zone(f, zone_idx_b);
+	zb = zbd_get_zone_from_offset(f, io_u->offset);
 	orig_zb = zb;
 
 	if (!zb->has_wp) {
@@ -2099,12 +2090,9 @@ 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_from_offset(f, io_u->offset);
-	z = zbd_get_zone(f, zone_idx);
-
+	z = zbd_get_zone_from_offset(f, io_u->offset);
 	if (!z->has_wp)
 		return 0;
 
-- 
2.31.1

Re: [PATCH 11/11] zbd: introduce zbd_get_zone_from_offset() helper

[Niklas Cassel]

On Fri, Dec 10, 2021 at 11:15:41AM +0900, Damien Le Moal wrote:
> Introduce the helper function zbd_get_zone_from_offset() to get a zone
> structure using a file offset. This replaces the common code pattern:
> 
> 	zone_idx = zbd_zone_idx_from_offset(f, _offset);
> 	z = zbd_get_zone(f, zone_idx);
> 
> with a single function call in many functions.

Since you took real code that you are removing, perhaps add an
example of the real code (with arguments) that you are adding.

> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---

Nice cleanup!

I guess this depends on if you liked my renaming suggestion in
patch 9/11 or not.

If you did, then perhaps this function could be renamed in a similar
way to something like:
zbd_offset_to_zone()

However, if you didn't like my renaming suggestion in patch 9/11, then
this name is fine, as it will be consistent with the previous patch.


Kind regards,
Niklas