Message ID | 20190729213416.1972-1-pbonzini@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | dma-helpers: ensure AIO callback is invoked after cancellation | expand |
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) >
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
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
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
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
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)
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(-)