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

[FUKUMOTO Yoshifumi]

On 2015/02/02 18:18, 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) )

Thank you for your patch, but it seems to have some issues.
If the snapshot whose vid is 0x0 is deleted after the above sequence,
deleting other snapshots of 00471718 gets to fail.

(The situation can be reproduced with the below sequence:
     $ dog vdi create 00471718 1G
     $ dog vdi snapshot 00471718    (repeat 8 times)
     $ dog vdi delete -s 8 00471718
     $ dog vdi delete -s 2 00471718 )

Thanks,
Yoshifumi Fukumoto

> 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.
> 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--;
>   	}
>   	ret = vdi_found ? SD_RES_NO_TAG : SD_RES_NO_VDI;
>   out:
> @@ -1452,13 +1458,23 @@ static int fill_vdi_info(unsigned long left, unsigned long right,
>   	int ret;
>
>   	if (left < right)
> -		return fill_vdi_info_range(left, right, iocb, info);
> +		return fill_vdi_info_range(left, right - 1, iocb, info);
> +
> +	if (!right)
> +		/*
> +		 * a special case right == 0
> +		 * because the variables left and right have values obtained by
> +		 * get_vdi_bitmap_range(), they mean used bitmap range is
> +		 * [left, right). If right == 0, it means used bitmap range is
> +		 * [left, SD_NR_VDIS].
> +		 */
> +		return fill_vdi_info_range(left, SD_NR_VDIS, iocb, info);
>
> -	ret = fill_vdi_info_range(0, right, iocb, info);
> +	ret = fill_vdi_info_range(0, right - 1, iocb, info);
>   	switch (ret) {
>   	case SD_RES_NO_VDI:
>   	case SD_RES_NO_TAG:
> -		ret = fill_vdi_info_range(left, SD_NR_VDIS - 1, iocb, info);
> +		ret = fill_vdi_info_range(left, SD_NR_VDIS, iocb, info);
>   		break;
>   	default:
>   		break;
>