[Qemu-devel] [PATCH 0/4] More RBD updates

[Qemu-devel] [PATCH 0/4] More RBD updates

[Sage Weil]

Hi,

Here are a few more improvements to the qemu rbd support.  The first 
patch makes the configuration file handling cleaner (do not error out if 
/etc/ceph/ceph.conf doesn't exist).  One allows characters in the conf 
string to be escaped, so you can (for example) specify an ip\:port (':' 
is used as a delimiter).

The last patch implements flush when rbd_flush() is available.  This lets 
us take advantage of write buffering in newer versions of librbd, which 
improves performance significantly for many workloads (including the 
trivial qemu-img convert).

Thanks!
sage


Sage Weil (4):
  rbd: ignore failures when reading from default conf location
  rbd: allow escaping in config string
  rbd: update comment heading
  rbd: call flush, if available

 block/rbd.c |   83 +++++++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 56 insertions(+), 27 deletions(-)

-- 
1.7.2.5

[Qemu-devel] [PATCH 1/4] rbd: ignore failures when reading from default conf location

[Sage Weil]

If we are reading from the default config location, ignore any failures.
It is perfectly legal for the user to specify exactly the options they need
and to not rely on any config file.

Signed-off-by: Sage Weil <sage@newdream.net>
---
 block/rbd.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 1b78d51..f64b2e0 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -298,11 +298,8 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options)
     }
 
     if (strstr(conf, "conf=") == NULL) {
-        if (rados_conf_read_file(cluster, NULL) < 0) {
-            error_report("error reading config file");
-            rados_shutdown(cluster);
-            return -EIO;
-        }
+        /* try default location, but ignore failure */
+        rados_conf_read_file(cluster, NULL);
     }
 
     if (conf[0] != '\0' &&
@@ -441,11 +438,8 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
     }
 
     if (strstr(conf, "conf=") == NULL) {
-        r = rados_conf_read_file(s->cluster, NULL);
-        if (r < 0) {
-            error_report("error reading config file");
-            goto failed_shutdown;
-        }
+        /* try default location, but ignore failure */
+        rados_conf_read_file(s->cluster, NULL);
     }
 
     if (conf[0] != '\0') {
-- 
1.7.2.5

[Qemu-devel] [PATCH 2/4] rbd: allow escaping in config string

[Sage Weil]

The config string is variously delimited by =, @, and /, depending on the
field.  Allow these characters to be escaped by preceeding them with \.

Signed-off-by: Sage Weil <sage@newdream.net>
---
 block/rbd.c |   29 +++++++++++++++++++++++++++--
 1 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index f64b2e0..43f0e63 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -104,8 +104,15 @@ static int qemu_rbd_next_tok(char *dst, int dst_len,
     *p = NULL;
 
     if (delim != '\0') {
-        end = strchr(src, delim);
-        if (end) {
+        for (end = src; *end; ++end) {
+            if (*end == delim) {
+                break;
+            }
+            if (*end == '\\') {
+                end++;
+            }
+        }
+        if (*end == delim) {
             *p = end + 1;
             *end = '\0';
         }
@@ -124,6 +131,19 @@ static int qemu_rbd_next_tok(char *dst, int dst_len,
     return 0;
 }
 
+static void qemu_rbd_unescape(char *src)
+{
+    char *p;
+
+    for (p = src; *src; ++src, ++p) {
+        if (*src == '\\') {
+            src++;
+        }
+        *p = *src;
+    }
+    *p = '\0';
+}
+
 static int qemu_rbd_parsename(const char *filename,
                               char *pool, int pool_len,
                               char *snap, int snap_len,
@@ -148,6 +168,7 @@ static int qemu_rbd_parsename(const char *filename,
         ret = -EINVAL;
         goto done;
     }
+    qemu_rbd_unescape(pool);
 
     if (strchr(p, '@')) {
         ret = qemu_rbd_next_tok(name, name_len, p, '@', "object name", &p);
@@ -155,9 +176,11 @@ static int qemu_rbd_parsename(const char *filename,
             goto done;
         }
         ret = qemu_rbd_next_tok(snap, snap_len, p, ':', "snap name", &p);
+        qemu_rbd_unescape(snap);
     } else {
         ret = qemu_rbd_next_tok(name, name_len, p, ':', "object name", &p);
     }
+    qemu_rbd_unescape(name);
     if (ret < 0 || !p) {
         goto done;
     }
@@ -213,6 +236,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
         if (ret < 0) {
             break;
         }
+        qemu_rbd_unescape(name);
 
         if (!p) {
             error_report("conf option %s has no value", name);
@@ -225,6 +249,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
         if (ret < 0) {
             break;
         }
+        qemu_rbd_unescape(value);
 
         if (strcmp(name, "conf") == 0) {
             ret = rados_conf_read_file(cluster, value);
-- 
1.7.2.5

[Qemu-devel] [PATCH 3/4] rbd: update comment heading

[Sage Weil]

Properly document the configuration string syntax and semantics.  Remove
(out of date) details about the librbd implementation.

Signed-off-by: Sage Weil <sage@newdream.net>
---
 block/rbd.c |   28 +++++++++++++---------------
 1 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 43f0e63..202e7ec 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -13,35 +13,33 @@
 
 #include "qemu-common.h"
 #include "qemu-error.h"
-
 #include "block_int.h"
 
 #include <rbd/librbd.h>
 
-
-
 /*
  * When specifying the image filename use:
  *
  * rbd:poolname/devicename[@snapshotname][:option1=value1[:option2=value2...]]
  *
- * poolname must be the name of an existing rados pool
+ * poolname must be the name of an existing rados pool.
  *
- * devicename is the basename for all objects used to
- * emulate the raw device.
+ * devicename is the name of the rbd image.
  *
- * Each option given is used to configure rados, and may be
- * any Ceph option, or "conf". The "conf" option specifies
- * a Ceph configuration file to read.
+ * Each option given is used to configure rados, and may be any valid
+ * Ceph option, "id", or "conf".
  *
- * Metadata information (image size, ...) is stored in an
- * object with the name "devicename.rbd".
+ * The "id" option indicates what user we should authenticate as to
+ * the Ceph cluster.  If it is excluded we will use the Ceph default
+ * (normally 'admin').
  *
- * The raw device is split into 4MB sized objects by default.
- * The sequencenumber is encoded in a 12 byte long hex-string,
- * and is attached to the devicename, separated by a dot.
- * e.g. "devicename.1234567890ab"
+ * The "conf" option specifies a Ceph configuration file to read.  If
+ * it is not specified, we will read from the default Ceph locations
+ * (e.g., /etc/ceph/ceph.conf).  To avoid reading _any_ configuration
+ * file, specify conf=/dev/null.
  *
+ * Configuration values containing :, @, or = can be escaped with a
+ * leading "\".
  */
 
 #define OBJ_MAX_SIZE (1UL << OBJ_DEFAULT_OBJ_ORDER)
-- 
1.7.2.5

[Qemu-devel] [PATCH 4/4] rbd: call flush, if available

[Sage Weil]

librbd recently added async writeback and flush support.  If the new
rbd_flush() call is available, call it.

Signed-off-by: Sage Weil <sage@newdream.net>
---
 block/rbd.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 202e7ec..af65924 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -705,6 +705,17 @@ static BlockDriverAIOCB *qemu_rbd_aio_writev(BlockDriverState *bs,
     return rbd_aio_rw_vector(bs, sector_num, qiov, nb_sectors, cb, opaque, 1);
 }
 
+static int qemu_rbd_flush(BlockDriverState *bs)
+{
+#if LIBRBD_VERSION_CODE >= LIBRBD_VERSION(0, 1, 1)
+    /* rbd_flush added in 0.1.1 */
+    BDRVRBDState *s = bs->opaque;
+    return rbd_flush(s->image);
+#else
+    return 0;
+#endif
+}
+
 static int qemu_rbd_getinfo(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BDRVRBDState *s = bs->opaque;
@@ -840,6 +851,7 @@ static BlockDriver bdrv_rbd = {
     .bdrv_file_open     = qemu_rbd_open,
     .bdrv_close         = qemu_rbd_close,
     .bdrv_create        = qemu_rbd_create,
+    .bdrv_flush         = qemu_rbd_flush,
     .bdrv_get_info      = qemu_rbd_getinfo,
     .create_options     = qemu_rbd_create_options,
     .bdrv_getlength     = qemu_rbd_getlength,
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 2/4] rbd: allow escaping in config string

[Kevin Wolf]

Am 15.09.2011 23:11, schrieb Sage Weil:
> The config string is variously delimited by =, @, and /, depending on the
> field.  Allow these characters to be escaped by preceeding them with \.
> 
> Signed-off-by: Sage Weil <sage@newdream.net>
> ---
>  block/rbd.c |   29 +++++++++++++++++++++++++++--
>  1 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index f64b2e0..43f0e63 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -104,8 +104,15 @@ static int qemu_rbd_next_tok(char *dst, int dst_len,
>      *p = NULL;
>  
>      if (delim != '\0') {
> -        end = strchr(src, delim);
> -        if (end) {
> +        for (end = src; *end; ++end) {
> +            if (*end == delim) {
> +                break;
> +            }
> +            if (*end == '\\') {
> +                end++;
> +            }
> +        }

If src ends with a backslash, you read beyond the end of the string.

> +        if (*end == delim) {
>              *p = end + 1;
>              *end = '\0';
>          }
> @@ -124,6 +131,19 @@ static int qemu_rbd_next_tok(char *dst, int dst_len,
>      return 0;
>  }
>  
> +static void qemu_rbd_unescape(char *src)
> +{
> +    char *p;
> +
> +    for (p = src; *src; ++src, ++p) {
> +        if (*src == '\\') {
> +            src++;
> +        }
> +        *p = *src;
> +    }
> +    *p = '\0';
> +}

This has the same problem.

Wouldn't it make sense to have the unescape integrated in
qemu_rbd_next_tok? Or are there any places where you would want to call
it without doing a qemu_rbd_unescape() afterwards?

Kevin

Re: [Qemu-devel] [PATCH 0/4] More RBD updates

[Kevin Wolf]

Am 15.09.2011 23:11, schrieb Sage Weil:
> Hi,
> 
> Here are a few more improvements to the qemu rbd support.  The first 
> patch makes the configuration file handling cleaner (do not error out if 
> /etc/ceph/ceph.conf doesn't exist).  One allows characters in the conf 
> string to be escaped, so you can (for example) specify an ip\:port (':' 
> is used as a delimiter).
> 
> The last patch implements flush when rbd_flush() is available.  This lets 
> us take advantage of write buffering in newer versions of librbd, which 
> improves performance significantly for many workloads (including the 
> trivial qemu-img convert).
> 
> Thanks!
> sage
> 
> 
> Sage Weil (4):
>   rbd: ignore failures when reading from default conf location
>   rbd: allow escaping in config string
>   rbd: update comment heading
>   rbd: call flush, if available
> 
>  block/rbd.c |   83 +++++++++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 56 insertions(+), 27 deletions(-)
> 

Thanks, applied patches 1/3/4, commented on patch 2.

Kevin

Re: [Qemu-devel] [PATCH 2/4] rbd: allow escaping in config string

[Sage Weil]

On Mon, 19 Sep 2011, Kevin Wolf wrote:
> If src ends with a backslash, you read beyond the end of the string.

Right, sending an updated patch.

> Wouldn't it make sense to have the unescape integrated in
> qemu_rbd_next_tok? Or are there any places where you would want to call
> it without doing a qemu_rbd_unescape() afterwards?

The conf string makes two passes through rbd_next_tok(), once to grab 
the whole conf string, and again to pull out each item.  We can't 
strip out escaping the first time through or else e.g. \: will turn into 
the : delimiter.

I thought about adding a flag to enable/disable the escaping, but 
explicitly doing the unescape seemed cleaner.

sage
>