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

[Liu Yuan <namei.unix@gmail.com>]

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