Re: [Qemu-devel] [PATCH] dma-helpers: ensure AIO callback is invoked after cancellation

[Paolo Bonzini]

On 29/07/19 23:46, John Snow wrote:
>> @@ -111,15 +112,12 @@ static void dma_complete(DMAAIOCB *dbs, int ret)
>>  {
>>      trace_dma_complete(dbs, ret, dbs->common.cb);
>>  
>> +    assert(!dbs->acb && !dbs->bh);
>>      dma_blk_unmap(dbs);
>>      if (dbs->common.cb) {
>>          dbs->common.cb(dbs->common.opaque, ret);
>>      }
>>      qemu_iovec_destroy(&dbs->iov);
>> -    if (dbs->bh) {
>> -        qemu_bh_delete(dbs->bh);
>> -        dbs->bh = NULL;
>> -    }
> 
> Now presumably handled by dma_aio_cancel,

No, it simply could never happen.  dma_complete is called here in dma_blk_cb:

    dbs->acb = NULL;
    dbs->offset += dbs->iov.size;

    if (dbs->sg_cur_index == dbs->sg->nsg || ret < 0) {
        dma_complete(dbs, ret);
        return;
    }

and the only way to reach that when dbs->bh becomes non-NULL is through 
reschedule_dma, which clears dbs->bh before invoking dma_blk_cb.

>>      if (dbs->acb) {
>> +        /* This will invoke dma_blk_cb.  */
> 
> uhh, does it?

Yes:

/* Async version of aio cancel. The caller is not blocked if the acb implements
 * cancel_async, otherwise we do nothing and let the request normally complete.
 * In either case the completion callback must be called. */

> this is maybe where I got lost reading this code.
> Isn't dbs->acb going to be what was returned from e.g.
> dma_blk_read_io_func, which ultimately uses blk_aio_em_aiocb_info, that
> has no cancel callback?

Right therefore the I/O will complete and the callback will be invoked.

> Well, here at least I am now on terra-firma that we're going to call the
> original callback with ECANCELED, which is a step towards code that
> isn't surprising my sensibilities.

Good. :)

Paolo