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

[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()"?

> 
>   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>
>