Re: virtio_blk_load() question

[Juan Quintela <quintela@redhat.com>]

OHMURA Kei <ohmura.kei@lab.ntt.co.jp> wrote:
> Hi,
>
> I have a question regarding virtio_blk_load().
> (qemu-kvm.git d1fa468c1cc03ea362d8fe3ed9269bab4d197510)
>
> VirtIOBlockReq structure is linked list of requests, but it doesn't seem to be
> properly linked in virtio_blk_load().
> ...
> req->next = s->rq;
> s->rq = req->next;
> ...

You are right.

> In this case, we're losing req, and s->rq always point to be same entry.
> If I'm understanding correctly, s->rq is NULL initially,
> and this would be kept.
>
> Although I'm not sure how these requests should be ordered, if the requests
> should be added to the head of list to restore the saved status by
> virtio_blk_save(), I think the following code is correct.  However, it seems to
> reverse the order of the requests, and I'm wondering whether that is
> appropriate.
>
> Would somebody tell me how virtio_blk_load() is working?

When I ported virtio to vmstate, I was unable to get that list not empty
for more than I tried.  It should be not empty in the case of one error
or similar, but I was not able to reproduce it.

> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index b80402d..267b16f 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -457,7 +457,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
>         VirtIOBlockReq *req = virtio_blk_alloc_request(s);
>         qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
>         req->next = s->rq;
> -        s->rq = req->next;
> +        s->rq = req;
>     }
>
>     return 0;

I agree this change is ok/needed.  Notice that my series ( [PATCH 0/9]
Virtio cleanups) that changes it to a QLIST and fixes it.

(althought again, I was never able to get that list with elements at the
time of migration).

Thanks, Juan.