[sheepdog] [PATCH 6/6] sheep: clear bit in vdi_deleted if vdi is created

[Hitoshi Mitake]

At Tue, 17 Mar 2015 14:51:32 +0900,
Hitoshi Mitake wrote:
> 
> At Tue, 17 Mar 2015 13:40:51 +0800,
> Liu Yuan wrote:
> > 
> > On Tue, Mar 17, 2015 at 01:32:44PM +0800, Liu Yuan wrote:
> > > On Tue, Mar 17, 2015 at 01:16:00PM +0800, Liu Yuan wrote:
> > > > On Tue, Mar 17, 2015 at 02:03:03PM +0900, Hitoshi Mitake wrote:
> > > > > At Tue, 17 Mar 2015 13:58:08 +0900,
> > > > > Hitoshi Mitake wrote:
> > > > > > 
> > > > > > At Tue, 17 Mar 2015 13:51:48 +0900,
> > > > > > Hitoshi Mitake wrote:
> > > > > > > 
> > > > > > > At Tue, 17 Mar 2015 12:42:35 +0800,
> > > > > > > Liu Yuan wrote:
> > > > > > > > 
> > > > > > > > On Mon, Mar 16, 2015 at 08:49:34PM +0800, Liu Yuan wrote:
> > > > > > > > > From: Liu Yuan <liuyuan at cmss.chinamobile.com>
> > > > > > > > > 
> > > > > > > > > This patch fixes following problem:
> > > > > > > > > 
> > > > > > > > > $ dog vdi create test 100M
> > > > > > > > > $ dog vdi delete test
> > > > > > > > > $ dog vdi create test 200M
> > > > > > > > > $ dog vdi list # expect show test, but nothing shows out.
> > > > > > > > > 
> > > > > > > > > Which was brought by
> > > > > > > > > 
> > > > > > > > > *commit f68feab7edc0ded86701a2e902d85616b24942ab
> > > > > > > > > *Author: Saeki Masaki <saeki.masaki at po.ntts.co.jp>
> > > > > > > > > *Date:   Wed Nov 26 10:50:46 2014 +0900
> > > > > > > > > 
> > > > > > > > >     sheep/dog: introduce new bitmap for delete vdi
> > > > > > > > > 
> > > > > > > > > Cc: Saeki Masaki <saeki.masaki at po.ntts.co.jp>
> > > > > > > > > Signed-off-by: Liu Yuan <liuyuan at cmss.chinamobile.com>
> > > > > > > > > ---
> > > > > > > > >  sheep/ops.c | 7 +++++--
> > > > > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/sheep/ops.c b/sheep/ops.c
> > > > > > > > > index 8a0f77c..464ae0b 100644
> > > > > > > > > --- a/sheep/ops.c
> > > > > > > > > +++ b/sheep/ops.c
> > > > > > > > > @@ -130,11 +130,14 @@ static int post_cluster_new_vdi(const struct sd_req *req, struct sd_rsp *rsp,
> > > > > > > > >  	unsigned long nr = rsp->vdi.vdi_id;
> > > > > > > > >  	int ret = rsp->result;
> > > > > > > > >  
> > > > > > > > > -	sd_info("req->vdi.base_vdi_id: %x, rsp->vdi.vdi_id: %x", req->vdi.base_vdi_id, rsp->vdi.vdi_id);
> > > > > > > > > +	sd_info("req->vdi.base_vdi_id: %x, rsp->vdi.vdi_id: %x",
> > > > > > > > > +		req->vdi.base_vdi_id, rsp->vdi.vdi_id);
> > > > > > > > >  
> > > > > > > > >  	sd_debug("done %d %lx", ret, nr);
> > > > > > > > > -	if (ret == SD_RES_SUCCESS)
> > > > > > > > > +	if (ret == SD_RES_SUCCESS) {
> > > > > > > > >  		atomic_set_bit(nr, sys->vdi_inuse);
> > > > > > > > > +		atomic_clear_bit(nr, sys->vdi_deleted);
> > > > > > > > > +	}
> > > > > > > > >  
> > > > > > > > >  	return ret;
> > > > > > > > >  }
> > > > > > > > > -- 
> > > > > > > > > 1.9.1
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > At least this patch fixes a real problem and should be applied, Hitoshi?
> > > > > > > > 
> > > > > > > > Yuan
> > > > > > > 
> > > > > > > Applied this one, thanks.
> > > > > > 
> > > > > > On the second thought, this problem cannot happen on the current
> > > > > > master. It is caused by the old invalid recycling. So I'll revert
> > > > > > it, OK?
> > > > > > # seems that there's no patches after this one, I'll simply rebase and
> > > > > > # update forcibly
> > > > > 
> > > > > Removed this one, sorry for confusion.
> > > > 
> > > > Current master actually have this problem because bit in vdi_deleted never get
> > > > cleared.
> > > 
> > > If you disable vid_gc(), this problem will show up. This patch actually fix a
> > > broader bug. For every newly created vid, either fresh, snapshot, clone, we
> > > should clear it in vdi_deleted.
> > 
> > The master dosen't have this problem because we don't reuse vid. If gc is
> > disabled, we should logically first clear the bit in the deleted because we
> > actually create a new one. This will prevent any future bugs and will allow our
> > in-house patch, which reuse vid as old algorithm work correctly.
> 
> If you really want to use the old recycling algorithm, a good place
> for clearing the vdi_deleted bitmap must be here:
> 
> 
> +		if (vdi_is_deleted(inode)) {
> +			/* Recycle the deleted inode for fresh vdi create */
> +			if (!iocb->create_snapshot)
> +				info->free_bit = i;
> 				/* clear vdi_deleted bit */
> +			continue;
> +		}
> 
> To be honest, using the old algorithm is a bad idea. The data
> corruption might be caused by operation miss and bugs in control plane
> (e.g. OpenStack Cinder). Because the corruption is invoked by wrong
> sequence of VDI creation, which cannot be prevented by sheep.

In addition, you need to know hashed values of VDI names before
creating any VDIs including snapshot and clones for avoiding the
corruption.

Thanks,
Hitoshi