[PATCH 0/6] Fix zone lock deadlock

[PATCH 0/6] Fix zone lock deadlock

[Naohiro Aota]

With zonemode=zbd and asynchronous ioengine, a thread takes a zone lock
before an I/O submission (in zbd_adjust_block() or
zbd_convert_to_open_zone()) and releases the lock after the I/O is put (in
zbd_put_io()).  With a small number of open zones and/or a large number of
jobs, threads can easily end up circular lock dependency and deadlocks. For
example, thread A sends an I/O to zone 0, so thread A holds a zone lock #0.
Then, thread A continues on zone 1 and try to acquire zone lock #1. At the
same time, thread B held zone lock #1, sent I/O to zone 1, and try to
acquire zone #0. Now, both threads are waiting for each other's lock, which
is never released.

This series fixes three problems to eliminate the deadlock. First, taking
all the zone locks should be avoided. zbd_process_swd() and
zbd_reset_zones() take the lock for all zones of the specified device,
preventing other threads from accessing different zones in parallel. While
it is not the root cause of the deadlock, such all zone locking easily
trigger a deadlock. So, this series reduces such contentions by (1)
eliminating unnecessary invocation of zbd_process_swd() and (2) changing to
take single zone at at time in zbd_reset_zones().

Secondly, zbd's I/O issuing path should expect lock contention with other
threads and handle the case properly. Commit 6f0c608564c3 ("zbd: Avoid
async I/O multi-job workload deadlock") also addressed this issue by using
pthread_mutex_try_lock() and io_u_quiesce(). However, there are more
pthread_mutex_lock() left to be fixed in the same way.

Finally, fio should clean up I/Os properly on an error case. Currently,
cleanup_pending_aio() and io_u_quiesce() fail to clean up I/Os with an
error. As a result, zone locks, which are held by an erroneous thread, are
kept held and blocks other threads to acquire the locks.

This series also add a test case to cause the deadlock with unpatched fio.

Patches 1 and 2 avoid long range lock holding to reduce lock contentions.

Patch 3 introduces zone_lock and use it to handle the lock contention case.

Patches 4 and 5 fix error path to clean up all the pending I/Os left.

Patch 6 adds the test.

Naohiro Aota (6):
  zbd: avoid initializing swd when unnecessary
  zbd: reset one zone at a time
  zbd: use zone_lock to lock a zone
  backend: always clean up pending aios
  io_u: ensure io_u_quiesce() to process all the IOs
  zbd: add test for stressing zone locking

 backend.c              |  5 ---
 io_u.c                 |  6 +--
 t/zbd/test-zbd-support | 30 +++++++++++++++
 zbd.c                  | 84 ++++++++++++++++--------------------------
 4 files changed, 65 insertions(+), 60 deletions(-)

-- 
2.25.1

[PATCH 1/6] zbd: avoid initializing swd when unnecessary

[Naohiro Aota]

When enable_check_swd == false, there is no use to initialize swd. Just
disable it in this case.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 zbd.c | 3 +++
 1 file changed, 3 insertions(+)

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

[PATCH 2/6] zbd: reset one zone at a time

[Naohiro Aota]

zbd_rest_zones() currently scans over device zones and try to reset as much
zones as possible at a time. However, this routine takes all the lock on
the range and causes a lot of lock contentions with other threads.

This commit change the behavior to hold the lock and examine one zone at a
time. While it will increase the number of ioctl() call when it need to
reset contiguous, the overhead of increased number of ioctl()s are anyway
amortized by device side's reset performance.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 zbd.c | 42 +++++++-----------------------------------
 1 file changed, 7 insertions(+), 35 deletions(-)

diff --git a/zbd.c b/zbd.c
index ddf1e6f3e008..18a55ea46ef9 100644
--- a/zbd.c
+++ b/zbd.c
@@ -707,7 +707,7 @@ 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, bool all_zones)
 {
-	struct fio_zone_info *z, *start_z = ze;
+	struct fio_zone_info *z;
 	const uint32_t min_bs = td->o.min_bs[DDIR_WRITE];
 	bool reset_wp;
 	int res = 0;
@@ -717,48 +717,20 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
 	assert(f->fd != -1);
 	for (z = zb; z < ze; z++) {
 		pthread_mutex_lock(&z->mutex);
-		switch (z->type) {
-		case BLK_ZONE_TYPE_SEQWRITE_REQ:
+		if (z->type == BLK_ZONE_TYPE_SEQWRITE_REQ) {
 			reset_wp = all_zones ? z->wp != z->start :
 					(td->o.td_ddir & TD_DDIR_WRITE) &&
 					z->wp % min_bs != 0;
-			if (start_z == ze && reset_wp) {
-				start_z = z;
-			} else if (start_z < ze && !reset_wp) {
-				dprint(FD_ZBD,
-				       "%s: resetting zones %u .. %u\n",
+			if (reset_wp) {
+				dprint(FD_ZBD, "%s: resetting zone %u\n",
 				       f->file_name,
-					zbd_zone_nr(f->zbd_info, start_z),
-					zbd_zone_nr(f->zbd_info, z));
-				if (zbd_reset_range(td, f, start_z->start,
-						z->start - start_z->start) < 0)
+				       zbd_zone_nr(f->zbd_info, z));
+				if (zbd_reset_zone(td, f, z) < 0)
 					res = 1;
-				start_z = ze;
 			}
-			break;
-		default:
-			if (start_z == ze)
-				break;
-			dprint(FD_ZBD, "%s: resetting zones %u .. %u\n",
-			       f->file_name, zbd_zone_nr(f->zbd_info, start_z),
-			       zbd_zone_nr(f->zbd_info, z));
-			if (zbd_reset_range(td, f, start_z->start,
-					    z->start - start_z->start) < 0)
-				res = 1;
-			start_z = ze;
-			break;
 		}
-	}
-	if (start_z < ze) {
-		dprint(FD_ZBD, "%s: resetting zones %u .. %u\n", f->file_name,
-			zbd_zone_nr(f->zbd_info, start_z),
-			zbd_zone_nr(f->zbd_info, z));
-		if (zbd_reset_range(td, f, start_z->start,
-				    z->start - start_z->start) < 0)
-			res = 1;
-	}
-	for (z = zb; z < ze; z++)
 		pthread_mutex_unlock(&z->mutex);
+	}
 
 	return res;
 }
-- 
2.25.1

[PATCH 3/6] zbd: use zone_lock to lock a zone

[Naohiro Aota]

commit 6f0c608564c3 ("zbd: Avoid async I/O multi-job workload deadlock")
introduced io_u_quiesce() when it failed to lock a zone to avoid deadlock.
This situation can happen on the other locking place like
zbd_convert_to_open_zone(). Thus, introduce common helper "zone_lock" to
lock a zone.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 zbd.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/zbd.c b/zbd.c
index 18a55ea46ef9..b2d9424972da 100644
--- a/zbd.c
+++ b/zbd.c
@@ -927,6 +927,24 @@ static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
 	f->zbd_info->zone_info[zone_idx].open = 0;
 }
 
+static void zone_lock(struct thread_data *td, struct fio_zone_info *z)
+{
+	/*
+	 * 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);
+	}
+}
+
 /*
  * Modify the offset of an I/O unit that does not refer to an open zone such
  * that it refers to an open zone. Close an open zone and open a new zone if
@@ -969,7 +987,7 @@ static struct fio_zone_info *zbd_convert_to_open_zone(struct thread_data *td,
 	for (;;) {
 		z = &f->zbd_info->zone_info[zone_idx];
 
-		pthread_mutex_lock(&z->mutex);
+		zone_lock(td, z);
 		pthread_mutex_lock(&f->zbd_info->mutex);
 		if (td->o.max_open_zones == 0)
 			goto examine_zone;
@@ -1017,7 +1035,7 @@ examine_zone:
 			z = &f->zbd_info->zone_info[zone_idx];
 		}
 		assert(is_valid_offset(f, z->start));
-		pthread_mutex_lock(&z->mutex);
+		zone_lock(td, z);
 		if (z->open)
 			continue;
 		if (zbd_open_zone(td, io_u, zone_idx))
@@ -1035,7 +1053,7 @@ examine_zone:
 
 		z = &f->zbd_info->zone_info[zone_idx];
 
-		pthread_mutex_lock(&z->mutex);
+		zone_lock(td, z);
 		if (z->wp + min_bs <= (z+1)->start)
 			goto out;
 		pthread_mutex_lock(&f->zbd_info->mutex);
@@ -1321,20 +1339,7 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 
 	zbd_check_swd(f);
 
-	/*
-	 * 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(&zb->mutex) != 0) {
-		if (!td_ioengine_flagged(td, FIO_SYNCIO))
-			io_u_quiesce(td);
-		pthread_mutex_lock(&zb->mutex);
-	}
+	zone_lock(td, zb);
 
 	switch (io_u->ddir) {
 	case DDIR_READ:
-- 
2.25.1

[PATCH 4/6] backend: always clean up pending aios

[Naohiro Aota]

cleanup_pending_aios() is called when a thread exits with error, so all the
call site of this function is under "if (td->error)". However, commit
d28174f0189c ("workqueue: ensure we see deferred error for IOs"), for some
reason, added "if (td->error) return" at the head of this function, making
this function practically void. Revert this part to ensure cleaning up
pending aios.

Besides, cleanup_pending_aios() should not return even when
io_u_queued_complete() failed. Because, it keeps in-flight aios left.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 backend.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/backend.c b/backend.c
index 936203dcb1c4..feb34e51382f 100644
--- a/backend.c
+++ b/backend.c
@@ -237,15 +237,10 @@ static void cleanup_pending_aio(struct thread_data *td)
 {
 	int r;
 
-	if (td->error)
-		return;
-
 	/*
 	 * get immediately available events, if any
 	 */
 	r = io_u_queued_complete(td, 0);
-	if (r < 0)
-		return;
 
 	/*
 	 * now cancel remaining active events
-- 
2.25.1

[PATCH 5/6] io_u: ensure io_u_quiesce() to process all the IOs

[Naohiro Aota]

Currently, when IO have an error io_u_quiesce() stops processing
in-flight IOs there and leaves other IOs non-completed. This is not a
desired behavior for io_u_quiesce(). Fix it by continuing even on
error.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 io_u.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/io_u.c b/io_u.c
index bcb893c5abad..5d62a76cb5a7 100644
--- a/io_u.c
+++ b/io_u.c
@@ -606,7 +606,7 @@ static inline enum fio_ddir get_rand_ddir(struct thread_data *td)
 
 int io_u_quiesce(struct thread_data *td)
 {
-	int ret = 0, completed = 0;
+	int ret = 0, completed = 0, err = 0;
 
 	/*
 	 * We are going to sleep, ensure that we flush anything pending as
@@ -625,7 +625,7 @@ int io_u_quiesce(struct thread_data *td)
 		if (ret > 0)
 			completed += ret;
 		else if (ret < 0)
-			break;
+			err = ret;
 	}
 
 	if (td->flags & TD_F_REGROW_LOGS)
@@ -634,7 +634,7 @@ int io_u_quiesce(struct thread_data *td)
 	if (completed)
 		return completed;
 
-	return ret;
+	return err;
 }
 
 static enum fio_ddir rate_ddir(struct thread_data *td, enum fio_ddir ddir)
-- 
2.25.1

[PATCH 6/6] zbd: add test for stressing zone locking

[Naohiro Aota]

Add a test to stress zone locking mechanism by having a large number of
threads with a small number of max_open_zones. Run 30 seconds time-based
fio under the timeout command. After 45 seconds, "timeout" kill -KILL the
fio process. If a zone lock deadlocks, fio is killed by the timeout
command, and this test fails. If not, fio runs to the end and this test
success.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 t/zbd/test-zbd-support | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index 5d079a8b7873..bd41fffb3298 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -755,6 +755,36 @@ test47() {
     grep -q 'zoneskip 1 is not a multiple of the device zone size' "${logfile}.${test_number}"
 }
 
+# Multiple overlapping random write jobs for the same drive and with a
+# limited number of open zones. This is similar to test29, but uses libaio
+# to stress test zone locking.
+test48() {
+    local i jobs=16 off opts=()
+
+    off=$((first_sequential_zone_sector * 512 + 64 * zone_size))
+    size=$((16*zone_size))
+    [ -n "$is_zbd" ] && reset_zone "$dev" $((off / 512))
+    opts=("--aux-path=/tmp" "--allow_file_create=0" "--significant_figures=10")
+    opts+=("--debug=zbd")
+    opts+=("--ioengine=libaio" "--rw=randwrite" "--direct=1")
+    opts+=("--time_based" "--runtime=30")
+    opts+=("--zonemode=zbd" "--zonesize=${zone_size}")
+    opts+=("--max_open_zones=4")
+    for ((i=0;i<jobs;i++)); do
+	opts+=("--name=job$i" "--filename=$dev" "--offset=$off" "--bs=16K")
+	opts+=("--io_size=$zone_size" "--iodepth=256" "--thread=1")
+	opts+=("--group_reporting=1")
+    done
+
+    fio=$(dirname "$0")/../../fio
+
+    { echo; echo "fio ${opts[*]}"; echo; } >>"${logfile}.${test_number}"
+
+    timeout -v -s KILL 45s \
+	    "${dynamic_analyzer[@]}" "$fio" "${opts[@]}" \
+	    >> "${logfile}.${test_number}" 2>&1 || return $?
+}
+
 tests=()
 dynamic_analyzer=()
 reset_all_zones=
-- 
2.25.1

Re: [PATCH 0/6] Fix zone lock deadlock

[Naohiro Aota]

On Fri, Feb 28, 2020 at 04:12:42PM +0900, Naohiro Aota wrote:
>With zonemode=zbd and asynchronous ioengine, a thread takes a zone lock
>before an I/O submission (in zbd_adjust_block() or
>zbd_convert_to_open_zone()) and releases the lock after the I/O is put (in
>zbd_put_io()).  With a small number of open zones and/or a large number of
>jobs, threads can easily end up circular lock dependency and deadlocks. For
>example, thread A sends an I/O to zone 0, so thread A holds a zone lock #0.
>Then, thread A continues on zone 1 and try to acquire zone lock #1. At the
>same time, thread B held zone lock #1, sent I/O to zone 1, and try to
>acquire zone #0. Now, both threads are waiting for each other's lock, which
>is never released.
>
>This series fixes three problems to eliminate the deadlock. First, taking
>all the zone locks should be avoided. zbd_process_swd() and
>zbd_reset_zones() take the lock for all zones of the specified device,
>preventing other threads from accessing different zones in parallel. While
>it is not the root cause of the deadlock, such all zone locking easily
>trigger a deadlock. So, this series reduces such contentions by (1)
>eliminating unnecessary invocation of zbd_process_swd() and (2) changing to
>take single zone at at time in zbd_reset_zones().
>
>Secondly, zbd's I/O issuing path should expect lock contention with other
>threads and handle the case properly. Commit 6f0c608564c3 ("zbd: Avoid
>async I/O multi-job workload deadlock") also addressed this issue by using
>pthread_mutex_try_lock() and io_u_quiesce(). However, there are more
>pthread_mutex_lock() left to be fixed in the same way.
>
>Finally, fio should clean up I/Os properly on an error case. Currently,
>cleanup_pending_aio() and io_u_quiesce() fail to clean up I/Os with an
>error. As a result, zone locks, which are held by an erroneous thread, are
>kept held and blocks other threads to acquire the locks.
>
>This series also add a test case to cause the deadlock with unpatched fio.
>
>Patches 1 and 2 avoid long range lock holding to reduce lock contentions.
>
>Patch 3 introduces zone_lock and use it to handle the lock contention case.
>
>Patches 4 and 5 fix error path to clean up all the pending I/Os left.
>
>Patch 6 adds the test.
>
>Naohiro Aota (6):
>  zbd: avoid initializing swd when unnecessary
>  zbd: reset one zone at a time
>  zbd: use zone_lock to lock a zone
>  backend: always clean up pending aios
>  io_u: ensure io_u_quiesce() to process all the IOs
>  zbd: add test for stressing zone locking
>
> backend.c              |  5 ---
> io_u.c                 |  6 +--
> t/zbd/test-zbd-support | 30 +++++++++++++++
> zbd.c                  | 84 ++++++++++++++++--------------------------
> 4 files changed, 65 insertions(+), 60 deletions(-)
>
>-- 
>2.25.1

Ping on this series (also on this
https://www.spinics.net/lists/fio/msg08322.html ).

Re: [PATCH 0/6] Fix zone lock deadlock

[Jens Axboe]

On 3/16/20 8:50 PM, Naohiro Aota wrote:
> On Fri, Feb 28, 2020 at 04:12:42PM +0900, Naohiro Aota wrote:
>> With zonemode=zbd and asynchronous ioengine, a thread takes a zone lock
>> before an I/O submission (in zbd_adjust_block() or
>> zbd_convert_to_open_zone()) and releases the lock after the I/O is put (in
>> zbd_put_io()).  With a small number of open zones and/or a large number of
>> jobs, threads can easily end up circular lock dependency and deadlocks. For
>> example, thread A sends an I/O to zone 0, so thread A holds a zone lock #0.
>> Then, thread A continues on zone 1 and try to acquire zone lock #1. At the
>> same time, thread B held zone lock #1, sent I/O to zone 1, and try to
>> acquire zone #0. Now, both threads are waiting for each other's lock, which
>> is never released.
>>
>> This series fixes three problems to eliminate the deadlock. First, taking
>> all the zone locks should be avoided. zbd_process_swd() and
>> zbd_reset_zones() take the lock for all zones of the specified device,
>> preventing other threads from accessing different zones in parallel. While
>> it is not the root cause of the deadlock, such all zone locking easily
>> trigger a deadlock. So, this series reduces such contentions by (1)
>> eliminating unnecessary invocation of zbd_process_swd() and (2) changing to
>> take single zone at at time in zbd_reset_zones().
>>
>> Secondly, zbd's I/O issuing path should expect lock contention with other
>> threads and handle the case properly. Commit 6f0c608564c3 ("zbd: Avoid
>> async I/O multi-job workload deadlock") also addressed this issue by using
>> pthread_mutex_try_lock() and io_u_quiesce(). However, there are more
>> pthread_mutex_lock() left to be fixed in the same way.
>>
>> Finally, fio should clean up I/Os properly on an error case. Currently,
>> cleanup_pending_aio() and io_u_quiesce() fail to clean up I/Os with an
>> error. As a result, zone locks, which are held by an erroneous thread, are
>> kept held and blocks other threads to acquire the locks.
>>
>> This series also add a test case to cause the deadlock with unpatched fio.
>>
>> Patches 1 and 2 avoid long range lock holding to reduce lock contentions.
>>
>> Patch 3 introduces zone_lock and use it to handle the lock contention case.
>>
>> Patches 4 and 5 fix error path to clean up all the pending I/Os left.
>>
>> Patch 6 adds the test.
>>
>> Naohiro Aota (6):
>>  zbd: avoid initializing swd when unnecessary
>>  zbd: reset one zone at a time
>>  zbd: use zone_lock to lock a zone
>>  backend: always clean up pending aios
>>  io_u: ensure io_u_quiesce() to process all the IOs
>>  zbd: add test for stressing zone locking
>>
>> backend.c              |  5 ---
>> io_u.c                 |  6 +--
>> t/zbd/test-zbd-support | 30 +++++++++++++++
>> zbd.c                  | 84 ++++++++++++++++--------------------------
>> 4 files changed, 65 insertions(+), 60 deletions(-)
>>
>> -- 
>> 2.25.1
> 
> Ping on this series (also on this
> https://www.spinics.net/lists/fio/msg08322.html ).

Damien, can you take a look please?

-- 
Jens Axboe

Re: [PATCH 0/6] Fix zone lock deadlock

[Damien Le Moal]

On 2020/03/18 10:49, Jens Axboe wrote:
> On 3/16/20 8:50 PM, Naohiro Aota wrote:
>> On Fri, Feb 28, 2020 at 04:12:42PM +0900, Naohiro Aota wrote:
>>> With zonemode=zbd and asynchronous ioengine, a thread takes a zone lock
>>> before an I/O submission (in zbd_adjust_block() or
>>> zbd_convert_to_open_zone()) and releases the lock after the I/O is put (in
>>> zbd_put_io()).  With a small number of open zones and/or a large number of
>>> jobs, threads can easily end up circular lock dependency and deadlocks. For
>>> example, thread A sends an I/O to zone 0, so thread A holds a zone lock #0.
>>> Then, thread A continues on zone 1 and try to acquire zone lock #1. At the
>>> same time, thread B held zone lock #1, sent I/O to zone 1, and try to
>>> acquire zone #0. Now, both threads are waiting for each other's lock, which
>>> is never released.
>>>
>>> This series fixes three problems to eliminate the deadlock. First, taking
>>> all the zone locks should be avoided. zbd_process_swd() and
>>> zbd_reset_zones() take the lock for all zones of the specified device,
>>> preventing other threads from accessing different zones in parallel. While
>>> it is not the root cause of the deadlock, such all zone locking easily
>>> trigger a deadlock. So, this series reduces such contentions by (1)
>>> eliminating unnecessary invocation of zbd_process_swd() and (2) changing to
>>> take single zone at at time in zbd_reset_zones().
>>>
>>> Secondly, zbd's I/O issuing path should expect lock contention with other
>>> threads and handle the case properly. Commit 6f0c608564c3 ("zbd: Avoid
>>> async I/O multi-job workload deadlock") also addressed this issue by using
>>> pthread_mutex_try_lock() and io_u_quiesce(). However, there are more
>>> pthread_mutex_lock() left to be fixed in the same way.
>>>
>>> Finally, fio should clean up I/Os properly on an error case. Currently,
>>> cleanup_pending_aio() and io_u_quiesce() fail to clean up I/Os with an
>>> error. As a result, zone locks, which are held by an erroneous thread, are
>>> kept held and blocks other threads to acquire the locks.
>>>
>>> This series also add a test case to cause the deadlock with unpatched fio.
>>>
>>> Patches 1 and 2 avoid long range lock holding to reduce lock contentions.
>>>
>>> Patch 3 introduces zone_lock and use it to handle the lock contention case.
>>>
>>> Patches 4 and 5 fix error path to clean up all the pending I/Os left.
>>>
>>> Patch 6 adds the test.
>>>
>>> Naohiro Aota (6):
>>>  zbd: avoid initializing swd when unnecessary
>>>  zbd: reset one zone at a time
>>>  zbd: use zone_lock to lock a zone
>>>  backend: always clean up pending aios
>>>  io_u: ensure io_u_quiesce() to process all the IOs
>>>  zbd: add test for stressing zone locking
>>>
>>> backend.c              |  5 ---
>>> io_u.c                 |  6 +--
>>> t/zbd/test-zbd-support | 30 +++++++++++++++
>>> zbd.c                  | 84 ++++++++++++++++--------------------------
>>> 4 files changed, 65 insertions(+), 60 deletions(-)
>>>
>>> -- 
>>> 2.25.1
>>
>> Ping on this series (also on this
>> https://www.spinics.net/lists/fio/msg08322.html ).
> 
> Damien, can you take a look please?
> 

My apologies. I should have sent a reviewed-by tag earlier.
I worked on these with Naohiro (who did all the heavy lifting).
Tested too and at least for SMR disks workloads, no problems detected.

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

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/6] Fix zone lock deadlock

[Jens Axboe]

On 3/17/20 8:00 PM, Damien Le Moal wrote:
> On 2020/03/18 10:49, Jens Axboe wrote:
>> On 3/16/20 8:50 PM, Naohiro Aota wrote:
>>> On Fri, Feb 28, 2020 at 04:12:42PM +0900, Naohiro Aota wrote:
>>>> With zonemode=zbd and asynchronous ioengine, a thread takes a zone lock
>>>> before an I/O submission (in zbd_adjust_block() or
>>>> zbd_convert_to_open_zone()) and releases the lock after the I/O is put (in
>>>> zbd_put_io()).  With a small number of open zones and/or a large number of
>>>> jobs, threads can easily end up circular lock dependency and deadlocks. For
>>>> example, thread A sends an I/O to zone 0, so thread A holds a zone lock #0.
>>>> Then, thread A continues on zone 1 and try to acquire zone lock #1. At the
>>>> same time, thread B held zone lock #1, sent I/O to zone 1, and try to
>>>> acquire zone #0. Now, both threads are waiting for each other's lock, which
>>>> is never released.
>>>>
>>>> This series fixes three problems to eliminate the deadlock. First, taking
>>>> all the zone locks should be avoided. zbd_process_swd() and
>>>> zbd_reset_zones() take the lock for all zones of the specified device,
>>>> preventing other threads from accessing different zones in parallel. While
>>>> it is not the root cause of the deadlock, such all zone locking easily
>>>> trigger a deadlock. So, this series reduces such contentions by (1)
>>>> eliminating unnecessary invocation of zbd_process_swd() and (2) changing to
>>>> take single zone at at time in zbd_reset_zones().
>>>>
>>>> Secondly, zbd's I/O issuing path should expect lock contention with other
>>>> threads and handle the case properly. Commit 6f0c608564c3 ("zbd: Avoid
>>>> async I/O multi-job workload deadlock") also addressed this issue by using
>>>> pthread_mutex_try_lock() and io_u_quiesce(). However, there are more
>>>> pthread_mutex_lock() left to be fixed in the same way.
>>>>
>>>> Finally, fio should clean up I/Os properly on an error case. Currently,
>>>> cleanup_pending_aio() and io_u_quiesce() fail to clean up I/Os with an
>>>> error. As a result, zone locks, which are held by an erroneous thread, are
>>>> kept held and blocks other threads to acquire the locks.
>>>>
>>>> This series also add a test case to cause the deadlock with unpatched fio.
>>>>
>>>> Patches 1 and 2 avoid long range lock holding to reduce lock contentions.
>>>>
>>>> Patch 3 introduces zone_lock and use it to handle the lock contention case.
>>>>
>>>> Patches 4 and 5 fix error path to clean up all the pending I/Os left.
>>>>
>>>> Patch 6 adds the test.
>>>>
>>>> Naohiro Aota (6):
>>>>  zbd: avoid initializing swd when unnecessary
>>>>  zbd: reset one zone at a time
>>>>  zbd: use zone_lock to lock a zone
>>>>  backend: always clean up pending aios
>>>>  io_u: ensure io_u_quiesce() to process all the IOs
>>>>  zbd: add test for stressing zone locking
>>>>
>>>> backend.c              |  5 ---
>>>> io_u.c                 |  6 +--
>>>> t/zbd/test-zbd-support | 30 +++++++++++++++
>>>> zbd.c                  | 84 ++++++++++++++++--------------------------
>>>> 4 files changed, 65 insertions(+), 60 deletions(-)
>>>>
>>>> -- 
>>>> 2.25.1
>>>
>>> Ping on this series (also on this
>>> https://www.spinics.net/lists/fio/msg08322.html ).
>>
>> Damien, can you take a look please?
>>
> 
> My apologies. I should have sent a reviewed-by tag earlier.
> I worked on these with Naohiro (who did all the heavy lifting).
> Tested too and at least for SMR disks workloads, no problems detected.
> 
> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
> Tested-by: Damien Le Moal <damien.lemoal@wdc.com>

Ah great, just wanted to double check. Thanks, series applied.

-- 
Jens Axboe