diff mbox series

[7/8] VirtIOBlock: protect rq with its own lock

Message ID 20220609143727.1151816-8-eesposit@redhat.com (mailing list archive)
State New, archived
Headers show
Series virtio-blk: removal of AioContext lock | expand

Commit Message

Emanuele Giuseppe Esposito June 9, 2022, 2:37 p.m. UTC
s->rq is pointing to the the VirtIOBlockReq list, and this list is
read/written in:

virtio_blk_reset = main loop, but caller calls ->stop_ioeventfd() and
drains, so no iothread runs in parallel
virtio_blk_save_device = main loop, but VM is stopped (migration), so
iothread has no work on request list
virtio_blk_load_device = same as save_device
virtio_blk_device_realize = iothread is not created yet
virtio_blk_handle_rw_error = io, here is why we need synchronization.
s is device state and is shared accross all queues. Right now there
is no problem, because iothread and main loop never access it at
the same time, but if we introduce 1 iothread -> n virtqueue and
1 virtqueue -> 1 iothread mapping we might have two iothreads
accessing the list at the same time
virtio_blk_process_queued_requests: io, same problem as above.

Therefore we need a virtio-blk to protect s->rq list.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 hw/block/virtio-blk.c          | 38 ++++++++++++++++++++++++++--------
 include/hw/virtio/virtio-blk.h |  5 ++++-
 2 files changed, 33 insertions(+), 10 deletions(-)

Comments

Stefan Hajnoczi July 5, 2022, 2:45 p.m. UTC | #1
On Thu, Jun 09, 2022 at 10:37:26AM -0400, Emanuele Giuseppe Esposito wrote:
> @@ -946,17 +955,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>       * stops all Iothreads.
>       */
>      blk_drain(s->blk);
> +    aio_context_release(ctx);
>  
>      /* We drop queued requests after blk_drain() because blk_drain() itself can
>       * produce them. */
> +    qemu_mutex_lock(&s->req_mutex);
>      while (s->rq) {
>          req = s->rq;
>          s->rq = req->next;
> +        qemu_mutex_unlock(&s->req_mutex);
>          virtqueue_detach_element(req->vq, &req->elem, 0);
>          virtio_blk_free_request(req);
> +        qemu_mutex_lock(&s->req_mutex);

Why is req_mutex dropped temporarily? At this point we don't really need
the req_mutex (all I/O should be stopped and drained), but maybe we
should do:

  WITH_QEMU_MUTEX(&s->req_mutex) {
      req = s->rq;
      s->rq = NULL;
  }

  ...process req list...

Otherwise:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Emanuele Giuseppe Esposito July 8, 2022, 9:33 a.m. UTC | #2
Am 05/07/2022 um 16:45 schrieb Stefan Hajnoczi:
> On Thu, Jun 09, 2022 at 10:37:26AM -0400, Emanuele Giuseppe Esposito wrote:
>> @@ -946,17 +955,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>>       * stops all Iothreads.
>>       */
>>      blk_drain(s->blk);
>> +    aio_context_release(ctx);
>>  
>>      /* We drop queued requests after blk_drain() because blk_drain() itself can
>>       * produce them. */
>> +    qemu_mutex_lock(&s->req_mutex);
>>      while (s->rq) {
>>          req = s->rq;
>>          s->rq = req->next;
>> +        qemu_mutex_unlock(&s->req_mutex);
>>          virtqueue_detach_element(req->vq, &req->elem, 0);
>>          virtio_blk_free_request(req);
>> +        qemu_mutex_lock(&s->req_mutex);
> 
> Why is req_mutex dropped temporarily? At this point we don't really need
> the req_mutex (all I/O should be stopped and drained), but maybe we
> should do:

Agree that maybe it is not useful to drop the mutex temporarily.

Regarding why req_mutex is not needed, yes I guess it isn't. Should I
get rid of this hunk at all, and maybe leave a comment like "no
synchronization needed, due to drain + ->stop_ioeventfd()"?

> 
>   WITH_QEMU_MUTEX(&s->req_mutex) {
>       req = s->rq;
>       s->rq = NULL;
>   }
> 
>   ...process req list...

Not sure what you mean here, we are looping on s->rq, so do we need to
protect also that? and why setting it to NULL? Sorry I am a little bit
lost here.

Thank you,
Emanuele

> 
> Otherwise:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>
Emanuele Giuseppe Esposito July 8, 2022, 11:22 a.m. UTC | #3
Am 08/07/2022 um 11:33 schrieb Emanuele Giuseppe Esposito:
> 
> 
> Am 05/07/2022 um 16:45 schrieb Stefan Hajnoczi:
>> On Thu, Jun 09, 2022 at 10:37:26AM -0400, Emanuele Giuseppe Esposito wrote:
>>> @@ -946,17 +955,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>>>       * stops all Iothreads.
>>>       */
>>>      blk_drain(s->blk);
>>> +    aio_context_release(ctx);
>>>  
>>>      /* We drop queued requests after blk_drain() because blk_drain() itself can
>>>       * produce them. */
>>> +    qemu_mutex_lock(&s->req_mutex);
>>>      while (s->rq) {
>>>          req = s->rq;
>>>          s->rq = req->next;
>>> +        qemu_mutex_unlock(&s->req_mutex);
>>>          virtqueue_detach_element(req->vq, &req->elem, 0);
>>>          virtio_blk_free_request(req);
>>> +        qemu_mutex_lock(&s->req_mutex);
>>
>> Why is req_mutex dropped temporarily? At this point we don't really need
>> the req_mutex (all I/O should be stopped and drained), but maybe we
>> should do:
> 
> Agree that maybe it is not useful to drop the mutex temporarily.
> 
> Regarding why req_mutex is not needed, yes I guess it isn't. Should I
> get rid of this hunk at all, and maybe leave a comment like "no
> synchronization needed, due to drain + ->stop_ioeventfd()"?

Actually, regarding this, I found why I added the lock:

https://patchew.org/QEMU/20220426085114.199647-1-eesposit@redhat.com/#584d7d1a-94cc-9ebb-363b-2fddb8d79f5b@redhat.com

So maybe it's better to add it.

> 
>>
>>   WITH_QEMU_MUTEX(&s->req_mutex) {
>>       req = s->rq;
>>       s->rq = NULL;
>>   }
>>
>>   ...process req list...
> 
> Not sure what you mean here, we are looping on s->rq, so do we need to
> protect also that? and why setting it to NULL? Sorry I am a little bit
> lost here.
> 
> Thank you,
> Emanuele
> 
>>
>> Otherwise:
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>
Stefan Hajnoczi July 12, 2022, 12:29 p.m. UTC | #4
On Fri, Jul 08, 2022 at 11:33:28AM +0200, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 05/07/2022 um 16:45 schrieb Stefan Hajnoczi:
> > On Thu, Jun 09, 2022 at 10:37:26AM -0400, Emanuele Giuseppe Esposito wrote:
> >> @@ -946,17 +955,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
> >>       * stops all Iothreads.
> >>       */
> >>      blk_drain(s->blk);
> >> +    aio_context_release(ctx);
> >>  
> >>      /* We drop queued requests after blk_drain() because blk_drain() itself can
> >>       * produce them. */
> >> +    qemu_mutex_lock(&s->req_mutex);
> >>      while (s->rq) {
> >>          req = s->rq;
> >>          s->rq = req->next;
> >> +        qemu_mutex_unlock(&s->req_mutex);
> >>          virtqueue_detach_element(req->vq, &req->elem, 0);
> >>          virtio_blk_free_request(req);
> >> +        qemu_mutex_lock(&s->req_mutex);
> > 
> > Why is req_mutex dropped temporarily? At this point we don't really need
> > the req_mutex (all I/O should be stopped and drained), but maybe we
> > should do:
> 
> Agree that maybe it is not useful to drop the mutex temporarily.
> 
> Regarding why req_mutex is not needed, yes I guess it isn't. Should I
> get rid of this hunk at all, and maybe leave a comment like "no
> synchronization needed, due to drain + ->stop_ioeventfd()"?
> 
> > 
> >   WITH_QEMU_MUTEX(&s->req_mutex) {
> >       req = s->rq;
> >       s->rq = NULL;
> >   }
> > 
> >   ...process req list...
> 
> Not sure what you mean here, we are looping on s->rq, so do we need to
> protect also that? and why setting it to NULL? Sorry I am a little bit
> lost here.

During reset we need to free the s->rq list and set the head pointer to
NULL.

If we want to access s->rq under s->req_mutex for consistency, then we
can fetch the list head into a local variable, drop the lock, and then
process the list (new items will not be added to the list anymore).

FWIW I think accessing s->rq under req_mutex for consistency is fine.
That makes the code easier to understand (no special case) and reduces
the danger of copy-pasting code into a context where a lock is required.

Stefan
Stefan Hajnoczi July 12, 2022, 12:34 p.m. UTC | #5
On Fri, Jul 08, 2022 at 01:22:58PM +0200, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 08/07/2022 um 11:33 schrieb Emanuele Giuseppe Esposito:
> > 
> > 
> > Am 05/07/2022 um 16:45 schrieb Stefan Hajnoczi:
> >> On Thu, Jun 09, 2022 at 10:37:26AM -0400, Emanuele Giuseppe Esposito wrote:
> >>> @@ -946,17 +955,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
> >>>       * stops all Iothreads.
> >>>       */
> >>>      blk_drain(s->blk);
> >>> +    aio_context_release(ctx);
> >>>  
> >>>      /* We drop queued requests after blk_drain() because blk_drain() itself can
> >>>       * produce them. */
> >>> +    qemu_mutex_lock(&s->req_mutex);
> >>>      while (s->rq) {
> >>>          req = s->rq;
> >>>          s->rq = req->next;
> >>> +        qemu_mutex_unlock(&s->req_mutex);
> >>>          virtqueue_detach_element(req->vq, &req->elem, 0);
> >>>          virtio_blk_free_request(req);
> >>> +        qemu_mutex_lock(&s->req_mutex);
> >>
> >> Why is req_mutex dropped temporarily? At this point we don't really need
> >> the req_mutex (all I/O should be stopped and drained), but maybe we
> >> should do:
> > 
> > Agree that maybe it is not useful to drop the mutex temporarily.
> > 
> > Regarding why req_mutex is not needed, yes I guess it isn't. Should I
> > get rid of this hunk at all, and maybe leave a comment like "no
> > synchronization needed, due to drain + ->stop_ioeventfd()"?
> 
> Actually, regarding this, I found why I added the lock:
> 
> https://patchew.org/QEMU/20220426085114.199647-1-eesposit@redhat.com/#584d7d1a-94cc-9ebb-363b-2fddb8d79f5b@redhat.com
> 
> So maybe it's better to add it.

I don't see anything obvious in Paolo's email that you linked. I think
he was saying it's safest to use a lock but not that it's necessary.

Can you clarify what you mean?

Stefan
diff mbox series

Patch

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e1aaa606ba..88c61457e1 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -109,8 +109,10 @@  static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
         /* Break the link as the next request is going to be parsed from the
          * ring again. Otherwise we may end up doing a double completion! */
         req->mr_next = NULL;
-        req->next = s->rq;
-        s->rq = req;
+        WITH_QEMU_LOCK_GUARD(&s->req_mutex) {
+            req->next = s->rq;
+            s->rq = req;
+        }
     } else if (action == BLOCK_ERROR_ACTION_REPORT) {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
         if (acct_failed) {
@@ -860,10 +862,16 @@  static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
 void virtio_blk_process_queued_requests(VirtIOBlock *s, bool is_bh)
 {
-    VirtIOBlockReq *req = s->rq;
+    VirtIOBlockReq *req;
     MultiReqBuffer mrb = {};
 
-    s->rq = NULL;
+    IO_CODE();
+
+    /* Detach queue from s->rq and process everything here */
+    WITH_QEMU_LOCK_GUARD(&s->req_mutex) {
+        req = s->rq;
+        s->rq = NULL;
+    }
 
     aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
     while (req) {
@@ -896,6 +904,7 @@  void virtio_blk_restart_bh(void *opaque)
 {
     VirtIOBlock *s = opaque;
 
+    IO_CODE();
     qemu_bh_delete(s->bh);
     s->bh = NULL;
 
@@ -946,17 +955,20 @@  static void virtio_blk_reset(VirtIODevice *vdev)
      * stops all Iothreads.
      */
     blk_drain(s->blk);
+    aio_context_release(ctx);
 
     /* We drop queued requests after blk_drain() because blk_drain() itself can
      * produce them. */
+    qemu_mutex_lock(&s->req_mutex);
     while (s->rq) {
         req = s->rq;
         s->rq = req->next;
+        qemu_mutex_unlock(&s->req_mutex);
         virtqueue_detach_element(req->vq, &req->elem, 0);
         virtio_blk_free_request(req);
+        qemu_mutex_lock(&s->req_mutex);
     }
-
-    aio_context_release(ctx);
+    qemu_mutex_unlock(&s->req_mutex);
 
     assert(!s->dataplane_started);
     blk_set_enable_write_cache(s->blk, s->original_wce);
@@ -1120,10 +1132,14 @@  static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
 static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
-    VirtIOBlockReq *req = s->rq;
+    VirtIOBlockReq *req;
 
     GLOBAL_STATE_CODE();
 
+    WITH_QEMU_LOCK_GUARD(&s->req_mutex) {
+        req = s->rq;
+    }
+
     while (req) {
         qemu_put_sbyte(f, 1);
 
@@ -1165,8 +1181,10 @@  static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
 
         req = qemu_get_virtqueue_element(vdev, f, sizeof(VirtIOBlockReq));
         virtio_blk_init_request(s, virtio_get_queue(vdev, vq_idx), req);
-        req->next = s->rq;
-        s->rq = req;
+        WITH_QEMU_LOCK_GUARD(&s->req_mutex) {
+            req->next = s->rq;
+            s->rq = req;
+        }
     }
 
     return 0;
@@ -1272,6 +1290,7 @@  static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
 
     virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
 
+    qemu_mutex_init(&s->req_mutex);
     s->blk = conf->conf.blk;
     s->rq = NULL;
     s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
@@ -1318,6 +1337,7 @@  static void virtio_blk_device_unrealize(DeviceState *dev)
     qemu_coroutine_dec_pool_size(conf->num_queues * conf->queue_size / 2);
     qemu_del_vm_change_state_handler(s->change);
     blockdev_mark_auto_del(s->blk);
+    qemu_mutex_destroy(&s->req_mutex);
     virtio_cleanup(vdev);
 }
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index c334353b5a..5cb59994a8 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -53,7 +53,6 @@  struct VirtIOBlockReq;
 struct VirtIOBlock {
     VirtIODevice parent_obj;
     BlockBackend *blk;
-    void *rq;
     QEMUBH *bh;
     VirtIOBlkConf conf;
     unsigned short sector_mask;
@@ -64,6 +63,10 @@  struct VirtIOBlock {
     struct VirtIOBlockDataPlane *dataplane;
     uint64_t host_features;
     size_t config_size;
+
+    /* While the VM is running, req_mutex protects rq.  */
+    QemuMutex req_mutex;
+    struct VirtIOBlockReq *rq;
 };
 
 typedef struct VirtIOBlockReq {