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

[Emanuele Giuseppe Esposito]

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