[sheepdog] [PATCH] sheep: handle VID overflow correctly

[Hitoshi Mitake]

At Tue, 3 Feb 2015 11:55:34 +0800,
Liu Yuan wrote:
> 
> On Mon, Feb 02, 2015 at 06:18:52PM +0900, Hitoshi Mitake wrote:
> > Current sheep cannot handle a case like this:
> > 1. iterate snapshot creation and let latest working VDI have VID 0xffffff
> > 2. create one more snapshot
> > 
> > (The situation can be reproduced with the below sequence:
> >  $ dog vdi create 00471718 1G
> >  $ dog vdi snapshot 00471718   (repeat 7 times) )
> > 
> > In this case, new VID becomes 0x000000. Current fill_vdi_info() and
> > fill_vdi_info_range() cannot handle this case. It comes from the below
> > two reasons:
> > 1. Recent change 00ecfb24ee46f2 introduced a bug which breaks
> >    fill_vdi_info_range() in a case of underflow of its variable i.
> 
> I don't yet look close into this problem, but from this description, should we
> fix the bug somewhere else? because
> 
> 1. fill_vdi_info_range() runs well in the past, so we'd better avoid to modify
>    it to fix the bug introduced by other patch(es).
> 
> 2. You mentioned it was introduced by 00ecfb24ee46f2, so we might better fix
>    00ecfb24ee46f2.
> 
> It would be great if you can elaberate the bug, what it is, why breaks
> fill_vdi_info_range() and if modify fill_vdi_info_range() is the only way to
> fix the problem.

The problem is related to --no-share for snapshot, its design seems to
have a problem. I'll redesign it and refine this patch as a part of
the rework.

Thanks,
Hitoshi

> 
> > 2. fill_vdi_info_range() assumes that its parameters, left and right,
> >    are obtained from get_vdi_bitmap_range(). get_vdi_bitmap_range()
> >    obtains left and right which mean the range of existing VIDs is
> >    [left, right), in other words, [left, right - 1]. So
> >    fill_vdi_info_range() starts checking from right - 1 to left. But
> >    it means fill_vdi_info_range() cannot check VID 0xffffff even VID
> >    overflow happens. So this patch lets fill_vdi_info_range() check
> >    from right to left, and also change callers' manner (it passes left
> >    and right - 1 in ordinal cases).
> > 
> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > ---
> >  sheep/vdi.c | 30 +++++++++++++++++++++++-------
> >  1 file changed, 23 insertions(+), 7 deletions(-)
> > 
> > diff --git a/sheep/vdi.c b/sheep/vdi.c
> > index 2889df6..3d14ccf 100644
> > --- a/sheep/vdi.c
> > +++ b/sheep/vdi.c
> > @@ -1404,9 +1404,11 @@ static int fill_vdi_info_range(uint32_t left, uint32_t right,
> >  		ret = SD_RES_NO_MEM;
> >  		goto out;
> >  	}
> > -	for (i = right - 1; i && i >= left; i--) {
> > +
> > +	i = right;
> > +	while (i >= left) {
> >  		if (!test_bit(i, sys->vdi_inuse))
> > -			continue;
> > +			goto next;
> >  
> >  		ret = sd_read_object(vid_to_vdi_oid(i), (char *)inode,
> >  				     SD_INODE_HEADER_SIZE, 0);
> > @@ -1420,10 +1422,10 @@ static int fill_vdi_info_range(uint32_t left, uint32_t right,
> >  				/* Read, delete, clone on snapshots */
> >  				if (!vdi_is_snapshot(inode)) {
> >  					vdi_found = true;
> > -					continue;
> > +					goto next;
> >  				}
> >  				if (!vdi_tag_match(iocb, inode))
> > -					continue;
> > +					goto next;
> >  			} else {
> >  				/*
> >  				 * Rollback & snap create, read, delete on
> > @@ -1438,6 +1440,10 @@ static int fill_vdi_info_range(uint32_t left, uint32_t right,
> >  			info->vid = inode->vdi_id;
> >  			goto out;
> >  		}
> > +next:
> > +		if (!i)
> > +			break;
> > +		i--;
> 
> I don't have the whole picture yet, but just for this fragment, we change the
> for loop into goto, is not so good for code clarity.
> 
> Thanks
> Yuan
> 
> -- 
> sheepdog mailing list
> sheepdog at lists.wpkg.org
> https://lists.wpkg.org/mailman/listinfo/sheepdog