3:15 a.m.
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.
Signed-off-by: Wido den Hollander <wido(a)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);
+
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
return -1;
}
--
1.9.1
2:42 p.m.
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.
Signed-off-by: Wido den Hollander <wido(a)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?
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?
Just trying to prevent some less than obvious issue because some build
environment doesn't have the "latest and greatest" librbd.h installed
John
return -1;
}
3:13 p.m.
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(a)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(a)redhat.com>
[1] http://ceph.com/docs/master/releases/
11:28 a.m.
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.
...
>> 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);
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
11:41 a.m.
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
3447
days inactive
3449
days old
5 comments
3 participants
participants (3)
-
John Ferlan -
Josh Durgin -
Wido den Hollander