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

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature