[Qemu-devel] [PATCH 0/2] sheepdog driver update related to VDI locking feature

[Qemu-devel] [PATCH 0/2] sheepdog driver update related to VDI locking feature

[Hitoshi Mitake]

Recently, sheepdog revived VDI locking functionality. This patch
updates sheepdog driver of QEMU for this feature.

Hitoshi Mitake (2):
  sheepdog: adopting protocol update for VDI locking
  sheepdog: improve error handling for a case of failed lock

 block/sheepdog.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH 1/2] sheepdog: adopting protocol update for VDI locking

[Hitoshi Mitake]

The update is required for supporting iSCSI multipath. It doesn't
affect behavior of QEMU driver but adding a new field to vdi request
struct is required.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Liu Yuan <namei.unix@gmail.com>
Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
---
 block/sheepdog.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 8d9350c..36f76f0 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -103,6 +103,9 @@
 #define SD_INODE_SIZE (sizeof(SheepdogInode))
 #define CURRENT_VDI_ID 0
 
+#define LOCK_TYPE_NORMAL 1
+#define LOCK_TYPE_SHARED 2      /* for iSCSI multipath */
+
 typedef struct SheepdogReq {
     uint8_t proto_ver;
     uint8_t opcode;
@@ -166,7 +169,8 @@ typedef struct SheepdogVdiReq {
     uint8_t copy_policy;
     uint8_t reserved[2];
     uint32_t snapid;
-    uint32_t pad[3];
+    uint32_t type;
+    uint32_t pad[2];
 } SheepdogVdiReq;
 
 typedef struct SheepdogVdiRsp {
@@ -1090,6 +1094,7 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
     memset(&hdr, 0, sizeof(hdr));
     if (lock) {
         hdr.opcode = SD_OP_LOCK_VDI;
+        hdr.type = LOCK_TYPE_NORMAL;
     } else {
         hdr.opcode = SD_OP_GET_VDI_INFO;
     }
@@ -1793,6 +1798,7 @@ static void sd_close(BlockDriverState *bs)
     memset(&hdr, 0, sizeof(hdr));
 
     hdr.opcode = SD_OP_RELEASE_VDI;
+    hdr.type = LOCK_TYPE_NORMAL;
     hdr.base_vdi_id = s->inode.vdi_id;
     wlen = strlen(s->name) + 1;
     hdr.data_length = wlen;
-- 
1.8.3.2

[Qemu-devel] [PATCH 2/2] sheepdog: improve error handling for a case of failed lock

[Hitoshi Mitake]

Recently, sheepdog revived its VDI locking functionality. This patch
updates sheepdog driver of QEMU for this feature:

1. Improve error message when QEMU fails to acquire lock of
VDI. Current sheepdog driver prints an error message "VDI isn't
locked" when it fails to acquire lock. It is a little bit confusing
because the mesage says VDI isn't locked but it is actually locked by
other VM. This patch modifies this confusing message.

2. Change error code for a case of failed locking. -EBUSY is a
suitable one.

Reported-by: Valerio Pachera <sirio81@gmail.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Liu Yuan <namei.unix@gmail.com>
Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
---
 block/sheepdog.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 36f76f0..0b3f86d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1112,9 +1112,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
 
     if (rsp->result != SD_RES_SUCCESS) {
         error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s",
+                   rsp->result == SD_RES_VDI_NOT_LOCKED ?
+                   "VDI is already locked by other VM" :
                    sd_strerror(rsp->result), filename, snapid, tag);
         if (rsp->result == SD_RES_NO_VDI) {
             ret = -ENOENT;
+        } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) {
+            ret = -EBUSY;
         } else {
             ret = -EIO;
         }
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 1/2] sheepdog: adopting protocol update for VDI locking

[Liu Yuan]

On Thu, Aug 07, 2014 at 04:28:39PM +0900, Hitoshi Mitake wrote:
> The update is required for supporting iSCSI multipath. It doesn't
> affect behavior of QEMU driver but adding a new field to vdi request
> struct is required.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Liu Yuan <namei.unix@gmail.com>
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> ---
>  block/sheepdog.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 8d9350c..36f76f0 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -103,6 +103,9 @@
>  #define SD_INODE_SIZE (sizeof(SheepdogInode))
>  #define CURRENT_VDI_ID 0
>  
> +#define LOCK_TYPE_NORMAL 1
> +#define LOCK_TYPE_SHARED 2      /* for iSCSI multipath */

How about

#define LOCK_TYPE_NORMAL 0
#define LOCK_TYPE_SHARED 1

Then we don't need this patch. Since qemu won't make use of multipath for the
near future, we should avoid adding stuff related to multipath to qemu driver.

Thanks
Yuan

> +
>  typedef struct SheepdogReq {
>      uint8_t proto_ver;
>      uint8_t opcode;
> @@ -166,7 +169,8 @@ typedef struct SheepdogVdiReq {
>      uint8_t copy_policy;
>      uint8_t reserved[2];
>      uint32_t snapid;
> -    uint32_t pad[3];
> +    uint32_t type;
> +    uint32_t pad[2];
>  } SheepdogVdiReq;
>  
>  typedef struct SheepdogVdiRsp {
> @@ -1090,6 +1094,7 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
>      memset(&hdr, 0, sizeof(hdr));
>      if (lock) {
>          hdr.opcode = SD_OP_LOCK_VDI;
> +        hdr.type = LOCK_TYPE_NORMAL;
>      } else {
>          hdr.opcode = SD_OP_GET_VDI_INFO;
>      }
> @@ -1793,6 +1798,7 @@ static void sd_close(BlockDriverState *bs)
>      memset(&hdr, 0, sizeof(hdr));
>  
>      hdr.opcode = SD_OP_RELEASE_VDI;
> +    hdr.type = LOCK_TYPE_NORMAL;
>      hdr.base_vdi_id = s->inode.vdi_id;
>      wlen = strlen(s->name) + 1;
>      hdr.data_length = wlen;
> -- 
> 1.8.3.2
> 

Re: [Qemu-devel] [PATCH 2/2] sheepdog: improve error handling for a case of failed lock

[Liu Yuan]

On Thu, Aug 07, 2014 at 04:28:40PM +0900, Hitoshi Mitake wrote:
> Recently, sheepdog revived its VDI locking functionality. This patch
> updates sheepdog driver of QEMU for this feature:
> 
> 1. Improve error message when QEMU fails to acquire lock of
> VDI. Current sheepdog driver prints an error message "VDI isn't
> locked" when it fails to acquire lock. It is a little bit confusing
> because the mesage says VDI isn't locked but it is actually locked by
> other VM. This patch modifies this confusing message.
> 
> 2. Change error code for a case of failed locking. -EBUSY is a
> suitable one.
> 
> Reported-by: Valerio Pachera <sirio81@gmail.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Liu Yuan <namei.unix@gmail.com>
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> ---
>  block/sheepdog.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 36f76f0..0b3f86d 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1112,9 +1112,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
>  
>      if (rsp->result != SD_RES_SUCCESS) {
>          error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s",
> +                   rsp->result == SD_RES_VDI_NOT_LOCKED ?

I'm puzzled by this check.

we use SD_RES_VDI_LOCKED to indicate vid is already locked, no?

> +                   "VDI is already locked by other VM" :
>                     sd_strerror(rsp->result), filename, snapid, tag);
>          if (rsp->result == SD_RES_NO_VDI) {
>              ret = -ENOENT;
> +        } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) {
> +            ret = -EBUSY;
>          } else {
>              ret = -EIO;
>          }

It is better to use switch case to handle the result.

Thanks
Yuan

Re: [Qemu-devel] [PATCH 1/2] sheepdog: adopting protocol update for VDI locking

[Hitoshi Mitake]

At Fri, 8 Aug 2014 13:20:39 +0800,
Liu Yuan wrote:
> 
> On Thu, Aug 07, 2014 at 04:28:39PM +0900, Hitoshi Mitake wrote:
> > The update is required for supporting iSCSI multipath. It doesn't
> > affect behavior of QEMU driver but adding a new field to vdi request
> > struct is required.
> > 
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Cc: Liu Yuan <namei.unix@gmail.com>
> > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> > ---
> >  block/sheepdog.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index 8d9350c..36f76f0 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -103,6 +103,9 @@
> >  #define SD_INODE_SIZE (sizeof(SheepdogInode))
> >  #define CURRENT_VDI_ID 0
> >  
> > +#define LOCK_TYPE_NORMAL 1
> > +#define LOCK_TYPE_SHARED 2      /* for iSCSI multipath */
> 
> How about
> 
> #define LOCK_TYPE_NORMAL 0
> #define LOCK_TYPE_SHARED 1
> 
> Then we don't need this patch. Since qemu won't make use of multipath for the
> near future, we should avoid adding stuff related to multipath to qemu driver.

(Cc-ing current Kazutaka-san's address)

I think this isn't a good idea. Because it means that sheep has an
assumption about padding field of the request data struct. This sort
of workaround can cause hard to find problems in the future.

Thanks,
Hitoshi

> 
> Thanks
> Yuan
> 
> > +
> >  typedef struct SheepdogReq {
> >      uint8_t proto_ver;
> >      uint8_t opcode;
> > @@ -166,7 +169,8 @@ typedef struct SheepdogVdiReq {
> >      uint8_t copy_policy;
> >      uint8_t reserved[2];
> >      uint32_t snapid;
> > -    uint32_t pad[3];
> > +    uint32_t type;
> > +    uint32_t pad[2];
> >  } SheepdogVdiReq;
> >  
> >  typedef struct SheepdogVdiRsp {
> > @@ -1090,6 +1094,7 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
> >      memset(&hdr, 0, sizeof(hdr));
> >      if (lock) {
> >          hdr.opcode = SD_OP_LOCK_VDI;
> > +        hdr.type = LOCK_TYPE_NORMAL;
> >      } else {
> >          hdr.opcode = SD_OP_GET_VDI_INFO;
> >      }
> > @@ -1793,6 +1798,7 @@ static void sd_close(BlockDriverState *bs)
> >      memset(&hdr, 0, sizeof(hdr));
> >  
> >      hdr.opcode = SD_OP_RELEASE_VDI;
> > +    hdr.type = LOCK_TYPE_NORMAL;
> >      hdr.base_vdi_id = s->inode.vdi_id;
> >      wlen = strlen(s->name) + 1;
> >      hdr.data_length = wlen;
> > -- 
> > 1.8.3.2
> > 

Re: [Qemu-devel] [PATCH 2/2] sheepdog: improve error handling for a case of failed lock

[Hitoshi Mitake]

At Fri, 8 Aug 2014 13:31:39 +0800,
Liu Yuan wrote:
> 
> On Thu, Aug 07, 2014 at 04:28:40PM +0900, Hitoshi Mitake wrote:
> > Recently, sheepdog revived its VDI locking functionality. This patch
> > updates sheepdog driver of QEMU for this feature:
> > 
> > 1. Improve error message when QEMU fails to acquire lock of
> > VDI. Current sheepdog driver prints an error message "VDI isn't
> > locked" when it fails to acquire lock. It is a little bit confusing
> > because the mesage says VDI isn't locked but it is actually locked by
> > other VM. This patch modifies this confusing message.
> > 
> > 2. Change error code for a case of failed locking. -EBUSY is a
> > suitable one.
> > 
> > Reported-by: Valerio Pachera <sirio81@gmail.com>
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Cc: Liu Yuan <namei.unix@gmail.com>
> > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> > ---
> >  block/sheepdog.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index 36f76f0..0b3f86d 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -1112,9 +1112,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
> >  
> >      if (rsp->result != SD_RES_SUCCESS) {
> >          error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s",
> > +                   rsp->result == SD_RES_VDI_NOT_LOCKED ?
> 
> I'm puzzled by this check.
> 
> we use SD_RES_VDI_LOCKED to indicate vid is already locked, no?

We use SD_RES_VDI_NOT_LOCKED for indicating locking by this VM fails.

> 
> > +                   "VDI is already locked by other VM" :
> >                     sd_strerror(rsp->result), filename, snapid, tag);
> >          if (rsp->result == SD_RES_NO_VDI) {
> >              ret = -ENOENT;
> > +        } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) {
> > +            ret = -EBUSY;
> >          } else {
> >              ret = -EIO;
> >          }
> 
> It is better to use switch case to handle the result.

using switch statement in this case only increases a number of lines
of code:

Current change:
        if (rsp->result == SD_RES_NO_VDI) {
            ret = -ENOENT;
        } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) {
...

Change with switch:
        switch (rsp->result) {
	    case SD_RES_NO_VDI:
            ret = -ENOENT;
	    break;
	    case SD_RES_VDI_NOT_LOCKED:
...

The change with switch statement requires one more line for break;. I
think if statement is suitable for this case.

Thanks,
Hitoshi

Re: [Qemu-devel] [PATCH 2/2] sheepdog: improve error handling for a case of failed lock

[Liu Yuan]

On Fri, Aug 08, 2014 at 03:17:59PM +0900, Hitoshi Mitake wrote:
> At Fri, 8 Aug 2014 13:31:39 +0800,
> Liu Yuan wrote:
> > 
> > On Thu, Aug 07, 2014 at 04:28:40PM +0900, Hitoshi Mitake wrote:
> > > Recently, sheepdog revived its VDI locking functionality. This patch
> > > updates sheepdog driver of QEMU for this feature:
> > > 
> > > 1. Improve error message when QEMU fails to acquire lock of
> > > VDI. Current sheepdog driver prints an error message "VDI isn't
> > > locked" when it fails to acquire lock. It is a little bit confusing
> > > because the mesage says VDI isn't locked but it is actually locked by
> > > other VM. This patch modifies this confusing message.
> > > 
> > > 2. Change error code for a case of failed locking. -EBUSY is a
> > > suitable one.
> > > 
> > > Reported-by: Valerio Pachera <sirio81@gmail.com>
> > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > Cc: Liu Yuan <namei.unix@gmail.com>
> > > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> > > ---
> > >  block/sheepdog.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > > index 36f76f0..0b3f86d 100644
> > > --- a/block/sheepdog.c
> > > +++ b/block/sheepdog.c
> > > @@ -1112,9 +1112,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
> > >  
> > >      if (rsp->result != SD_RES_SUCCESS) {
> > >          error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s",
> > > +                   rsp->result == SD_RES_VDI_NOT_LOCKED ?
> > 
> > I'm puzzled by this check.
> > 
> > we use SD_RES_VDI_LOCKED to indicate vid is already locked, no?
> 
> We use SD_RES_VDI_NOT_LOCKED for indicating locking by this VM fails.
> 
> > 
> > > +                   "VDI is already locked by other VM" :

But this message said it was locked by others, and we have SD_RES_VDI_LOCKED for
this case.

We need fix sheep daemon for this case to return SD_RES_VDI_LOCKED for already
locked case and NOT_LOCKED for other sheep internal errors.

> > >                     sd_strerror(rsp->result), filename, snapid, tag);
> > >          if (rsp->result == SD_RES_NO_VDI) {
> > >              ret = -ENOENT;
> > > +        } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) {
> > > +            ret = -EBUSY;
> > >          } else {
> > >              ret = -EIO;
> > >          }
> > 
> > It is better to use switch case to handle the result.
> 
> using switch statement in this case only increases a number of lines
> of code:
> 
> Current change:
>         if (rsp->result == SD_RES_NO_VDI) {
>             ret = -ENOENT;
>         } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) {
> ...
> 
> Change with switch:
>         switch (rsp->result) {
> 	    case SD_RES_NO_VDI:
>             ret = -ENOENT;
> 	    break;
> 	    case SD_RES_VDI_NOT_LOCKED:
> ...
> 
> The change with switch statement requires one more line for break;. I
> think if statement is suitable for this case.

If you insist on 'if-else' over swtich case, it is fine with me. But I'd suggest
switch-case because it looks cleaner and easier to understand if we have more
than 2 branches.

Thanks
Yuan

Re: [Qemu-devel] [PATCH 1/2] sheepdog: adopting protocol update for VDI locking

[Liu Yuan]

On Fri, Aug 08, 2014 at 03:12:17PM +0900, Hitoshi Mitake wrote:
> At Fri, 8 Aug 2014 13:20:39 +0800,
> Liu Yuan wrote:
> > 
> > On Thu, Aug 07, 2014 at 04:28:39PM +0900, Hitoshi Mitake wrote:
> > > The update is required for supporting iSCSI multipath. It doesn't
> > > affect behavior of QEMU driver but adding a new field to vdi request
> > > struct is required.
> > > 
> > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > Cc: Liu Yuan <namei.unix@gmail.com>
> > > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> > > ---
> > >  block/sheepdog.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > > index 8d9350c..36f76f0 100644
> > > --- a/block/sheepdog.c
> > > +++ b/block/sheepdog.c
> > > @@ -103,6 +103,9 @@
> > >  #define SD_INODE_SIZE (sizeof(SheepdogInode))
> > >  #define CURRENT_VDI_ID 0
> > >  
> > > +#define LOCK_TYPE_NORMAL 1
> > > +#define LOCK_TYPE_SHARED 2      /* for iSCSI multipath */
> > 
> > How about
> > 
> > #define LOCK_TYPE_NORMAL 0
> > #define LOCK_TYPE_SHARED 1
> > 
> > Then we don't need this patch. Since qemu won't make use of multipath for the
> > near future, we should avoid adding stuff related to multipath to qemu driver.
> 
> (Cc-ing current Kazutaka-san's address)
> 
> I think this isn't a good idea. Because it means that sheep has an
> assumption about padding field of the request data struct. This sort
> of workaround can cause hard to find problems in the future.
> 

The point is, how to keep backward compatibilty? E.g, old QEMU with present
sheep daemon with lock features. Then QEMU will send 0 instead of 1 to the sheep
daemon and based on how you handle the invalid value, these might cause problems.

Suppose we have 1 old QEMU and 1 present QEMU who try to start the same image A.
Old QEMU will send invalid 0 to sheep daemon. We shouldn't deny it because it
can run with old sheep and should run on new sheep too. Then this image A isn't
locked due to invalid 0 value. Then present QEMU send correct LOCK signal and
will wrongly set up the image.

Start with 0 instead of 1, in my option, is reasonable to keep backward
compatibility.

Thanks
Yuan

Re: [Qemu-devel] [PATCH 1/2] sheepdog: adopting protocol update for VDI locking

[Hitoshi Mitake]

At Fri, 8 Aug 2014 15:49:37 +0800,
Liu Yuan wrote:
> 
> On Fri, Aug 08, 2014 at 03:12:17PM +0900, Hitoshi Mitake wrote:
> > At Fri, 8 Aug 2014 13:20:39 +0800,
> > Liu Yuan wrote:
> > > 
> > > On Thu, Aug 07, 2014 at 04:28:39PM +0900, Hitoshi Mitake wrote:
> > > > The update is required for supporting iSCSI multipath. It doesn't
> > > > affect behavior of QEMU driver but adding a new field to vdi request
> > > > struct is required.
> > > > 
> > > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > Cc: Liu Yuan <namei.unix@gmail.com>
> > > > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> > > > ---
> > > >  block/sheepdog.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > > > index 8d9350c..36f76f0 100644
> > > > --- a/block/sheepdog.c
> > > > +++ b/block/sheepdog.c
> > > > @@ -103,6 +103,9 @@
> > > >  #define SD_INODE_SIZE (sizeof(SheepdogInode))
> > > >  #define CURRENT_VDI_ID 0
> > > >  
> > > > +#define LOCK_TYPE_NORMAL 1
> > > > +#define LOCK_TYPE_SHARED 2      /* for iSCSI multipath */
> > > 
> > > How about
> > > 
> > > #define LOCK_TYPE_NORMAL 0
> > > #define LOCK_TYPE_SHARED 1
> > > 
> > > Then we don't need this patch. Since qemu won't make use of multipath for the
> > > near future, we should avoid adding stuff related to multipath to qemu driver.
> > 
> > (Cc-ing current Kazutaka-san's address)
> > 
> > I think this isn't a good idea. Because it means that sheep has an
> > assumption about padding field of the request data struct. This sort
> > of workaround can cause hard to find problems in the future.
> > 
> 
> The point is, how to keep backward compatibilty? E.g, old QEMU with present
> sheep daemon with lock features. Then QEMU will send 0 instead of 1 to the sheep
> daemon and based on how you handle the invalid value, these might cause problems.
> 
> Suppose we have 1 old QEMU and 1 present QEMU who try to start the same image A.
> Old QEMU will send invalid 0 to sheep daemon. We shouldn't deny it because it
> can run with old sheep and should run on new sheep too. Then this image A isn't
> locked due to invalid 0 value. Then present QEMU send correct LOCK signal and
> will wrongly set up the image.
> 
> Start with 0 instead of 1, in my option, is reasonable to keep backward
> compatibility.

I don't think so. Because the backward compatibility of the locking
functionality is already broken in the far past.

As Meng repoted in the sheepdog mailing list, old QEMU can issue
locking request to sheep with vid == 0:
http://lists.wpkg.org/pipermail/sheepdog/2014-August/015438.html

Even we set the default lock type as 0, the old QEMU cannot issue a
correct locking request. I'll post a patch for incrementing protocol
version number later. But before doing that, I also want to clean
DISCARD request. Because this request cannot work with snapshot (not
only with the new GC algorithm. The old naive algorithm cannot work
with the DISCARD request).

Thanks,
Hitoshi

Re: [Qemu-devel] [PATCH 2/2] sheepdog: improve error handling for a case of failed lock

[Hitoshi Mitake]

At Fri, 8 Aug 2014 14:43:16 +0800,
Liu Yuan wrote:
> 
> On Fri, Aug 08, 2014 at 03:17:59PM +0900, Hitoshi Mitake wrote:
> > At Fri, 8 Aug 2014 13:31:39 +0800,
> > Liu Yuan wrote:
> > > 
> > > On Thu, Aug 07, 2014 at 04:28:40PM +0900, Hitoshi Mitake wrote:
> > > > Recently, sheepdog revived its VDI locking functionality. This patch
> > > > updates sheepdog driver of QEMU for this feature:
> > > > 
> > > > 1. Improve error message when QEMU fails to acquire lock of
> > > > VDI. Current sheepdog driver prints an error message "VDI isn't
> > > > locked" when it fails to acquire lock. It is a little bit confusing
> > > > because the mesage says VDI isn't locked but it is actually locked by
> > > > other VM. This patch modifies this confusing message.
> > > > 
> > > > 2. Change error code for a case of failed locking. -EBUSY is a
> > > > suitable one.
> > > > 
> > > > Reported-by: Valerio Pachera <sirio81@gmail.com>
> > > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > Cc: Liu Yuan <namei.unix@gmail.com>
> > > > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> > > > ---
> > > >  block/sheepdog.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > > > index 36f76f0..0b3f86d 100644
> > > > --- a/block/sheepdog.c
> > > > +++ b/block/sheepdog.c
> > > > @@ -1112,9 +1112,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
> > > >  
> > > >      if (rsp->result != SD_RES_SUCCESS) {
> > > >          error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s",
> > > > +                   rsp->result == SD_RES_VDI_NOT_LOCKED ?
> > > 
> > > I'm puzzled by this check.
> > > 
> > > we use SD_RES_VDI_LOCKED to indicate vid is already locked, no?
> > 
> > We use SD_RES_VDI_NOT_LOCKED for indicating locking by this VM fails.
> > 
> > > 
> > > > +                   "VDI is already locked by other VM" :
> 
> But this message said it was locked by others, and we have SD_RES_VDI_LOCKED for
> this case.
> 
> We need fix sheep daemon for this case to return SD_RES_VDI_LOCKED for already
> locked case and NOT_LOCKED for other sheep internal errors.

OK, I'll change it in v2.

> 
> > > >                     sd_strerror(rsp->result), filename, snapid, tag);
> > > >          if (rsp->result == SD_RES_NO_VDI) {
> > > >              ret = -ENOENT;
> > > > +        } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) {
> > > > +            ret = -EBUSY;
> > > >          } else {
> > > >              ret = -EIO;
> > > >          }
> > > 
> > > It is better to use switch case to handle the result.
> > 
> > using switch statement in this case only increases a number of lines
> > of code:
> > 
> > Current change:
> >         if (rsp->result == SD_RES_NO_VDI) {
> >             ret = -ENOENT;
> >         } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) {
> > ...
> > 
> > Change with switch:
> >         switch (rsp->result) {
> > 	    case SD_RES_NO_VDI:
> >             ret = -ENOENT;
> > 	    break;
> > 	    case SD_RES_VDI_NOT_LOCKED:
> > ...
> > 
> > The change with switch statement requires one more line for break;. I
> > think if statement is suitable for this case.
> 
> If you insist on 'if-else' over swtich case, it is fine with me. But I'd suggest
> switch-case because it looks cleaner and easier to understand if we have more
> than 2 branches.

Yes I think if-else is suitable for this case. It is easy for anybody
to understand the above simple branch.

Thanks,
Hitoshi

Re: [Qemu-devel] [PATCH 1/2] sheepdog: adopting protocol update for VDI locking

[Liu Yuan]

On Mon, Aug 11, 2014 at 11:17:33AM +0900, Hitoshi Mitake wrote:
> At Fri, 8 Aug 2014 15:49:37 +0800,
> Liu Yuan wrote:
> > 
> > On Fri, Aug 08, 2014 at 03:12:17PM +0900, Hitoshi Mitake wrote:
> > > At Fri, 8 Aug 2014 13:20:39 +0800,
> > > Liu Yuan wrote:
> > > > 
> > > > On Thu, Aug 07, 2014 at 04:28:39PM +0900, Hitoshi Mitake wrote:
> > > > > The update is required for supporting iSCSI multipath. It doesn't
> > > > > affect behavior of QEMU driver but adding a new field to vdi request
> > > > > struct is required.
> > > > > 
> > > > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > Cc: Liu Yuan <namei.unix@gmail.com>
> > > > > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> > > > > ---
> > > > >  block/sheepdog.c | 8 +++++++-
> > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > > > > index 8d9350c..36f76f0 100644
> > > > > --- a/block/sheepdog.c
> > > > > +++ b/block/sheepdog.c
> > > > > @@ -103,6 +103,9 @@
> > > > >  #define SD_INODE_SIZE (sizeof(SheepdogInode))
> > > > >  #define CURRENT_VDI_ID 0
> > > > >  
> > > > > +#define LOCK_TYPE_NORMAL 1
> > > > > +#define LOCK_TYPE_SHARED 2      /* for iSCSI multipath */
> > > > 
> > > > How about
> > > > 
> > > > #define LOCK_TYPE_NORMAL 0
> > > > #define LOCK_TYPE_SHARED 1
> > > > 
> > > > Then we don't need this patch. Since qemu won't make use of multipath for the
> > > > near future, we should avoid adding stuff related to multipath to qemu driver.
> > > 
> > > (Cc-ing current Kazutaka-san's address)
> > > 
> > > I think this isn't a good idea. Because it means that sheep has an
> > > assumption about padding field of the request data struct. This sort
> > > of workaround can cause hard to find problems in the future.
> > > 
> > 
> > The point is, how to keep backward compatibilty? E.g, old QEMU with present
> > sheep daemon with lock features. Then QEMU will send 0 instead of 1 to the sheep
> > daemon and based on how you handle the invalid value, these might cause problems.
> > 
> > Suppose we have 1 old QEMU and 1 present QEMU who try to start the same image A.
> > Old QEMU will send invalid 0 to sheep daemon. We shouldn't deny it because it
> > can run with old sheep and should run on new sheep too. Then this image A isn't
> > locked due to invalid 0 value. Then present QEMU send correct LOCK signal and
> > will wrongly set up the image.
> > 
> > Start with 0 instead of 1, in my option, is reasonable to keep backward
> > compatibility.
> 
> I don't think so. Because the backward compatibility of the locking
> functionality is already broken in the far past.
> 
> As Meng repoted in the sheepdog mailing list, old QEMU can issue
> locking request to sheep with vid == 0:
> http://lists.wpkg.org/pipermail/sheepdog/2014-August/015438.html

I don't really understand why we can't start with 0 and can't keep backward
compatibility. By the way, I think the link has nothing to do with qemu, it is
a bug in sheep.

locking has two state, one is lock and the other unlock.

We choose 0 to mean 'lock' the vdi and 1 to 'unlock' the vdi.

So both old and new QEMU issue 0 to lock the image and 'release' request to
unlock the image. What is in the way?

> 
> Even we set the default lock type as 0, the old QEMU cannot issue a
> correct locking request. 

why?

> I'll post a patch for incrementing protocol
> version number later. But before doing that, I also want to clean
> DISCARD request. Because this request cannot work with snapshot (not
> only with the new GC algorithm. The old naive algorithm cannot work
> with the DISCARD request).

If you remove discard, what if users run new qemu with old sheep, which make
use of old algorithm and people expect discard to work?

I don't think remove operation from protocol is good idea. If sheep daemon can't
support discard, you can simply ask the sheep daemon to return success without
doing anything if it doesn't support discard.

Thanks
Yuan

Re: [Qemu-devel] [PATCH 1/2] sheepdog: adopting protocol update for VDI locking

[Liu Yuan]

On Mon, Aug 11, 2014 at 11:34:56AM +0800, Liu Yuan wrote:
> On Mon, Aug 11, 2014 at 11:17:33AM +0900, Hitoshi Mitake wrote:
> > At Fri, 8 Aug 2014 15:49:37 +0800,
> > Liu Yuan wrote:
> > > 
> > > On Fri, Aug 08, 2014 at 03:12:17PM +0900, Hitoshi Mitake wrote:
> > > > At Fri, 8 Aug 2014 13:20:39 +0800,
> > > > Liu Yuan wrote:
> > > > > 
> > > > > On Thu, Aug 07, 2014 at 04:28:39PM +0900, Hitoshi Mitake wrote:
> > > > > > The update is required for supporting iSCSI multipath. It doesn't
> > > > > > affect behavior of QEMU driver but adding a new field to vdi request
> > > > > > struct is required.
> > > > > > 
> > > > > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > > > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > > Cc: Liu Yuan <namei.unix@gmail.com>
> > > > > > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > > > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> > > > > > ---
> > > > > >  block/sheepdog.c | 8 +++++++-
> > > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > > > > > index 8d9350c..36f76f0 100644
> > > > > > --- a/block/sheepdog.c
> > > > > > +++ b/block/sheepdog.c
> > > > > > @@ -103,6 +103,9 @@
> > > > > >  #define SD_INODE_SIZE (sizeof(SheepdogInode))
> > > > > >  #define CURRENT_VDI_ID 0
> > > > > >  
> > > > > > +#define LOCK_TYPE_NORMAL 1
> > > > > > +#define LOCK_TYPE_SHARED 2      /* for iSCSI multipath */
> > > > > 
> > > > > How about
> > > > > 
> > > > > #define LOCK_TYPE_NORMAL 0
> > > > > #define LOCK_TYPE_SHARED 1
> > > > > 
> > > > > Then we don't need this patch. Since qemu won't make use of multipath for the
> > > > > near future, we should avoid adding stuff related to multipath to qemu driver.
> > > > 
> > > > (Cc-ing current Kazutaka-san's address)
> > > > 
> > > > I think this isn't a good idea. Because it means that sheep has an
> > > > assumption about padding field of the request data struct. This sort
> > > > of workaround can cause hard to find problems in the future.
> > > > 
> > > 
> > > The point is, how to keep backward compatibilty? E.g, old QEMU with present
> > > sheep daemon with lock features. Then QEMU will send 0 instead of 1 to the sheep
> > > daemon and based on how you handle the invalid value, these might cause problems.
> > > 
> > > Suppose we have 1 old QEMU and 1 present QEMU who try to start the same image A.
> > > Old QEMU will send invalid 0 to sheep daemon. We shouldn't deny it because it
> > > can run with old sheep and should run on new sheep too. Then this image A isn't
> > > locked due to invalid 0 value. Then present QEMU send correct LOCK signal and
> > > will wrongly set up the image.
> > > 
> > > Start with 0 instead of 1, in my option, is reasonable to keep backward
> > > compatibility.
> > 
> > I don't think so. Because the backward compatibility of the locking
> > functionality is already broken in the far past.
> > 
> > As Meng repoted in the sheepdog mailing list, old QEMU can issue
> > locking request to sheep with vid == 0:
> > http://lists.wpkg.org/pipermail/sheepdog/2014-August/015438.html
> 
> I don't really understand why we can't start with 0 and can't keep backward
> compatibility. By the way, I think the link has nothing to do with qemu, it is
> a bug in sheep.
> 
> locking has two state, one is lock and the other unlock.
> 
> We choose 0 to mean 'lock' the vdi and 1 to 'unlock' the vdi.
> 
> So both old and new QEMU issue 0 to lock the image and 'release' request to
> unlock the image. What is in the way?
> 
> > 
> > Even we set the default lock type as 0, the old QEMU cannot issue a
> > correct locking request. 
> 
> why?
> 
> > I'll post a patch for incrementing protocol
> > version number later. But before doing that, I also want to clean
> > DISCARD request. Because this request cannot work with snapshot (not
> > only with the new GC algorithm. The old naive algorithm cannot work
> > with the DISCARD request).
> 
> If you remove discard, what if users run new qemu with old sheep, which make
> use of old algorithm and people expect discard to work?

Okay, you mean discard can't work with snapshots, but it should work with
non-snapshots, so for the users of non-snapshots, people can expect it to work.
As stated in my last email, you can handle this problem transparently without
modification of protocol.

QEMU --> discard[offset, lenght] --> sheep

sheep:
  if req on snapshot
     return success;
  else
     return sheep_handle_discard().

Thanks
Yuan