[Qemu-devel] [PATCH] sheepdog: don't update inode when create_and_write fails

[Qemu-devel] [PATCH] sheepdog: don't update inode when create_and_write fails

[Liu Yuan]

From: Liu Yuan <tailai.ly@taobao.com>

For the error case such as SD_RES_NO_SPACE, we shouldn't update the inode bitmap
to avoid the scenario that the object is allocated but wasn't created at the
server side. This will result in VM's IO error on the failed object.

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
---
 block/sheepdog.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index a48f58c..ef9e653 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -697,6 +697,12 @@ static void coroutine_fn aio_read_response(void *opaque)
 
     acb = aio_req->aiocb;
 
+    if (rsp.result != SD_RES_SUCCESS) {
+        acb->ret = -EIO;
+        error_report("%s", sd_strerror(rsp.result));
+	goto err;
+    }
+
     switch (acb->aiocb_type) {
     case AIOCB_WRITE_UDATA:
         /* this coroutine context is no longer suitable for co_recv
@@ -736,11 +742,7 @@ static void coroutine_fn aio_read_response(void *opaque)
         break;
     }
 
-    if (rsp.result != SD_RES_SUCCESS) {
-        acb->ret = -EIO;
-        error_report("%s", sd_strerror(rsp.result));
-    }
-
+err:
     free_aio_req(s, aio_req);
     if (!acb->nr_pending) {
         /*
-- 
1.7.9.5

[Qemu-devel] [PATCH] sheepdog: don't update inode when create_and_write fails

[Liu Yuan]

From: Liu Yuan <tailai.ly@taobao.com>

For the error case such as SD_RES_NO_SPACE, we shouldn't update the inode bitmap
to avoid the scenario that the object is allocated but wasn't created at the
server side. This will result in VM's IO error on the failed object.

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
---
 block/sheepdog.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index a48f58c..6116316 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -697,6 +697,12 @@ static void coroutine_fn aio_read_response(void *opaque)
 
     acb = aio_req->aiocb;
 
+    if (rsp.result != SD_RES_SUCCESS) {
+        acb->ret = -EIO;
+        error_report("%s", sd_strerror(rsp.result));
+        goto err;
+    }
+
     switch (acb->aiocb_type) {
     case AIOCB_WRITE_UDATA:
         /* this coroutine context is no longer suitable for co_recv
@@ -736,11 +742,7 @@ static void coroutine_fn aio_read_response(void *opaque)
         break;
     }
 
-    if (rsp.result != SD_RES_SUCCESS) {
-        acb->ret = -EIO;
-        error_report("%s", sd_strerror(rsp.result));
-    }
-
+err:
     free_aio_req(s, aio_req);
     if (!acb->nr_pending) {
         /*
-- 
1.7.9.5

[Qemu-devel] [PATCH Update] sheepdog: don't update inode when create_and_write fails

[Liu Yuan]

From: Liu Yuan <tailai.ly@taobao.com>

For the error case such as SD_RES_NO_SPACE, we shouldn't update the inode bitmap
to avoid the scenario that the object is allocated but wasn't created at the
server side. This will result in VM's IO error on the failed object.

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
---
 Update
  - update the coding style and passed checkpath

 block/sheepdog.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index a48f58c..6116316 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -697,6 +697,12 @@ static void coroutine_fn aio_read_response(void *opaque)
 
     acb = aio_req->aiocb;
 
+    if (rsp.result != SD_RES_SUCCESS) {
+        acb->ret = -EIO;
+        error_report("%s", sd_strerror(rsp.result));
+        goto err;
+    }
+
     switch (acb->aiocb_type) {
     case AIOCB_WRITE_UDATA:
         /* this coroutine context is no longer suitable for co_recv
@@ -736,11 +742,7 @@ static void coroutine_fn aio_read_response(void *opaque)
         break;
     }
 
-    if (rsp.result != SD_RES_SUCCESS) {
-        acb->ret = -EIO;
-        error_report("%s", sd_strerror(rsp.result));
-    }
-
+err:
     free_aio_req(s, aio_req);
     if (!acb->nr_pending) {
         /*
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH Update] sheepdog: don't update inode when create_and_write fails

[MORITA Kazutaka]

At Sat, 15 Dec 2012 17:59:12 +0800,
Liu Yuan wrote:
> 
> From: Liu Yuan <tailai.ly@taobao.com>
> 
> For the error case such as SD_RES_NO_SPACE, we shouldn't update the inode bitmap
> to avoid the scenario that the object is allocated but wasn't created at the
> server side. This will result in VM's IO error on the failed object.
> 
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
> ---
>  Update
>   - update the coding style and passed checkpath
> 
>  block/sheepdog.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index a48f58c..6116316 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -697,6 +697,12 @@ static void coroutine_fn aio_read_response(void *opaque)
>  
>      acb = aio_req->aiocb;
>  
> +    if (rsp.result != SD_RES_SUCCESS) {
> +        acb->ret = -EIO;
> +        error_report("%s", sd_strerror(rsp.result));
> +        goto err;
> +    }
> +
>      switch (acb->aiocb_type) {
>      case AIOCB_WRITE_UDATA:
>          /* this coroutine context is no longer suitable for co_recv
> @@ -736,11 +742,7 @@ static void coroutine_fn aio_read_response(void *opaque)
>          break;
>      }
>  
> -    if (rsp.result != SD_RES_SUCCESS) {
> -        acb->ret = -EIO;
> -        error_report("%s", sd_strerror(rsp.result));
> -    }
> -
> +err:
>      free_aio_req(s, aio_req);
>      if (!acb->nr_pending) {
>          /*

send_pending_req() needs to be called even in error case.  Rather than
moving the error check, I think it looks better to update
s->inode.data_vdi_id only when rsp.result is SD_RES_SUCCESS.

Thanks,

Kazutaka

Re: [Qemu-devel] [PATCH Update] sheepdog: don't update inode when create_and_write fails

[Liu Yuan]

On 12/17/2012 11:43 AM, MORITA Kazutaka wrote:
> send_pending_req() needs to be called even in error case.  Rather than
> moving the error check, I think it looks better to update
> s->inode.data_vdi_id only when rsp.result is SD_RES_SUCCESS.

Why can't we check the rsp.result in the first place? double check
rsp.result inside one function looks a little bit complicated to me.

How about
+    if (rsp.result != SD_RES_SUCCESS) {
+        acb->ret = -EIO;
+        error_report("%s", sd_strerror(rsp.result));
+        send_pending_req(s, aio_req->oid);
+        goto err;
+
+    }

By the way, seems that
  send_pending_req(s, vid_to_data_oid(s->inode.vdi_id, idx));
can be reduced to
  send_pending_req(s, aio_req->oid);

If yes, I can send another patch to fix.

Thanks,
Yuan

Re: [Qemu-devel] [PATCH Update] sheepdog: don't update inode when create_and_write fails

[MORITA Kazutaka]

At Mon, 17 Dec 2012 13:22:31 +0800,
Liu Yuan wrote:
> 
> On 12/17/2012 11:43 AM, MORITA Kazutaka wrote:
> > send_pending_req() needs to be called even in error case.  Rather than
> > moving the error check, I think it looks better to update
> > s->inode.data_vdi_id only when rsp.result is SD_RES_SUCCESS.
> 
> Why can't we check the rsp.result in the first place? double check
> rsp.result inside one function looks a little bit complicated to me.
> 
> How about
> +    if (rsp.result != SD_RES_SUCCESS) {
> +        acb->ret = -EIO;
> +        error_report("%s", sd_strerror(rsp.result));
> +        send_pending_req(s, aio_req->oid);
> +        goto err;
> +
> +    }

Either is okay, but "s->co_recv = NULL" is necessary before
send_pending_req() because we can receive another response while
sending pending requests.  In addition, it is better to call
send_pending_req() only when we update inode; in most cases, we don't
need to traverse pending list.

> 
> By the way, seems that
>   send_pending_req(s, vid_to_data_oid(s->inode.vdi_id, idx));
> can be reduced to
>   send_pending_req(s, aio_req->oid);
> 
> If yes, I can send another patch to fix.

Looks correct.

Thanks,

Kazutaka

[Qemu-devel] [PATCH v2 1/2] sheepdog: don't update inode when create_and_write fails

[Liu Yuan]

From: Liu Yuan <tailai.ly@taobao.com>

For the error case such as SD_RES_NO_SPACE, we shouldn't update the inode bitmap
to avoid the scenario that the object is allocated but wasn't created at the
server side. This will result in VM's IO error on the failed object.

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
---
 block/sheepdog.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index a48f58c..ef7bc81 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -714,10 +714,11 @@ static void coroutine_fn aio_read_response(void *opaque)
              * and max_dirty_data_idx are changed to include updated
              * index between them.
              */
-            s->inode.data_vdi_id[idx] = s->inode.vdi_id;
-            s->max_dirty_data_idx = MAX(idx, s->max_dirty_data_idx);
-            s->min_dirty_data_idx = MIN(idx, s->min_dirty_data_idx);
-
+            if (rsp.result == SD_RES_SUCCESS) {
+                s->inode.data_vdi_id[idx] = s->inode.vdi_id;
+                s->max_dirty_data_idx = MAX(idx, s->max_dirty_data_idx);
+                s->min_dirty_data_idx = MIN(idx, s->min_dirty_data_idx);
+            }
             /*
              * Some requests may be blocked because simultaneous
              * create requests are not allowed, so we search the
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 2/2] sheepdog: pass oid directly to send_pending_req()

[Liu Yuan]

From: Liu Yuan <tailai.ly@taobao.com>

Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
---
 block/sheepdog.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index ef7bc81..ceabc00 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -724,7 +724,7 @@ static void coroutine_fn aio_read_response(void *opaque)
              * create requests are not allowed, so we search the
              * pending requests here.
              */
-            send_pending_req(s, vid_to_data_oid(s->inode.vdi_id, idx));
+            send_pending_req(s, aio_req->oid);
         }
         break;
     case AIOCB_READ_UDATA:
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: pass oid directly to send_pending_req()

[MORITA Kazutaka]

At Mon, 17 Dec 2012 14:17:27 +0800,
Liu Yuan wrote:
> 
> From: Liu Yuan <tailai.ly@taobao.com>
> 
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
> ---
>  block/sheepdog.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

Re: [Qemu-devel] [PATCH v2 1/2] sheepdog: don't update inode when create_and_write fails

[MORITA Kazutaka]

At Mon, 17 Dec 2012 14:17:26 +0800,
Liu Yuan wrote:
> 
> From: Liu Yuan <tailai.ly@taobao.com>
> 
> For the error case such as SD_RES_NO_SPACE, we shouldn't update the inode bitmap
> to avoid the scenario that the object is allocated but wasn't created at the
> server side. This will result in VM's IO error on the failed object.
> 
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
> ---
>  block/sheepdog.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

Re: [Qemu-devel] [PATCH v2 1/2] sheepdog: don't update inode when create_and_write fails

[Stefan Hajnoczi]

On Mon, Dec 17, 2012 at 02:17:26PM +0800, Liu Yuan wrote:
> From: Liu Yuan <tailai.ly@taobao.com>
> 
> For the error case such as SD_RES_NO_SPACE, we shouldn't update the inode bitmap
> to avoid the scenario that the object is allocated but wasn't created at the
> server side. This will result in VM's IO error on the failed object.
> 
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
> ---
>  block/sheepdog.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Thanks, applied to the block tree:
https://github.com/stefanha/qemu/commits/block

Stefan