Re: [Qemu-devel] [PATCH 3/5] dma-helpers: rewrite completion/cancellation

[Kevin Wolf <kwolf@redhat.com>]

Am 07.09.2011 17:21, schrieb Paolo Bonzini:
> This fixes various problems with completion/cancellation:
> 
> * If DMA encounters a bounce buffer conflict, and the DMA operation is
> canceled before the bottom half fires, bad things happen.
> 
> * memory is not unmapped after cancellation, again causing problems
> when doing DMA to I/O areas
> 
> * cancellation could leak the iovec
> 
> and probably more that I've missed.  The patch fixes them by sharing
> the cleanup code between completion and cancellation.  The dma_bdrv_cb
> now returns a boolean completed/not completed flag, and the wrapper
> dma_continue takes care of tasks to do upon completion.
> 
> Most of these are basically impossible in practice, but it is better
> to be tidy...
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  dma-helpers.c |   37 ++++++++++++++++++++++++-------------
>  1 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index ca97e14..cc8c4e3 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -58,7 +58,7 @@ static void reschedule_dma(void *opaque)
>  
>      qemu_bh_delete(dbs->bh);
>      dbs->bh = NULL;
> -    dma_bdrv_cb(opaque, 0);
> +    dma_bdrv_cb(dbs, 0);
>  }
>  
>  static void continue_after_map_failure(void *opaque)
> @@ -78,6 +78,21 @@ static void dma_bdrv_unmap(DMAAIOCB *dbs)
>                                    dbs->iov.iov[i].iov_len, !dbs->to_dev,
>                                    dbs->iov.iov[i].iov_len);
>      }
> +    qemu_iovec_reset(&dbs->iov);
> +}
> +
> +static void dma_complete(DMAAIOCB *dbs, int ret)
> +{
> +    dma_bdrv_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_release(dbs);
>  }
>  
>  static void dma_bdrv_cb(void *opaque, int ret)
> @@ -89,12 +104,9 @@ static void dma_bdrv_cb(void *opaque, int ret)
>      dbs->acb = NULL;
>      dbs->sector_num += dbs->iov.size / 512;
>      dma_bdrv_unmap(dbs);
> -    qemu_iovec_reset(&dbs->iov);
>  
>      if (dbs->sg_cur_index == dbs->sg->nsg || ret < 0) {
> -        dbs->common.cb(dbs->common.opaque, ret);
> -        qemu_iovec_destroy(&dbs->iov);
> -        qemu_aio_release(dbs);
> +        dma_complete(dbs, ret);
>          return;
>      }
>  
> @@ -120,9 +132,8 @@ static void dma_bdrv_cb(void *opaque, int ret)
>      dbs->acb = dbs->io_func(dbs->bs, dbs->sector_num, &dbs->iov,
>                              dbs->iov.size / 512, dma_bdrv_cb, dbs);
>      if (!dbs->acb) {
> -        dma_bdrv_unmap(dbs);
> -        qemu_iovec_destroy(&dbs->iov);
> -        return;
> +        dbs->common.cb = NULL;
> +        dma_complete(dbs, -ENOMEM);

Why don't we call the callback here? I know that it already was this way
before your patch, but isn't that a bug?

Also, I think it should be -EIO instead of -ENOMEM (even though it
doesn't make any difference if we don't call the callback)

>      }
>  }
>  
> @@ -131,8 +142,12 @@ static void dma_aio_cancel(BlockDriverAIOCB *acb)
>      DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common);
>  
>      if (dbs->acb) {
> -        bdrv_aio_cancel(dbs->acb);
> +        BlockDriverAIOCB *acb = dbs->acb;
> +        dbs->acb = NULL;
> +        bdrv_aio_cancel(acb);
>      }
> +    dbs->common.cb = NULL;
> +    dma_complete(dbs, 0);

Did you consider that there are block drivers that implement
bdrv_aio_cancel() as waiting for completion of outstanding requests? I
think in that case dma_complete() may be called twice. For most of it,
this shouldn't be a problem, but I think it doesn't work with the
qemu_aio_release(dbs).

Kevin