| Message ID | 20220609143727.1151816-8-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-blk: removal of AioContext lock | expand |
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>
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> >
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> >>
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
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 --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 {
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(-)