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

[Liu Yuan]

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.

> 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