Re: virtio_blk_load() question

[Juan Quintela <quintela@redhat.com>]

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.