Re: [PATCH] rbd: Use RBD format 2 by default when creating images.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 16-07-15 18:28, John Ferlan wrote:
> 
> 
> On 07/14/2015 04:13 PM, Josh Durgin wrote:
>> On 07/14/2015 12:42 PM, John Ferlan wrote:
>>> 
>>> 
>>> On 07/14/2015 04:15 AM, Wido den Hollander wrote:
>>>> We used to look at the librbd code version and depending on
>>>> that we would invoke rbd_create3() or rbd_create().
>>>> 
>>>> Since librbd version 0.67.9 we can however tell RBD that it
>>>> should create rbd format 2 images even if we invoke
>>>> rbd_create().
>>>> 
>>>> The>> less options we pass to librbd, the more we can lean on
>>>> the sane defaults it uses.
>>>> 
>>>> For rbd_create3() we had things like the stripe count and
>>>> unit hardcoded in libvirt and that might cause problems down
>>>> the road.
>> 
>> Hardcoding the feature bits is even worse. I think this is the
>> right approach.
>> 
> 
> OK - just trying to be sure...  and since no one else spoke up
> yet, seems the approach is fine.
> 

Yes, I think Josh gave the right comments.

> ...
> 
>>>> virStorageBackendRBDCreateImage(rados_ioctx_t io, char *name,
>>>> long capacity) { int order = 0; -#if LIBRBD_VERSION_CODE >
>>>> 260 -    uint64_t features = 3; -    uint64_t stripe_count =
>>>> 1; -    uint64_t stripe_unit = 4194304; - -    if
>>>> (rbd_create3(io, name, capacity, features, &order, -
>>>> stripe_unit, stripe_count) < 0) { -#else if (rbd_create(io,
>>>> name, capacity, &order) < 0) { -#endif
> 
> This change fails make syntax-check due to unnecessary curly
> braces.
> 
> Before I push, the question I see a second issue...
> 
> The current code returns -1 regardless of error, which is then used
> in a call to virReportSystemError(-r,...)...  That means regardless
> of the error you're returning EPERM which could throw someone off
> if something other than PERM was returned from rbd_create
> 
> I can fix that as well by :
> 
> return rbd_create(io, name, capacity, &order);
> 

Seems like a sane approach to me. Less if-statements and the code
still works just fine.

You can apply my patch and fix this afterwards? Or do you want a new
patch from me?

Wido

> 
> FWIW: This reporting snafu was introduced by commit id '761491e'
> which just took the return of virStorageBackendRBDCreateImage and
> used it as the basis for the message generated.
> 
> 
> John
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]