[PATCH 0/3] block: protect BlockBackend->queued_requests with a lock

[PATCH 0/3] block: protect BlockBackend->queued_requests with a lock

[Stefan Hajnoczi]

QEMU block layer multi-queue support involves running I/O requests from
multiple threads. Shared state must be protected somehow to avoid thread-safety
issues.

The BlockBackend->queued_requests CoQueue is accessed without a lock and will
likely be corrupted when multiple threads queue requests at the same time.

This patch series make BlockBackend->queued_requests thread-safe.

Stefan Hajnoczi (3):
  block: make BlockBackend->quiesce_counter atomic
  block: make BlockBackend->disable_request_queuing atomic
  block: protect BlockBackend->queued_requests with a lock

 block/block-backend.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

-- 
2.39.2

[PATCH 1/3] block: make BlockBackend->quiesce_counter atomic

[Stefan Hajnoczi]

The main loop thread increments/decrements BlockBackend->quiesce_counter
when drained sections begin/end. The counter is read in the I/O code
path. Therefore this field is used to communicate between threads
without a lock.

Use qatomic_set()/qatomic_read() to make it clear that this field is
accessed by multiple threads.

Acquire/release are not necessary because the BlockBackend->in_flight
counter already uses sequentially consistent accesses and running I/O
requests hold that counter when blk_wait_while_drained() is called.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/block-backend.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 278b04ce69..f00bf2ab35 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -80,7 +80,7 @@ struct BlockBackend {
     NotifierList remove_bs_notifiers, insert_bs_notifiers;
     QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers;
 
-    int quiesce_counter;
+    int quiesce_counter; /* atomic: written under BQL, read by other threads */
     CoQueue queued_requests;
     bool disable_request_queuing;
 
@@ -1057,7 +1057,7 @@ void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops,
     blk->dev_opaque = opaque;
 
     /* Are we currently quiesced? Should we enforce this right now? */
-    if (blk->quiesce_counter && ops && ops->drained_begin) {
+    if (qatomic_read(&blk->quiesce_counter) && ops && ops->drained_begin) {
         ops->drained_begin(opaque);
     }
 }
@@ -1271,7 +1271,7 @@ static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
 {
     assert(blk->in_flight > 0);
 
-    if (blk->quiesce_counter && !blk->disable_request_queuing) {
+    if (qatomic_read(&blk->quiesce_counter) && !blk->disable_request_queuing) {
         blk_dec_in_flight(blk);
         qemu_co_queue_wait(&blk->queued_requests, NULL);
         blk_inc_in_flight(blk);
@@ -2568,7 +2568,9 @@ static void blk_root_drained_begin(BdrvChild *child)
     BlockBackend *blk = child->opaque;
     ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
 
-    if (++blk->quiesce_counter == 1) {
+    int new_counter = qatomic_read(&blk->quiesce_counter) + 1;
+    qatomic_set(&blk->quiesce_counter, new_counter);
+    if (new_counter == 1) {
         if (blk->dev_ops && blk->dev_ops->drained_begin) {
             blk->dev_ops->drained_begin(blk->dev_opaque);
         }
@@ -2586,7 +2588,7 @@ static bool blk_root_drained_poll(BdrvChild *child)
 {
     BlockBackend *blk = child->opaque;
     bool busy = false;
-    assert(blk->quiesce_counter);
+    assert(qatomic_read(&blk->quiesce_counter));
 
     if (blk->dev_ops && blk->dev_ops->drained_poll) {
         busy = blk->dev_ops->drained_poll(blk->dev_opaque);
@@ -2597,12 +2599,14 @@ static bool blk_root_drained_poll(BdrvChild *child)
 static void blk_root_drained_end(BdrvChild *child)
 {
     BlockBackend *blk = child->opaque;
-    assert(blk->quiesce_counter);
+    assert(qatomic_read(&blk->quiesce_counter));
 
     assert(blk->public.throttle_group_member.io_limits_disabled);
     qatomic_dec(&blk->public.throttle_group_member.io_limits_disabled);
 
-    if (--blk->quiesce_counter == 0) {
+    int new_counter = qatomic_read(&blk->quiesce_counter) - 1;
+    qatomic_set(&blk->quiesce_counter, new_counter);
+    if (new_counter == 0) {
         if (blk->dev_ops && blk->dev_ops->drained_end) {
             blk->dev_ops->drained_end(blk->dev_opaque);
         }
-- 
2.39.2

[PATCH 2/3] block: make BlockBackend->disable_request_queuing atomic

[Stefan Hajnoczi]

This field is accessed by multiple threads without a lock. Use explicit
qatomic_read()/qatomic_set() calls. There is no need for acquire/release
because blk_set_disable_request_queuing() doesn't provide any
guarantees (it helps that it's used at BlockBackend creation time and
not when there is I/O in flight).

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/block-backend.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index f00bf2ab35..ec7eafd7fb 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -82,7 +82,7 @@ struct BlockBackend {
 
     int quiesce_counter; /* atomic: written under BQL, read by other threads */
     CoQueue queued_requests;
-    bool disable_request_queuing;
+    bool disable_request_queuing; /* atomic */
 
     VMChangeStateEntry *vmsh;
     bool force_allow_inactivate;
@@ -1232,7 +1232,7 @@ void blk_set_allow_aio_context_change(BlockBackend *blk, bool allow)
 void blk_set_disable_request_queuing(BlockBackend *blk, bool disable)
 {
     IO_CODE();
-    blk->disable_request_queuing = disable;
+    qatomic_set(&blk->disable_request_queuing, disable);
 }
 
 static int coroutine_fn GRAPH_RDLOCK
@@ -1271,7 +1271,8 @@ static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
 {
     assert(blk->in_flight > 0);
 
-    if (qatomic_read(&blk->quiesce_counter) && !blk->disable_request_queuing) {
+    if (qatomic_read(&blk->quiesce_counter) &&
+        !qatomic_read(&blk->disable_request_queuing)) {
         blk_dec_in_flight(blk);
         qemu_co_queue_wait(&blk->queued_requests, NULL);
         blk_inc_in_flight(blk);
-- 
2.39.2

[PATCH 3/3] block: protect BlockBackend->queued_requests with a lock

[Stefan Hajnoczi]

The CoQueue API offers thread-safety via the lock argument that
qemu_co_queue_wait() and qemu_co_enter_next() take. BlockBackend
currently does not make use of the lock argument. This means that
multiple threads submitting I/O requests can corrupt the CoQueue's
QSIMPLEQ.

Add a QemuMutex and pass it to CoQueue APIs so that the queue is
protected. While we're at it, also assert that the queue is empty when
the BlockBackend is deleted.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/block-backend.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index ec7eafd7fb..c4dd16f4da 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -81,6 +81,7 @@ struct BlockBackend {
     QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers;
 
     int quiesce_counter; /* atomic: written under BQL, read by other threads */
+    QemuMutex queued_requests_lock; /* protects queued_requests */
     CoQueue queued_requests;
     bool disable_request_queuing; /* atomic */
 
@@ -368,6 +369,7 @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm)
 
     block_acct_init(&blk->stats);
 
+    qemu_mutex_init(&blk->queued_requests_lock);
     qemu_co_queue_init(&blk->queued_requests);
     notifier_list_init(&blk->remove_bs_notifiers);
     notifier_list_init(&blk->insert_bs_notifiers);
@@ -485,6 +487,8 @@ static void blk_delete(BlockBackend *blk)
     assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));
     assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));
     assert(QLIST_EMPTY(&blk->aio_notifiers));
+    assert(qemu_co_queue_empty(&blk->queued_requests));
+    qemu_mutex_destroy(&blk->queued_requests_lock);
     QTAILQ_REMOVE(&block_backends, blk, link);
     drive_info_del(blk->legacy_dinfo);
     block_acct_cleanup(&blk->stats);
@@ -1273,9 +1277,16 @@ static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
 
     if (qatomic_read(&blk->quiesce_counter) &&
         !qatomic_read(&blk->disable_request_queuing)) {
+        /*
+         * Take lock before decrementing in flight counter so main loop thread
+         * waits for us to enqueue ourselves before it can leave the drained
+         * section.
+         */
+        qemu_mutex_lock(&blk->queued_requests_lock);
         blk_dec_in_flight(blk);
-        qemu_co_queue_wait(&blk->queued_requests, NULL);
+        qemu_co_queue_wait(&blk->queued_requests, &blk->queued_requests_lock);
         blk_inc_in_flight(blk);
+        qemu_mutex_unlock(&blk->queued_requests_lock);
     }
 }
 
@@ -2611,9 +2622,12 @@ static void blk_root_drained_end(BdrvChild *child)
         if (blk->dev_ops && blk->dev_ops->drained_end) {
             blk->dev_ops->drained_end(blk->dev_opaque);
         }
-        while (qemu_co_enter_next(&blk->queued_requests, NULL)) {
+        qemu_mutex_lock(&blk->queued_requests_lock);
+        while (qemu_co_enter_next(&blk->queued_requests,
+                                  &blk->queued_requests_lock)) {
             /* Resume all queued requests */
         }
+        qemu_mutex_unlock(&blk->queued_requests_lock);
     }
 }
 
-- 
2.39.2

Re: [PATCH 1/3] block: make BlockBackend->quiesce_counter atomic

[Hanna Czenczek]

On 27.02.23 21:57, Stefan Hajnoczi wrote:
> The main loop thread increments/decrements BlockBackend->quiesce_counter
> when drained sections begin/end. The counter is read in the I/O code
> path. Therefore this field is used to communicate between threads
> without a lock.
>
> Use qatomic_set()/qatomic_read() to make it clear that this field is
> accessed by multiple threads.
>
> Acquire/release are not necessary because the BlockBackend->in_flight
> counter already uses sequentially consistent accesses and running I/O
> requests hold that counter when blk_wait_while_drained() is called.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/block-backend.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 278b04ce69..f00bf2ab35 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c

[...]

> @@ -2568,7 +2568,9 @@ static void blk_root_drained_begin(BdrvChild *child)
>       BlockBackend *blk = child->opaque;
>       ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
>   
> -    if (++blk->quiesce_counter == 1) {
> +    int new_counter = qatomic_read(&blk->quiesce_counter) + 1;
> +    qatomic_set(&blk->quiesce_counter, new_counter);
> +    if (new_counter == 1) {
>           if (blk->dev_ops && blk->dev_ops->drained_begin) {
>               blk->dev_ops->drained_begin(blk->dev_opaque);
>           }

[...]

> @@ -2597,12 +2599,14 @@ static bool blk_root_drained_poll(BdrvChild *child)

[...]

>       assert(blk->public.throttle_group_member.io_limits_disabled);
>       qatomic_dec(&blk->public.throttle_group_member.io_limits_disabled);
>   
> -    if (--blk->quiesce_counter == 0) {
> +    int new_counter = qatomic_read(&blk->quiesce_counter) - 1;
> +    qatomic_set(&blk->quiesce_counter, new_counter);

I don’t quite understand why you decided not to use simple atomic 
increments/decrements with just SeqCst in these places.  Maybe it is 
fine this way, but it isn’t trivial to see.  As far as I understand, 
these aren’t hot paths, so I don’t think we’d lose performance by using 
fully atomic operations here.

Hanna

Re: [PATCH 2/3] block: make BlockBackend->disable_request_queuing atomic

[Hanna Czenczek]

On 27.02.23 21:57, Stefan Hajnoczi wrote:
> This field is accessed by multiple threads without a lock. Use explicit
> qatomic_read()/qatomic_set() calls. There is no need for acquire/release
> because blk_set_disable_request_queuing() doesn't provide any
> guarantees (it helps that it's used at BlockBackend creation time and
> not when there is I/O in flight).
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/block-backend.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Re: [PATCH 3/3] block: protect BlockBackend->queued_requests with a lock

[Hanna Czenczek]

On 27.02.23 21:57, Stefan Hajnoczi wrote:
> The CoQueue API offers thread-safety via the lock argument that
> qemu_co_queue_wait() and qemu_co_enter_next() take. BlockBackend
> currently does not make use of the lock argument. This means that
> multiple threads submitting I/O requests can corrupt the CoQueue's
> QSIMPLEQ.
>
> Add a QemuMutex and pass it to CoQueue APIs so that the queue is
> protected. While we're at it, also assert that the queue is empty when
> the BlockBackend is deleted.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/block-backend.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Re: [PATCH 1/3] block: make BlockBackend->quiesce_counter atomic

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]

On Fri, Mar 03, 2023 at 04:29:54PM +0100, Hanna Czenczek wrote:
> On 27.02.23 21:57, Stefan Hajnoczi wrote:
> > The main loop thread increments/decrements BlockBackend->quiesce_counter
> > when drained sections begin/end. The counter is read in the I/O code
> > path. Therefore this field is used to communicate between threads
> > without a lock.
> > 
> > Use qatomic_set()/qatomic_read() to make it clear that this field is
> > accessed by multiple threads.
> > 
> > Acquire/release are not necessary because the BlockBackend->in_flight
> > counter already uses sequentially consistent accesses and running I/O
> > requests hold that counter when blk_wait_while_drained() is called.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   block/block-backend.c | 18 +++++++++++-------
> >   1 file changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index 278b04ce69..f00bf2ab35 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> 
> [...]
> 
> > @@ -2568,7 +2568,9 @@ static void blk_root_drained_begin(BdrvChild *child)
> >       BlockBackend *blk = child->opaque;
> >       ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
> > -    if (++blk->quiesce_counter == 1) {
> > +    int new_counter = qatomic_read(&blk->quiesce_counter) + 1;
> > +    qatomic_set(&blk->quiesce_counter, new_counter);
> > +    if (new_counter == 1) {
> >           if (blk->dev_ops && blk->dev_ops->drained_begin) {
> >               blk->dev_ops->drained_begin(blk->dev_opaque);
> >           }
> 
> [...]
> 
> > @@ -2597,12 +2599,14 @@ static bool blk_root_drained_poll(BdrvChild *child)
> 
> [...]
> 
> >       assert(blk->public.throttle_group_member.io_limits_disabled);
> >       qatomic_dec(&blk->public.throttle_group_member.io_limits_disabled);
> > -    if (--blk->quiesce_counter == 0) {
> > +    int new_counter = qatomic_read(&blk->quiesce_counter) - 1;
> > +    qatomic_set(&blk->quiesce_counter, new_counter);
> 
> I don’t quite understand why you decided not to use simple atomic
> increments/decrements with just SeqCst in these places.  Maybe it is fine
> this way, but it isn’t trivial to see.  As far as I understand, these aren’t
> hot paths, so I don’t think we’d lose performance by using fully atomic
> operations here.

Good idea. It would be much easier to read.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]