diff mbox

[RFC,v2,6/8] dma: Implement .cancel_async

Message ID 1409033298-5720-7-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng Aug. 26, 2014, 6:08 a.m. UTC 
Just forward the request to bdrv_aio_cancel_async. Use a flag to fix the
ret value in completion code. Also check memory address before calling
dma_memory_unmap.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 dma-helpers.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Paolo Bonzini Aug. 26, 2014, 8:46 a.m. UTC | #1 
Il 26/08/2014 08:08, Fam Zheng ha scritto:
> +    if (dbs->cancelled) {
> +        ret = -ECANCELED;
> +    }

Why is dbs->cancelled necessary?

>      dma_bdrv_unmap(dbs);
>      if (dbs->common.cb) {
>          dbs->common.cb(dbs->common.opaque, ret);
> @@ -141,6 +148,9 @@ static void dma_bdrv_cb(void *opaque, int ret)
>  
>      trace_dma_bdrv_cb(dbs, ret);
>  
> +    if (dbs->cancelled) {
> +        ret = -ECANCELED;
> +    }
>      dbs->acb = NULL;
>      dbs->sector_num += dbs->iov.size / 512;
>  
> @@ -185,6 +195,7 @@ static void dma_aio_cancel(BlockDriverAIOCB *acb)
>  
>      trace_dma_aio_cancel(dbs);
>  
> +    dbs->cancelled = true;
>      if (dbs->acb) {
>          BlockDriverAIOCB *acb = dbs->acb;
>          dbs->acb = NULL;
> @@ -196,9 +207,25 @@ static void dma_aio_cancel(BlockDriverAIOCB *acb)
>      dma_complete(dbs, 0);
>  }
>  
> +static void dma_aio_cancel_async(BlockDriverAIOCB *acb)
> +{
> +    DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common);
> +
> +    trace_dma_aio_cancel(dbs);
> +
> +    dbs->cancelled = true;
> +    if (dbs->acb) {
> +        acb = dbs->acb;
> +        dbs->acb = NULL;

Why do you need to set dbs->acb to NULL, since the callback is going to
be called?

Paolo

> +        bdrv_aio_cancel_async(acb);
> +    }
> +}
Fam Zheng Aug. 26, 2014, 9:21 a.m. UTC | #2 
On Tue, 08/26 10:46, Paolo Bonzini wrote:
> Il 26/08/2014 08:08, Fam Zheng ha scritto:
> > +    if (dbs->cancelled) {
> > +        ret = -ECANCELED;
> > +    }
> 
> Why is dbs->cancelled necessary?

Request may complete after bdrv_aio_cancel_async with other status, this flag
is checked to fix the status to -ECANCELED.

> 
> >      dma_bdrv_unmap(dbs);
> >      if (dbs->common.cb) {
> >          dbs->common.cb(dbs->common.opaque, ret);
> > @@ -141,6 +148,9 @@ static void dma_bdrv_cb(void *opaque, int ret)
> >  
> >      trace_dma_bdrv_cb(dbs, ret);
> >  
> > +    if (dbs->cancelled) {
> > +        ret = -ECANCELED;
> > +    }
> >      dbs->acb = NULL;
> >      dbs->sector_num += dbs->iov.size / 512;
> >  
> > @@ -185,6 +195,7 @@ static void dma_aio_cancel(BlockDriverAIOCB *acb)
> >  
> >      trace_dma_aio_cancel(dbs);
> >  
> > +    dbs->cancelled = true;
> >      if (dbs->acb) {
> >          BlockDriverAIOCB *acb = dbs->acb;
> >          dbs->acb = NULL;
> > @@ -196,9 +207,25 @@ static void dma_aio_cancel(BlockDriverAIOCB *acb)
> >      dma_complete(dbs, 0);
> >  }
> >  
> > +static void dma_aio_cancel_async(BlockDriverAIOCB *acb)
> > +{
> > +    DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common);
> > +
> > +    trace_dma_aio_cancel(dbs);
> > +
> > +    dbs->cancelled = true;
> > +    if (dbs->acb) {
> > +        acb = dbs->acb;
> > +        dbs->acb = NULL;
> 
> Why do you need to set dbs->acb to NULL, since the callback is going to
> be called?

Just copied from dma_aio_cancel, but seems not particularly useful. It reminds
me that the one in dma_aio_cancel also looks suspicious.

Fam
Paolo Bonzini Aug. 26, 2014, 9:57 a.m. UTC | #3 
Il 26/08/2014 11:21, Fam Zheng ha scritto:
> On Tue, 08/26 10:46, Paolo Bonzini wrote:
>> Il 26/08/2014 08:08, Fam Zheng ha scritto:
>>> +    if (dbs->cancelled) {
>>> +        ret = -ECANCELED;
>>> +    }
>>
>> Why is dbs->cancelled necessary?
> 
> Request may complete after bdrv_aio_cancel_async with other status, this flag
> is checked to fix the status to -ECANCELED.

Ah, that's because the operation could be partly incomplete?

I think it would be better to call dma_complete with -ECANCELED, instead
of doing the same dbs->cancelled check twice.  For example, in dma_bdrv_cb:

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

>>> +static void dma_aio_cancel_async(BlockDriverAIOCB *acb)
>>> +{
>>> +    DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common);
>>> +
>>> +    trace_dma_aio_cancel(dbs);
>>> +
>>> +    dbs->cancelled = true;
>>> +    if (dbs->acb) {
>>> +        acb = dbs->acb;
>>> +        dbs->acb = NULL;
>>
>> Why do you need to set dbs->acb to NULL, since the callback is going to
>> be called?
> 
> Just copied from dma_aio_cancel, but seems not particularly useful. It reminds
> me that the one in dma_aio_cancel also looks suspicious.

Yeah, that one looks useless too...

Paolo
diff mbox

Patch

diff --git a/dma-helpers.c b/dma-helpers.c
index 499b52b..4a03e18 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -74,6 +74,7 @@  typedef struct {
     uint64_t sector_num;
     DMADirection dir;
     bool in_cancel;
+    bool cancelled;
     int sg_cur_index;
     dma_addr_t sg_cur_byte;
     QEMUIOVector iov;
@@ -105,6 +106,9 @@  static void dma_bdrv_unmap(DMAAIOCB *dbs)
     int i;
 
     for (i = 0; i < dbs->iov.niov; ++i) {
+        if (!(dbs->iov.iov[i].iov_base && dbs->iov.iov[i].iov_len)) {
+            break;
+        }
         dma_memory_unmap(dbs->sg->as, dbs->iov.iov[i].iov_base,
                          dbs->iov.iov[i].iov_len, dbs->dir,
                          dbs->iov.iov[i].iov_len);
@@ -116,6 +120,9 @@  static void dma_complete(DMAAIOCB *dbs, int ret)
 {
     trace_dma_complete(dbs, ret, dbs->common.cb);
 
+    if (dbs->cancelled) {
+        ret = -ECANCELED;
+    }
     dma_bdrv_unmap(dbs);
     if (dbs->common.cb) {
         dbs->common.cb(dbs->common.opaque, ret);
@@ -141,6 +148,9 @@  static void dma_bdrv_cb(void *opaque, int ret)
 
     trace_dma_bdrv_cb(dbs, ret);
 
+    if (dbs->cancelled) {
+        ret = -ECANCELED;
+    }
     dbs->acb = NULL;
     dbs->sector_num += dbs->iov.size / 512;
 
@@ -185,6 +195,7 @@  static void dma_aio_cancel(BlockDriverAIOCB *acb)
 
     trace_dma_aio_cancel(dbs);
 
+    dbs->cancelled = true;
     if (dbs->acb) {
         BlockDriverAIOCB *acb = dbs->acb;
         dbs->acb = NULL;
@@ -196,9 +207,25 @@  static void dma_aio_cancel(BlockDriverAIOCB *acb)
     dma_complete(dbs, 0);
 }
 
+static void dma_aio_cancel_async(BlockDriverAIOCB *acb)
+{
+    DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common);
+
+    trace_dma_aio_cancel(dbs);
+
+    dbs->cancelled = true;
+    if (dbs->acb) {
+        acb = dbs->acb;
+        dbs->acb = NULL;
+        bdrv_aio_cancel_async(acb);
+    }
+}
+
+
 static const AIOCBInfo dma_aiocb_info = {
     .aiocb_size         = sizeof(DMAAIOCB),
     .cancel             = dma_aio_cancel,
+    .cancel_async       = dma_aio_cancel_async,
 };
 
 BlockDriverAIOCB *dma_bdrv_io(
@@ -218,6 +245,7 @@  BlockDriverAIOCB *dma_bdrv_io(
     dbs->sg_cur_byte = 0;
     dbs->dir = dir;
     dbs->in_cancel = false;
+    dbs->cancelled = false;
     dbs->io_func = io_func;
     dbs->bh = NULL;
     qemu_iovec_init(&dbs->iov, sg->nsg);