On Wed, 2012-06-20 at 16:25 -0500, Anthony Liguori wrote:
> > +static void dma_aio_cancel(BlockDriverAIOCB *acb)
> > +{
> > + DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common);
> > +
> > + trace_dma_aio_cancel(dbs);
> > +
> > + 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);
>
> So this cancellation stuff is hopelessly broken
>
> It's simply not possible to fully cancel pending DMA in a synchronous
> callback.
>
> Indeed, bdrv_aio_cancel ends up having a nasty little loop in it:
Yes, it's broken. Note that the patch didn't add the above function,
only moved it around.
In any case, I've decided to just drop that patch completely from the
series. IE. I'm not adding the dma_memory_map_with_cancel() variant,
there's no point since:
- Nothing will call the cancel callback today and possibly for a while
- Nothing passes a cancel callback other than the bdrv stuff and that
callback is hopelessly broken as you mentioned above.
So there's just no point. We will add an optional cancel callback again
later when we eventually decide to sort that problem out properly, it
will be an asynchronous cancel, ie, just "initiate" the cancellation,
and I'll probably add that as an argument to the normal dma_memory_map()
(adding NULL to all callers that don't care) at that point.
For now, let's not add a known to be broken and unused interface.
Cheers,
Ben.
