Am 09.09.2011 14:43, schrieb Paolo Bonzini: >>> @@ -120,9 +132,8 @@ static void dma_bdrv_cb(void *opaque, int ret) >>> dbs->acb = dbs->io_func(dbs->bs, dbs->sector_num, &dbs->iov, >>> dbs->iov.size / 512, dma_bdrv_cb, dbs); >>> if (!dbs->acb) { >>> - dma_bdrv_unmap(dbs); >>> - qemu_iovec_destroy(&dbs->iov); >>> - return; >>> + dbs->common.cb = NULL; >>> + dma_complete(dbs, -ENOMEM); >> >> Why don't we call the callback here? I know that it already was this >> way before your patch, but isn't that a bug? >> >> 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? > By the way, I > remember reading (from you?) that this is bogus and it would be cleaner > if callers of functions returning an AIOCB just assumed the return value > to be non-NULL. > > I checked now, and the following block drivers can return a NULL AIOCB > even if qemu_aio_get succeeds: > > * blkdebug (easily fixed ;)) > > * curl (seems like it also boils down to out-of-memory) > > * rbd (can fail in librbd; might defer completion with an error to a > bottom half instead) > > * linux-aio (when io_submit fails; might fall back to posix-aio-compat > instead). 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. >>> @@ -131,8 +142,12 @@ static void dma_aio_cancel(BlockDriverAIOCB >>> *acb) >>> DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common); >>> >>> if (dbs->acb) { >>> - bdrv_aio_cancel(dbs->acb); >>> + BlockDriverAIOCB *acb = dbs->acb; >>> + dbs->acb = NULL; >>> + bdrv_aio_cancel(acb); >>> } >>> + dbs->common.cb = NULL; >>> + dma_complete(dbs, 0); >> >> 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) > In short, this patch can be dropped, but it shows more problems. :) I'd rather have it fixed than dropped :-) Kevin