| Message ID | 1407396520-2720-2-git-send-email-mitake.hitoshi@lab.ntt.co.jp |
|---|---|
| State | New |
| Headers | show |
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 >
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 > >
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
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
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
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
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;
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(-)