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

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

[Paolo Bonzini]

dma_aio_cancel unschedules the BH if there is one, which corresponds
to the reschedule_dma case of dma_blk_cb.  This can stall the DMA
permanently, because dma_complete will never get invoked and therefore
nobody will ever invoke the original AIO callback in dbs->common.cb.

Fix this by invoking the callback (which is ensured to happen after
a bdrv_aio_cancel_async, or done manually in the dbs->bh case), and
add assertions to check that the DMA state machine is indeed waiting
for dma_complete or reschedule_dma, but never both.

Reported-by: John Snow <jsnow@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 dma-helpers.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index 2d7e02d..d3871dc 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -90,6 +90,7 @@ static void reschedule_dma(void *opaque)
 {
     DMAAIOCB *dbs = (DMAAIOCB *)opaque;
 
+    assert(!dbs->acb && dbs->bh);
     qemu_bh_delete(dbs->bh);
     dbs->bh = NULL;
     dma_blk_cb(dbs, 0);
@@ -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;
-    }
     qemu_aio_unref(dbs);
 }
 
@@ -179,14 +177,21 @@ static void dma_aio_cancel(BlockAIOCB *acb)
 
     trace_dma_aio_cancel(dbs);
 
+    assert(!(dbs->acb && dbs->bh));
     if (dbs->acb) {
+        /* This will invoke dma_blk_cb.  */
         blk_aio_cancel_async(dbs->acb);
+        return;
     }
+
     if (dbs->bh) {
         cpu_unregister_map_client(dbs->bh);
         qemu_bh_delete(dbs->bh);
         dbs->bh = NULL;
     }
+    if (dbs->common.cb) {
+        dbs->common.cb(dbs->common.opaque, -ECANCELED);
+    }
 }
 
 static AioContext *dma_get_aio_context(BlockAIOCB *acb)
-- 
1.8.3.1

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

[John Snow]

On 7/29/19 5:34 PM, Paolo Bonzini wrote:
> dma_aio_cancel unschedules the BH if there is one, which corresponds
> to the reschedule_dma case of dma_blk_cb.  This can stall the DMA
> permanently, because dma_complete will never get invoked and therefore
> nobody will ever invoke the original AIO callback in dbs->common.cb.
> 
> Fix this by invoking the callback (which is ensured to happen after
> a bdrv_aio_cancel_async, or done manually in the dbs->bh case), and
> add assertions to check that the DMA state machine is indeed waiting
> for dma_complete or reschedule_dma, but never both.
> 
> Reported-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  dma-helpers.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 2d7e02d..d3871dc 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -90,6 +90,7 @@ static void reschedule_dma(void *opaque)
>  {
>      DMAAIOCB *dbs = (DMAAIOCB *)opaque;
>  
> +    assert(!dbs->acb && dbs->bh);
>      qemu_bh_delete(dbs->bh);
>      dbs->bh = NULL;
>      dma_blk_cb(dbs, 0);
> @@ -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,

>      qemu_aio_unref(dbs);
>  }
>  
> @@ -179,14 +177,21 @@ static void dma_aio_cancel(BlockAIOCB *acb)
>  
>      trace_dma_aio_cancel(dbs);
>  
> +    assert(!(dbs->acb && dbs->bh));
>      if (dbs->acb) {
> +        /* This will invoke dma_blk_cb.  */

uhh, does it? 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?

I thought this was just going to NOP entirely. No?

>          blk_aio_cancel_async(dbs->acb);
> +        return;
>      }
> +
>      if (dbs->bh) {
>          cpu_unregister_map_client(dbs->bh);
>          qemu_bh_delete(dbs->bh);
>          dbs->bh = NULL;
>      }
> +    if (dbs->common.cb) {
> +        dbs->common.cb(dbs->common.opaque, -ECANCELED);
> +    }

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.

>  }
>  
>  static AioContext *dma_get_aio_context(BlockAIOCB *acb)
> 

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

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

[John Snow]

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

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

[John Snow]

On 7/29/19 5:34 PM, Paolo Bonzini wrote:
> dma_aio_cancel unschedules the BH if there is one, which corresponds
> to the reschedule_dma case of dma_blk_cb.  This can stall the DMA
> permanently, because dma_complete will never get invoked and therefore
> nobody will ever invoke the original AIO callback in dbs->common.cb.
> 
> Fix this by invoking the callback (which is ensured to happen after
> a bdrv_aio_cancel_async, or done manually in the dbs->bh case), and
> add assertions to check that the DMA state machine is indeed waiting
> for dma_complete or reschedule_dma, but never both.
> 
> Reported-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

No maintainer here, I guess; Paolo will you be pulling this or should I
do it as part of the other IDE fixes I need to make?

--js

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

[John Snow]

On 8/13/19 6:40 PM, John Snow wrote:
> 
> 
> On 7/29/19 5:34 PM, Paolo Bonzini wrote:
>> dma_aio_cancel unschedules the BH if there is one, which corresponds
>> to the reschedule_dma case of dma_blk_cb.  This can stall the DMA
>> permanently, because dma_complete will never get invoked and therefore
>> nobody will ever invoke the original AIO callback in dbs->common.cb.
>>
>> Fix this by invoking the callback (which is ensured to happen after
>> a bdrv_aio_cancel_async, or done manually in the dbs->bh case), and
>> add assertions to check that the DMA state machine is indeed waiting
>> for dma_complete or reschedule_dma, but never both.
>>
>> Reported-by: John Snow <jsnow@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> No maintainer here, I guess; Paolo will you be pulling this or should I
> do it as part of the other IDE fixes I need to make?
> 

Nevermind, I made a decision.

--js