(no subject)

(no subject)

[Stefan Priebe]

From Stefan Priebe <s.priebe-2Lf/h1ldwEHR5kwTpVNS9A@public.gmane.org> # This line is ignored.
From: Stefan Priebe <s.priebe-2Lf/h1ldwEHR5kwTpVNS9A@public.gmane.org>
Cc: pve-devel-KmHT29P9Uc/4CZzEM2C48g@public.gmane.org
Cc: pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Cc: ceph-devel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: QEMU/PATCH: rbd block driver: fix race between completition and cancel
In-Reply-To:


ve-devel-KmHT29P9Uc/4CZzEM2C48g@public.gmane.org
pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
ceph-devel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

[PATCH] rbd block driver fix race between aio completition and aio cancel

[Stefan Priebe]

From: Stefan Priebe <s.priebe@profhost.ag>

This one fixes a race qemu also had in iscsi block driver between
cancellation and io completition.

qemu_rbd_aio_cancel was not synchronously waiting for the end of
the command.

It also removes the useless cancelled flag and introduces instead
a status flag with EINPROGRESS like iscsi block driver.

Signed-off-by: Stefan Priebe <s.priebe@profihost.ag>
---
  block/rbd.c |   19 ++++++++++++-------
  1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 5a0f79f..7b3bcbb 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -76,7 +76,7 @@ typedef struct RBDAIOCB {
      int64_t sector_num;
      int error;
      struct BDRVRBDState *s;
-    int cancelled;
+    int status;
  } RBDAIOCB;
   typedef struct RADOSCB {
@@ -376,9 +376,7 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
      RBDAIOCB *acb = rcb->acb;
      int64_t r;
  -    if (acb->cancelled) {
-        qemu_vfree(acb->bounce);
-        qemu_aio_release(acb);
+    if (acb->bh) {
          goto done;
      }
  @@ -406,9 +404,12 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
              acb->ret = r;
          }
      }
+    acb->status = acb->ret;
+
      /* Note that acb->bh can be NULL in case where the aio was 
cancelled */
      acb->bh = qemu_bh_new(rbd_aio_bh_cb, acb);
      qemu_bh_schedule(acb->bh);
+
  done:
      g_free(rcb);
  }
@@ -573,7 +574,10 @@ static void qemu_rbd_close(BlockDriverState *bs)
  static void qemu_rbd_aio_cancel(BlockDriverAIOCB *blockacb)
  {
      RBDAIOCB *acb = (RBDAIOCB *) blockacb;
-    acb->cancelled = 1;
+
+    while (acb->status == -EINPROGRESS) {
+        qemu_aio_wait();
+    }
  }
   static AIOPool rbd_aio_pool = {
@@ -642,10 +646,11 @@ static void rbd_aio_bh_cb(void *opaque)
          qemu_iovec_from_buf(acb->qiov, 0, acb->bounce, acb->qiov->size);
      }
      qemu_vfree(acb->bounce);
-    acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
      qemu_bh_delete(acb->bh);
      acb->bh = NULL;
  +    acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
+
      qemu_aio_release(acb);
  }
  @@ -689,8 +694,8 @@ static BlockDriverAIOCB 
*rbd_start_aio(BlockDriverState *bs,
      acb->ret = 0;
      acb->error = 0;
      acb->s = s;
-    acb->cancelled = 0;
      acb->bh = NULL;
+    acb->status = -EINPROGRESS;
       if (cmd == RBD_AIO_WRITE) {
          qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
-- 
1.7.10.4

[PATCH] rbd block driver fix race between aio completition and aio cancel

[Stefan Priebe]

This one fixes a race which qemu had also in iscsi block driver
between cancellation and io completition.

qemu_rbd_aio_cancel was not synchronously waiting for the end of
the command.

To archieve this it introduces a new status flag which uses
-EINPROGRESS.

Signed-off-by: Stefan Priebe <s.priebe@profihost.ag>
---
 block/rbd.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 0384c6c..783c3d7 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -77,6 +77,7 @@ typedef struct RBDAIOCB {
     int error;
     struct BDRVRBDState *s;
     int cancelled;
+    int status;
 } RBDAIOCB;
 
 typedef struct RADOSCB {
@@ -376,12 +377,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
     RBDAIOCB *acb = rcb->acb;
     int64_t r;
 
-    if (acb->cancelled) {
-        qemu_vfree(acb->bounce);
-        qemu_aio_release(acb);
-        goto done;
-    }
-
     r = rcb->ret;
 
     if (acb->cmd == RBD_AIO_WRITE ||
@@ -406,10 +401,11 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
             acb->ret = r;
         }
     }
+    acb->status = 0;
+
     /* Note that acb->bh can be NULL in case where the aio was cancelled */
     acb->bh = qemu_bh_new(rbd_aio_bh_cb, acb);
     qemu_bh_schedule(acb->bh);
-done:
     g_free(rcb);
 }
 
@@ -574,6 +570,12 @@ static void qemu_rbd_aio_cancel(BlockDriverAIOCB *blockacb)
 {
     RBDAIOCB *acb = (RBDAIOCB *) blockacb;
     acb->cancelled = 1;
+
+    while (acb->status == -EINPROGRESS) {
+        qemu_aio_wait();
+    }
+
+    qemu_aio_release(acb);
 }
 
 static AIOPool rbd_aio_pool = {
@@ -646,7 +648,8 @@ static void rbd_aio_bh_cb(void *opaque)
     qemu_bh_delete(acb->bh);
     acb->bh = NULL;
 
-    qemu_aio_release(acb);
+    if (!acb->cancelled)
+        qemu_aio_release(acb);
 }
 
 static int rbd_aio_discard_wrapper(rbd_image_t image,
@@ -691,6 +694,7 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState *bs,
     acb->s = s;
     acb->cancelled = 0;
     acb->bh = NULL;
+    acb->status = -EINPROGRESS;
 
     if (cmd == RBD_AIO_WRITE) {
         qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
@@ -737,7 +741,8 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState *bs,
 failed:
     g_free(rcb);
     s->qemu_aio_count--;
-    qemu_aio_release(acb);
+    if (!acb->cancelled)
+        qemu_aio_release(acb);
     return NULL;
 }
 
-- 
1.7.10.4

Re: [PATCH] rbd block driver fix race between aio completition and aio cancel

[Josh Durgin]

On 11/22/2012 02:00 AM, Stefan Priebe wrote:
> This one fixes a race which qemu had also in iscsi block driver
> between cancellation and io completition.
>
> qemu_rbd_aio_cancel was not synchronously waiting for the end of
> the command.
>
> To archieve this it introduces a new status flag which uses
> -EINPROGRESS.
>
> Signed-off-by: Stefan Priebe <s.priebe@profihost.ag>
> ---
>   block/rbd.c |   23 ++++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 0384c6c..783c3d7 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -77,6 +77,7 @@ typedef struct RBDAIOCB {
>       int error;
>       struct BDRVRBDState *s;
>       int cancelled;
> +    int status;
>   } RBDAIOCB;
>
>   typedef struct RADOSCB {
> @@ -376,12 +377,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>       RBDAIOCB *acb = rcb->acb;
>       int64_t r;
>
> -    if (acb->cancelled) {
> -        qemu_vfree(acb->bounce);
> -        qemu_aio_release(acb);
> -        goto done;
> -    }
> -
>       r = rcb->ret;
>
>       if (acb->cmd == RBD_AIO_WRITE ||
> @@ -406,10 +401,11 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>               acb->ret = r;
>           }
>       }
> +    acb->status = 0;
> +
>       /* Note that acb->bh can be NULL in case where the aio was cancelled */
>       acb->bh = qemu_bh_new(rbd_aio_bh_cb, acb);
>       qemu_bh_schedule(acb->bh);
> -done:
>       g_free(rcb);
>   }
>
> @@ -574,6 +570,12 @@ static void qemu_rbd_aio_cancel(BlockDriverAIOCB *blockacb)
>   {
>       RBDAIOCB *acb = (RBDAIOCB *) blockacb;
>       acb->cancelled = 1;
> +
> +    while (acb->status == -EINPROGRESS) {
> +        qemu_aio_wait();
> +    }
> +

There should be a qemu_vfree(acb->bounce); here

> +    qemu_aio_release(acb);
>   }
>
>   static AIOPool rbd_aio_pool = {
> @@ -646,7 +648,8 @@ static void rbd_aio_bh_cb(void *opaque)
>       qemu_bh_delete(acb->bh);
>       acb->bh = NULL;
>
> -    qemu_aio_release(acb);
> +    if (!acb->cancelled)
> +        qemu_aio_release(acb);
>   }
>
>   static int rbd_aio_discard_wrapper(rbd_image_t image,
> @@ -691,6 +694,7 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState *bs,
>       acb->s = s;
>       acb->cancelled = 0;
>       acb->bh = NULL;
> +    acb->status = -EINPROGRESS;
>
>       if (cmd == RBD_AIO_WRITE) {
>           qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
> @@ -737,7 +741,8 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState *bs,
>   failed:
>       g_free(rcb);
>       s->qemu_aio_count--;
> -    qemu_aio_release(acb);
> +    if (!acb->cancelled)

qemu_vfree(acb->bounce) should be here as well, although that's a
separate bug that's probably never hit.

> +        qemu_aio_release(acb);
>       return NULL;
>   }
>
>

Re: [PATCH] rbd block driver fix race between aio completition and aio cancel

[Stefan Hajnoczi]

On Thu, Nov 22, 2012 at 11:00:19AM +0100, Stefan Priebe wrote:
> @@ -406,10 +401,11 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>              acb->ret = r;
>          }
>      }
> +    acb->status = 0;
> +

I suggest doing this in the BH.  The qemu_aio_wait() loop in
qemu_rbd_aio_cancel() needs to wait until the BH has executed.  By
clearing status in the BH we ensure that no matter in which order
qemu_aio_wait() invokes BHs and callbacks, we'll always wait until the
BH has completed before ending the while loop in qemu_rbd_aio_cancel().

> @@ -737,7 +741,8 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState *bs,
>  failed:
>      g_free(rcb);
>      s->qemu_aio_count--;
> -    qemu_aio_release(acb);
> +    if (!acb->cancelled)
> +        qemu_aio_release(acb);
>      return NULL;
>  }

This scenario is impossible.  We haven't returned the acb back to the
caller yet so they could not have invoked qemu_aio_cancel().

Stefan

Re: [PATCH] rbd block driver fix race between aio completition and aio cancel

[Stefan Priebe - Profihost AG]

Hi,

i hope i've done everything correctly. I've send a new v4 patch.

Am 29.11.2012 14:58, schrieb Stefan Hajnoczi:
> On Thu, Nov 22, 2012 at 11:00:19AM +0100, Stefan Priebe wrote:
>> @@ -406,10 +401,11 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>>               acb->ret = r;
>>           }
>>       }
>> +    acb->status = 0;
>> +
>
> I suggest doing this in the BH.  The qemu_aio_wait() loop in
> qemu_rbd_aio_cancel() needs to wait until the BH has executed.  By
> clearing status in the BH we ensure that no matter in which order
> qemu_aio_wait() invokes BHs and callbacks, we'll always wait until the
> BH has completed before ending the while loop in qemu_rbd_aio_cancel().
>
>> @@ -737,7 +741,8 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState *bs,
>>   failed:
>>       g_free(rcb);
>>       s->qemu_aio_count--;
>> -    qemu_aio_release(acb);
>> +    if (!acb->cancelled)
>> +        qemu_aio_release(acb);
>>       return NULL;
>>   }
>
> This scenario is impossible.  We haven't returned the acb back to the
> caller yet so they could not have invoked qemu_aio_cancel().

Greets,
Stefan

Re: [PATCH] rbd block driver fix race between aio completition and aio cancel

[Paolo Bonzini]

> > @@ -574,6 +570,12 @@ static void
> > qemu_rbd_aio_cancel(BlockDriverAIOCB *blockacb)
> >   {
> >       RBDAIOCB *acb = (RBDAIOCB *) blockacb;
> >       acb->cancelled = 1;
> > +
> > +    while (acb->status == -EINPROGRESS) {
> > +        qemu_aio_wait();
> > +    }
> > +
> 
> There should be a qemu_vfree(acb->bounce); here

No, because the BH will have run at this point and you'd doubly-free
the buffer.

Paolo

> > +    qemu_aio_release(acb);
> >   }
> >
> >   static AIOPool rbd_aio_pool = {
> > @@ -646,7 +648,8 @@ static void rbd_aio_bh_cb(void *opaque)
> >       qemu_bh_delete(acb->bh);
> >       acb->bh = NULL;
> >
> > -    qemu_aio_release(acb);
> > +    if (!acb->cancelled)
> > +        qemu_aio_release(acb);
> >   }
> >
> >   static int rbd_aio_discard_wrapper(rbd_image_t image,
> > @@ -691,6 +694,7 @@ static BlockDriverAIOCB
> > *rbd_start_aio(BlockDriverState *bs,
> >       acb->s = s;
> >       acb->cancelled = 0;
> >       acb->bh = NULL;
> > +    acb->status = -EINPROGRESS;
> >
> >       if (cmd == RBD_AIO_WRITE) {
> >           qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
> > @@ -737,7 +741,8 @@ static BlockDriverAIOCB
> > *rbd_start_aio(BlockDriverState *bs,
> >   failed:
> >       g_free(rcb);
> >       s->qemu_aio_count--;
> > -    qemu_aio_release(acb);
> > +    if (!acb->cancelled)
> 
> qemu_vfree(acb->bounce) should be here as well, although that's a
> separate bug that's probably never hit.
> 
> > +        qemu_aio_release(acb);
> >       return NULL;
> >   }
> >
> >
> 
>