[Qemu-devel] [PATCH v5 RESENT 0/2] sheepdog: add user-defined redundancy option

[Qemu-devel] [PATCH v5 RESENT 0/2] sheepdog: add user-defined redundancy option

[Liu Yuan]

v5:
 - use pstrcpy instead of strncpy
 - fix a segfalt for 'null' string option string

v4:
 - fix do_sd_create that forgot to pass nr_copies
 - fix parse_redundancy dealing with replicated vdi

v3:
 - rework is_numeric

v2:
 - fix a typo in comment and commit log

 This patch set add one sheepdog specific option for qemu-img to control
 redundancy.

 This patch set is on top of Kevin's block tree.

Liu Yuan (2):
  sheepdog: refactor do_sd_create()
  sheepdog: support user-defined redundancy option

 block/sheepdog.c          |  127 +++++++++++++++++++++++++++++++++++++--------
 include/block/block_int.h |    1 +
 2 files changed, 105 insertions(+), 23 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH v5 1/2] sheepdog: refactor do_sd_create()

[Liu Yuan]

We can actually use BDRVSheepdogState *s to pass most of the parameters.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Liu Yuan <namei.unix@gmail.com>
---
 block/sheepdog.c |   37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index ef387de..66b3ea8 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1464,9 +1464,7 @@ out:
     return ret;
 }
 
-static int do_sd_create(BDRVSheepdogState *s, char *filename, int64_t vdi_size,
-                        uint32_t base_vid, uint32_t *vdi_id, int snapshot,
-                        uint8_t copy_policy)
+static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot)
 {
     SheepdogVdiReq hdr;
     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
@@ -1483,11 +1481,11 @@ static int do_sd_create(BDRVSheepdogState *s, char *filename, int64_t vdi_size,
      * does not fit in buf?  For now, just truncate and avoid buffer overrun.
      */
     memset(buf, 0, sizeof(buf));
-    pstrcpy(buf, sizeof(buf), filename);
+    pstrcpy(buf, sizeof(buf), s->name);
 
     memset(&hdr, 0, sizeof(hdr));
     hdr.opcode = SD_OP_NEW_VDI;
-    hdr.vdi_id = base_vid;
+    hdr.vdi_id = s->inode.vdi_id;
 
     wlen = SD_MAX_VDI_LEN;
 
@@ -1495,8 +1493,8 @@ static int do_sd_create(BDRVSheepdogState *s, char *filename, int64_t vdi_size,
     hdr.snapid = snapshot;
 
     hdr.data_length = wlen;
-    hdr.vdi_size = vdi_size;
-    hdr.copy_policy = copy_policy;
+    hdr.vdi_size = s->inode.vdi_size;
+    hdr.copy_policy = s->inode.copy_policy;
 
     ret = do_req(fd, (SheepdogReq *)&hdr, buf, &wlen, &rlen);
 
@@ -1507,7 +1505,7 @@ static int do_sd_create(BDRVSheepdogState *s, char *filename, int64_t vdi_size,
     }
 
     if (rsp->result != SD_RES_SUCCESS) {
-        error_report("%s, %s", sd_strerror(rsp->result), filename);
+        error_report("%s, %s", sd_strerror(rsp->result), s->inode.name);
         return -EIO;
     }
 
@@ -1568,23 +1566,21 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
                      Error **errp)
 {
     int ret = 0;
-    uint32_t vid = 0, base_vid = 0;
-    int64_t vdi_size = 0;
+    uint32_t vid = 0;
     char *backing_file = NULL;
     BDRVSheepdogState *s;
-    char vdi[SD_MAX_VDI_LEN], tag[SD_MAX_VDI_TAG_LEN];
+    char tag[SD_MAX_VDI_TAG_LEN];
     uint32_t snapid;
     bool prealloc = false;
     Error *local_err = NULL;
 
     s = g_malloc0(sizeof(BDRVSheepdogState));
 
-    memset(vdi, 0, sizeof(vdi));
     memset(tag, 0, sizeof(tag));
     if (strstr(filename, "://")) {
-        ret = sd_parse_uri(s, filename, vdi, &snapid, tag);
+        ret = sd_parse_uri(s, filename, s->name, &snapid, tag);
     } else {
-        ret = parse_vdiname(s, filename, vdi, &snapid, tag);
+        ret = parse_vdiname(s, filename, s->name, &snapid, tag);
     }
     if (ret < 0) {
         goto out;
@@ -1592,7 +1588,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
 
     while (options && options->name) {
         if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
-            vdi_size = options->value.n;
+            s->inode.vdi_size = options->value.n;
         } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
             backing_file = options->value.s;
         } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
@@ -1610,7 +1606,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
         options++;
     }
 
-    if (vdi_size > SD_MAX_VDI_SIZE) {
+    if (s->inode.vdi_size > SD_MAX_VDI_SIZE) {
         error_report("too big image size");
         ret = -EINVAL;
         goto out;
@@ -1645,12 +1641,11 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
             goto out;
         }
 
-        base_vid = s->inode.vdi_id;
         bdrv_unref(bs);
     }
 
     /* TODO: allow users to specify copy number */
-    ret = do_sd_create(s, vdi, vdi_size, base_vid, &vid, 0, 0);
+    ret = do_sd_create(s, &vid, 0);
     if (!prealloc || ret) {
         goto out;
     }
@@ -1833,8 +1828,7 @@ static int sd_create_branch(BDRVSheepdogState *s)
      * false bail out.
      */
     deleted = sd_delete(s);
-    ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, &vid,
-                       !deleted, s->inode.copy_policy);
+    ret = do_sd_create(s, &vid, !deleted);
     if (ret) {
         goto out;
     }
@@ -2097,8 +2091,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
         goto cleanup;
     }
 
-    ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid,
-                       1, s->inode.copy_policy);
+    ret = do_sd_create(s, &new_vid, 1);
     if (ret < 0) {
         error_report("failed to create inode for snapshot. %s",
                      strerror(errno));
-- 
1.7.9.5

[Qemu-devel] [PATCH v5 2/2] sheepdog: support user-defined redundancy option

[Liu Yuan]

Sheepdog support two kinds of redundancy, full replication and erasure coding.

# create a fully replicated vdi with x copies
 -o redundancy=x (1 <= x <= SD_MAX_COPIES)

# create a erasure coded vdi with x data strips and y parity strips
 -o redundancy=x:y (x must be one of {2,4,8,16} and 1 <= y < SD_EC_MAX_STRIP)

E.g, to convert a vdi into sheepdog vdi 'test' with 8:3 erasure coding scheme

$ qemu-img convert -o redundancy=8:3 linux-0.2.img sheepdog:test

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Liu Yuan <namei.unix@gmail.com>
---
 block/sheepdog.c          |   90 ++++++++++++++++++++++++++++++++++++++++++++-
 include/block/block_int.h |    1 +
 2 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 66b3ea8..a267d31 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -91,6 +91,14 @@
 #define SD_NR_VDIS   (1U << 24)
 #define SD_DATA_OBJ_SIZE (UINT64_C(1) << 22)
 #define SD_MAX_VDI_SIZE (SD_DATA_OBJ_SIZE * MAX_DATA_OBJS)
+/*
+ * For erasure coding, we use at most SD_EC_MAX_STRIP for data strips and
+ * (SD_EC_MAX_STRIP - 1) for parity strips
+ *
+ * SD_MAX_COPIES is sum of number of data trips and parity strips.
+ */
+#define SD_EC_MAX_STRIP 16
+#define SD_MAX_COPIES (SD_EC_MAX_STRIP * 2 - 1)
 
 #define SD_INODE_SIZE (sizeof(SheepdogInode))
 #define CURRENT_VDI_ID 0
@@ -1495,6 +1503,7 @@ static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot)
     hdr.data_length = wlen;
     hdr.vdi_size = s->inode.vdi_size;
     hdr.copy_policy = s->inode.copy_policy;
+    hdr.copies = s->inode.nr_copies;
 
     ret = do_req(fd, (SheepdogReq *)&hdr, buf, &wlen, &rlen);
 
@@ -1562,6 +1571,76 @@ out:
     return ret;
 }
 
+static bool is_numeric(const char *s)
+{
+    const char *p = s;
+
+    if (*p) {
+        char c;
+
+        while ((c = *p++))
+            if (!isdigit(c)) {
+                return false;
+            }
+        return true;
+    }
+    return false;
+}
+
+/*
+ * Sheepdog support two kinds of redundancy, full replication and erasure
+ * coding.
+ *
+ * # create a fully replicated vdi with x copies
+ * -o redundancy=x (1 <= x <= SD_MAX_COPIES)
+ *
+ * # create a erasure coded vdi with x data strips and y parity strips
+ * -o redundancy=x:y (x must be one of {2,4,8,16} and 1 <= y < SD_EC_MAX_STRIP)
+ */
+static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
+{
+    struct SheepdogInode *inode = &s->inode;
+    const char *n1, *n2;
+    uint8_t copy, parity;
+    char p[10];
+
+    pstrcpy(p, sizeof(p), opt);
+    n1 = strtok(p, ":");
+    n2 = strtok(NULL, ":");
+
+    if (!n1 || !is_numeric(n1) || (n2 && !is_numeric(n2))) {
+        return -EINVAL;
+    }
+
+    copy = strtol(n1, NULL, 10);
+    if (copy > SD_MAX_COPIES) {
+        return -EINVAL;
+    }
+    if (!n2) {
+        inode->copy_policy = 0;
+        inode->nr_copies = copy;
+        return 0;
+    }
+
+    if (copy != 2 && copy != 4 && copy != 8 && copy != 16) {
+        return -EINVAL;
+    }
+
+    parity = strtol(n2, NULL, 10);
+    if (parity >= SD_EC_MAX_STRIP || parity == 0) {
+        return -EINVAL;
+    }
+
+    /*
+     * 4 bits for parity and 4 bits for data.
+     * We have to compress upper data bits because it can't represent 16
+     */
+    inode->copy_policy = ((copy / 2) << 4) + parity;
+    inode->nr_copies = copy + parity;
+
+    return 0;
+}
+
 static int sd_create(const char *filename, QEMUOptionParameter *options,
                      Error **errp)
 {
@@ -1602,6 +1681,11 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
                 ret = -EINVAL;
                 goto out;
             }
+        } else if (!strcmp(options->name, BLOCK_OPT_REDUNDANCY)) {
+            ret = parse_redundancy(s, options->value.s);
+            if (ret < 0) {
+                goto out;
+            }
         }
         options++;
     }
@@ -1644,7 +1728,6 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
         bdrv_unref(bs);
     }
 
-    /* TODO: allow users to specify copy number */
     ret = do_sd_create(s, &vid, 0);
     if (!prealloc || ret) {
         goto out;
@@ -2416,6 +2499,11 @@ static QEMUOptionParameter sd_create_options[] = {
         .type = OPT_STRING,
         .help = "Preallocation mode (allowed values: off, full)"
     },
+    {
+        .name = BLOCK_OPT_REDUNDANCY,
+        .type = OPT_STRING,
+        .help = "Redundancy of the image"
+    },
     { NULL }
 };
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a48731d..b90862f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -53,6 +53,7 @@
 #define BLOCK_OPT_COMPAT_LEVEL      "compat"
 #define BLOCK_OPT_LAZY_REFCOUNTS    "lazy_refcounts"
 #define BLOCK_OPT_ADAPTER_TYPE      "adapter_type"
+#define BLOCK_OPT_REDUNDANCY        "redundancy"
 
 typedef struct BdrvTrackedRequest {
     BlockDriverState *bs;
-- 
1.7.9.5

Re: [Qemu-devel] [sheepdog] [PATCH v5 RESENT 0/2] sheepdog: add user-defined redundancy option

[MORITA Kazutaka]

At Fri,  1 Nov 2013 23:10:11 +0800,
Liu Yuan wrote:
> 
> v5:
>  - use pstrcpy instead of strncpy
>  - fix a segfalt for 'null' string option string
> 
> v4:
>  - fix do_sd_create that forgot to pass nr_copies
>  - fix parse_redundancy dealing with replicated vdi
> 
> v3:
>  - rework is_numeric
> 
> v2:
>  - fix a typo in comment and commit log
> 
>  This patch set add one sheepdog specific option for qemu-img to control
>  redundancy.
> 
>  This patch set is on top of Kevin's block tree.
> 
> Liu Yuan (2):
>   sheepdog: refactor do_sd_create()
>   sheepdog: support user-defined redundancy option
> 
>  block/sheepdog.c          |  127 +++++++++++++++++++++++++++++++++++++--------
>  include/block/block_int.h |    1 +
>  2 files changed, 105 insertions(+), 23 deletions(-)

Reviewed-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

Re: [Qemu-devel] [PATCH v5 2/2] sheepdog: support user-defined redundancy option

[Stefan Hajnoczi]

On Fri, Nov 01, 2013 at 11:10:13PM +0800, Liu Yuan wrote:
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 66b3ea8..a267d31 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -91,6 +91,14 @@
>  #define SD_NR_VDIS   (1U << 24)
>  #define SD_DATA_OBJ_SIZE (UINT64_C(1) << 22)
>  #define SD_MAX_VDI_SIZE (SD_DATA_OBJ_SIZE * MAX_DATA_OBJS)
> +/*
> + * For erasure coding, we use at most SD_EC_MAX_STRIP for data strips and
> + * (SD_EC_MAX_STRIP - 1) for parity strips
> + *
> + * SD_MAX_COPIES is sum of number of data trips and parity strips.

s/data trips/data strips/

> +static bool is_numeric(const char *s)
> +{
> +    const char *p = s;
> +
> +    if (*p) {
> +        char c;
> +
> +        while ((c = *p++))
> +            if (!isdigit(c)) {
> +                return false;
> +            }
> +        return true;
> +    }
> +    return false;
> +}
> +
> +/*
> + * Sheepdog support two kinds of redundancy, full replication and erasure
> + * coding.
> + *
> + * # create a fully replicated vdi with x copies
> + * -o redundancy=x (1 <= x <= SD_MAX_COPIES)
> + *
> + * # create a erasure coded vdi with x data strips and y parity strips
> + * -o redundancy=x:y (x must be one of {2,4,8,16} and 1 <= y < SD_EC_MAX_STRIP)
> + */
> +static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
> +{
> +    struct SheepdogInode *inode = &s->inode;
> +    const char *n1, *n2;
> +    uint8_t copy, parity;
> +    char p[10];
> +
> +    pstrcpy(p, sizeof(p), opt);
> +    n1 = strtok(p, ":");
> +    n2 = strtok(NULL, ":");
> +
> +    if (!n1 || !is_numeric(n1) || (n2 && !is_numeric(n2))) {
> +        return -EINVAL;
> +    }
> +
> +    copy = strtol(n1, NULL, 10);
> +    if (copy > SD_MAX_COPIES) {
> +        return -EINVAL;
> +    }
> +    if (!n2) {
> +        inode->copy_policy = 0;
> +        inode->nr_copies = copy;
> +        return 0;
> +    }
> +
> +    if (copy != 2 && copy != 4 && copy != 8 && copy != 16) {
> +        return -EINVAL;
> +    }
> +
> +    parity = strtol(n2, NULL, 10);
> +    if (parity >= SD_EC_MAX_STRIP || parity == 0) {
> +        return -EINVAL;
> +    }
> +
> +    /*
> +     * 4 bits for parity and 4 bits for data.
> +     * We have to compress upper data bits because it can't represent 16
> +     */
> +    inode->copy_policy = ((copy / 2) << 4) + parity;
> +    inode->nr_copies = copy + parity;
> +
> +    return 0;
> +}

The string manipulation can be simplified using sscanf(3) and
is_numeric() can be dropped:

static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
{
    struct SheepdogInode *inode = &s->inode;
    uint8_t copy, parity;
    int n;

    n = sscanf(opt, "%hhu:%hhu", &copy, &parity);
    if (n != 1 && n != 2) {
        return -EINVAL;
    }
    if (copy > SD_MAX_COPIES) {
        return -EINVAL;
    }
    if (n == 1) {
        inode->copy_policy = 0;
        inode->nr_copies = copy;
        return 0;
    }
    if (copy != 2 && copy != 4 && copy != 8 && copy != 16) {
        return -EINVAL;
    }
    if (parity >= SD_EC_MAX_STRIP || parity == 0) {
        return -EINVAL;
    }
    /*
     * 4 bits for parity and 4 bits for data.
     * We have to compress upper data bits because it can't represent 16
     */
    inode->copy_policy = ((copy / 2) << 4) + parity;
    inode->nr_copies = copy + parity;
    return 0;
}

Re: [Qemu-devel] [PATCH v5 2/2] sheepdog: support user-defined redundancy option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]

On 11/05/2013 07:37 AM, Stefan Hajnoczi wrote:

>> +
>> +    copy = strtol(n1, NULL, 10);
>> +    if (copy > SD_MAX_COPIES) {
>> +        return -EINVAL;
>> +    }

> 
> The string manipulation can be simplified using sscanf(3) and
> is_numeric() can be dropped:
> 
> static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
> {
>     struct SheepdogInode *inode = &s->inode;
>     uint8_t copy, parity;
>     int n;
> 
>     n = sscanf(opt, "%hhu:%hhu", &copy, &parity);

Personally, I detest the use of sscanf() to parse integers out of
strings, because POSIX says that behavior is undefined if overflow
occurs.  For internal strings, you can get away with it.  But for
untrusted input that did not originate in your process, a user can mess
you up by passing a string that parses larger than the integer you are
trying to store into, where the behavior is unspecified whether it wraps
around module 256, parses additional digits, or any other odd behavior.
 By the time you've added code to sanitize untrusted input, it's just as
fast to use strtol() anyways.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v5 2/2] sheepdog: support user-defined redundancy option

[Stefan Hajnoczi]

On Tue, Nov 05, 2013 at 08:46:07AM -0700, Eric Blake wrote:
> On 11/05/2013 07:37 AM, Stefan Hajnoczi wrote:
> 
> >> +
> >> +    copy = strtol(n1, NULL, 10);
> >> +    if (copy > SD_MAX_COPIES) {
> >> +        return -EINVAL;
> >> +    }
> 
> > 
> > The string manipulation can be simplified using sscanf(3) and
> > is_numeric() can be dropped:
> > 
> > static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
> > {
> >     struct SheepdogInode *inode = &s->inode;
> >     uint8_t copy, parity;
> >     int n;
> > 
> >     n = sscanf(opt, "%hhu:%hhu", &copy, &parity);
> 
> Personally, I detest the use of sscanf() to parse integers out of
> strings, because POSIX says that behavior is undefined if overflow
> occurs.  For internal strings, you can get away with it.  But for
> untrusted input that did not originate in your process, a user can mess
> you up by passing a string that parses larger than the integer you are
> trying to store into, where the behavior is unspecified whether it wraps
> around module 256, parses additional digits, or any other odd behavior.
>  By the time you've added code to sanitize untrusted input, it's just as
> fast to use strtol() anyways.

Hmm...I didn't know that overflow was undefined behavior in POSIX :(.

In that case forget sscanf(3) can look at the strtol(3) result for
errors.  There's still no need for a custom is_numeric() function.

Stefan

Re: [Qemu-devel] [PATCH v5 2/2] sheepdog: support user-defined redundancy option

[Liu Yuan]

On Wed, Nov 06, 2013 at 10:34:24AM +0100, Stefan Hajnoczi wrote:
> On Tue, Nov 05, 2013 at 08:46:07AM -0700, Eric Blake wrote:
> > On 11/05/2013 07:37 AM, Stefan Hajnoczi wrote:
> > 
> > >> +
> > >> +    copy = strtol(n1, NULL, 10);
> > >> +    if (copy > SD_MAX_COPIES) {
> > >> +        return -EINVAL;
> > >> +    }
> > 
> > > 
> > > The string manipulation can be simplified using sscanf(3) and
> > > is_numeric() can be dropped:
> > > 
> > > static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
> > > {
> > >     struct SheepdogInode *inode = &s->inode;
> > >     uint8_t copy, parity;
> > >     int n;
> > > 
> > >     n = sscanf(opt, "%hhu:%hhu", &copy, &parity);
> > 
> > Personally, I detest the use of sscanf() to parse integers out of
> > strings, because POSIX says that behavior is undefined if overflow
> > occurs.  For internal strings, you can get away with it.  But for
> > untrusted input that did not originate in your process, a user can mess
> > you up by passing a string that parses larger than the integer you are
> > trying to store into, where the behavior is unspecified whether it wraps
> > around module 256, parses additional digits, or any other odd behavior.
> >  By the time you've added code to sanitize untrusted input, it's just as
> > fast to use strtol() anyways.
> 
> Hmm...I didn't know that overflow was undefined behavior in POSIX :(.
> 
> In that case forget sscanf(3) can look at the strtol(3) result for
> errors.  There's still no need for a custom is_numeric() function.

Thanks for your comments, I'll remove is_numeric() for v6

Thanks
Yuan