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
signature.asc
Description: PGP signature
