master - python-lvm: Make second lv.snapshot() argument optional

master - python-lvm: Make second lv.snapshot() argument optional

[Andy Grover]

Gitweb:        http://git.fedorahosted.org/git/?p=lvm2.git;a=commitdiff;h=0958905b1bc0a59d34a4092377bae7c59f424181
Commit:        0958905b1bc0a59d34a4092377bae7c59f424181
Parent:        0e3093979e901384aecb22e08d83b5acb2ef12c1
Author:        Andy Grover <agrover@redhat.com>
AuthorDate:    Fri Dec 14 14:10:41 2012 -0800
Committer:     Andy Grover <agrover@redhat.com>
CommitterDate: Fri Dec 14 14:10:41 2012 -0800

python-lvm: Make second lv.snapshot() argument optional

If no size is given, size defaults to 0, which in lvm_lv_snapshot will
allocate extents equal to the original LV be allocated for the new
snapshot.

Signed-off-by: Andy Grover <agrover@redhat.com>
---
 python/liblvm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/liblvm.c b/python/liblvm.c
index 906825e..4db507c 100644
--- a/python/liblvm.c
+++ b/python/liblvm.c
@@ -1317,12 +1317,12 @@ static PyObject *
 liblvm_lvm_lv_snapshot(lvobject *self, PyObject *args)
 {
 	const char *vgname;
-	uint64_t size;
+	uint64_t size = 0;
 	lvobject *lvobj;
 
 	LV_VALID(self);
 
-	if (!PyArg_ParseTuple(args, "sl", &vgname, &size)) {
+	if (!PyArg_ParseTuple(args, "s|l", &vgname, &size)) {
 		return NULL;
 	}
 

master - python-lvm: Make second lv.snapshot() argument optional

[Zdenek Kabelac]

Dne 14.12.2012 23:11, Andy Grover napsal(a):
> Gitweb:        http://git.fedorahosted.org/git/?p=lvm2.git;a=commitdiff;h=0958905b1bc0a59d34a4092377bae7c59f424181
> Commit:        0958905b1bc0a59d34a4092377bae7c59f424181
> Parent:        0e3093979e901384aecb22e08d83b5acb2ef12c1
> Author:        Andy Grover <agrover@redhat.com>
> AuthorDate:    Fri Dec 14 14:10:41 2012 -0800
> Committer:     Andy Grover <agrover@redhat.com>
> CommitterDate: Fri Dec 14 14:10:41 2012 -0800
>
> python-lvm: Make second lv.snapshot() argument optional
>
> If no size is given, size defaults to 0, which in lvm_lv_snapshot will
> allocate extents equal to the original LV be allocated for the new
> snapshot.
>


Possibly not the ideal solution here.

1) we have some clash with command line 'API'
    user creates snapshot of thin volume
       - without giving size he get thin volume snaphost.
         lvcreate -s
       - with size specified he gets 'old' snapshot
         (snapshot outside of the pool)
         lvcreate -s -l|-L

2) using same size of origin is not enough to cover
    i.e. full rewrite of origin device - you need some
    'extra' size to store snapshot's metadata (remapping blocks)

Zdenek

master - python-lvm: Make second lv.snapshot() argument optional

[Andy Grover]

On 12/15/2012 01:34 PM, Zdenek Kabelac wrote:
> Dne 14.12.2012 23:11, Andy Grover napsal(a):
>> If no size is given, size defaults to 0, which in lvm_lv_snapshot will
>> allocate extents equal to the original LV be allocated for the new
>> snapshot.
>>
> 
> 
> Possibly not the ideal solution here.
> 
> 1) we have some clash with command line 'API'
>    user creates snapshot of thin volume
>       - without giving size he get thin volume snaphost.
>         lvcreate -s
>       - with size specified he gets 'old' snapshot
>         (snapshot outside of the pool)
>         lvcreate -s -l|-L

Makes sense. So should we do the same for the Python API -- size = None
-> thin snap, size = >=0 -> 'old' snap with the given size? OR might we
even be OK with just supporting 'new' snaps, and get rid of the size
parameter altogether?

Regards -- Andy

master - python-lvm: Make second lv.snapshot() argument optional

[Zdenek Kabelac]

Dne 16.12.2012 03:27, Andy Grover napsal(a):
> On 12/15/2012 01:34 PM, Zdenek Kabelac wrote:
>> Dne 14.12.2012 23:11, Andy Grover napsal(a):
>>> If no size is given, size defaults to 0, which in lvm_lv_snapshot will
>>> allocate extents equal to the original LV be allocated for the new
>>> snapshot.
>>>
>>
>>
>> Possibly not the ideal solution here.
>>
>> 1) we have some clash with command line 'API'
>>     user creates snapshot of thin volume
>>        - without giving size he get thin volume snaphost.
>>          lvcreate -s
>>        - with size specified he gets 'old' snapshot
>>          (snapshot outside of the pool)
>>          lvcreate -s -l|-L
>
> Makes sense. So should we do the same for the Python API -- size = None
> -> thin snap, size = >=0 -> 'old' snap with the given size? OR might we
> even be OK with just supporting 'new' snaps, and get rid of the size
> parameter altogether?
>

The 'old-snaps' should/will remain supported - since you may want to snap
data outside of the pool - i.e. you want limit maximum size the snapshot 
takes. Or you have very big 'chunk size' of the thin pool - and you may
want to take snapshot of the volume in that pool - so use of snapshot
with significantly smaller chunks size is preferable.

'zero' size snapshot (>=0) - I think this case is actually unsupportable - the 
minimum size for snapshot is I think 2 chunks (1 for data 1 for metadata) - so 
anything smaller should be rejected by metadata validator -  so I think  0 
should be equal to 'None'.

Zdenek

master - python-lvm: Make second lv.snapshot() argument optional

[Tony Asleson]

On 12/15/2012 03:34 PM, Zdenek Kabelac wrote:
 > 2) using same size of origin is not enough to cover
>    i.e. full rewrite of origin device - you need some
>    'extra' size to store snapshot's metadata (remapping blocks)

How does a user calculate the correct size?

I would like to see an option to pass on snapshot creation that can
safely create a snapshot where every block can get written.

Regards,
Tony