Am 09.09.2011 15:12, schrieb Paolo Bonzini: > 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?
http://wiki.qemu.org/BlockRoadmap This one? Adding it there now. >>>> 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. Why would it leak? To clarify, what I'm thinking of is: static void dma_aio_cancel(BlockDriverAIOCB *acb) { DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common); if (dbs->acb) { BlockDriverAIOCB *acb = dbs->acb; dbs->acb = NULL; dbs->in_cancel = true; bdrv_aio_cancel(acb); dbs->in_cancel = false; } dbs->common.cb = NULL; dma_complete(dbs, 0); } And then in dma_complete: ... if (!dbs->in_cancel) { qemu_aio_release(dbs); } } So the release that we avoid is the release in the callback that may or may not be called indirectly by bdrv_aio_cancel. We always call dma_complete at the end of dma_aio_cancel so that it will be properly freed. > 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. How that? dma_bdrv_unmap is called only afterwards, isn't it? Kevin