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-eespo...@redhat.com/#584d7d1a-94cc-9ebb-363b-2fddb8d79...@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 <stefa...@redhat.com>
>>
