virtio: verify that all outstanding buffers are flushed
PATCH v2 1/1 virtio: verify that all outstanding buffers are flushed
Add sanity check to address the following concern:
During migration, all we pass the index of the request; the rest can be re-read from the ring.
This is not generally enough if any available requests are outstanding. Imagine a ring of size 4. Below A means available U means used.
A 1
A 2
U 2
A 2
U 2
A 2
U 2
A 2
U 2
At this point available ring has wrapped around, the only way to know head 1 is outstanding is because backend has stored this info somewhere.
The reason we manage to migrate without tracking this in migration state is because we flush outstanding requests before migration.
This flush is device-specific though, let's add
a safeguard in virtio core to ensure it's done properly.
---
Changes from v1:
v1 was against the wrong tree, it didn't build against qemu.git
hw/virtio.c | 2 ++ 1 file changed, 2 insertions(+)
hw/virtio.c +2 -0
@@ -788,6 +788,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)if (vdev->vq[i].vring.num ==0 )break ; +assert (!vdev->vq[i].inuse); +qemu_put_be32 (f, vdev->vq[i].vring.num);qemu_put_be64 (f, vdev->vq[i].pa);qemu_put_be16s (f, &vdev->vq[i].last_avail_idx);
Signature · 1 line
MST
Quoted 28 lines · Michael S. Tsirkin
Add sanity check to address the following concern:
During migration, all we pass the index of the request; the rest can be re-read from the ring.
This is not generally enough if any available requests are outstanding. Imagine a ring of size 4. Below A means available U means used.
A 1
A 2
U 2
A 2
U 2
A 2
U 2
A 2
U 2At this point available ring has wrapped around, the only way to know head 1 is outstanding is because backend has stored this info somewhere.
The reason we manage to migrate without tracking this in migration state is because we flush outstanding requests before migration.
This flush is device-specific though, let's add
a safeguard in virtio core to ensure it's done properly.
Quoted 8 lines
Signed-off-by Michael S. Tsirkin <[email protected]>---
Changes from v1:
v1 was against the wrong tree, it didn't build against qemu.githw/virtio.c | 2 ++ 1 file changed, 2 insertions(+)
VirtIOBlock->rq can trigger the assertion.
IIUC hw/virtio-blk.c may handle I/O errors by keeping the request pending and on a list (->rq). This allows the user to restart them after, for example, adding more space to the host file system containing the disk image file.
We keep a list of failed requests and we migrate this list. So I think inuse != 0 when migrating with pending failed I/O requests.
Stefan
Il 12/12/2012 14:50, Stefan Hajnoczi ha scritto:
Quoted 9 lines
VirtIOBlock->rqcan trigger the assertion.IIUC
hw/virtio-blk.cmay handle I/O errors by keeping the request pending and on a list (->rq). This allows the user to restart them after, for example, adding more space to the host file system containing the disk image file.We keep a list of failed requests and we migrate this list. So I think inuse != 0 when migrating with pending failed I/O requests.
Same for virtio-scsi. Each request in that case is sent as part of the SCSIDevice that it refers to, via callbacks in SCSIBusInfo.
Paolo
Quoted 8 lines · Paolo Bonzini
Il 12/12/2012 14:50, Stefan Hajnoczi ha scritto:
Quoted 7 lines
VirtIOBlock->rqcan trigger the assertion.IIUC
hw/virtio-blk.cmay handle I/O errors by keeping the request pending and on a list (->rq). This allows the user to restart them after, for example, adding more space to the host file system containing the disk image file.
Quoted 7 lines
Quoted 2 lines
We keep a list of failed requests and we migrate this list. So I think inuse != 0 when migrating with pending failed I/O requests.
Same for virtio-scsi. Each request in that case is sent as part of the SCSIDevice that it refers to, via callbacks in SCSIBusInfo.
Paolo
Looks like this will leak ring entries.
All I see is: virtio_scsi_load calling virtio_load. When the loading side will get last avail index it will assume all requests up to that value have completed, so it will never put the missing heads in the used ring.
And it is at this point too late for
the source to change the used ring as guest memory has migrated.
Signature · 1 line
MST
Il 12/12/2012 15:30, Michael S. Tsirkin ha scritto:
Quoted 2 lines
Quoted 2 lines
Same for virtio-scsi. Each request in that case is sent as part of the SCSIDevice that it refers to, via callbacks in SCSIBusInfo.
It is in virtio_scsi_load_request.
Quoted 7 lines
Looks like this will leak ring entries.
All I see is:
virtio_scsi_loadcallingvirtio_load. When the loading side will get last avail index it will assume all requests up to that value have completed, so it will never put the missing heads in the used ring.
Ok, so we need some API for virtio-{blk,scsi} to communicate back the indexes of in-flight requests to virtio. The indexes are known from the VirtQueueElement, so that's fine.
Even better would be a virtio_save_request/virtio_load_request API...
Paolo
Quoted 14 lines · Paolo Bonzini
Il 12/12/2012 15:30, Michael S. Tsirkin ha scritto:
Quoted 2 lines
Quoted 2 lines
Same for virtio-scsi. Each request in that case is sent as part of the SCSIDevice that it refers to, via callbacks in SCSIBusInfo.
It is in
virtio_scsi_load_request.Quoted 7 lines
Looks like this will leak ring entries.
All I see is:
virtio_scsi_loadcallingvirtio_load. When the loading side will get last avail index it will assume all requests up to that value have completed, so it will never put the missing heads in the used ring.
Quoted 7 lines
Ok, so we need some API for virtio-{blk,scsi} to communicate back the indexes of in-flight requests to virtio. The indexes are known from the VirtQueueElement, so that's fine.
Even better would be a virtio_save_request/virtio_load_request API...
Paolo
So you are saying this is a bug then? Great. This is exactly what the assert above is out there to catch.
And you really can't fix it without breaking
migration compatibility.
As step 1, I think we should just complete all outstanding requests when VM stops.
Yes it means you can't do the retry hack after migration but this is hardly common scenario.
Signature · 1 line
MST
Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto:
Quoted 7 lines
Quoted 5 lines
Ok, so we need some API for virtio-{blk,scsi} to communicate back the indexes of in-flight requests to virtio. The indexes are known from the VirtQueueElement, so that's fine.
Even better would be a virtio_save_request/virtio_load_request API...
So you are saying this is a bug then? Great.
I'm not sure what you mean by "it will never put the missing heads in the used ring". The serialized requests are put in the used rings when they are completed and virtio-{blk,scsi} calls virtqueue_push. Is the problem if you have a vring that looks like this:
A A U A U U A A
? Which heads are leaked? {0,1}, {2} or {6,7}? Or a combination thereof?
Also, I'm not sure your "fix" (crash QEMU) is correct. I hope we can make it work.
Quoted 2 lines
This is exactly what the assert above is out there to catch. And you really can't fix it without breaking migration compatibility.
Why not? The index in the vring is in the migration data.
Quoted 5 lines
As step 1, I think we should just complete all outstanding requests when VM stops.
Yes it means you can't do the retry hack after migration but this is hardly common scenario.
I disagree...
Paolo
Quoted 9 lines · Paolo Bonzini
Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto:
Quoted 7 lines
Quoted 5 lines
Ok, so we need some API for virtio-{blk,scsi} to communicate back the indexes of in-flight requests to virtio. The indexes are known from the VirtQueueElement, so that's fine.
Even better would be a virtio_save_request/virtio_load_request API...
So you are saying this is a bug then? Great.
Quoted 8 lines
I'm not sure what you mean by "it will never put the missing heads in the used ring". The serialized requests are put in the used rings when they are completed and virtio-{blk,scsi} calls
virtqueue_push. Is the problem if you have a vring that looks like this:A A U A U U A A
? Which heads are leaked? {0,1}, {2} or {6,7}? Or a combination thereof?
I don't know what A A U A U U A A means.
Pls see the example of the problem in the original commit log.
Quoted 2 lines
Also, I'm not sure your "fix" (crash QEMU) is correct. I hope we can make it work.
Of course assert is not a fix.
The patch is just to catch bugs like this one.
To make it work, complete all requests when vm is stopped. This needs to be done in the specific device, the assert will catch buggy devices.
Quoted 4 lines
Quoted 2 lines
This is exactly what the assert above is out there to catch. And you really can't fix it without breaking migration compatibility.
Why not? The index in the vring is in the migration data.
index is not enough if requests are outstanding.
Pls check the example in the log of the patch.
Quoted 9 lines
Quoted 5 lines
As step 1, I think we should just complete all outstanding requests when VM stops.
Yes it means you can't do the retry hack after migration but this is hardly common scenario.
I disagree...
Paolo
Disagree with what? You are saying it's common?
Signature · 1 line
MST
Il 12/12/2012 16:25, Michael S. Tsirkin ha scritto:
Quoted 15 lines · Paolo Bonzini
Quoted 14 lines
Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto:
Quoted 7 lines
Quoted 5 lines
Ok, so we need some API for virtio-{blk,scsi} to communicate back the indexes of in-flight requests to virtio. The indexes are known from the VirtQueueElement, so that's fine.
Even better would be a virtio_save_request/virtio_load_request API...
So you are saying this is a bug then? Great.
I'm not sure what you mean by "it will never put the missing heads in the used ring". The serialized requests are put in the used rings when they are completed and virtio-{blk,scsi} calls
virtqueue_push. Is the problem if you have a vring that looks like this:
Quoted 5 lines
Quoted 3 lines
A A U A U U A A
? Which heads are leaked? {0,1}, {2} or {6,7}? Or a combination thereof?
I don't know what A A U A U U A A means.
Right, not very clear.... It means that descriptor 2 and 4 and 5 are free, while the others are in-flight.
Quoted 1 line
To make it work, complete all requests when vm is stopped.
That's not a choice, sorry.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
This is exactly what the assert above is out there to catch. And you really can't fix it without breaking migration compatibility.
Why not? The index in the vring is in the migration data.
index is not enough if requests are outstanding.
Sorry, I meant the descriptor index.
Quoted 1 line
Pls check the example in the log of the patch.
I'm likewise not sure what you meant by
A 1
A 2
U 2
A 2
U 2
A 2
U 2
A 2 <---
U 2
If I understand it, before the point marked with the arrow, the avail ring is
1 2 2 2
vring_avail_idx(vq) == 3
last_avail_idx == 3
After, it is
2 2 2 2
vring_avail_idx(vq) == 4
last_avail_idx == 3
What's wrong with that?
You wrote "the only way to know head 1 is outstanding is because backend has stored this info somewhere". But the backend _is_ tracking it (by serializing and then restoring the VirtQueueElement) and no leak happens because virtqueue_fill/flush will put the head on the used ring sooner or later.
Quoted 9 lines
Quoted 7 lines
Quoted 5 lines
As step 1, I think we should just complete all outstanding requests when VM stops.
Yes it means you can't do the retry hack after migration but this is hardly common scenario.
I disagree...
Disagree with what? You are saying it's common?
It's not common, but you cannot block migration because you have an I/O error. Solving the error may involve migrating the guests away from that host.
Paolo
Quoted 66 lines · Paolo Bonzini
Il 12/12/2012 16:25, Michael S. Tsirkin ha scritto:
Quoted 15 lines · Paolo Bonzini
Quoted 14 lines
Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto:
Quoted 7 lines
Quoted 5 lines
Ok, so we need some API for virtio-{blk,scsi} to communicate back the indexes of in-flight requests to virtio. The indexes are known from the VirtQueueElement, so that's fine.
Even better would be a virtio_save_request/virtio_load_request API...
So you are saying this is a bug then? Great.
I'm not sure what you mean by "it will never put the missing heads in the used ring". The serialized requests are put in the used rings when they are completed and virtio-{blk,scsi} calls
virtqueue_push. Is the problem if you have a vring that looks like this:Quoted 5 lines
Quoted 3 lines
A A U A U U A A
? Which heads are leaked? {0,1}, {2} or {6,7}? Or a combination thereof?
I don't know what A A U A U U A A means.
Right, not very clear.... It means that descriptor 2 and 4 and 5 are free, while the others are in-flight.
Quoted 1 line
To make it work, complete all requests when vm is stopped.
That's not a choice, sorry.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
This is exactly what the assert above is out there to catch. And you really can't fix it without breaking migration compatibility.
Why not? The index in the vring is in the migration data.
index is not enough if requests are outstanding.
Sorry, I meant the descriptor index.
Quoted 1 line
Pls check the example in the log of the patch.
I'm likewise not sure what you meant by
A 1 A 2 U 2 A 2 U 2 A 2 U 2 A 2 <--- U 2If I understand it, before the point marked with the arrow, the avail ring is
1 2 2 2 vring_avail_idx(vq) == 3 last_avail_idx == 3After, it is
2 2 2 2 vring_avail_idx(vq) == 4 last_avail_idx == 3
Quoted 7 lines
What's wrong with that?
You wrote "the only way to know head 1 is outstanding is because backend has stored this info somewhere". But the backend _is_ tracking it (by serializing and then restoring the VirtQueueElement) and no leak happens because virtqueue_fill/flush will put the head on the used ring sooner or later.
If you did this before save vm inuse would be 0.
You said that at the point where we save state,
some entries are outstanding. It is too late to
put head at that point.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
As step 1, I think we should just complete all outstanding requests when VM stops.
Yes it means you can't do the retry hack after migration but this is hardly common scenario.
I disagree...
Quoted 7 lines
Quoted 1 line
Disagree with what? You are saying it's common?
It's not common, but you cannot block migration because you have an I/O error. Solving the error may involve migrating the guests away from that host.
Paolo
No, you should complete with error.
Il 12/12/2012 17:37, Michael S. Tsirkin ha scritto:
Quoted 7 lines
Quoted 5 lines
You wrote "the only way to know head 1 is outstanding is because backend has stored this info somewhere". But the backend _is_ tracking it (by serializing and then restoring the VirtQueueElement) and no leak happens because virtqueue_fill/flush will put the head on the used ring sooner or later.
If you did this before save vm inuse would be 0.
No, I won't. I want a simple API that the device can call to keep inuse up-to-date. Perhaps a bit ugly compared to just saving inuse, but it works. Or are there other bits that need resyncing besides inuse? Bits that cannot be recovered from the existing migration data?
Quoted 3 lines
You said that at the point where we save state,
some entries are outstanding. It is too late to
put head at that point.
I don't want to put head on the source. I want to put it on the destination, when the request is completed. Same as it is done now, with bugfixes of course. Are there any problems doing so, except that inuse will not be up-to-date (easily fixed)?
Quoted 5 lines
Quoted 3 lines
It's not common, but you cannot block migration because you have an I/O error. Solving the error may involve migrating the guests away from that host.
No, you should complete with error.
Knowing that the request will fail, the admin will not be able to do migration, even if that will solve the error transparently.
Paolo
Quoted 7 lines · Paolo Bonzini
Il 12/12/2012 17:37, Michael S. Tsirkin ha scritto:
Quoted 6 lines
Quoted 5 lines
You wrote "the only way to know head 1 is outstanding is because backend has stored this info somewhere". But the backend _is_ tracking it (by serializing and then restoring the VirtQueueElement) and no leak happens because virtqueue_fill/flush will put the head on the used ring sooner or later.
Quoted 6 lines
Quoted 1 line
If you did this before save vm inuse would be 0.
No, I won't. I want a simple API that the device can call to keep inuse up-to-date. Perhaps a bit ugly compared to just saving inuse, but it works. Or are there other bits that need resyncing besides inuse? Bits that cannot be recovered from the existing migration data?
Saving inuse counter is useless. We need to know which requests are outstanding if we want to retry them on remote.
Quoted 8 lines
Quoted 3 lines
You said that at the point where we save state,
some entries are outstanding. It is too late to
put head at that point.I don't want to put head on the source. I want to put it on the destination, when the request is completed. Same as it is done now, with bugfixes of course. Are there any problems doing so, except that inuse will not be up-to-date (easily fixed)?
You have an outstanding request that is behind last avail index. You do not want to complete it. You migrate. There is no way for remote to understand that the request is outstanding.
Quoted 10 lines
Quoted 5 lines
Quoted 3 lines
It's not common, but you cannot block migration because you have an I/O error. Solving the error may involve migrating the guests away from that host.
No, you should complete with error.
Knowing that the request will fail, the admin will not be able to do migration, even if that will solve the error transparently.
Paolo
You are saying there's no way to complete all requests?
Signature · 1 line
MST
Il 12/12/2012 18:14, Michael S. Tsirkin ha scritto:
Quoted 10 lines · Paolo Bonzini
Quoted 9 lines
Il 12/12/2012 17:37, Michael S. Tsirkin ha scritto:
Quoted 7 lines
Quoted 5 lines
You wrote "the only way to know head 1 is outstanding is because backend has stored this info somewhere". But the backend _is_ tracking it (by serializing and then restoring the VirtQueueElement) and no leak happens because virtqueue_fill/flush will put the head on the used ring sooner or later.
If you did this before save vm inuse would be 0.
Quoted 7 lines
Quoted 4 lines
No, I won't. I want a simple API that the device can call to keep inuse up-to-date. Perhaps a bit ugly compared to just saving inuse, but it works. Or are there other bits that need resyncing besides inuse? Bits that cannot be recovered from the existing migration data?
Saving inuse counter is useless. We need to know which requests are outstanding if we want to retry them on remote.
And that's what virtio-blk and virtio-scsi have been doing for years. They store the VirtQueueElement including the index and the sglists. Can you explain *why* the index is not enough to reconstruct the state on the destination? There may be bugs and you may need help from virtio_blk_load, but that's okay.
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
You said that at the point where we save state,
some entries are outstanding. It is too late to
put head at that point.
Quoted 8 lines
Quoted 4 lines
I don't want to put head on the source. I want to put it on the destination, when the request is completed. Same as it is done now, with bugfixes of course. Are there any problems doing so, except that inuse will not be up-to-date (easily fixed)?
You have an outstanding request that is behind last avail index. You do not want to complete it. You migrate. There is no way for remote to understand that the request is outstanding.
The savevm callbacks know which request is outstanding and pass the information to the destination. See virtio_blk_save and virtio_blk_load.
What is not clear, and you haven't explained, is how you get to a bug in the handling of the avail ring. What's wrong with this explanation:
A 1
A 2
U 2
A 2
U 2
A 2
U 2
A 2 <---
U 2
where before the point marked with the arrow, the avail ring is
1 2 2 2
vring_avail_idx(vq) == 3
last_avail_idx == 3
and after the point marked with the arrow, the avail ring is
2 2 2 2
vring_avail_idx(vq) == 4
last_avail_idx == 3
?!?
Quoted 10 lines
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
It's not common, but you cannot block migration because you have an I/O error. Solving the error may involve migrating the guests away from that host.
No, you should complete with error.
Knowing that the request will fail, the admin will not be able to do migration, even if that will solve the error transparently.
You are saying there's no way to complete all requests?
With an error, yes. Transparently after fixing the error (which may involve migration), no.
Paolo
Quoted 16 lines · Paolo Bonzini
Il 12/12/2012 18:14, Michael S. Tsirkin ha scritto:
Quoted 15 lines · Paolo Bonzini
Quoted 13 lines
Il 12/12/2012 17:37, Michael S. Tsirkin ha scritto:
Quoted 7 lines
Quoted 5 lines
You wrote "the only way to know head 1 is outstanding is because backend has stored this info somewhere". But the backend _is_ tracking it (by serializing and then restoring the VirtQueueElement) and no leak happens because virtqueue_fill/flush will put the head on the used ring sooner or later.
If you did this before save vm inuse would be 0.
No, I won't. I want a simple API that the device can call to keep inuse up-to-date. Perhaps a bit ugly compared to just saving inuse, but it works. Or are there other bits that need resyncing besides inuse? Bits that cannot be recovered from the existing migration data?
Quoted 4 lines
Quoted 2 lines
Saving inuse counter is useless. We need to know which requests are outstanding if we want to retry them on remote.
And that's what virtio-blk and virtio-scsi have been doing for years.
I don't see it - all I see in save is virtio_save. there's the extra code to save the elements in flight and send them to remote?
Quoted 41 lines
They store the VirtQueueElement including the index and the sglists. Can you explain *why* the index is not enough to reconstruct the state on the destination? There may be bugs and you may need help from
virtio_blk_load, but that's okay.Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
You said that at the point where we save state,
some entries are outstanding. It is too late to
put head at that point.Quoted 8 lines
Quoted 4 lines
I don't want to put head on the source. I want to put it on the destination, when the request is completed. Same as it is done now, with bugfixes of course. Are there any problems doing so, except that inuse will not be up-to-date (easily fixed)?
You have an outstanding request that is behind last avail index. You do not want to complete it. You migrate. There is no way for remote to understand that the request is outstanding.
The savevm callbacks know which request is outstanding and pass the information to the destination. See
virtio_blk_saveandvirtio_blk_load.What is not clear, and you haven't explained, is how you get to a bug in the handling of the avail ring. What's wrong with this explanation:
A 1 A 2 U 2 A 2 U 2 A 2 U 2 A 2 <--- U 2where before the point marked with the arrow, the avail ring is
1 2 2 2 vring_avail_idx(vq) == 3 last_avail_idx == 3
Quoted 7 lines
and after the point marked with the arrow, the avail ring is
2 2 2 2 vring_avail_idx(vq) == 4 last_avail_idx == 3?!?
You need to retry A1 on remote. How do you do that? There's no way to find out it has not been completed
from the ring itself.
Quoted 15 lines
Quoted 10 lines
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
It's not common, but you cannot block migration because you have an I/O error. Solving the error may involve migrating the guests away from that host.
No, you should complete with error.
Knowing that the request will fail, the admin will not be able to do migration, even if that will solve the error transparently.
You are saying there's no way to complete all requests?
With an error, yes. Transparently after fixing the error (which may involve migration), no.
Paolo
Il 12/12/2012 20:23, Michael S. Tsirkin ha scritto:
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Saving inuse counter is useless. We need to know which requests are outstanding if we want to retry them on remote.
And that's what virtio-blk and virtio-scsi have been doing for years.
I don't see it - all I see in save is
virtio_save.
static void virtio_blk_save(QEMUFile *f, void *opaque)
{
VirtIOBlock *s = opaque;
VirtIOBlockReq *req = s->rq;
virtio_save(&s->vdev, f);
while (req) {
qemu_put_sbyte(f, 1);
qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
req = req->next;
}
qemu_put_sbyte(f, 0);
}virtio-scsi does it in virtio_scsi_save_request.
Quoted 3 lines
You need to retry A1 on remote. How do you do that? There's no way to find out it has not been completed
from the ring itself.
virtio_blk_dma_restart_bh and scsi_dma_restart_bh do it.
Paolo
Quoted 9 lines · Paolo Bonzini
Il 12/12/2012 20:23, Michael S. Tsirkin ha scritto:
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Saving inuse counter is useless. We need to know which requests are outstanding if we want to retry them on remote.
And that's what virtio-blk and virtio-scsi have been doing for years.
I don't see it - all I see in save is
virtio_save.static void
virtio_blk_save(QEMUFile *f, void *opaque)
Quoted 9 lines
{
VirtIOBlock *s = opaque;
VirtIOBlockReq *req = s->rq;
virtio_save(&s->vdev, f);
while (req) {
qemu_put_sbyte(f, 1);
qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));Ow. Does it really save VirtQueueElement?
typedef struct VirtQueueElement
{
unsigned int index;
unsigned int out_num;
unsigned int in_num;
hwaddr in_addr[VIRTQUEUE_MAX_SIZE];
hwaddr out_addr[VIRTQUEUE_MAX_SIZE];
struct iovec in_sg[VIRTQUEUE_MAX_SIZE];
struct iovec out_sg[VIRTQUEUE_MAX_SIZE];
} VirtQueueElement; Complete with pointers into qemu memory and all? That's got to hurt.
All we really need is the index.
Quoted 8 lines
req = req->next; } qemu_put_sbyte(f, 0); }virtio-scsi does it in
virtio_scsi_save_request.
Quoted 7 lines
Quoted 3 lines
You need to retry A1 on remote. How do you do that? There's no way to find out it has not been completed
from the ring itself.
virtio_blk_dma_restart_bhandscsi_dma_restart_bhdo it.Paolo
Okay, so the only bug is inuse getting negative right? So all we need to do is fix up the inuse value after restoring the outstanding requests - basically count the restored buffers and set inuse accordingly.
Signature · 1 line
MST
Quoted 33 lines · Michael S. Tsirkin
Quoted 9 lines · Paolo Bonzini
Il 12/12/2012 20:23, Michael S. Tsirkin ha scritto:
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Saving inuse counter is useless. We need to know which requests are outstanding if we want to retry them on remote.
And that's what virtio-blk and virtio-scsi have been doing for years.
I don't see it - all I see in save is
virtio_save.static void
virtio_blk_save(QEMUFile *f, void *opaque)Quoted 9 lines
{ VirtIOBlock *s = opaque; VirtIOBlockReq *req = s->rq; virtio_save(&s->vdev, f); while (req) { qemu_put_sbyte(f, 1); qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));Ow. Does it really save VirtQueueElement?
typedef struct VirtQueueElement
{ unsigned int index; unsigned int out_num; unsigned int in_num; hwaddr in_addr[VIRTQUEUE_MAX_SIZE]; hwaddr out_addr[VIRTQUEUE_MAX_SIZE]; struct iovec in_sg[VIRTQUEUE_MAX_SIZE]; struct iovec out_sg[VIRTQUEUE_MAX_SIZE]; } VirtQueueElement;
Quoted 5 lines
Complete with pointers into qemu memory and all? That's got to hurt.
All we really need is the index.
Yes, take a look at the series I sent out that scrubs all of this to just send the index and the addresses of the element.
We technically should save the addresses and sizes too. It makes it a heck of a lot safer then re-reading guest memory since we do some validation on the size of the sg elements.
But we could get away with only saving the index if we really wanted to.
Regards,
Anthony Liguori
Quoted 23 lines
Quoted 8 lines
req = req->next; } qemu_put_sbyte(f, 0); }virtio-scsi does it in
virtio_scsi_save_request.Quoted 7 lines
Quoted 3 lines
You need to retry A1 on remote. How do you do that? There's no way to find out it has not been completed
from the ring itself.
virtio_blk_dma_restart_bhandscsi_dma_restart_bhdo it.Paolo
Okay, so the only bug is inuse getting negative right? So all we need to do is fix up the inuse value after restoring the outstanding requests - basically count the restored buffers and set inuse accordingly.
Signature · 1 line
MST
Quoted 22 lines · Anthony Liguori
Quoted 20 lines · Michael S. Tsirkin
Quoted 18 lines · Paolo Bonzini
Il 12/12/2012 20:23, Michael S. Tsirkin ha scritto:
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Saving inuse counter is useless. We need to know which requests are outstanding if we want to retry them on remote.
And that's what virtio-blk and virtio-scsi have been doing for years.
I don't see it - all I see in save is
virtio_save.static void
virtio_blk_save(QEMUFile *f, void *opaque){ VirtIOBlock *s = opaque; VirtIOBlockReq *req = s->rq; virtio_save(&s->vdev, f); while (req) { qemu_put_sbyte(f, 1); qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
Quoted 7 lines
Quoted 7 lines
Ow. Does it really save VirtQueueElement?
typedef struct VirtQueueElement
{ unsigned int index; unsigned int out_num; unsigned int in_num;
BTW there's a hole after in_num which is uninitialized, that's also a nasty thing to send on the wire.
Quoted 6 lines
Quoted 6 lines
hwaddr in_addr[VIRTQUEUE_MAX_SIZE]; hwaddr out_addr[VIRTQUEUE_MAX_SIZE]; struct iovec in_sg[VIRTQUEUE_MAX_SIZE]; struct iovec out_sg[VIRTQUEUE_MAX_SIZE]; } VirtQueueElement;
Quoted 8 lines
Quoted 5 lines
Complete with pointers into qemu memory and all? That's got to hurt.
All we really need is the index.
Yes, take a look at the series I sent out that scrubs all of this to just send the index and the addresses of the element.
Will do.
Quoted 1 line
We technically should save the addresses and sizes too.
I guess as long as these are guest addresses, not ther qemu ones.
Quoted 5 lines
It makes it a
heck of a lot safer then re-reading guest memory since we do some validation on the size of the sg elements.But we could get away with only saving the index if we really wanted to.
I guess re-validating it is needed anyway: we should not trust remote more than we trust the guest.
Quoted 27 lines
Regards,
Anthony Liguori
Quoted 23 lines
Quoted 8 lines
req = req->next; } qemu_put_sbyte(f, 0); }virtio-scsi does it in
virtio_scsi_save_request.Quoted 7 lines
Quoted 3 lines
You need to retry A1 on remote. How do you do that? There's no way to find out it has not been completed
from the ring itself.
virtio_blk_dma_restart_bhandscsi_dma_restart_bhdo it.Paolo
Okay, so the only bug is inuse getting negative right? So all we need to do is fix up the inuse value after restoring the outstanding requests - basically count the restored buffers and set inuse accordingly.
Signature · 1 line
MST
Quoted 6 lines · Anthony Liguori
Yes, take a look at the series I sent out that scrubs all of this to just send the index and the addresses of the element.
We technically should save the addresses and sizes too. It makes it a heck of a lot safer then re-reading guest memory since we do some validation on the size of the sg elements.
Not really.
The guest puts the descriptors in the ring and leaves them there until the device acks. If it changes them once they're exposed but before they're acked, it can get either before or after version, and always could.
Cheers,
Rusty.
Quoted 11 lines
Quoted 4 lines
We technically should save the addresses and sizes too. It makes it a heck of a lot safer then re-reading guest memory since we do some validation on the size of the sg elements.
Not really.
The guest puts the descriptors in the ring and leaves them there until the device acks. If it changes them once they're exposed but before they're acked, it can get either before or after version, and always could.
The problems start when the guest tries to race against QEMU and defy the validation. Always using the validated version is a bit easier than redoing the validation after migration.
Paolo
Quoted 6 lines · Paolo Bonzini
Quoted 6 lines
Quoted 3 lines
We technically should save the addresses and sizes too. It makes it a heck of a lot safer then re-reading guest memory since we do some validation on the size of the sg elements.
Not really.
Quoted 8 lines
Quoted 4 lines
The guest puts the descriptors in the ring and leaves them there until the device acks. If it changes them once they're exposed but before they're acked, it can get either before or after version, and always could.
The problems start when the guest tries to race against QEMU and defy the validation. Always using the validated version is a bit easier than redoing the validation after migration.
Exactly.
Regards,
Anthony Liguori
Quoted 2 lines
Paolo
Il 12/12/2012 22:19, Michael S. Tsirkin ha scritto:
Quoted 33 lines · Paolo Bonzini
Quoted 9 lines
Il 12/12/2012 20:23, Michael S. Tsirkin ha scritto:
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Saving inuse counter is useless. We need to know which requests are outstanding if we want to retry them on remote.
And that's what virtio-blk and virtio-scsi have been doing for years.
I don't see it - all I see in save is
virtio_save.static void
virtio_blk_save(QEMUFile *f, void *opaque)Quoted 9 lines
{ VirtIOBlock *s = opaque; VirtIOBlockReq *req = s->rq; virtio_save(&s->vdev, f); while (req) { qemu_put_sbyte(f, 1); qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));Ow. Does it really save VirtQueueElement?
typedef struct VirtQueueElement
{ unsigned int index; unsigned int out_num; unsigned int in_num; hwaddr in_addr[VIRTQUEUE_MAX_SIZE]; hwaddr out_addr[VIRTQUEUE_MAX_SIZE]; struct iovec in_sg[VIRTQUEUE_MAX_SIZE]; struct iovec out_sg[VIRTQUEUE_MAX_SIZE]; } VirtQueueElement;
Quoted 1 line
Complete with pointers into qemu memory and all?
Yes, that sucks (also the endianness is totally broken), but the pointers into QEMU memory are restored afterwards by remapping the address. Like Anthony, I'm not sure whether reloading the sglist from guest memory is safe. It would require re-validation of everything.
We _can_ trust remote. Things like races on connecting to the incoming-migration socket are best handled outside QEMU with a firewall or a separate network that is not guest-accessible. I'm pretty sure that virtio-blk is the latest of our worries here.
Quoted 4 lines
Okay, so the only bug is inuse getting negative right? So all we need to do is fix up the inuse value after restoring the outstanding requests - basically count the restored buffers and set inuse accordingly.
Yes, I agree.
Paolo
Am 12.12.2012 17:37, schrieb Michael S. Tsirkin:
Quoted 14 lines · Paolo Bonzini
Quoted 13 lines
Il 12/12/2012 16:25, Michael S. Tsirkin ha scritto:
Quoted 11 lines · Paolo Bonzini
Quoted 8 lines
Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto:
Quoted 5 lines
As step 1, I think we should just complete all outstanding requests when VM stops.
Yes it means you can't do the retry hack after migration but this is hardly common scenario.
I disagree...
Disagree with what? You are saying it's common?
Quoted 7 lines
Quoted 5 lines
It's not common, but you cannot block migration because you have an I/O error. Solving the error may involve migrating the guests away from that host.
Paolo
No, you should complete with error.
Just to confirm that this isn't possible: rerror/werror=stop is a supported feature, and I do get bug reports when it breaks (including migration while the VM is stopped for an I/O error).
Makes it common enough that breaking it is definitely not an option, right?
Kevin
Quoted 23 lines · Kevin Wolf
Am 12.12.2012 17:37, schrieb Michael S. Tsirkin:
Quoted 14 lines · Paolo Bonzini
Quoted 13 lines
Il 12/12/2012 16:25, Michael S. Tsirkin ha scritto:
Quoted 11 lines · Paolo Bonzini
Quoted 8 lines
Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto:
Quoted 5 lines
As step 1, I think we should just complete all outstanding requests when VM stops.
Yes it means you can't do the retry hack after migration but this is hardly common scenario.
I disagree...
Disagree with what? You are saying it's common?
Quoted 7 lines
Quoted 5 lines
It's not common, but you cannot block migration because you have an I/O error. Solving the error may involve migrating the guests away from that host.
Paolo
No, you should complete with error.
Quoted 7 lines
Just to confirm that this isn't possible: rerror/werror=stop is a supported feature, and I do get bug reports when it breaks (including migration while the VM is stopped for an I/O error).
Makes it common enough that breaking it is definitely not an option, right?
Kevin
Nod. So we need to fix inuse which is currently broken. We still can have an assert but it needs to be done more carefully, counting outstanding requests and checking it matches inuse.
Signature · 1 line
MST
Quoted 38 lines · Stefan Hajnoczi
Quoted 28 lines · Michael S. Tsirkin
Add sanity check to address the following concern:
During migration, all we pass the index of the request; the rest can be re-read from the ring.
This is not generally enough if any available requests are outstanding. Imagine a ring of size 4. Below A means available U means used.
A 1
A 2
U 2
A 2
U 2
A 2
U 2
A 2
U 2At this point available ring has wrapped around, the only way to know head 1 is outstanding is because backend has stored this info somewhere.
The reason we manage to migrate without tracking this in migration state is because we flush outstanding requests before migration.
This flush is device-specific though, let's add
a safeguard in virtio core to ensure it's done properly.Quoted 8 lines
Signed-off-by Michael S. Tsirkin <[email protected]>---
Changes from v1:
v1 was against the wrong tree, it didn't build against qemu.githw/virtio.c | 2 ++ 1 file changed, 2 insertions(+)
Quoted 8 lines
VirtIOBlock->rqcan trigger the assertion.IIUC
hw/virtio-blk.cmay handle I/O errors by keeping the request pending and on a list (->rq). This allows the user to restart them after, for example, adding more space to the host file system containing the disk image file.We keep a list of failed requests and we migrate this list.
Could not find it. It needs to be in virtio-blk in order to know to update the used ring once it completes, right?
Quoted 4 lines
So I think
inuse != 0 when migrating with pending failed I/O requests.Stefan
Okay but let's make sure this is not a bug.