Re: [Qemu-devel] [PATCH 3/5] dma-helpers: rewrite completion/cancellation

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


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

In short, this patch can be dropped, but it shows more problems. :)

Paolo