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

[John Snow <jsnow@redhat.com>]

On 7/29/19 5:51 PM, Paolo Bonzini wrote:
> 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. */
> 

Right, right -- the comment can SAY anything it likes about what the
"contract" is ...

OK, so it's more as if EITHER the cancel callback will invoke
dma_blk_cb, OR the acb object there will ... eventually ... through
normal execution.

OK, ok, ok. Getting it, slowly, slowly.

I think this comment is confusing, actually -- because dma_blk_cb might
not actually get invoked in the stack below this call. We only know it
might.

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

From the other email:

***oh***.

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

OK, this seems right to me then; the last puzzle piece is that Fam added
no-op returns for ECANCELED to the IDE originators of such DMA requests,
but now that I see the pathways beneath here I think it'd be /never/
right to ignore them.

If you cancel IDE's DMA out from under it, I think the IDE state machine
ought to treat it as an error, yes.

Thanks for the help, Paolo!
--js