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

Josh Durgin jdurgin at redhat.com
Tue Jul 14 20:13:48 UTC 2015


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.

>> Signed-off-by: Wido den Hollander <wido at widodh.nl>
>> ---
>>   src/storage/storage_backend_rbd.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>
>
>> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
>> index 8e8d7a7..936ad18 100644
>> --- a/src/storage/storage_backend_rbd.c
>> +++ b/src/storage/storage_backend_rbd.c
>> @@ -66,6 +66,7 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
>>       const char *client_mount_timeout = "30";
>>       const char *mon_op_timeout = "30";
>>       const char *osd_op_timeout = "30";
>> +    const char *rbd_default_format = "2";
>>
>>       if (authdef) {
>>           VIR_DEBUG("Using cephx authorization, username: %s", authdef->username);
>> @@ -211,6 +212,14 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
>>       VIR_DEBUG("Setting RADOS option rados_osd_op_timeout to %s", osd_op_timeout);
>>       rados_conf_set(ptr->cluster, "rados_osd_op_timeout", osd_op_timeout);
>>
>> +    /*
>> +     * Librbd supports creating RBD format 2 images. We no longer have to invoke
>> +     * rbd_create3(), we can tell librbd to default to format 2.
>> +     * This leaves us to simply use rbd_create() and use the default behavior of librbd
>> +     */
>> +    VIR_DEBUG("Setting RADOS option rbd_default_format to %s", rbd_default_format);
>> +    rados_conf_set(ptr->cluster, "rbd_default_format", rbd_default_format);
>> +
>
> I assume (from above) 0.67.9 is the first time this option is recognized?

It's recognized in the bobtail (since 0.56.7) and cuttlefish (since 
0.61.3) series as well. It was actually in all the dumpling (0.67.x)
releases.

>>       ptr->starttime = time(0);
>>       r = rados_connect(ptr->cluster);
>>       if (r < 0) {
>> @@ -475,16 +484,7 @@ static int 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
>
>
>
> Not quite my area of expertise, but since this was a build time
> check/change wouldn't this then impose a certain minimum version of rbd
> on the libvirt package build/install environment (eg librbd1-devel)?
> That is - if this were applied and installed on some host that didn't
> have at least 0.67.9, then what would happen? Or one with less than 0.56?

The build time check was for the rbd_create3() function. This patch
removes that usage, and relies on the "rbd_default*" ceph options
which have no build time requirement.

At runtime a non-existent option will return ENOENT, which isn't checked 
in this patch. I think that's fine, especially since dumpling (0.67.x) 
is no longer maintained [1].

> Just trying to prevent some less than obvious issue because some build
> environment doesn't have the "latest and greatest" librbd.h installed

I'm glad you're vigilant about these, they're important. In this case 
the patch looks good to me:

Reviewed-by: Josh Durgin <jdurgin at redhat.com>

[1] http://ceph.com/docs/master/releases/




More information about the libvir-list mailing list