[libvirt] [PATCH 2/2] rbd: Implement buildVolFrom using RBD cloning

[John Ferlan]

On 01/22/2016 08:14 AM, Wido den Hollander wrote:
> 
> 
> On 21-01-16 21:55, John Ferlan wrote:
>>
>>
>> On 12/23/2015 04:29 AM, Wido den Hollander wrote:
>>> RBD supports cloning by creating a snapshot, protecting it and create
>>> a child image based on that snapshot afterwards.
>>>
>>> The RBD storage driver will try to find a snapshot with zero deltas between
>>> the current state of the original volume and the snapshot.
>>>
>>> If such a snapshot is found a clone/child image will be created using
>>> the rbd_clone2() function from librbd.
>>>
>>> It will use the same features, strip size and stripe count as the parent image.
>>>
>>> This implementation will only create a single snapshot on the parent image if
>>> this never changes. That should improve performance when removing the parent
>>> image at some point.
>>>
>>> During build the decision will be made to user rbd_diff_iterate() or rbd_diff_iterate2().
>>> The latter is faster, but only available on Ceph versions after 0.94 (Hammer).
>>>
>>> Cloning is only supported if RBD format 2 is used. All images created by libvirt
>>> are already format 2.
>>>
>>> If a RBD format 1 image is used as the original volume libvirt will return VIR_ERR_OPERATION_UNSUPPORTED
>>
>> Long line...  and the API should return 0 or -1 (based on other
>> backends)... Caller doesn't expect that VIR_ERR* on return...
>>
>> I assume you realize that 'virsh vol-create-from' and 'vol-clone' end up
>> using the same API...
>>
>> FWIW: I reviewed bottom up - I may also have been distracted more than
>> once looking for specific code. Hopefully this is coherent ;-)
>>
> 
> 
> Thanks for all the feedback, really appreciate it!
> 
> Working on a revised version of all the patches right now. Rebasing a
> lot, editing commits, etc, etc.
> 
> The coding style changes are new to me though. I run make syntax-check
> and that works. Never knew that this was needed:
> 
> static int
> virStorageRBDXXXXXXX
> 
> All the exisiting code does:
> 
> static int virStorageRBDXXXXXX

Tough to make a syntax-check rule since there'd be a lot of cleanup
before it could be 'enforced'. This has been more of a code review type
effort as new functions are created or function arguments are changed.
Similarly two blank lines between functions.  It's stylistic - the more
code that can follow, the better/easier to read other code.

BTW: Use of 'ret' instead of the 'r' from each rbd_* call (IOW: -errno
values) is definitely something I've seen crop up in a few recent bz's
so it's fresher on my mind.

> 
> Anyway, I'll get back with new patches. Ignore anything which is on the
> list right now.
> 
> Btw, a Pull Request mechanism like Github would be so much easier than
> sending patches per e-mail ;)
> 

And there's a segment of folks that aren't fans of github...

John