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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

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

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