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

[Hitoshi Mitake]

At Tue, 17 Mar 2015 14:10:08 +0800,
Liu Yuan wrote:
> 
> On Tue, Mar 17, 2015 at 02:59:12PM +0900, Hitoshi Mitake wrote:
> > At Tue, 17 Mar 2015 13:54:16 +0800,
> > Liu Yuan wrote:
> > > 
> > > On Tue, Mar 17, 2015 at 01:40:51PM +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.
> > > > 
> > > 
> > > The master just work-around this problem and hide it, no? If we set bit in
> > > vdi_inuse but this bit is also set in vdi_deleted too, isn't a bug? Or am I
> > > missing something?
> > 
> > If the situation can arise, it would be a bug. But current master
> > doesn't allow it because every bit of vdi_deleted must be 0 if
> > corresponding vdi_inuse is 0. And if they become 1 once, they will
> > never back to 0.
> 
> Okay, but
>    set_vdi_inuse(vid);
>    clear_vdi_deleted(vid);
> 
> is at least, not do any harm. Actually, maintaining related states into separate
> functions might cause troubles, put these two functions into one will prevent
> future bugs. Current code scatter functions that manipulate two tightly related
> bitmaps in different places on their own, would be a hidden factor to create bugs
> in the future, I'm afraid.

Putting alert messages, assert, etc would be ok for defensive
programming. But it is too dangerous to clear deleted bitmap and
continue becase they are really critical data structures.

Thanks,
Hitoshi