virtio_blk_load() question

PATCH OHMURA Kei 2 participants 5 messages Mar 18, 2010 – Mar 19, 2010 KVM

lore

Share & tools

lore

OHMURA Kei

author

2010-03-18 04:30 UTC #0 lore raw

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?

hw/virtio-blk.c +1 -1

index b80402d..267b16f 100644
@@ -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;

Juan Quintela

2010-03-18 07:37 UTC #1 lore raw

Quoted 11 lines · OHMURA Kei

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.

Quoted 11 lines

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.

hw/virtio-blk.c static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id) · line 463

Quoted 13 lines

hw/virtio-blk.c +1 -1

index b80402d..267b16f 100644
@@ -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.

OHMURA Kei

author

2010-03-18 09:42 UTC #2 lore raw

Thanks for your reply.

Quoted 3 lines

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.

Quoted 2 lines

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/[email protected]/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()?

Juan Quintela

2010-03-18 12:07 UTC #3 lore raw

Quoted 17 lines · OHMURA Kei

Thanks for your reply.
Quoted 3 lines
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.
Quoted 2 lines
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.

Quoted 6 lines

http://www.mail-archive.com/[email protected]/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.

OHMURA Kei

author

2010-03-19 02:53 UTC #4 lore raw

Quoted 4 lines · Juan Quintela

Really, ordering doesn't matter (in this case):
see virtio-blk.c:virtio_blk_dma_restart_bh()

Quoted 8 lines

    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.

Quoted 8 lines

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.