Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

[Michael S. Tsirkin]

On Wed, Dec 12, 2012 at 04:01:27PM +0100, Paolo Bonzini wrote:
> Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto:
> > > 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?

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.

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

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

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

-- 
MST