[PATCH v2 0/7] Assorted bug fixes and improvements

[PATCH v2 0/7] Assorted bug fixes and improvements

[Damien Le Moal]

The first 3 patches of this series fix bugs with write_iolog option
initialization (crash on initialization failure) and with zonemode=zbd
(one possible deadlock for read workloads and an assert trigerring with
verify option enabled).

The following 4 patches are cleanups and improvements of zbd_xxx()
functions to reduce unnecessary overhead when zonemode=zbd is not used.

Comments are welcome !

* Changes from v1:
  - Dropped the patches introducing file and thread zbd function
    pointers. Further reducing the overhead on fio core due to the
    presence of zbd_xxx() functions will require major code surgery.
    Until then, the new patches 4, 5 and 6 provide smaller improvements.
  - Added patch 7 to improve tests execution time.

Damien Le Moal (7):
  iolog: Fix write_iolog_close()
  zbd: Fix potential deadlock on read operations
  zbd: Fix read with verify
  zbd: Optimize zbd_file_reset()
  zbd: Rename zbd_init()
  io_u: Optimize set_rw_ddir()
  t/zbd: Use max-jobs=16 option

 filesetup.c            |  4 ++--
 io_u.c                 |  3 ++-
 iolog.c                |  3 +++
 t/zbd/test-zbd-support |  4 ++--
 zbd.c                  | 22 +++++++++-------------
 zbd.h                  |  8 +++++++-
 6 files changed, 25 insertions(+), 19 deletions(-)

-- 
2.25.4

[PATCH v2 1/7] iolog: Fix write_iolog_close()

[Damien Le Moal]

If the init_iolog() call from backend.c thread_main() fails (e.g. wrong
file path given), td->iolog_f is not set but write_iolog_close() is
still called from thread_main() error processing. This causes a seg
fault and unclean termination of fio. Fix this by changing
write_iolog_close() to do nothing if td->iolog_f is NULL.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 iolog.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/iolog.c b/iolog.c
index 917a446c..4a79fc46 100644
--- a/iolog.c
+++ b/iolog.c
@@ -342,6 +342,9 @@ void trim_io_piece(const struct io_u *io_u)
 
 void write_iolog_close(struct thread_data *td)
 {
+	if (!td->iolog_f)
+		return;
+
 	fflush(td->iolog_f);
 	fclose(td->iolog_f);
 	free(td->iolog_buf);
-- 
2.25.4

[PATCH v2 2/7] zbd: Fix potential deadlock on read operations

[Damien Le Moal]

For read-only workloads, zbd_find_zone() has a similar zone locking
behavior as for write IOs: zones to be read are locked when an IO is
prepared and unlocked when the IO completes. With an asynchronous IO
engine, this can create deadlocks if 2 threads are trying to read the
same 2 zones. For instance, if thread A already has a lock on zone 1
and is waiting for a lock on zone 2 while thread B already has a lock
on zone 2 and waiting for a lock on zone 1.

The fix is similar to previous fixes for this potential deadlock,
namely, use zone_lock() instead of directly calling pthread_mutex_lock()
to ensure that a thread issues the IOs it already has prepared if it
encounters a locked zone, doing so ensuring forward progress.

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

diff --git a/zbd.c b/zbd.c
index 8dc3c397..5aaf1e2c 100644
--- a/zbd.c
+++ b/zbd.c
@@ -1141,7 +1141,7 @@ 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) {
-			pthread_mutex_lock(&z1->mutex);
+			zone_lock(td, z1);
 			if (z1->start + min_bs <= z1->wp)
 				return z1;
 			pthread_mutex_unlock(&z1->mutex);
@@ -1150,7 +1150,7 @@ zbd_find_zone(struct thread_data *td, struct io_u *io_u,
 		}
 		if (td_random(td) && z2 >= zf &&
 		    z2->cond != ZBD_ZONE_COND_OFFLINE) {
-			pthread_mutex_lock(&z2->mutex);
+			zone_lock(td, z2);
 			if (z2->start + min_bs <= z2->wp)
 				return z2;
 			pthread_mutex_unlock(&z2->mutex);
-- 
2.25.4

[PATCH v2 3/7] zbd: Fix read with verify

[Damien Le Moal]

For a read only workload with verify option enabled, executing
zbd_replay_write_order() will ignore target zones that are full and try
to open another zone. This either triggers an assert if max_open_zones
is unused, or result in verify failing. Fix this by executing
zbd_replay_write_order() only for writing workloads. This fix is also
consistent with the fact that zoned devices do not implicitly open
zones for read operations.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 zbd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/zbd.c b/zbd.c
index 5aaf1e2c..c30454b9 100644
--- a/zbd.c
+++ b/zbd.c
@@ -1409,7 +1409,8 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	switch (io_u->ddir) {
 	case DDIR_READ:
 		if (td->runstate == TD_VERIFYING) {
-			zb = zbd_replay_write_order(td, io_u, zb);
+			if (td_write(td))
+				zb = zbd_replay_write_order(td, io_u, zb);
 			goto accept;
 		}
 		/*
-- 
2.25.4

[PATCH v2 4/7] zbd: Optimize zbd_file_reset()

[Damien Le Moal]

For a job not writing, a device zones will not be reset by executing
zbc_file_reset() so there is no need to scan all zones of the job
operating range. Avoid this overhead by returning early for jobs that
are not writing.

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

diff --git a/zbd.c b/zbd.c
index c30454b9..a64fd0c7 100644
--- a/zbd.c
+++ b/zbd.c
@@ -743,8 +743,7 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
 
 			reset_wp = z->wp != z->start;
 		} else {
-			reset_wp = (td->o.td_ddir & TD_DDIR_WRITE) &&
-					z->wp % min_bs != 0;
+			reset_wp = z->wp % min_bs != 0;
 		}
 		if (reset_wp) {
 			dprint(FD_ZBD, "%s: resetting zone %u\n",
@@ -856,7 +855,7 @@ void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 	struct fio_zone_info *zb, *ze;
 	uint32_t zone_idx_e;
 
-	if (!f->zbd_info)
+	if (!f->zbd_info || !td_write(td))
 		return;
 
 	zb = &f->zbd_info->zone_info[zbd_zone_idx(f, f->file_offset)];
@@ -869,7 +868,6 @@ void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 	 * writing data, which causes data loss.
 	 */
 	zbd_reset_zones(td, f, zb, ze, td->o.verify != VERIFY_NONE &&
-			(td->o.td_ddir & TD_DDIR_WRITE) &&
 			td->runstate != TD_VERIFYING);
 	zbd_reset_write_cnt(td, f);
 }
-- 
2.25.4

[PATCH v2 5/7] zbd: Rename zbd_init()

[Damien Le Moal]

Clarify the execution context of zbd_init() by renaming this function
to zbd_setup_files() as it is called from the setup_files() function.
While at it, wrap the use of zbd_free_zone_info() into the inline
function zbd_close_file() to avoid an unecessary function call when
closing files that are not zoned block device files of zonemode=zbd
jobs, that is, files that do not have zbd_info initialized.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 filesetup.c | 4 ++--
 zbd.c       | 5 ++---
 zbd.h       | 8 +++++++-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/filesetup.c b/filesetup.c
index 8a4091fc..49c54b81 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -1268,7 +1268,7 @@ done:
 	td_restore_runstate(td, old_state);
 
 	if (td->o.zone_mode == ZONE_MODE_ZBD) {
-		err = zbd_init(td);
+		err = zbd_setup_files(td);
 		if (err)
 			goto err_out;
 	}
@@ -1469,7 +1469,7 @@ void close_and_free_files(struct thread_data *td)
 			td_io_unlink_file(td, f);
 		}
 
-		zbd_free_zone_info(f);
+		zbd_close_file(f);
 
 		if (use_free)
 			free(f->file_name);
diff --git a/zbd.c b/zbd.c
index a64fd0c7..db6d09f2 100644
--- a/zbd.c
+++ b/zbd.c
@@ -546,8 +546,7 @@ void zbd_free_zone_info(struct fio_file *f)
 {
 	uint32_t refcount;
 
-	if (!f->zbd_info)
-		return;
+	assert(f->zbd_info);
 
 	pthread_mutex_lock(&f->zbd_info->mutex);
 	refcount = --f->zbd_info->refcount;
@@ -592,7 +591,7 @@ static int zbd_init_zone_info(struct thread_data *td, struct fio_file *file)
 	return ret;
 }
 
-int zbd_init(struct thread_data *td)
+int zbd_setup_files(struct thread_data *td)
 {
 	struct fio_file *f;
 	int i;
diff --git a/zbd.h b/zbd.h
index 5a660399..e8dd3d6d 100644
--- a/zbd.h
+++ b/zbd.h
@@ -77,8 +77,8 @@ struct zoned_block_device_info {
 	struct fio_zone_info	zone_info[0];
 };
 
+int zbd_setup_files(struct thread_data *td);
 void zbd_free_zone_info(struct fio_file *f);
-int zbd_init(struct thread_data *td);
 void zbd_file_reset(struct thread_data *td, struct fio_file *f);
 bool zbd_unaligned_write(int error_code);
 void setup_zbd_zone_mode(struct thread_data *td, struct io_u *io_u);
@@ -87,6 +87,12 @@ enum fio_ddir zbd_adjust_ddir(struct thread_data *td, struct io_u *io_u,
 enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u);
 char *zbd_write_status(const struct thread_stat *ts);
 
+static inline void zbd_close_file(struct fio_file *f)
+{
+	if (f->zbd_info)
+		zbd_free_zone_info(f);
+}
+
 static inline void zbd_queue_io_u(struct io_u *io_u, enum fio_q_status status)
 {
 	if (io_u->zbd_queue_io) {
-- 
2.25.4

[PATCH v2 6/7] io_u: Optimize set_rw_ddir()

[Damien Le Moal]

There is no need to execute zbd_adjust_ddir() for a job that is not
using zonemode=zbd. So move the job mode test out of zbd_adjust_ddir()
and conditionally execute this function by first testing the job mode
in set_rw_ddir().

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 io_u.c | 3 ++-
 zbd.c  | 4 +---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/io_u.c b/io_u.c
index 18e94617..aa8808b8 100644
--- a/io_u.c
+++ b/io_u.c
@@ -746,7 +746,8 @@ static void set_rw_ddir(struct thread_data *td, struct io_u *io_u)
 {
 	enum fio_ddir ddir = get_rw_ddir(td);
 
-	ddir = zbd_adjust_ddir(td, io_u, ddir);
+	if (td->o.zone_mode == ZONE_MODE_ZBD)
+		ddir = zbd_adjust_ddir(td, io_u, ddir);
 
 	if (td_trimwrite(td)) {
 		struct fio_file *f = io_u->file;
diff --git a/zbd.c b/zbd.c
index db6d09f2..d8fc7ef5 100644
--- a/zbd.c
+++ b/zbd.c
@@ -1346,9 +1346,7 @@ enum fio_ddir zbd_adjust_ddir(struct thread_data *td, struct io_u *io_u,
 	 * devices with all empty zones. Overwrite the first I/O direction as
 	 * write to make sure data to read exists.
 	 */
-	if (td->o.zone_mode != ZONE_MODE_ZBD ||
-	    ddir != DDIR_READ ||
-	    !td_rw(td))
+	if (ddir != DDIR_READ || !td_rw(td))
 		return ddir;
 
 	if (io_u->file->zbd_info->sectors_with_data ||
-- 
2.25.4

[PATCH v2 7/7] t/zbd: Use max-jobs=16 option

[Damien Le Moal]

Use max-jobs option to reduce memory usage and speedup execution of
test-zbd-support.

With --max-jobs=16, twice the largest number of jobs used in all test
cases, the execution time of test-zbd-support against a zoned nullblk
device is lowered from 64s to 41s on a laptop.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 t/zbd/test-zbd-support | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index be889f34..de05f438 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -93,8 +93,8 @@ run_fio() {
 
     fio=$(dirname "$0")/../../fio
 
-    opts=("--aux-path=/tmp" "--allow_file_create=0" \
-			    "--significant_figures=10" "$@")
+    opts=("--max-jobs=16" "--aux-path=/tmp" "--allow_file_create=0" \
+	  "--significant_figures=10" "$@")
     opts+=(${var_opts[@]})
     { echo; echo "fio ${opts[*]}"; echo; } >>"${logfile}.${test_number}"
 
-- 
2.25.4

Re: [PATCH v2 0/7] Assorted bug fixes and improvements

[Damien Le Moal]

On 2020/05/08 16:56, Damien Le Moal wrote:
> The first 3 patches of this series fix bugs with write_iolog option
> initialization (crash on initialization failure) and with zonemode=zbd
> (one possible deadlock for read workloads and an assert trigerring with
> verify option enabled).
> 
> The following 4 patches are cleanups and improvements of zbd_xxx()
> functions to reduce unnecessary overhead when zonemode=zbd is not used.
> 
> Comments are welcome !
> 
> * Changes from v1:
>   - Dropped the patches introducing file and thread zbd function
>     pointers. Further reducing the overhead on fio core due to the
>     presence of zbd_xxx() functions will require major code surgery.
>     Until then, the new patches 4, 5 and 6 provide smaller improvements.
>   - Added patch 7 to improve tests execution time.
> 
> Damien Le Moal (7):
>   iolog: Fix write_iolog_close()
>   zbd: Fix potential deadlock on read operations
>   zbd: Fix read with verify
>   zbd: Optimize zbd_file_reset()
>   zbd: Rename zbd_init()
>   io_u: Optimize set_rw_ddir()
>   t/zbd: Use max-jobs=16 option
> 
>  filesetup.c            |  4 ++--
>  io_u.c                 |  3 ++-
>  iolog.c                |  3 +++
>  t/zbd/test-zbd-support |  4 ++--
>  zbd.c                  | 22 +++++++++-------------
>  zbd.h                  |  8 +++++++-
>  6 files changed, 25 insertions(+), 19 deletions(-)
> 

Jens,

Any comment on this series ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/7] Assorted bug fixes and improvements

[Jens Axboe]

On 5/8/20 1:56 AM, Damien Le Moal wrote:
> The first 3 patches of this series fix bugs with write_iolog option
> initialization (crash on initialization failure) and with zonemode=zbd
> (one possible deadlock for read workloads and an assert trigerring with
> verify option enabled).
> 
> The following 4 patches are cleanups and improvements of zbd_xxx()
> functions to reduce unnecessary overhead when zonemode=zbd is not used.
> 
> Comments are welcome !

Sorry for the delay, took a look at them and don't see anything
wrong. I'll get them applied, thanks.

-- 
Jens Axboe