GlusterFS support in QEMU - v9

qemu: URI parsing library

From: Paolo Bonzini <[email protected]>

Add a new URI parsing library to QEMU. The code has been borrowed from libxml2 and libvirt.

Da: "Richard W.M. Jones" <[email protected]>
A: "Bharata B Rao" <[email protected]>
Cc: [email protected], "Paolo Bonzini" <[email protected]>, "Kevin Wolf" <[email protected]>, "Avi Kivity" <[email protected]>, "Anthony Liguori" <[email protected]>, "Stefan Hajnoczi" <[email protected]>, "Blue Swirl" <[email protected]>, "Anand Avati" <[email protected]>, "Vijay Bellur" <[email protected]>, "Amar Tumballi" <[email protected]>, "Harsh Bora" <[email protected]>, "Daniel Veillard" <[email protected]>
Inviato: Lunedì, 24 settembre 2012 12:27:38
Oggetto: Re: [PATCH v9 2/4] qemu: URI parsing library

Quoted 4 lines · Bharata B Rao

qemu: URI parsing library

From: Paolo Bonzini <[email protected]>

Quoted 2 lines

Add a new URI parsing library to QEMU. The code has been borrowed from libxml2 and libvirt.

Rather than duplicating the libxml2 code, I think it would be better to depend on libxml2. While it's not ideal to add more external dependencies, libxml2 is very commonly available. Also libxml2 gets frequent security updates and you never know when the URI code here might be found to contain some sort of bug / security issue.

This will need a change to the qemu configure script of course.

Rich.

--
Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/

A better plan would be to incorporate this code into glib, completing the extremely sparse URI support that is already there. However, we would not be able to use it anyway, because we support compiling on old glib versions.

Quoted 4 lines

A better plan would be to incorporate this code into glib, completing the extremely sparse URI support that is already there. However, we would not be able to use it anyway, because we support compiling on old glib versions.

If the same (or very similar) API is retained,

we could fall back on
libxml2 when glib uri parsing is unavailable.

Eventually we bump our
minimum version requirement and drop the libxml2 dependency (or find out that xml has invaded our code base and we can't get rid of it).

Il 27/09/2012 11:03, Avi Kivity ha scritto:

Quoted 7 lines · Paolo Bonzini

Quoted 4 lines

A better plan would be to incorporate this code into glib, completing the extremely sparse URI support that is already there. However, we would not be able to use it anyway, because we support compiling on old glib versions.

If the same (or very similar) API is retained,

Yes, there is exactly one change in the API (modulo renaming) so we could just use some #defines or wrappers.

Quoted 2 lines

we could fall back on
libxml2 when glib uri parsing is unavailable.

That's an interesting idea. The assumption that glib wants URI parsing is not proved, but it may work out.

BTW I'm fine by this assuming:
- someone keeps an eye on libxml2 upstream in case some security problem shows up in that part of the code

- the code used is the 2.9.0 one where I added quite a bit of cleanup on memory handling.

block/gluster.c new file +32 -0

+static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
+{
+    URI *uri;
+    QueryParams *qp = NULL;
+    bool is_unix = false;
+    int ret = 0;
+    char *unescape_str = NULL;
+
+    uri = uri_parse(filename);
+    if (!uri) {
+        return -EINVAL;
+    }
+
+    /* transport */
+    if (!strcmp(uri->scheme, "gluster")) {
+        gconf->transport = g_strdup("tcp");
+    } else if (!strcmp(uri->scheme, "gluster+tcp")) {
+        gconf->transport = g_strdup("tcp");
+    } else if (!strcmp(uri->scheme, "gluster+unix")) {
+        gconf->transport = g_strdup("unix");
+        is_unix = true;
+    } else if (!strcmp(uri->scheme, "gluster+rdma")) {
+        gconf->transport = g_strdup("rdma");
+    } else {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    ret = parse_volume_options(gconf, uri->path);
+    if (ret < 0) {
+        goto out;
+    }

Quoted 1 line

+ qp = query_params_parse(unescape_str);

query_params_parse already does the unescaping.

block: Support GlusterFS as a QEMU block backend.

From: Bharata B Rao <[email protected]>

This patch adds gluster as the new block backend in QEMU. This gives QEMU the ability to boot VM images from gluster volumes. Its already possible to boot from VM images on gluster volumes using FUSE mount, but this patchset provides the ability to boot VM images from gluster volumes by by-passing the FUSE layer in gluster. This is made possible by using libgfapi routines to perform IO on gluster volumes directly.

VM Image on gluster volume is specified like this:

file=gluster[+transport]://[server[:port]]/volname/image[?socket=...]

'gluster' is the protocol.

'transport' specifies the transport type used to connect to gluster management daemon (glusterd). Valid transport types are tcp, unix and rdma. If a transport type isn't specified, then tcp type is assumed.

'server' specifies the server where the volume file specification for the given volume resides. This can be either hostname, ipv4 address or ipv6 address. ipv6 address needs to be within square brackets [ ]. If transport type is 'unix', then server field is ignored, but the 'socket' field needs to be populated with the path to unix domain socket.

'port' is the port number on which glusterd is listening. This is optional and if not specified, QEMU will send 0 which will make gluster to use the default port. port is ignored for unix type of transport.

'volname' is the name of the gluster volume which contains the VM image.

'image' is the path to the actual VM image that resides on gluster volume.

Examples:

file=gluster://1.2.3.4/testvol/a.img file=gluster+tcp://1.2.3.4/testvol/a.img file=gluster+tcp://1.2.3.4:24007/testvol/dir/a.img file=gluster+tcp://[1:2:3:4:5:6:7:8]/testvol/dir/a.img file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir/a.img file=gluster+tcp://server.domain.com:24007/testvol/dir/a.img file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket file=gluster+rdma://1.2.3.4:24007/testvol/a.img

Signed-off-by Bharata B Rao <[email protected]>

---

 block/Makefile.objs |    1 
 block/gluster.c     |  642 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 643 insertions(+), 0 deletions(-)
 create mode 100644 block/gluster.c

block/Makefile.objs +1 -0

index b5754d3..a1ae67f 100644
@@ -9,3 +9,4 @@ block-obj-$(CONFIG_POSIX) += raw-posix.o
 block-obj-$(CONFIG_LIBISCSI) += iscsi.o
 block-obj-$(CONFIG_CURL) += curl.o
 block-obj-$(CONFIG_RBD) += rbd.o
+block-obj-$(CONFIG_GLUSTERFS) += gluster.o

block/gluster.c new file +62 -0

new file mode 100644
index 0000000..a2f8303
@@ -0,0 +1,642 @@
+/*
+ * GlusterFS backend for QEMU
+ *
+ * Copyright (C) 2012 Bharata B Rao <[email protected]>
+ *
+ * Pipe handling mechanism in AIO implementation is derived from
+ * block/rbd.c. Hence,
+ *
+ * Copyright (C) 2010-2011 Christian Brunner <[email protected]>,
+ *                         Josh Durgin <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+#include <glusterfs/api/glfs.h>
+#include "block_int.h"
+#include "qemu_socket.h"
+#include "uri.h"
+
+typedef struct GlusterAIOCB {
+    BlockDriverAIOCB common;
+    int64_t size;
+    int ret;
+    bool *finished;
+    QEMUBH *bh;
+} GlusterAIOCB;
+
+typedef struct BDRVGlusterState {
+    struct glfs *glfs;
+    int fds[2];
+    struct glfs_fd *fd;
+    int qemu_aio_count;
+    int event_reader_pos;
+    GlusterAIOCB *event_acb;
+} BDRVGlusterState;
+
+#define GLUSTER_FD_READ  0
+#define GLUSTER_FD_WRITE 1
+
+typedef struct GlusterConf {
+    char *server;
+    int port;
+    char *volname;
+    char *image;
+    char *transport;
+} GlusterConf;
+
+static void qemu_gluster_gconf_free(GlusterConf *gconf)
+{
+    g_free(gconf->server);
+    g_free(gconf->volname);
+    g_free(gconf->image);
+    g_free(gconf->transport);
+    g_free(gconf);
+}
+
+static int parse_volume_options(GlusterConf *gconf, char *path)
+{
+    char *token, *saveptr;

block/gluster.c new file +73 -0

+    if (!token) {
+        return -EINVAL;
+    }
+    gconf->image = g_strdup(token);
+    return 0;
+}
+
+/*
+ * file=gluster[+transport]://[server[:port]]/volname/image[?socket=...]
+ *
+ * 'gluster' is the protocol.
+ *
+ * 'transport' specifies the transport type used to connect to gluster
+ * management daemon (glusterd). Valid transport types are
+ * tcp, unix and rdma. If a transport type isn't specified, then tcp
+ * type is assumed.
+ *
+ * 'server' specifies the server where the volume file specification for
+ * the given volume resides. This can be either hostname, ipv4 address
+ * or ipv6 address. ipv6 address needs to be within square brackets [ ].
+ * If transport type is 'unix', then server field is ignored, but the
+ * 'socket' field needs to be populated with the path to unix domain
+ * socket.
+ *
+ * 'port' is the port number on which glusterd is listening. This is optional
+ * and if not specified, QEMU will send 0 which will make gluster to use the
+ * default port. port is ignored for unix type of transport.
+ *
+ * 'volname' is the name of the gluster volume which contains the VM image.
+ *
+ * 'image' is the path to the actual VM image that resides on gluster volume.
+ *
+ * Examples:
+ *
+ * file=gluster://1.2.3.4/testvol/a.img
+ * file=gluster+tcp://1.2.3.4/testvol/a.img
+ * file=gluster+tcp://1.2.3.4:24007/testvol/dir/a.img
+ * file=gluster+tcp://[1:2:3:4:5:6:7:8]/testvol/dir/a.img
+ * file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir/a.img
+ * file=gluster+tcp://server.domain.com:24007/testvol/dir/a.img
+ * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
+ * file=gluster+rdma://1.2.3.4:24007/testvol/a.img
+ */
+static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
+{
+    URI *uri;
+    QueryParams *qp = NULL;
+    bool is_unix = false;
+    int ret = 0;
+    char *unescape_str = NULL;
+
+    uri = uri_parse(filename);
+    if (!uri) {
+        return -EINVAL;
+    }
+
+    /* transport */
+    if (!strcmp(uri->scheme, "gluster")) {
+        gconf->transport = g_strdup("tcp");
+    } else if (!strcmp(uri->scheme, "gluster+tcp")) {
+        gconf->transport = g_strdup("tcp");
+    } else if (!strcmp(uri->scheme, "gluster+unix")) {
+        gconf->transport = g_strdup("unix");
+        is_unix = true;
+    } else if (!strcmp(uri->scheme, "gluster+rdma")) {
+        gconf->transport = g_strdup("rdma");
+    } else {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    ret = parse_volume_options(gconf, uri->path);
+    if (ret < 0) {

block/gluster.c new file +49 -0

+    } else {
+        gconf->server = g_strdup(uri->server);
+        gconf->port = uri->port;
+    }
+
+out:
+    if (qp) {
+        query_params_free(qp);
+    }
+    g_free(unescape_str);
+    uri_free(uri);
+    return ret;
+}
+
+static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
+{
+    struct glfs *glfs = NULL;
+    int ret;
+
+    ret = qemu_gluster_parseuri(gconf, filename);
+    if (ret < 0) {
+        error_report("Usage: file=gluster[+transport]://[server[:port]]/"
+            "volname/image[?socket=...]");
+        errno = -ret;
+        goto out;
+    }
+
+    glfs = glfs_new(gconf->volname);
+    if (!glfs) {
+        goto out;
+    }
+
+    ret = glfs_set_volfile_server(glfs, gconf->transport, gconf->server,
+            gconf->port);
+    if (ret < 0) {
+        goto out;
+    }
+
+    /*
+     * TODO: Use GF_LOG_ERROR instead of hard code value of 4 here when
+     * GlusterFS makes GF_LOG_* macros available to libgfapi users.
+     */
+    ret = glfs_set_logging(glfs, "-", 4);
+    if (ret < 0) {
+        goto out;
+    }
+
+    ret = glfs_init(glfs);
+    if (ret) {

block/gluster.c new file +60 -0

+    }
+    return NULL;
+}
+
+static void qemu_gluster_complete_aio(GlusterAIOCB *acb, BDRVGlusterState *s)
+{
+    int ret;
+    bool *finished = acb->finished;
+    BlockDriverCompletionFunc *cb = acb->common.cb;
+    void *opaque = acb->common.opaque;
+
+    if (!acb->ret || acb->ret == acb->size) {
+        ret = 0; /* Success */
+    } else if (acb->ret < 0) {
+        ret = acb->ret; /* Read/Write failed */
+    } else {
+        ret = -EIO; /* Partial read/write - fail it */
+    }
+
+    s->qemu_aio_count--;
+    qemu_aio_release(acb);
+    cb(opaque, ret);
+    if (finished) {
+        *finished = true;
+    }
+}
+
+static void qemu_gluster_aio_event_reader(void *opaque)
+{
+    BDRVGlusterState *s = opaque;
+    ssize_t ret;
+
+    do {
+        char *p = (char *)&s->event_acb;
+
+        ret = read(s->fds[GLUSTER_FD_READ], p + s->event_reader_pos,
+                   sizeof(s->event_acb) - s->event_reader_pos);
+        if (ret > 0) {
+            s->event_reader_pos += ret;
+            if (s->event_reader_pos == sizeof(s->event_acb)) {
+                s->event_reader_pos = 0;
+                qemu_gluster_complete_aio(s->event_acb, s);
+            }
+        }
+    } while (ret < 0 && errno == EINTR);
+}
+
+static int qemu_gluster_aio_flush_cb(void *opaque)
+{
+    BDRVGlusterState *s = opaque;
+
+    return (s->qemu_aio_count > 0);
+}
+
+static int qemu_gluster_open(BlockDriverState *bs, const char *filename,
+    int bdrv_flags)
+{
+    BDRVGlusterState *s = bs->opaque;
+    int open_flags = 0;
+    int ret = 0;

block/gluster.c new file +104 -0

+    if (bdrv_flags & BDRV_O_RDWR) {
+        open_flags |= O_RDWR;
+    } else {
+        open_flags |= O_RDONLY;
+    }
+
+    if ((bdrv_flags & BDRV_O_NOCACHE)) {
+        open_flags |= O_DIRECT;
+    }
+
+    s->fd = glfs_open(s->glfs, gconf->image, open_flags);
+    if (!s->fd) {
+        ret = -errno;
+        goto out;
+    }
+
+    ret = qemu_pipe(s->fds);
+    if (ret < 0) {
+        ret = -errno;
+        goto out;
+    }
+    fcntl(s->fds[GLUSTER_FD_READ], F_SETFL, O_NONBLOCK);
+    qemu_aio_set_fd_handler(s->fds[GLUSTER_FD_READ],
+        qemu_gluster_aio_event_reader, NULL, qemu_gluster_aio_flush_cb, s);
+
+out:
+    qemu_gluster_gconf_free(gconf);
+    if (!ret) {
+        return ret;
+    }
+    if (s->fd) {
+        glfs_close(s->fd);
+    }
+    if (s->glfs) {
+        glfs_fini(s->glfs);
+    }
+    return ret;
+}
+
+static int qemu_gluster_create(const char *filename,
+        QEMUOptionParameter *options)
+{
+    struct glfs *glfs;
+    struct glfs_fd *fd;
+    int ret = 0;
+    int64_t total_size = 0;
+    GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
+
+    glfs = qemu_gluster_init(gconf, filename);
+    if (!glfs) {
+        ret = -errno;
+        goto out;
+    }
+
+    while (options && options->name) {
+        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
+            total_size = options->value.n / BDRV_SECTOR_SIZE;
+        }
+        options++;
+    }
+
+    fd = glfs_creat(glfs, gconf->image,
+        O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | S_IWUSR);
+    if (!fd) {
+        ret = -errno;
+    } else {
+        if (glfs_ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
+            ret = -errno;
+        }
+        if (glfs_close(fd) != 0) {
+            ret = -errno;
+        }
+    }
+out:
+    qemu_gluster_gconf_free(gconf);
+    if (glfs) {
+        glfs_fini(glfs);
+    }
+    return ret;
+}
+
+static void qemu_gluster_aio_cancel(BlockDriverAIOCB *blockacb)
+{
+    GlusterAIOCB *acb = (GlusterAIOCB *)blockacb;
+    bool finished = false;
+
+    acb->finished = &finished;
+    while (!finished) {
+        qemu_aio_wait();
+    }
+}
+
+static AIOPool gluster_aio_pool = {
+    .aiocb_size = sizeof(GlusterAIOCB),
+    .cancel = qemu_gluster_aio_cancel,
+};
+
+static int qemu_gluster_send_pipe(BDRVGlusterState *s, GlusterAIOCB *acb)
+{
+    int ret = 0;
+
+    while (1) {
+        int fd = s->fds[GLUSTER_FD_WRITE];
+

Quoted 11 lines

block/gluster.c line 411

Quoted 11 lines

block/gluster.c new file +11 -0

+
+        ret = write(fd, (void *)&acb, sizeof(acb));
+        if (ret >= 0) {
+            break;
+        }
+        if (errno == EINTR) {
+            continue;
+        }
+        if (errno != EAGAIN) {
+            break;
+        }

Variatio delectat? ;-)

How about just do { ... } while (errno == EINTR || errno == EAGAIN); ?

Am 24.09.2012 11:13, schrieb Bharata B Rao:

block/gluster.c line 72

Quoted 13 lines

block/gluster.c new file +13 -0

+static int parse_volume_options(GlusterConf *gconf, char *path)
+{
+    char *token, *saveptr;
+
+    /* volname */
+    token = strtok_r(path, "/", &saveptr);
+    if (!token) {
+        return -EINVAL;
+    }
+    gconf->volname = g_strdup(token);
+
+    /* image */
+    token = strtok_r(NULL, "?", &saveptr);

If I understand uri.c right, there is no ? in the path, so there's no reason to call strtok. You could just use the rest of the string.

Quoted 10 lines

block/gluster.c new file +9 -0

+    if (!token) {
+        return -EINVAL;
+    }
+    gconf->image = g_strdup(token);
+    return 0;
+}
+
+
+    if (uri->query) {

block/gluster.c line 155

Quoted 6 lines

block/gluster.c new file +6 -0

+        unescape_str = uri_string_unescape(uri->query, -1, NULL);
+        if (!unescape_str) {
+            ret = -EINVAL;
+            goto out;
+        }
+    }

I agree with Paolo here, this need to go away.

block/gluster.c line 168

Quoted 7 lines

block/gluster.c new file +7 -0

+
+    if (is_unix) {
+        if (strcmp(qp->p[0].name, "socket")) {
+            ret = -EINVAL;
+            goto out;
+        }
+        gconf->server = g_strdup(qp->p[0].value);

Maybe add a check that uri->server is empty?

Quoted 54 lines

block/gluster.c new file +53 -0

+    } else {
+        gconf->server = g_strdup(uri->server);
+        gconf->port = uri->port;
+    }
+
+out:
+    if (qp) {
+        query_params_free(qp);
+    }
+    g_free(unescape_str);
+    uri_free(uri);
+    return ret;
+}
+
+static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
+{
+    struct glfs *glfs = NULL;
+    int ret;
+
+    ret = qemu_gluster_parseuri(gconf, filename);
+    if (ret < 0) {
+        error_report("Usage: file=gluster[+transport]://[server[:port]]/"
+            "volname/image[?socket=...]");
+        errno = -ret;
+        goto out;
+    }
+
+    glfs = glfs_new(gconf->volname);
+    if (!glfs) {
+        goto out;
+    }
+
+    ret = glfs_set_volfile_server(glfs, gconf->transport, gconf->server,
+            gconf->port);
+    if (ret < 0) {
+        goto out;
+    }
+
+    /*
+     * TODO: Use GF_LOG_ERROR instead of hard code value of 4 here when
+     * GlusterFS makes GF_LOG_* macros available to libgfapi users.
+     */
+    ret = glfs_set_logging(glfs, "-", 4);
+    if (ret < 0) {
+        goto out;
+    }
+
+    ret = glfs_init(glfs);
+    if (ret) {
+        error_report("Gluster connection failed for server=%s port=%d "
+             "volume=%s image=%s transport=%s\n", gconf->server, gconf->port,
+             gconf->volname, gconf->image, gconf->transport);
+        goto out;

block/gluster.c line 227

Quoted 6 lines

block/gluster.c new file +6 -0

+    }
+    return glfs;
+
+out:
+    if (glfs) {
+        glfs_fini(glfs);

Does this corrupt errno?

Quoted 10 lines

block/gluster.c new file +10 -0

+static int qemu_gluster_open(BlockDriverState *bs, const char *filename,
+    int bdrv_flags)
+{
+    BDRVGlusterState *s = bs->opaque;
+    int open_flags = 0;
+    int ret = 0;
+    GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
+
+    s->glfs = qemu_gluster_init(gconf, filename);
+    if (!s->glfs) {

block/gluster.c line 297

Quoted 6 lines

block/gluster.c new file +6 -0

+        ret = -errno;
+        goto out;
+    }
+
+    open_flags |=  O_BINARY;
+    open_flags &= ~O_ACCMODE;

open_flags == O_BINARY here, so no O_ACCMODE bits to clear.

Quoted 8 lines

block/gluster.c new file +7 -0

+static int qemu_gluster_send_pipe(BDRVGlusterState *s, GlusterAIOCB *acb)
+{
+    int ret = 0;
+
+    while (1) {
+        int fd = s->fds[GLUSTER_FD_WRITE];
+

block/gluster.c line 411

Quoted 10 lines

block/gluster.c new file +10 -0

+        ret = write(fd, (void *)&acb, sizeof(acb));
+        if (ret >= 0) {
+            break;
+        }
+        if (errno == EINTR) {
+            continue;
+        }
+        if (errno != EAGAIN) {
+            break;
+        }

Variatio delectat? ;-)

How about just do { ... } while (errno == EINTR || errno == EAGAIN); ?

Quoted 16 lines

Quoted 16 lines

+static int parse_volume_options(GlusterConf *gconf, char *path)

Quoted 4 lines

Quoted 4 lines

block/gluster.c new file +4 -0

+{
+    char *token, *saveptr;
+
+    /* volname */

Quoted 8 lines

block/gluster.c line 72

Quoted 8 lines

block/gluster.c new file +8 -0

+    token = strtok_r(path, "/", &saveptr);
+    if (!token) {
+        return -EINVAL;
+    }
+    gconf->volname = g_strdup(token);
+
+    /* image */
+    token = strtok_r(NULL, "?", &saveptr);

If I understand uri.c right, there is no ? in the path, so there's no reason to call strtok. You could just use the rest of the string.

As you note, I don't need 2nd strtok strictly since the rest of the string is available in saveptr. But I thought using saveptr is not ideal or preferred. I wanted to use the most appropriate/safe delimiter to extract the image string in the 2nd strtok and decided to use '?'.

If you think using saveptr is fine, then I could use that as below...

    /* image */
    if (!*saveptr) {
        return -EINVAL;
    }
    gconf->image = g_strdup(saveptr);

Il 26/09/2012 18:11, Bharata B Rao ha scritto:

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

+static int parse_volume_options(GlusterConf *gconf, char *path)

Quoted 4 lines

Quoted 4 lines

block/gluster.c new file +4 -0

+{
+    char *token, *saveptr;
+
+    /* volname */

Quoted 8 lines

block/gluster.c line 72

Quoted 8 lines

block/gluster.c new file +8 -0

+    token = strtok_r(path, "/", &saveptr);
+    if (!token) {
+        return -EINVAL;
+    }
+    gconf->volname = g_strdup(token);
+
+    /* image */
+    token = strtok_r(NULL, "?", &saveptr);

If I understand uri.c right, there is no ? in the path, so there's no reason to call strtok. You could just use the rest of the string.

Actually there could be a %3F which uri.c would unescape to ? (only the query part is left escaped), so your usage of strtok_r is incorrect.

Quoted 5 lines

As you note, I don't need 2nd strtok strictly since the rest of the string is available in saveptr. But I thought using saveptr is not ideal or preferred. I wanted to use the most appropriate/safe delimiter to extract the image string in the 2nd strtok and decided to use '?'.

I don't think it is defined what saveptr points to.
http://publib.boulder.ibm.com/infocenter/pseries/v5r3/index.jsp?topic=/com.ibm.aix.basetechref/doc/basetrf2/strtok_r.htm
says "the strtok_r subroutine also updates the Pointer parameter with the starting address of the token following the first occurrence of the Separators parameter". I read this as:

    *saveptr = token + strlen(token) + 1;

which is consistent with this strtok example from the C standard:

    #include <string.h>
    static char str[] = "?a???b,";
    char *t;

    t = strtok(str, "?");  // t points to the token "a"
    t = strtok(str, ",");  // t points to the token "??b"

Have you tested this code with multiple consecutive slashes?

Quoted 9 lines

If you think using saveptr is fine, then I could use that as below...

    /* image */
    if (!*saveptr) {
        return -EINVAL;
    }
    gconf->image = g_strdup(saveptr);

I would avoid strtok_r completely:

    char *p = path + strcpsn(path, "/");
    if (*p == '\0') {
        return -EINVAL;
    }
    gconf->volname = g_strndup(path, p - path);

    p += strspn(p, "/");
    if (*p == '\0') {
        return -EINVAL;
    }
    gconf->image = g_strdup(p);

Quoted 15 lines

Quoted 4 lines

As you note, I don't need 2nd strtok strictly since the rest of the string is available in saveptr. But I thought using saveptr is not ideal or preferred. I wanted to use the most appropriate/safe delimiter to extract the image string in the 2nd strtok and decided to use '?'.

I don't think it is defined what saveptr points to.
http://publib.boulder.ibm.com/infocenter/pseries/v5r3/index.jsp?topic=/com.ibm.aix.basetechref/doc/basetrf2/strtok_r.htm
says "the strtok_r subroutine also updates the Pointer parameter with the starting address of the token following the first occurrence of the Separators parameter". I read this as:

    *saveptr = token + strlen(token) + 1;

which is consistent with this strtok example from the C standard:

Quoted 8 lines

    #include <string.h>
    static char str[] = "?a???b,";
    char *t;

    t = strtok(str, "?");  // t points to the token "a"
    t = strtok(str, ",");  // t points to the token "??b"

Have you tested this code with multiple consecutive slashes?

Yes, both 2-strtok and strtok+saveptr approach work correctly (= as per my expectation!) with multiple consecutive slashes. I do understand that 2-strtok approach will not work when we have %3F in the path component of the URI.

For URIs like gluster://server/volname/path/to/image, both the approaches extract image as "path/to/image".

For URIs like gluster://server/volname//path/to/image, both the approaches extract image as "/path/to/image".

For gluster://server/volname////path/to/image, the image is extracted as "//path/to/image".

Quoted 4 lines

Quoted 4 lines

    /* image */
    if (!*saveptr) {
        return -EINVAL;
    }

Quoted 5 lines

Quoted 2 lines

    gconf->image = g_strdup(saveptr);

I would avoid strtok_r completely:

Quoted 11 lines

    char *p = path + strcpsn(path, "/");
    if (*p == '\0') {
        return -EINVAL;
    }
    gconf->volname = g_strndup(path, p - path);

    p += strspn(p, "/");
    if (*p == '\0') {
        return -EINVAL;
    }
    gconf->image = g_strdup(p);

This isn't working because for a URI like gluster://server/volname/path/to/image, uri_parse() will give "/volname/path/to/image" in uri->path. I would have expected to see uri->path as "volname/path/to/image" (without 1st slash).

Note that gluster is currently able to resolve image paths that don't have a preceding slash (like dir/a.img). But I guess we should support specifying complete image paths too (like /dir/a.img)

Quoted 3 lines

For gluster://server/volname////path/to/image, the image is extracted as "//path/to/image".

Should there be three /'s here? I assume it's just a typo.

I'm concerned that there is no documentation of what saveptr actually points to. In many cases the strtok specification doesn't leave much free room, but in the case you're testing here:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

    /* image */
    if (!*saveptr) {
        return -EINVAL;
    }

strtok_r may just as well leave saveptr = NULL for example.

Quoted 18 lines

Quoted 6 lines

Quoted 3 lines

    gconf->image = g_strdup(saveptr);

I would avoid strtok_r completely:

Quoted 11 lines

    char *p = path + strcpsn(path, "/");
    if (*p == '\0') {
        return -EINVAL;
    }
    gconf->volname = g_strndup(path, p - path);

    p += strspn(p, "/");
    if (*p == '\0') {
        return -EINVAL;
    }
    gconf->image = g_strdup(p);

Quoted 4 lines

This isn't working because for a URI like gluster://server/volname/path/to/image, uri_parse() will give "/volname/path/to/image" in uri->path. I would have expected to see uri->path as "volname/path/to/image" (without 1st slash).

Ok, that's easy enough to fix with an extra strspn,

    char *p = path + strpsn(path, "/");
    p += strcspn(p, "/");

Quoted 5 lines

Note that gluster is currently able to resolve image paths that don't have a preceding slash (like dir/a.img). But I guess we should support specifying complete image paths too (like /dir/a.img)

How would the URIs look like?

Quoted 2 lines

Quoted 2 lines

How would the URIs look like?

gluster://server/testvol//dir/a.img ?
Isn't this a valid URI ? Here volname=testvol and image=/dir/a.img.

If that's valid and needs to be supported, then the above strspn based parsing logic needs some rewrite.

Il 24/09/2012 11:13, Bharata B Rao ha scritto:

block/gluster.c line 307

Quoted 5 lines

block/gluster.c new file +5 -0

+
+    if ((bdrv_flags&  BDRV_O_NOCACHE)) {
+        open_flags |= O_DIRECT;
+    }
+

If I understand correctly what I was told, this prevents the brick server from using its own buffer cache. This is quite different from what we do for example over NFS (where the host does no caching, but nothing prevents it on the remote server).

I think these 3 lines should be removed. We're bypassing the host buffer cache just by virtue of using a userspace driver, and that's what cache=none cares about.

Paolo

Quoted 4 lines

I think these 3 lines should be removed. We're bypassing the host buffer cache just by virtue of using a userspace driver, and that's what cache=none cares about.

O_DIRECT also has an effect on the behavior of the "client side" (the part within the qemu) of Gluster stack as well. I presume the intention of O_DIRECT is to minimize use of memory (whether as host' page cache or buffered data in user space). To that end it is a good idea to leave O_DIRECT flag set.

The behavior of whether gluster bricks need to get the O_DIRECT propagated or not is a different issue. We are exploring the possibility of not sending O_DIRECT flag over the wire to mimic NFS behavior. That would be independent of the qemu block driver setting the open flag.

Il 03/10/2012 19:58, Anand Avati ha scritto:

Quoted 11 lines

Quoted 4 lines

I think these 3 lines should be removed. We're bypassing the host buffer cache just by virtue of using a userspace driver, and that's what cache=none cares about.

O_DIRECT also has an effect on the behavior of the "client side" (the part within the qemu) of Gluster stack as well. I presume the intention of O_DIRECT is to minimize use of memory (whether as host' page cache or buffered data in user space). To that end it is a good idea to leave O_DIRECT flag set.

Quoted 4 lines

The behavior of whether gluster bricks need to get the O_DIRECT propagated or not is a different issue. We are exploring the possibility of not sending O_DIRECT flag over the wire to mimic NFS behavior. That would be independent of the qemu block driver setting the open flag.

What is the effect of O_DIRECT on the client exactly?

Quoted 18 lines

Il 03/10/2012 19:58, Anand Avati ha scritto:

Quoted 16 lines

Quoted 5 lines

I think these 3 lines should be removed. We're bypassing the host buffer cache just by virtue of using a userspace driver, and that's what
cache=none cares about.

O_DIRECT also has an effect on the behavior of the "client side" (the part within the qemu) of Gluster stack as well. I presume the intention of O_DIRECT is to minimize use of memory (whether as host' page cache or buffered data in user space). To that end it is a good idea to leave O_DIRECT flag set.

The behavior of whether gluster bricks need to get the O_DIRECT propagated or not is a different issue. We are exploring the possibility of not sending O_DIRECT flag over the wire to mimic NFS behavior. That would be independent of the qemu block driver setting the open flag.

Quoted 1 line

What is the effect of O_DIRECT on the client exactly?

To avoid caching in the io-cache module, disable read-ahead etc (if those translators are loaded). The behavior in write-behind is tunable. You could either disable write-behind entirely (which will happen once libgfapi supports 0-copy/RDMA) or perform a sliding-window like size-limited write-behind (defaults to 1MB).