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

[Hitoshi Mitake]

At Fri, 19 Sep 2014 11:17:27 +0800,
Ruoyu wrote:
> 
> 
> 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.

I believe empty tags cause no problems. Because looking up snapshots
with "" tag isn't allowed. Actually, this rule isn't changed for a
long time.

Thanks,
Hitoshi

> >
> > 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");
> >>
> 
>