[sheepdog] [PATCH v2] dog: allow vdi snapshot without snapshot tag

[Ruoyu]

On 2014年09月19日 10:46, Hitoshi Mitake wrote:
> At Fri, 19 Sep 2014 10:40:38 +0800,
> Ruoyu wrote:
>>
>> On 2014年09月19日 10:30, Hitoshi Mitake wrote:
>>> At Fri, 19 Sep 2014 10:22:07 +0800,
>>> Ruoyu wrote:
>>>> The problem is still existed.
>>> Really? On my environment, I couldn't reproduce the problem. Can I see
>>> your command sequence?
>> $ dog/dog cluster format
>> __
>> ()'`;
>> /\|`
>> / | Caution! The cluster is not empty.
>> (/_)_|_ Are you sure you want to continue? [yes/no]: yes
>> using backend plain store
>> $ dog/dog vdi create test 4M -P
>> 100.0 % [=====================================================] 4.0 MB /
>> 4.0 MB
>> $ dog/dog vdi snapshot test <-- it is allowed
>> $ dog/dog vdi snapshot test <-- should be forbidden
>> $ dog/dog vdi list <-- both snapshots tagged with ""
>> Name Id Size Used Shared Creation time VDI id Copies Tag
>> s test 1 4.0 MB 4.0 MB 0.0 MB 2014-09-19 10:36 7c2b25 3
>> s test 2 4.0 MB 0.0 MB 4.0 MB 2014-09-19 10:36 7c2b26 3
>> test 0 4.0 MB 0.0 MB 4.0 MB 2014-09-19 10:36 7c2b27 3
> Ah, sorry. The empty snapshot tag "" is a thing which should be
> ignored. So the above behavior is correct. Current master forbids it
> and Valerio reported it as a problem.
>
> v2 patch forbids command sequence like this:
>
> $ dog/dog vdi snapshot test -s asdf
> $ dog/dog vdi snapshot test -s asdf
> Failed to create snapshot for test, maybe snapshot id (0) or tag (asdf) is existed
>
> Actually, v2 can pass 030 test which consists a test case for
> duplicated snapshot tags.
It is OK if empty snapshot tag "" is allowed.
I am afraid whether empty snapshot tag could cause an unexpected problem.
>
> Thanks,
> Hitoshi
>
>>>> How about disabling empty snapshot tag which will make it simple?
>>> It introduces big incompatibility. I want to avoid it.
>>>
>>> Thanks,
>>> Hitoshi
>>>
>>>> On 2014年09月19日 09:37, Hitoshi Mitake wrote:
>>>>> The commit a21bf27906b23448b92cca9943e1019105ffac2f makes
>>>>> $ dog vdi snapshot <vdi>
>>>>> fail if the <vdi> is an ordinal vdi because newly created VDIs have
>>>>> snapid 0. This patch avoids the failure with checking the VDI is
>>>>> snapshot or not.
>>>>>
>>>>> Cc: Ruoyu <liangry at ucweb.com>
>>>>> Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
>>>>> ---
>>>>>   dog/vdi.c | 30 ++++++++++++++++++++++--------
>>>>>   1 file changed, 22 insertions(+), 8 deletions(-)
>>>>>
>>>>> v2: correct vdi existence checking
>>>>>
>>>>> diff --git a/dog/vdi.c b/dog/vdi.c
>>>>> index fa6130e..58b0a71 100644
>>>>> --- a/dog/vdi.c
>>>>> +++ b/dog/vdi.c
>>>>> @@ -567,6 +567,7 @@ static int vdi_snapshot(int argc, char **argv)
>>>>>   	int vs_count = 0;
>>>>>   	struct node_id owners[SD_MAX_COPIES];
>>>>>   	int nr_owners = 0, nr_issued_prevent_inode_update = 0;
>>>>> +	bool fail_if_snapshot = false;
>>>>>   
>>>>>   	if (vdi_cmd_data.snapshot_id != 0) {
>>>>>   		sd_err("Please specify a non-integer value for "
>>>>> @@ -584,16 +585,29 @@ static int vdi_snapshot(int argc, char **argv)
>>>>>   	case SD_RES_NO_TAG:
>>>>>   		break;
>>>>>   	default:
>>>>> -		sd_err("Failed to create snapshot for %s, maybe "
>>>>> -			"snapshot id (%d) or tag (%s) is existed",
>>>>> -			vdiname, vdi_cmd_data.snapshot_id,
>>>>> -			vdi_cmd_data.snapshot_tag);
>>>>> -		return EXIT_FAILURE;
>>>>> +		fail_if_snapshot = true;
>>>>> +		break;
>>>>>   	}
>>>>>   
>>>>> -	ret = read_vdi_obj(vdiname, 0, "", &vid, inode, SD_INODE_HEADER_SIZE);
>>>>> -	if (ret != EXIT_SUCCESS)
>>>>> -		return ret;
>>>>> +	if (fail_if_snapshot) {
>>>>> +		ret = dog_read_object(vid_to_vdi_oid(vid), inode,
>>>>> +				      SD_INODE_HEADER_SIZE, 0, true);
>>>>> +		if (ret != EXIT_SUCCESS)
>>>>> +			return ret;
>>>>> +
>>>>> +		if (vdi_is_snapshot(inode)) {
>>>>> +			sd_err("Failed to create snapshot for %s, maybe "
>>>>> +			       "snapshot id (%d) or tag (%s) is existed",
>>>>> +			       vdiname, vdi_cmd_data.snapshot_id,
>>>>> +			       vdi_cmd_data.snapshot_tag);
>>>>> +			return EXIT_FAILURE;
>>>>> +		}
>>>>> +	} else {
>>>>> +		ret = read_vdi_obj(vdiname, 0, "", &vid, inode,
>>>>> +				   SD_INODE_HEADER_SIZE);
>>>>> +		if (ret != EXIT_SUCCESS)
>>>>> +			return ret;
>>>>> +	}
>>>>>   
>>>>>   	if (inode->store_policy) {
>>>>>   		sd_err("creating a snapshot of hypervolume is not supported");
>>