virtio_blk_load() question

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;
...
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?

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.

Thanks for your reply.

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

Actually, I wasn't able to get that condition either.
We're having problem in loading continuously sent VM image, and were
looking deeper into the device models.  We were doubting the
virtio_blk_load() first, but seems to be different.

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

I guess you're mentioning the following patch, and it's good to know
that.

http://www.mail-archive.com/qemu-devel@nongnu.org/msg27324.html

However, although QLIST_INSERT_HEAD is used, virtio_blk_save() is
adding requests to the tail of the list, and if we need to keep the
order of outstading requests, shouldn't we put incoming requests to
the tail in virtio_blk_load()?

OHMURA Kei <ohmura.kei@lab.ntt.co.jp> wrote:
> Thanks for your reply.
>
>> 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.
>
> Actually, I wasn't able to get that condition either.
> We're having problem in loading continuously sent VM image, and were
> looking deeper into the device models.  We were doubting the
> virtio_blk_load() first, but seems to be different.
>
>> 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.
>
> I guess you're mentioning the following patch, and it's good to know
> that.
>
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg27324.html
>
> However, although QLIST_INSERT_HEAD is used, virtio_blk_save() is
> adding requests to the tail of the list, and if we need to keep the
> order of outstading requests, shouldn't we put incoming requests to
> the tail in virtio_blk_load()?

Really, ordering doesn't matter (in this case):

see virtio-blk.c:virtio_blk_dma_restart_bh()

    QLIST_FOREACH_SAFE(req, &rq_copy, next, next_req) {
        QLIST_REMOVE(req, next);
        virtio_blk_handle_request(req, &mrb);
    }

This mean that we are just removing from the beggining and addin from
the beginnig (i.e. reversing).  Adding by the beggining made it easier,
but I can change if you mean.

Notice that except if there are any errors (I was not able to trigger
it, but didnt' try too hard), that list is going to be syncked in the 

        qemu_aio_flush();
        bdrv_flush_all();

in migrate_fd_put_ready(), so it is not trivial to hit it and probably
the difference is just theoretical.

Later, Juan.

On 2010/03/18 21:07, Juan Quintela wrote:
> Really, ordering doesn't matter (in this case):
> 
> see virtio-blk.c:virtio_blk_dma_restart_bh()
> 
>     QLIST_FOREACH_SAFE(req, &rq_copy, next, next_req) {
>         QLIST_REMOVE(req, next);
>         virtio_blk_handle_request(req, &mrb);
>     }
> 
> This mean that we are just removing from the beggining and addin from
> the beginnig (i.e. reversing).  Adding by the beggining made it easier,
> but I can change if you mean.

Thanks, I understood.
However, since it's difficult to understand this at first glance,
it would be great if you could modify, but I'm OK if you could just add
comments on this to the exiting patch.  It's kind for a newbie like me.


> Notice that except if there are any errors (I was not able to trigger
> it, but didnt' try too hard), that list is going to be syncked in the 
> 
>         qemu_aio_flush();
>         bdrv_flush_all();
> 
> in migrate_fd_put_ready(), so it is not trivial to hit it and probably
> the difference is just theoretical.

Thank you for your information.