On 09/09/2011 02:59 PM, Kevin Wolf wrote:
Also, I think it should be -EIO instead of -ENOMEM (even though it
doesn't make any difference if we don't call the callback)
If I understood the code correctly, dbs->io_func can only fail if it
fails to get an AIOCB, which is basically out-of-memory.
Yeah, maybe you're right with the error code. Anyway, should we call the
callback?
Considering that out-of-memory cannot happen and a couple of drivers do
return NULL, you're right about going for EIO and calling the callback.
I think it would make sense to require block drivers to return a valid
ACB (qemu_aio_get never returns NULL). If they have an error to report
they should schedule a BH that calls the callback.
Perhaps you can write it down on the Wiki? There is already a block
driver braindump page, right?
Did you consider that there are block drivers that implement
bdrv_aio_cancel() as waiting for completion of outstanding requests? I
think in that case dma_complete() may be called twice. For most of it,
this shouldn't be a problem, but I think it doesn't work with the
qemu_aio_release(dbs).
Right. But then what to do (short of inventing reference counting
of some sort for AIOCBs) with those that don't? Leaking should not
be acceptable, should it?
Hm, not sure. This whole cancellation stuff is so broken...
Maybe we should really refcount dbs (actually it would be more like a
bool in_cancel that means that dma_complete doesn't release the AIOCB)
But then it would leak for the drivers that do not wait for completion?
The problem is that the caller specifies what you should do but you do
not know it.
In fact it may be worse than just the qemu_aio_release: if the driver is
waiting for the request to complete, it will write over the bounce
buffer after dma_bdrv_unmap has been called.
In other words, I don't think this is fixable at all without reference
counting _all_ AIOCBs.
In short, this patch can be dropped, but it shows more problems. :)
I'd rather have it fixed than dropped :-)
Me too. However, I'm looking at this because SCSI reset cancels all
pending I/O, and I would like to do a SCSI reset (and report unit
attention) just before migration, as an easy way to avoid saving the
pending requests in the migration stream. But if it's just leaking the
iovec on the migration source, I really do not care much, not yet at
least, because it's going to exit anyway. The other problems only
happen with DMA over I/O areas, which virtio-scsi won't do. I don't
think it's really fixable, so I quit. :)
Paolo