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

2012-12-16 Thread MORITA Kazutaka 
At Sat, 15 Dec 2012 17:59:12 +0800,
Liu Yuan wrote:
 
 From: Liu Yuan tailai...@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.kazut...@lab.ntt.co.jp
 Cc: Kevin Wolf kw...@redhat.com
 Signed-off-by: Liu Yuan tailai...@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

2012-12-16 Thread 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

2012-12-16 Thread 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