[Update] sheepdog: don't update inode when create_and_write fails

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(-)

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

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

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