[PATCH 0/2] block: change reqs_lock to QemuMutex

[PATCH 0/2] block: change reqs_lock to QemuMutex

[Stefan Hajnoczi]

As part of the ongoing multi-queue QEMU block layer work, I found that CoMutex
reqs_lock scales poorly when more IOThreads are added. These patches double
IOPS in the 4 IOThreads randread benchmark that I have been running with my
out-of-tree virtio-blk-iothread-vq-mapping branch
(https://gitlab.com/stefanha/qemu/-/commits/virtio-blk-iothread-vq-mapping).

These patches can be merged in preparation for virtio-blk multi-queue block
layer support.

Stefan Hajnoczi (2):
  block: minimize bs->reqs_lock section in tracked_request_end()
  block: change reqs_lock to QemuMutex

 include/block/block_int-common.h |  2 +-
 block.c                          |  4 +++-
 block/io.c                       | 30 ++++++++++++++++++------------
 3 files changed, 22 insertions(+), 14 deletions(-)

-- 
2.41.0

[PATCH 1/2] block: minimize bs->reqs_lock section in tracked_request_end()

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/io.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 055fcf7438..85d5176256 100644
--- a/block/io.c
+++ b/block/io.c
@@ -593,8 +593,14 @@ static void coroutine_fn tracked_request_end(BdrvTrackedRequest *req)
 
     qemu_co_mutex_lock(&req->bs->reqs_lock);
     QLIST_REMOVE(req, list);
+    qemu_co_mutex_unlock(&req->bs->reqs_lock);
+
+    /*
+     * At this point qemu_co_queue_wait(&req->wait_queue, ...) won't be called
+     * anymore because the request has been removed from the list, so it's safe
+     * to restart the queue outside reqs_lock to minimize the critical section.
+     */
     qemu_co_queue_restart_all(&req->wait_queue);
-    qemu_co_mutex_unlock(&req->bs->reqs_lock);
 }
 
 /**
-- 
2.41.0

[PATCH 2/2] block: change reqs_lock to QemuMutex

[Stefan Hajnoczi]

CoMutex has poor performance when lock contention is high. The tracked
requests list is accessed frequently and performance suffers in QEMU
multi-queue block layer scenarios.

It is not necessary to use CoMutex for the requests lock. The lock is
always released across coroutine yield operations. It is held for
relatively short periods of time and it is not beneficial to yield when
the lock is held by another coroutine.

Change the lock type from CoMutex to QemuMutex to improve multi-queue
block layer performance. fio randread bs=4k iodepth=64 with 4 IOThreads
handling a virtio-blk device with 8 virtqueues improves from 254k to
517k IOPS (+203%). Full benchmark results and configuration details are
available here:
https://gitlab.com/stefanha/virt-playbooks/-/commit/980c40845d540e3669add1528739503c2e817b57

In the future we may wish to introduce thread-local tracked requests
lists to avoid lock contention completely. That would be much more
involved though.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/block/block_int-common.h |  2 +-
 block.c                          |  4 +++-
 block/io.c                       | 24 ++++++++++++------------
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 74195c3004..7a1e678031 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1231,7 +1231,7 @@ struct BlockDriverState {
     unsigned int write_gen;               /* Current data generation */
 
     /* Protected by reqs_lock.  */
-    CoMutex reqs_lock;
+    QemuMutex reqs_lock;
     QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
     CoQueue flush_queue;                  /* Serializing flush queue */
     bool active_flush_req;                /* Flush request in flight? */
diff --git a/block.c b/block.c
index a307c151a8..989131156a 100644
--- a/block.c
+++ b/block.c
@@ -415,7 +415,7 @@ BlockDriverState *bdrv_new(void)
     for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
         QLIST_INIT(&bs->op_blockers[i]);
     }
-    qemu_co_mutex_init(&bs->reqs_lock);
+    qemu_mutex_init(&bs->reqs_lock);
     qemu_mutex_init(&bs->dirty_bitmap_mutex);
     bs->refcnt = 1;
     bs->aio_context = qemu_get_aio_context();
@@ -5476,6 +5476,8 @@ static void bdrv_delete(BlockDriverState *bs)
 
     bdrv_close(bs);
 
+    qemu_mutex_destroy(&bs->reqs_lock);
+
     g_free(bs);
 }
 
diff --git a/block/io.c b/block/io.c
index 85d5176256..35be335d2a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -591,9 +591,9 @@ static void coroutine_fn tracked_request_end(BdrvTrackedRequest *req)
         qatomic_dec(&req->bs->serialising_in_flight);
     }
 
-    qemu_co_mutex_lock(&req->bs->reqs_lock);
+    qemu_mutex_lock(&req->bs->reqs_lock);
     QLIST_REMOVE(req, list);
-    qemu_co_mutex_unlock(&req->bs->reqs_lock);
+    qemu_mutex_unlock(&req->bs->reqs_lock);
 
     /*
      * At this point qemu_co_queue_wait(&req->wait_queue, ...) won't be called
@@ -627,9 +627,9 @@ static void coroutine_fn tracked_request_begin(BdrvTrackedRequest *req,
 
     qemu_co_queue_init(&req->wait_queue);
 
-    qemu_co_mutex_lock(&bs->reqs_lock);
+    qemu_mutex_lock(&bs->reqs_lock);
     QLIST_INSERT_HEAD(&bs->tracked_requests, req, list);
-    qemu_co_mutex_unlock(&bs->reqs_lock);
+    qemu_mutex_unlock(&bs->reqs_lock);
 }
 
 static bool tracked_request_overlaps(BdrvTrackedRequest *req,
@@ -793,9 +793,9 @@ bdrv_wait_serialising_requests(BdrvTrackedRequest *self)
         return;
     }
 
-    qemu_co_mutex_lock(&bs->reqs_lock);
+    qemu_mutex_lock(&bs->reqs_lock);
     bdrv_wait_serialising_requests_locked(self);
-    qemu_co_mutex_unlock(&bs->reqs_lock);
+    qemu_mutex_unlock(&bs->reqs_lock);
 }
 
 void coroutine_fn bdrv_make_request_serialising(BdrvTrackedRequest *req,
@@ -803,12 +803,12 @@ void coroutine_fn bdrv_make_request_serialising(BdrvTrackedRequest *req,
 {
     IO_CODE();
 
-    qemu_co_mutex_lock(&req->bs->reqs_lock);
+    qemu_mutex_lock(&req->bs->reqs_lock);
 
     tracked_request_set_serialising(req, align);
     bdrv_wait_serialising_requests_locked(req);
 
-    qemu_co_mutex_unlock(&req->bs->reqs_lock);
+    qemu_mutex_unlock(&req->bs->reqs_lock);
 }
 
 int bdrv_check_qiov_request(int64_t offset, int64_t bytes,
@@ -3002,7 +3002,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
         goto early_exit;
     }
 
-    qemu_co_mutex_lock(&bs->reqs_lock);
+    qemu_mutex_lock(&bs->reqs_lock);
     current_gen = qatomic_read(&bs->write_gen);
 
     /* Wait until any previous flushes are completed */
@@ -3012,7 +3012,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 
     /* Flushes reach this point in nondecreasing current_gen order.  */
     bs->active_flush_req = true;
-    qemu_co_mutex_unlock(&bs->reqs_lock);
+    qemu_mutex_unlock(&bs->reqs_lock);
 
     /* Write back all layers by calling one driver function */
     if (bs->drv->bdrv_co_flush) {
@@ -3100,11 +3100,11 @@ out:
         bs->flushed_gen = current_gen;
     }
 
-    qemu_co_mutex_lock(&bs->reqs_lock);
+    qemu_mutex_lock(&bs->reqs_lock);
     bs->active_flush_req = false;
     /* Return value is ignored - it's ok if wait queue is empty */
     qemu_co_queue_next(&bs->flush_queue);
-    qemu_co_mutex_unlock(&bs->reqs_lock);
+    qemu_mutex_unlock(&bs->reqs_lock);
 
 early_exit:
     bdrv_dec_in_flight(bs);
-- 
2.41.0

Re: [PATCH 1/2] block: minimize bs->reqs_lock section in tracked_request_end()

[Eric Blake]

On Tue, Aug 08, 2023 at 11:58:51AM -0400, Stefan Hajnoczi wrote:
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/io.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/io.c b/block/io.c
> index 055fcf7438..85d5176256 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -593,8 +593,14 @@ static void coroutine_fn tracked_request_end(BdrvTrackedRequest *req)
>  
>      qemu_co_mutex_lock(&req->bs->reqs_lock);
>      QLIST_REMOVE(req, list);
> +    qemu_co_mutex_unlock(&req->bs->reqs_lock);
> +
> +    /*
> +     * At this point qemu_co_queue_wait(&req->wait_queue, ...) won't be called
> +     * anymore because the request has been removed from the list, so it's safe
> +     * to restart the queue outside reqs_lock to minimize the critical section.
> +     */
>      qemu_co_queue_restart_all(&req->wait_queue);
> -    qemu_co_mutex_unlock(&req->bs->reqs_lock);
>  }
>  
>  /**
> -- 
> 2.41.0
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 2/2] block: change reqs_lock to QemuMutex

[Eric Blake]

On Tue, Aug 08, 2023 at 11:58:52AM -0400, Stefan Hajnoczi wrote:
> CoMutex has poor performance when lock contention is high. The tracked
> requests list is accessed frequently and performance suffers in QEMU
> multi-queue block layer scenarios.
> 
> It is not necessary to use CoMutex for the requests lock. The lock is
> always released across coroutine yield operations. It is held for
> relatively short periods of time and it is not beneficial to yield when
> the lock is held by another coroutine.
> 
> Change the lock type from CoMutex to QemuMutex to improve multi-queue
> block layer performance. fio randread bs=4k iodepth=64 with 4 IOThreads
> handling a virtio-blk device with 8 virtqueues improves from 254k to
> 517k IOPS (+203%). Full benchmark results and configuration details are
> available here:
> https://gitlab.com/stefanha/virt-playbooks/-/commit/980c40845d540e3669add1528739503c2e817b57

Nice!

> 
> In the future we may wish to introduce thread-local tracked requests
> lists to avoid lock contention completely. That would be much more
> involved though.

Indeed.

> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/block/block_int-common.h |  2 +-
>  block.c                          |  4 +++-
>  block/io.c                       | 24 ++++++++++++------------
>  3 files changed, 16 insertions(+), 14 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 0/2] block: change reqs_lock to QemuMutex

[Kevin Wolf]

Am 08.08.2023 um 17:58 hat Stefan Hajnoczi geschrieben:
> As part of the ongoing multi-queue QEMU block layer work, I found that CoMutex
> reqs_lock scales poorly when more IOThreads are added. These patches double
> IOPS in the 4 IOThreads randread benchmark that I have been running with my
> out-of-tree virtio-blk-iothread-vq-mapping branch
> (https://gitlab.com/stefanha/qemu/-/commits/virtio-blk-iothread-vq-mapping).
> 
> These patches can be merged in preparation for virtio-blk multi-queue block
> layer support.

Thanks, applied to the block branch.

Kevin