GlusterFS support in QEMU - v6

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://server:[port]/volname/image[?transport=socket]

'gluster' is the protocol.

'server' specifies the server where the volume file specification for the given volume resides. This can be either hostname or ipv4 address or ipv6 address. ipv6 address needs to be with in square brackets [ ].

port' is the port number on which gluster management daemon (glusterd) is listening. This is optional and if not specified, QEMU will send 0 which will make libgfapi to use the default port.

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

'image' is the path to the actual VM image in the gluster volume.

'transport' specifies the transport used to connect to glusterd. This is optional and if not specified, socket transport is used.

Examples:

file=gluster://1.2.3.4/testvol/a.img file=gluster://1.2.3.4:5000/testvol/dir/a.img?transport=socket file=gluster://[1:2:3:4:5:6:7:8]/testvol/dir/a.img file=gluster://[1:2:3:4:5:6:7:8]:5000/testvol/dir/a.img?transport=socket file=gluster://server.domain.com:5000/testvol/dir/a.img

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

Reviewed-by Stefan Hajnoczi <[email protected]>

---

 block/Makefile.objs |    1 
 block/gluster.c     |  623 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 624 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 +0 -0

new file mode 100644
index 0000000..bbbaea8

block/gluster.c new file +47 -0

+ *
+ * Copyright (C) 2012 Bharata B Rao <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include <glusterfs/api/glfs.h>
+#include "block_int.h"
+
+typedef struct GlusterAIOCB {
+    BlockDriverAIOCB common;
+    bool canceled;
+    int64_t size;
+    int ret;
+} GlusterAIOCB;
+
+typedef struct BDRVGlusterState {
+    struct glfs *glfs;
+    int fds[2];
+    struct glfs_fd *fd;
+    int qemu_aio_count;
+} BDRVGlusterState;
+
+#define GLUSTER_FD_READ 0
+#define GLUSTER_FD_WRITE 1
+
+typedef struct GlusterURI {
+    char *server;
+    int port;
+    char *volname;
+    char *image;
+    char *transport;
+} GlusterURI;
+
+static void qemu_gluster_uri_free(GlusterURI *uri)
+{
+    g_free(uri->server);
+    g_free(uri->volname);
+    g_free(uri->image);
+    g_free(uri->transport);
+    g_free(uri);
+}
+
+/*
+ * We don't validate the transport option obtained here but
+ * instead depend on gluster to flag an error.

block/gluster.c new file +19 -0

+    int ret = -EINVAL;
+    char *token, *saveptr;
+    char *p, *q = server;
+
+    p = strchr(server, '[');
+    if (p) {
+        /* [ipv6] */
+        if (p != server) {
+            /* [ not in the beginning */
+            goto out;
+        }
+        q++;
+        p = strrchr(p, ']');
+        if (!p) {
+            /* No matching ] */
+            goto out;
+        }
+        *p++ = '\0';
+        uri->server = g_strdup(q);

block/gluster.c new file +5 -0

+        }
+        token = strtok_r(server, ":", &saveptr);
+        if (!token) {
+            goto out;
+        }

block/gluster.c new file +33 -0

+ *
+ * 'gluster' is the protocol.
+ *
+ * 'server' specifies the server where the volume file specification for
+ * the given volume resides. This can be either hostname or ipv4 address
+ * or ipv6 address. ipv6 address needs to be with in square brackets [ ].
+ *
+ * 'port' is the port number on which gluster management daemon (glusterd) is
+ * listening. This is optional and if not specified, QEMU will send 0 which
+ * will make libgfapi to use the default port.
+ *
+ * 'volname' is the name of the gluster volume which contains the VM image.
+ *
+ * 'image' is the path to the actual VM image in the gluster volume.
+ *
+ * 'transport' specifies the transport used to connect to glusterd. This is
+ * optional and if not specified, socket transport is used.
+ *
+ * Examples:
+ *
+ * file=gluster://1.2.3.4/testvol/a.img
+ * file=gluster://1.2.3.4:5000/testvol/dir/a.img?transport=socket
+ * file=gluster://[1:2:3:4:5:6:7:8]/testvol/dir/a.img
+ * file=gluster://[1:2:3:4:5:6:7:8]:5000/testvol/dir/a.img?transport=socket
+ * file=gluster://server.domain.com:5000/testvol/dir/a.img
+ *
+ * We just do minimal checking of the gluster options and mostly ensure
+ * that all the expected elements of the URI are present. We depend on libgfapi
+ * APIs to return appropriate errors in case of invalid arguments.
+ */
+static int qemu_gluster_parseuri(GlusterURI *uri, const char *filename)
+{
+    char *token, *saveptr;

block/gluster.c new file +17 -0

+
+    /* server */
+    token = strtok_r(p, "/", &saveptr);
+    if (!token) {
+        goto out;
+    }
+
+    ret = parse_server(uri, token);
+    if (ret < 0) {
+        goto out;
+    }
+
+    /* volname */
+    token = strtok_r(NULL, "/", &saveptr);
+    if (!token) {
+        ret = -EINVAL;
+        goto out;

block/gluster.c new file +19 -0

+
+    /* transport */
+    token = strtok_r(NULL, "?", &saveptr);
+    ret = parse_transport(uri, token);
+    if (ret < 0) {
+        goto out;
+     }
+
+    /* Flag error for extra options */
+    token = strtok_r(NULL, "?", &saveptr);
+    if (token) {
+        ret = -EINVAL;
+        goto out;
+    }
+    ret = 0;
+out:
+    g_free(r);
+    return ret;
+}

block/gluster.c new file +62 -0

+        errno = -ret;
+        goto out;
+    }
+
+    glfs = glfs_new(uri->volname);
+    if (!glfs) {
+        goto out;
+    }
+
+    ret = glfs_set_volfile_server(glfs, uri->transport, uri->server,
+        uri->port);
+    if (ret < 0) {
+        goto out;
+    }
+
+    /*
+     * TODO: Use GF_LOG_ERROR instead of hard code value of 4 here when
+     * GlusterFS exports it in a header.
+     */
+    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", uri->server, uri->port,
+             uri->volname, uri->image, uri->transport);
+        goto out;
+    }
+    return glfs;
+
+out:
+    if (glfs) {
+        glfs_fini(glfs);
+    }
+    return NULL;
+}
+
+static void qemu_gluster_complete_aio(GlusterAIOCB *acb)
+{
+    int ret;
+
+    if (acb->canceled) {
+        qemu_aio_release(acb);
+        return;
+    }
+
+    if (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 */
+    }
+    acb->common.cb(acb->common.opaque, ret);
+    qemu_aio_release(acb);
+}
+
+static void qemu_gluster_aio_event_reader(void *opaque)
+{

block/gluster.c new file +33 -0

+}
+
+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;
+    GlusterURI *uri = g_malloc0(sizeof(GlusterURI));
+
+    s->glfs = qemu_gluster_init(uri, filename);
+    if (!s->glfs) {
+        ret = -errno;
+        goto out;
+    }
+
+    open_flags |=  O_BINARY;
+    open_flags &= ~O_ACCMODE;
+    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;

block/gluster.c new file +60 -0

+        goto out;
+    }
+    fcntl(s->fds[0], F_SETFL, O_NONBLOCK);
+    fcntl(s->fds[1], 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_uri_free(uri);
+    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;
+    GlusterURI *uri = g_malloc0(sizeof(GlusterURI));
+
+    glfs = qemu_gluster_init(uri, 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, uri->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_uri_free(uri);
+    if (glfs) {
+        glfs_fini(glfs);
+    }

block/gluster.c new file +19 -0

+
+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) {
+        fd_set wfd;
+        int fd = s->fds[GLUSTER_FD_WRITE];
+
+        ret = write(fd, (void *)&acb, sizeof(acb));
+        if (ret >= 0) {
+            break;
+        }
+        if (errno == EINTR) {
+            continue;

block/gluster.c new file +11 -0

+    }
+    return ret;
+}
+
+static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void *arg)
+{
+    GlusterAIOCB *acb = (GlusterAIOCB *)arg;
+    BDRVGlusterState *s = acb->common.bs->opaque;
+
+    acb->ret = ret;
+    if (qemu_gluster_send_pipe(s, acb) < 0) {

block/gluster.c new file +118 -0

+    }
+}
+
+static BlockDriverAIOCB *qemu_gluster_aio_rw(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque, int write)
+{
+    int ret;
+    GlusterAIOCB *acb;
+    BDRVGlusterState *s = bs->opaque;
+    size_t size;
+    off_t offset;
+
+    offset = sector_num * BDRV_SECTOR_SIZE;
+    size = nb_sectors * BDRV_SECTOR_SIZE;
+    s->qemu_aio_count++;
+
+    acb = qemu_aio_get(&gluster_aio_pool, bs, cb, opaque);
+    acb->size = size;
+    acb->ret = 0;
+    acb->canceled = false;
+
+    if (write) {
+        ret = glfs_pwritev_async(s->fd, qiov->iov, qiov->niov, offset, 0,
+            &gluster_finish_aiocb, acb);
+    } else {
+        ret = glfs_preadv_async(s->fd, qiov->iov, qiov->niov, offset, 0,
+            &gluster_finish_aiocb, acb);
+    }
+
+    if (ret < 0) {
+        goto out;
+    }
+    return &acb->common;
+
+out:
+    s->qemu_aio_count--;
+    qemu_aio_release(acb);
+    return NULL;
+}
+
+static BlockDriverAIOCB *qemu_gluster_aio_readv(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    return qemu_gluster_aio_rw(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
+}
+
+static BlockDriverAIOCB *qemu_gluster_aio_writev(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    return qemu_gluster_aio_rw(bs, sector_num, qiov, nb_sectors, cb, opaque, 1);
+}
+
+static BlockDriverAIOCB *qemu_gluster_aio_flush(BlockDriverState *bs,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    int ret;
+    GlusterAIOCB *acb;
+    BDRVGlusterState *s = bs->opaque;
+
+    acb = qemu_aio_get(&gluster_aio_pool, bs, cb, opaque);
+    acb->size = 0;
+    acb->ret = 0;
+    acb->canceled = false;
+    s->qemu_aio_count++;
+
+    ret = glfs_fsync_async(s->fd, &gluster_finish_aiocb, acb);
+    if (ret < 0) {
+        goto out;
+    }
+    return &acb->common;
+
+out:
+    s->qemu_aio_count--;
+    qemu_aio_release(acb);
+    return NULL;
+}
+
+static int64_t qemu_gluster_getlength(BlockDriverState *bs)
+{
+    BDRVGlusterState *s = bs->opaque;
+    struct stat st;
+    int ret;
+
+    ret = glfs_fstat(s->fd, &st);
+    if (ret < 0) {
+        return -errno;
+    } else {
+        return st.st_size;
+    }
+}
+
+static void qemu_gluster_close(BlockDriverState *bs)
+{
+    BDRVGlusterState *s = bs->opaque;
+
+    if (s->fd) {
+        glfs_close(s->fd);
+        s->fd = NULL;
+    }
+    glfs_fini(s->glfs);
+}
+
+static QEMUOptionParameter qemu_gluster_create_options[] = {
+    {
+        .name = BLOCK_OPT_SIZE,
+        .type = OPT_SIZE,
+        .help = "Virtual disk size"
+    },
+    { NULL }
+};
+
+static BlockDriver bdrv_gluster = {
+    .format_name = "gluster",
+    .protocol_name = "gluster",
+    .instance_size = sizeof(BDRVGlusterState),

Quoted 25 lines

block/gluster.c new file +25 -0

+static int parse_server(GlusterURI *uri, char *server)
+{
+    int ret = -EINVAL;
+    char *token, *saveptr;
+    char *p, *q = server;
+
+    p = strchr(server, '[');
+    if (p) {
+        /* [ipv6] */
+        if (p != server) {
+            /* [ not in the beginning */
+            goto out;
+        }
+        q++;
+        p = strrchr(p, ']');
+        if (!p) {
+            /* No matching ] */
+            goto out;
+        }
+        *p++ = '\0';
+        uri->server = g_strdup(q);
+
+        if (*p) {
+            if (*p != ':') {
+                /* [ipv6] followed by something other than : */

block/gluster.c line 111

Quoted 6 lines

block/gluster.c new file +6 -0

+                goto out;
+            }
+            uri->port = strtoul(++p, NULL, 0);
+            if (uri->port < 0) {
+                goto out;
+            }

This accepts inputs where the colon isn't followed by any number.

block/gluster.c line 136

Quoted 8 lines

block/gluster.c new file +8 -0

+        if (token) {
+            uri->port = strtoul(token, NULL, 0);
+            if (uri->port < 0) {
+                goto out;
+            }
+        } else {
+            uri->port = 0;
+        }

The port parsing code is duplicated in IPv4 and IPv6, even though it's really the same.

Quoted 4 lines

block/gluster.c new file +4 -0

+static struct glfs *qemu_gluster_init(GlusterURI *uri, const char *filename)
+{
+    struct glfs *glfs = NULL;
+    int ret;

block/gluster.c line 243

Quoted 5 lines

block/gluster.c new file +5 -0

+
+    ret = qemu_gluster_parseuri(uri, filename);
+    if (ret < 0) {
+        error_report("Usage: file=gluster://server[:port]/volname/image"
+            "[?transport=socket]");

Is 'socket' really the only valid transport and will it stay like this without changes to qemu?

block/gluster.c line 315

Quoted 13 lines

block/gluster.c new file +13 -0

+
+static void qemu_gluster_aio_event_reader(void *opaque)
+{
+    BDRVGlusterState *s = opaque;
+    GlusterAIOCB *event_acb;
+    int event_reader_pos = 0;
+    ssize_t ret;
+
+    do {
+        char *p = (char *)&event_acb;
+
+        ret = read(s->fds[GLUSTER_FD_READ], p + event_reader_pos,
+                   sizeof(event_acb) - event_reader_pos);

So you're reading in a pointer address from a pipe? This is fun.

block/gluster.c line 324

Quoted 9 lines

block/gluster.c new file +9 -0

+        if (ret > 0) {
+            event_reader_pos += ret;
+            if (event_reader_pos == sizeof(event_acb)) {
+                event_reader_pos = 0;
+                qemu_gluster_complete_aio(event_acb);
+                s->qemu_aio_count--;
+            }
+        }
+    } while (ret < 0 && errno == EINTR);

In case of a short read the read data is just discarded? Maybe event_reader_pos was supposed to static?

Quoted 4 lines

block/gluster.c new file +4 -0

+
+static void qemu_gluster_aio_cancel(BlockDriverAIOCB *blockacb)
+{
+    GlusterAIOCB *acb = (GlusterAIOCB *)blockacb;

block/gluster.c line 437

Quoted 4 lines

block/gluster.c new file +4 -0

+
+    acb->common.cb(acb->common.opaque, -ECANCELED);
+    acb->canceled = true;
+}

After having called acb->common.cb you must not write any longer to the memory pointed at by the qiov. Either you can really cancel the request, or you need to wait until it completes.

Quoted 26 lines

block/gluster.c new file +25 -0

+
+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) {
+        fd_set wfd;
+        int fd = s->fds[GLUSTER_FD_WRITE];
+
+        ret = write(fd, (void *)&acb, sizeof(acb));
+        if (ret >= 0) {
+            break;
+        }
+        if (errno == EINTR) {
+            continue;
+        }
+        if (errno != EAGAIN) {
+            break;
+        }
+
+        FD_ZERO(&wfd);

block/gluster.c line 466

Quoted 4 lines

block/gluster.c new file +4 -0

+        FD_SET(fd, &wfd);
+        do {
+            ret = select(fd + 1, NULL, &wfd, NULL, NULL);
+        } while (ret < 0 && errno == EINTR);

What's the idea behind this? While we're hanging in this loop noone will read anything from the pipe, so it's unlikely that it magically becomes ready.

Quoted 18 lines

block/gluster.c new file +17 -0

+    }
+    return ret;
+}
+
+static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void *arg)
+{
+    GlusterAIOCB *acb = (GlusterAIOCB *)arg;
+    BDRVGlusterState *s = acb->common.bs->opaque;
+
+    acb->ret = ret;
+    if (qemu_gluster_send_pipe(s, acb) < 0) {
+        /*
+         * Gluster AIO callback thread failed to notify the waiting
+         * QEMU thread about IO completion. Nothing much can be done
+         * here but to abruptly abort.
+         *
+         * FIXME: Check if the read side of the fd handler can somehow

block/gluster.c line 487

Quoted 4 lines

block/gluster.c new file +4 -0

+         * be notified of this failure paving the way for a graceful exit.
+         */
+        error_report("Gluster failed to notify QEMU about IO completion");
+        abort();

In the extreme case you may choose to make this disk inaccessible (something like bs->drv = NULL), but abort() kills the whole VM and should only be called when there is a bug.

Kevin, Thanks for your review. I will address all of your comments in the next iteration, but have a few questions/comments on the others...

Quoted 25 lines · Kevin Wolf

Quoted 25 lines

block/gluster.c new file +25 -0

+static int parse_server(GlusterURI *uri, char *server)
+{
+    int ret = -EINVAL;
+    char *token, *saveptr;
+    char *p, *q = server;
+
+    p = strchr(server, '[');
+    if (p) {
+        /* [ipv6] */
+        if (p != server) {
+            /* [ not in the beginning */
+            goto out;
+        }
+        q++;
+        p = strrchr(p, ']');
+        if (!p) {
+            /* No matching ] */
+            goto out;
+        }
+        *p++ = '\0';
+        uri->server = g_strdup(q);
+
+        if (*p) {
+            if (*p != ':') {
+                /* [ipv6] followed by something other than : */

Quoted 8 lines

block/gluster.c line 111

Quoted 6 lines

block/gluster.c new file +6 -0

+                goto out;
+            }
+            uri->port = strtoul(++p, NULL, 0);
+            if (uri->port < 0) {
+                goto out;
+            }

This accepts inputs where the colon isn't followed by any number.

Yes, and that will result in port=0, which is default. So this is to cater for cases like gluster://[1:2:3:4:5]:/volname/image

In any case, let me see if I can get rid of this altogether and reuse qemu-sockets.c:inet_parse().

Quoted 9 lines

Quoted 8 lines

block/gluster.c new file +8 -0

+        if (token) {
+            uri->port = strtoul(token, NULL, 0);
+            if (uri->port < 0) {
+                goto out;
+            }
+        } else {
+            uri->port = 0;
+        }

Quoted 2 lines

The port parsing code is duplicated in IPv4 and IPv6, even though it's really the same.

Being such a small piece of code, I didn't think it deserves to be made a function on its own and re-used. But if you really want it that way, I can do.

Quoted 4 lines

Quoted 4 lines

block/gluster.c new file +4 -0

+static struct glfs *qemu_gluster_init(GlusterURI *uri, const char *filename)
+{
+    struct glfs *glfs = NULL;
+    int ret;

Quoted 8 lines

block/gluster.c line 243

Quoted 5 lines

block/gluster.c new file +5 -0

+
+    ret = qemu_gluster_parseuri(uri, filename);
+    if (ret < 0) {
+        error_report("Usage: file=gluster://server[:port]/volname/image"
+            "[?transport=socket]");

Is 'socket' really the only valid transport and will it stay like this without changes to qemu?

There are others like 'unix' and 'rdma'. I will fix this error message to reflect that.

However QEMU needn't change for such transport types because I am not interpreting the transport type in QEMU but instead passing it on directly to GlusterFS.

Quoted 26 lines

block/gluster.c line 315

Quoted 13 lines

block/gluster.c new file +13 -0

+
+static void qemu_gluster_aio_event_reader(void *opaque)
+{
+    BDRVGlusterState *s = opaque;
+    GlusterAIOCB *event_acb;
+    int event_reader_pos = 0;
+    ssize_t ret;
+
+    do {
+        char *p = (char *)&event_acb;
+
+        ret = read(s->fds[GLUSTER_FD_READ], p + event_reader_pos,
+                   sizeof(event_acb) - event_reader_pos);

So you're reading in a pointer address from a pipe? This is fun.

Quoted 9 lines

block/gluster.c new file +9 -0

+        if (ret > 0) {
+            event_reader_pos += ret;
+            if (event_reader_pos == sizeof(event_acb)) {
+                event_reader_pos = 0;
+                qemu_gluster_complete_aio(event_acb);
+                s->qemu_aio_count--;
+            }
+        }
+    } while (ret < 0 && errno == EINTR);

Quoted 2 lines

In case of a short read the read data is just discarded? Maybe event_reader_pos was supposed to static?

In earlier versions event_reader_pos was part of BDRVGlusterState and I made it local in subsequent versions and that is causing this problem. Will fix.

Quoted 4 lines

Quoted 4 lines

block/gluster.c new file +4 -0

+
+static void qemu_gluster_aio_cancel(BlockDriverAIOCB *blockacb)
+{
+    GlusterAIOCB *acb = (GlusterAIOCB *)blockacb;

Quoted 8 lines

block/gluster.c line 437

Quoted 4 lines

block/gluster.c new file +4 -0

+
+    acb->common.cb(acb->common.opaque, -ECANCELED);
+    acb->canceled = true;
+}

After having called acb->common.cb you must not write any longer to the memory pointed at by the qiov. Either you can really cancel the request, or you need to wait until it completes.

I don't think I can cancel the request that has already been sent to gluster server. block/qed.c introduces acb->finished and waits on it in qed_aio_canel. Do you suggest I follow that model ?

Quoted 25 lines

Quoted 25 lines

block/gluster.c new file +25 -0

+
+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) {
+        fd_set wfd;
+        int fd = s->fds[GLUSTER_FD_WRITE];
+
+        ret = write(fd, (void *)&acb, sizeof(acb));
+        if (ret >= 0) {
+            break;
+        }
+        if (errno == EINTR) {
+            continue;
+        }
+        if (errno != EAGAIN) {
+            break;
+        }
+
+        FD_ZERO(&wfd);

Quoted 8 lines

block/gluster.c line 466

Quoted 4 lines

block/gluster.c new file +4 -0

+        FD_SET(fd, &wfd);
+        do {
+            ret = select(fd + 1, NULL, &wfd, NULL, NULL);
+        } while (ret < 0 && errno == EINTR);

What's the idea behind this? While we're hanging in this loop noone will read anything from the pipe, so it's unlikely that it magically becomes ready.

I write to the pipe and wait for the reader to read it. The reader (qemu_gluster_aio_event_reader) is already waiting on the other end of the pipe.

Quoted 18 lines

Quoted 18 lines

block/gluster.c new file +17 -0

+    }
+    return ret;
+}
+
+static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void *arg)
+{
+    GlusterAIOCB *acb = (GlusterAIOCB *)arg;
+    BDRVGlusterState *s = acb->common.bs->opaque;
+
+    acb->ret = ret;
+    if (qemu_gluster_send_pipe(s, acb) < 0) {
+        /*
+         * Gluster AIO callback thread failed to notify the waiting
+         * QEMU thread about IO completion. Nothing much can be done
+         * here but to abruptly abort.
+         *
+         * FIXME: Check if the read side of the fd handler can somehow

Quoted 8 lines

block/gluster.c line 487

Quoted 4 lines

block/gluster.c new file +4 -0

+         * be notified of this failure paving the way for a graceful exit.
+         */
+        error_report("Gluster failed to notify QEMU about IO completion");
+        abort();

In the extreme case you may choose to make this disk inaccessible (something like bs->drv = NULL), but abort() kills the whole VM and should only be called when there is a bug.

There have been concerns raised about this earlier too. I settled for this since I couldn't see a better way out and I could see the precedence for this in posix-aio-compat.c

So I could just do the necessary cleanup, set bs->drv to NULL and return from here ? But how do I wake up the QEMU thread that is waiting on the read side of the pipe ? W/o that, the QEMU thread that waits on the read side of the pipe is still hung.

Quoted 3 lines

Yes, and that will result in port=0, which is default. So this is to cater for cases like gluster://[1:2:3:4:5]:/volname/image

So you consider this a valid URL? I would have expected it to invalid. But let me see, there must be some official definition of an URL...

Alright, so RFC 2234 says that having no digits after the colon is valid. It also says that you shouldn't generate such URLs. And it doesn't say what it means when it's there... Common interpretation seems to be that it's treated as if it wasn't specified, i.e. the default port for the schema is used.

So if 0 is the default port for glusterfs, your code looks okay. But it doesn't seem to be a very useful default port number.

Quoted 6 lines

Quoted 2 lines

Is 'socket' really the only valid transport and will it stay like this without changes to qemu?

There are others like 'unix' and 'rdma'. I will fix this error message to reflect that.

Quoted 3 lines

However QEMU needn't change for such transport types because I am not interpreting the transport type in QEMU but instead passing it on directly to GlusterFS.

Maybe then just specify "[?transport=...]" instead of giving a specific option value?

Quoted 28 lines

Quoted 27 lines

Quoted 13 lines

block/gluster.c new file +13 -0

+static int qemu_gluster_send_pipe(BDRVGlusterState *s, GlusterAIOCB *acb)
+{
+    int ret = 0;
+    while (1) {
+        fd_set wfd;
+        int fd = s->fds[GLUSTER_FD_WRITE];
+
+        ret = write(fd, (void *)&acb, sizeof(acb));
+        if (ret >= 0) {
+            break;
+        }
+        if (errno == EINTR) {
+            continue;

block/gluster.c line 466

Quoted 10 lines

block/gluster.c new file +10 -0

+        }
+        if (errno != EAGAIN) {
+            break;
+        }
+
+        FD_ZERO(&wfd);
+        FD_SET(fd, &wfd);
+        do {
+            ret = select(fd + 1, NULL, &wfd, NULL, NULL);
+        } while (ret < 0 && errno == EINTR);

What's the idea behind this? While we're hanging in this loop noone will read anything from the pipe, so it's unlikely that it magically becomes ready.

Quoted 3 lines

I write to the pipe and wait for the reader to read it. The reader (qemu_gluster_aio_event_reader) is already waiting on the other end of the pipe.

qemu_gluster_aio_even_reader() isn't called while we're looping here. It will only be called from the main loop, after this function has returned.

Quoted 13 lines

Quoted 3 lines

Yes, and that will result in port=0, which is default. So this is to cater for cases like gluster://[1:2:3:4:5]:/volname/image

So you consider this a valid URL? I would have expected it to invalid. But let me see, there must be some official definition of an URL...

Alright, so RFC 2234 says that having no digits after the colon is valid. It also says that you shouldn't generate such URLs. And it doesn't say what it means when it's there... Common interpretation seems to be that it's treated as if it wasn't specified, i.e. the default port for the schema is used.

Quoted 2 lines

So if 0 is the default port for glusterfs, your code looks okay. But it doesn't seem to be a very useful default port number.

I know, but gluster prefers to be called with port=0 which will be interpreted as "default" by it.

While we are at this, let me bring out another issue. Gluster supports 3 transport types:

- socket in which case the server will be hostname, ipv4 or ipv4 address.
- rdma in which case server will be interpreted similar to socket.
- unix in which case server will be a path to unix domain socket and this will look like any other filesystem path. (Eg. /tmp/glusterd.socket)

I don't think we can fit 'unix' within the standard URI scheme (RFC 3986) easily, but I am planning to specify the 'unix' transport as below:

gluster://[/path/to/unix/domain/socket]/volname/image?transport=unix

i,e., I am asking the user to put the unix domain socket path within square brackets when transport type is unix.

Do you think this is fine ?

Quoted 32 lines

Quoted 31 lines

Quoted 19 lines

Quoted 19 lines

block/gluster.c new file +19 -0

+static int qemu_gluster_send_pipe(BDRVGlusterState *s, GlusterAIOCB *acb)
+{
+    int ret = 0;
+    while (1) {
+        fd_set wfd;
+        int fd = s->fds[GLUSTER_FD_WRITE];
+
+        ret = write(fd, (void *)&acb, sizeof(acb));
+        if (ret >= 0) {
+            break;
+        }
+        if (errno == EINTR) {
+            continue;
+        }
+        if (errno != EAGAIN) {
+            break;
+        }
+
+        FD_ZERO(&wfd);

Quoted 8 lines

block/gluster.c line 466

Quoted 4 lines

block/gluster.c new file +4 -0

+        FD_SET(fd, &wfd);
+        do {
+            ret = select(fd + 1, NULL, &wfd, NULL, NULL);
+        } while (ret < 0 && errno == EINTR);

What's the idea behind this? While we're hanging in this loop noone will read anything from the pipe, so it's unlikely that it magically becomes ready.

I write to the pipe and wait for the reader to read it. The reader (qemu_gluster_aio_event_reader) is already waiting on the other end of the pipe.

Quoted 2 lines

qemu_gluster_aio_even_reader() isn't called while we're looping here. It will only be called from the main loop, after this function has returned.

May be I am not understanding you correctly here. Let me be a bit verbose. [...]

Quoted 14 lines

While we are at this, let me bring out another issue. Gluster supports 3 transport types:

- socket in which case the server will be hostname, ipv4 or ipv4 address.
- rdma in which case server will be interpreted similar to socket.
- unix in which case server will be a path to unix domain socket and this will look like any other filesystem path. (Eg. /tmp/glusterd.socket)

I don't think we can fit 'unix' within the standard URI scheme (RFC 3986) easily, but I am planning to specify the 'unix' transport as below:

gluster://[/path/to/unix/domain/socket]/volname/image?transport=unix

Quoted 4 lines

i,e., I am asking the user to put the unix domain socket path within square brackets when transport type is unix.

Do you think this is fine ?

Never saw something like this before, but it does seem reasonable to me. Excludes ] from the valid characters in the file name of the socket, but that shouldn't be a problem in practice.

Quoted 19 lines · Kevin Wolf

Quoted 18 lines

While we are at this, let me bring out another issue. Gluster supports 3 transport types:

- socket in which case the server will be hostname, ipv4 or ipv4 address.
- rdma in which case server will be interpreted similar to socket.
- unix in which case server will be a path to unix domain socket and this will look like any other filesystem path. (Eg. /tmp/glusterd.socket)

I don't think we can fit 'unix' within the standard URI scheme (RFC 3986) easily, but I am planning to specify the 'unix' transport as below:

gluster://[/path/to/unix/domain/socket]/volname/image?transport=unix

i,e., I am asking the user to put the unix domain socket path within square brackets when transport type is unix.

Do you think this is fine ?

Quoted 3 lines

Never saw something like this before, but it does seem reasonable to me. Excludes ] from the valid characters in the file name of the socket, but that shouldn't be a problem in practice.

Bikeshedding, but I prefer

gluster:///path/to/unix/domain/socket:/volname/image?transport=unix

as being more similar to file://, or even

gluster:///path/to/unix/domain/socket/volname/image?transport=unix

with the last two components implied to be part of the payload, not the path.

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

I don't think we can fit 'unix' within the standard URI scheme (RFC 3986) easily, but I am planning to specify the 'unix' transport as below:

gluster://[/path/to/unix/domain/socket]/volname/image?transport=unix

i,e., I am asking the user to put the unix domain socket path within square brackets when transport type is unix.

Never saw something like this before, but it does seem reasonable to me. Excludes ] from the valid characters in the file name of the socket, but that shouldn't be a problem in practice.

Quoted 3 lines

Bikeshedding, but I prefer

gluster:///path/to/unix/domain/socket:/volname/image?transport=unix

Unless you or others strongly feel about this, I would like to go with [ ] based spec, which I feel is less prone to errors like missing a colon by mistake :)

Quoted 20 lines · Avi Kivity

Quoted 19 lines · Kevin Wolf

Quoted 18 lines

While we are at this, let me bring out another issue. Gluster supports 3 transport types:

- socket in which case the server will be hostname, ipv4 or ipv4 address.
- rdma in which case server will be interpreted similar to socket.
- unix in which case server will be a path to unix domain socket and this will look like any other filesystem path. (Eg. /tmp/glusterd.socket)

I don't think we can fit 'unix' within the standard URI scheme (RFC 3986) easily, but I am planning to specify the 'unix' transport as below:

gluster://[/path/to/unix/domain/socket]/volname/image?transport=unix

i,e., I am asking the user to put the unix domain socket path within square brackets when transport type is unix.

Do you think this is fine ?

Quoted 7 lines

Quoted 3 lines

Never saw something like this before, but it does seem reasonable to me. Excludes ] from the valid characters in the file name of the socket, but that shouldn't be a problem in practice.

Bikeshedding, but I prefer

gluster:///path/to/unix/domain/socket:/volname/image?transport=unix

So if the unix domain socket is /tmp/glusterd.socket, then this would look like:

gluster:///tmp/glusterd.socket:/volname/image?transport=unix.

So you are saying : will the separator b/n the unix domain socket path and rest of the URI components.

Unless you or others strongly feel about this, I would like to go with [ ] based spec, which I feel is less prone to errors like missing a colon by mistake :)

Quoted 3 lines

as being more similar to file://, or even

Quoted 4 lines

gluster:///path/to/unix/domain/socket/volname/image?transport=unix

with the last two components implied to be part of the payload, not the path.

Note that image is a file path by itself like /dir1/a.img. So I guess it becomes difficult to figure out where the unix domain socket path ends and rest of the URI components begin w/o a separator in between.

Quoted 4 lines · Bharata B Rao

Quoted 3 lines · Avi Kivity

Quoted 2 lines · Kevin Wolf

Quoted 2 lines

While we are at this, let me bring out another issue. Gluster

supports 3

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

transport types:

- socket in which case the server will be hostname, ipv4 or ipv4

address.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

- rdma in which case server will be interpreted similar to socket.
- unix in which case server will be a path to unix domain socket

and this

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

will look like any other filesystem path. (Eg.

/tmp/glusterd.socket)

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I don't think we can fit 'unix' within the standard URI scheme (RFC

3986)

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 8 lines

easily, but I am planning to specify the 'unix' transport as below:

gluster://[/path/to/unix/domain/socket]/volname/image?transport=unix

i,e., I am asking the user to put the unix domain socket path within square brackets when transport type is unix.

Do you think this is fine ?

Never saw something like this before, but it does seem reasonable to

me.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Excludes ] from the valid characters in the file name of the socket,

but

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

that shouldn't be a problem in practice.

Bikeshedding, but I prefer

gluster:///path/to/unix/domain/socket:/volname/image?transport=unix

So if the unix domain socket is /tmp/glusterd.socket, then this would

look like:

Quoted 13 lines

gluster:///tmp/glusterd.socket:/volname/image?transport=unix.

So you are saying : will the separator b/n the unix domain socket path and rest of the URI components.

Unless you or others strongly feel about this, I would like to go with [ ] based spec, which I feel is less prone to errors like missing a colon by mistake :)

Quoted 3 lines

as being more similar to file://, or even

Quoted 8 lines

Quoted 4 lines

gluster:///path/to/unix/domain/socket/volname/image?transport=unix

with the last two components implied to be part of the payload, not the path.

Note that image is a file path by itself like /dir1/a.img. So I guess it becomes difficult to figure out where the unix domain socket path ends and rest of the URI components begin w/o a separator in between.

IMHO this is all gross. URIs already have a well defined way to provide multiple parameters, dealing with escaping of special characters. ie query parameters. The whole benefit of using URI syntax is to let apps process the URIs using standard APIs. We should not be trying to define some extra magic encoding to let us stuff 2 separate parameters into the path component
since that means apps have to now write custom parsing code again. Either the UNIX socket path, or the volume path should be in the URI path, not both. The other part should be a URI parameter. I'd really expect us to use:

gluster:///volname/image?transport=unix&sockpath=/path/to/unix/sock

gluster:///volname/image?transport=unix&sockpath=/path/to/unix/sock

IMHO this is all gross. URIs already have a well defined way to provide multiple parameters, dealing with escaping of special characters. ie query parameters. The whole benefit of using URI syntax is to let apps process the URIs using standard APIs. We should not be trying to define some extra magic encoding to let us stuff 2 separate parameters into the path component since that means apps have to now write custom parsing code again. Either the UNIX socket path, or the volume path should be in the URI path, not both. The other part should be a URI parameter. I'd really expect us to use:

gluster:///volname/image?transport=unix&sockpath=/path/to/unix/sock

Quoted 10 lines · Daniel P. Berrange

IMHO this is all gross. URIs already have a well defined way to provide multiple parameters, dealing with escaping of special characters. ie query parameters. The whole benefit of using URI syntax is to let apps process the URIs using standard APIs. We should not be trying to define some extra magic encoding to let us stuff 2 separate parameters into the path component since that means apps have to now write custom parsing code again. Either the UNIX socket path, or the volume path should be in the URI path, not both. The other part should be a URI parameter. I'd really expect us to use:

Quoted 1 line

gluster:///volname/image?transport=unix&sockpath=/path/to/unix/sock

^why 3 /// here ? volname is not a path, but image is.

So when transport=socket or rdma, is it acceptable to still use the following format ?

gluster://server[:port]/volname/path/to/image[?transport=...]

With that, the different options possible are

unix - gluster://testvol/dir/a.img?transport=unix&sockpath=/tmp/glusterd.socket ipv4 - gluster://1.2.3.4:0/testvol/dir/a.img?transport=socket

ipv6 - gluster://[1:2:3:4:5:6:7:8]:0/testvol/a.img hostname - gluster://example.org/testvol/dir/a.img rdma - gluster://1.2.3.4:0/testvol/a.img?transport=rdma

Does this look complete from the specification point of view ?

Quoted 1 line

gluster:///volname/image?transport=unix&sockpath=/path/to/unix/sock

^why 3 /// here ? volname is not a path, but image is.

gluster://server[:port]/volname/path/to/image[?transport=...]

With that, the different options possible are

unix - gluster://testvol/dir/a.img?transport=unix&sockpath=/tmp/glusterd.socket ipv4 - gluster://1.2.3.4:0/testvol/dir/a.img?transport=socket
ipv6 - gluster://[1:2:3:4:5:6:7:8]:0/testvol/a.img
hostname - gluster://example.org/testvol/dir/a.img
rdma - gluster://1.2.3.4:0/testvol/a.img?transport=rdma

Does this look complete from the specification point of view ?

Il 07/09/2012 05:24, Bharata B Rao ha scritto:

Quoted 2 lines

Quoted 1 line

gluster:///volname/image?transport=unix&sockpath=/path/to/unix/sock

^why 3 /// here ? volname is not a path, but image is.

Because the host is the local computer, i.e. empty.

Quoted 11 lines

gluster://server[:port]/volname/path/to/image[?transport=...]

With that, the different options possible are

unix - gluster://testvol/dir/a.img?transport=unix&sockpath=/tmp/glusterd.socket ipv4 - gluster://1.2.3.4:0/testvol/dir/a.img?transport=socket
ipv6 - gluster://[1:2:3:4:5:6:7:8]:0/testvol/a.img
hostname - gluster://example.org/testvol/dir/a.img
rdma - gluster://1.2.3.4:0/testvol/a.img?transport=rdma

Does this look complete from the specification point of view ?

Hmm, why don't we do the exact same thing as libvirt (http://libvirt.org/remote.html):

ipv4 - gluster+tcp://1.2.3.4:0/testvol/dir/a.img
ipv6 - gluster+tcp://[1:2:3:4:5:6:7:8]:0/testvol/a.img host - gluster+tcp://example.org/testvol/dir/a.img
unix - gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket rdma - gluster+rdma://1.2.3.4:0/testvol/a.img

Where the default transport is tcp (i.e. gluster+tcp is the same as gluster). This is easily extensible, theoretically you could have something like this:

ssh - gluster+ssh://user@host/testvol/a.img?socket=/tmp/glusterd.socket

Am 07.09.2012 11:36, schrieb Paolo Bonzini:

Quoted 11 lines

Hmm, why don't we do the exact same thing as libvirt (http://libvirt.org/remote.html):

ipv4 - gluster+tcp://1.2.3.4:0/testvol/dir/a.img
ipv6 - gluster+tcp://[1:2:3:4:5:6:7:8]:0/testvol/a.img host - gluster+tcp://example.org/testvol/dir/a.img
unix - gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket rdma - gluster+rdma://1.2.3.4:0/testvol/a.img

Where the default transport is tcp (i.e. gluster+tcp is the same as gluster). This is easily extensible, theoretically you could have something like this:

Quoted 1 line

ssh - gluster+ssh://user@host/testvol/a.img?socket=/tmp/glusterd.socket

I like this. Having the type only as a parameter when it decides how the schema must be parsed has bothered me from the start, but I hadn't thought of this way.

Strong +1 from me.

Kevin

FYI, bdrv_find_protocol() fails for protocols like this. It detects the protocol as "gluster+tcp" and compares it with drv->protocol_name (which is only "gluster").

I guess I will have to fix bdrv_find_protocol() to handle the '+' within protocol string correctly.

Quoted 43 lines · Avi Kivity

Quoted 20 lines · Kevin Wolf

Quoted 19 lines

Quoted 18 lines

While we are at this, let me bring out another issue. Gluster supports 3 transport types:

- socket in which case the server will be hostname, ipv4 or ipv4 address.
- rdma in which case server will be interpreted similar to socket.
- unix in which case server will be a path to unix domain socket and this will look like any other filesystem path. (Eg. /tmp/glusterd.socket)

I don't think we can fit 'unix' within the standard URI scheme (RFC 3986) easily, but I am planning to specify the 'unix' transport as below:

gluster://[/path/to/unix/domain/socket]/volname/image?transport=unix

i,e., I am asking the user to put the unix domain socket path within square brackets when transport type is unix.

Do you think this is fine ?

Quoted 7 lines

Quoted 3 lines

Never saw something like this before, but it does seem reasonable to me. Excludes ] from the valid characters in the file name of the socket, but that shouldn't be a problem in practice.

Bikeshedding, but I prefer

gluster:///path/to/unix/domain/socket:/volname/image?transport=unix

So if the unix domain socket is /tmp/glusterd.socket, then this would look like:

gluster:///tmp/glusterd.socket:/volname/image?transport=unix.

So you are saying : will the separator b/n the unix domain socket path and rest of the URI components.

Unless you or others strongly feel about this, I would like to go with [ ] based spec, which I feel is less prone to errors like missing a colon by mistake :)

Quoted 3 lines

as being more similar to file://, or even

Quoted 8 lines

Quoted 4 lines

gluster:///path/to/unix/domain/socket/volname/image?transport=unix

with the last two components implied to be part of the payload, not the path.

Note that image is a file path by itself like /dir1/a.img. So I guess it becomes difficult to figure out where the unix domain socket path ends and rest of the URI components begin w/o a separator in between.

IMHO this is all gross. URIs already have a well defined way to provide multiple parameters, dealing with escaping of special characters. ie query parameters. The whole benefit of using URI syntax is to let apps process the URIs using standard APIs. We should not be trying to define some extra magic encoding to let us stuff 2 separate parameters into the path component since that means apps have to now write custom parsing code again. Either the UNIX socket path, or the volume path should be in the URI path, not both. The other part should be a URI parameter. I'd really expect us to use:

gluster:///volname/image?transport=unix&sockpath=/path/to/unix/sock

Am 06.09.2012 17:47, schrieb Daniel P. Berrange:

Quoted 63 lines · Bharata B Rao

Quoted 43 lines · Avi Kivity

Quoted 20 lines · Kevin Wolf

Quoted 19 lines

Quoted 18 lines

While we are at this, let me bring out another issue. Gluster supports 3 transport types:

- socket in which case the server will be hostname, ipv4 or ipv4 address.
- rdma in which case server will be interpreted similar to socket.
- unix in which case server will be a path to unix domain socket and this will look like any other filesystem path. (Eg. /tmp/glusterd.socket)

I don't think we can fit 'unix' within the standard URI scheme (RFC 3986) easily, but I am planning to specify the 'unix' transport as below:

gluster://[/path/to/unix/domain/socket]/volname/image?transport=unix

i,e., I am asking the user to put the unix domain socket path within square brackets when transport type is unix.

Do you think this is fine ?

Quoted 7 lines

Quoted 3 lines

Never saw something like this before, but it does seem reasonable to me. Excludes ] from the valid characters in the file name of the socket, but that shouldn't be a problem in practice.

Bikeshedding, but I prefer

gluster:///path/to/unix/domain/socket:/volname/image?transport=unix

So if the unix domain socket is /tmp/glusterd.socket, then this would look like:

gluster:///tmp/glusterd.socket:/volname/image?transport=unix.

So you are saying : will the separator b/n the unix domain socket path and rest of the URI components.

Unless you or others strongly feel about this, I would like to go with [ ] based spec, which I feel is less prone to errors like missing a colon by mistake :)

Quoted 3 lines

as being more similar to file://, or even

Quoted 8 lines

Quoted 4 lines

gluster:///path/to/unix/domain/socket/volname/image?transport=unix

with the last two components implied to be part of the payload, not the path.

Note that image is a file path by itself like /dir1/a.img. So I guess it becomes difficult to figure out where the unix domain socket path ends and rest of the URI components begin w/o a separator in between.

IMHO this is all gross. URIs already have a well defined way to provide multiple parameters, dealing with escaping of special characters. ie query parameters. The whole benefit of using URI syntax is to let apps process the URIs using standard APIs. We should not be trying to define some extra magic encoding to let us stuff 2 separate parameters into the path component since that means apps have to now write custom parsing code again. Either the UNIX socket path, or the volume path should be in the URI path, not both. The other part should be a URI parameter. I'd really expect us to use:

Quoted 1 line

gluster:///volname/image?transport=unix&sockpath=/path/to/unix/sock

I think doing it the other way round would be more logical:

gluster+unix:///path/to/unix/sock?image=volname/image

This way you have the socket first, which you also must open first. Having it as a parameter without which you can't make sense of the path feels a bit less than ideal.

Quoted 4 lines

Quoted 4 lines

I think doing it the other way round would be more logical:

gluster+unix:///path/to/unix/sock?image=volname/image

Quoted 3 lines

Quoted 3 lines

This way you have the socket first, which you also must open first. Having it as a parameter without which you can't make sense of the path feels a bit less than ideal.

The issue here is that the volume/path/to/image part is something that is required for all gluster transports. The /path/to/unix/sock is something that is only required for the unix transport. To have consistent URI scheme across all transports, you really want the volume/path/to/image bit to use the URI path component.

Quoted 26 lines

Quoted 13 lines

Quoted 13 lines

block/gluster.c new file +13 -0

+static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void *arg)
+{
+    GlusterAIOCB *acb = (GlusterAIOCB *)arg;
+    BDRVGlusterState *s = acb->common.bs->opaque;
+
+    acb->ret = ret;
+    if (qemu_gluster_send_pipe(s, acb) < 0) {
+        /*
+         * Gluster AIO callback thread failed to notify the waiting
+         * QEMU thread about IO completion. Nothing much can be done
+         * here but to abruptly abort.
+         *
+         * FIXME: Check if the read side of the fd handler can somehow

Quoted 8 lines

block/gluster.c line 487

Quoted 4 lines

block/gluster.c new file +4 -0

+         * be notified of this failure paving the way for a graceful exit.
+         */
+        error_report("Gluster failed to notify QEMU about IO completion");
+        abort();

In the extreme case you may choose to make this disk inaccessible (something like bs->drv = NULL), but abort() kills the whole VM and should only be called when there is a bug.

There have been concerns raised about this earlier too. I settled for this since I couldn't see a better way out and I could see the precedence for this in posix-aio-compat.c

Quoted 4 lines

So I could just do the necessary cleanup, set bs->drv to NULL and return from here ? But how do I wake up the QEMU thread that is waiting on the read side of the pipe ? W/o that, the QEMU thread that waits on the read side of the pipe is still hung.

There is no other thread. But you're right, you should probably unregister the aio_fd_handler and any other pending callbacks.

Quoted 26 lines

Quoted 26 lines

Quoted 13 lines

Quoted 13 lines

block/gluster.c new file +13 -0

+static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void *arg)
+{
+    GlusterAIOCB *acb = (GlusterAIOCB *)arg;
+    BDRVGlusterState *s = acb->common.bs->opaque;
+
+    acb->ret = ret;
+    if (qemu_gluster_send_pipe(s, acb) < 0) {
+        /*
+         * Gluster AIO callback thread failed to notify the waiting
+         * QEMU thread about IO completion. Nothing much can be done
+         * here but to abruptly abort.
+         *
+         * FIXME: Check if the read side of the fd handler can somehow

Quoted 8 lines

block/gluster.c line 487

Quoted 4 lines

block/gluster.c new file +4 -0

+         * be notified of this failure paving the way for a graceful exit.
+         */
+        error_report("Gluster failed to notify QEMU about IO completion");
+        abort();

In the extreme case you may choose to make this disk inaccessible (something like bs->drv = NULL), but abort() kills the whole VM and should only be called when there is a bug.

There have been concerns raised about this earlier too. I settled for this since I couldn't see a better way out and I could see the precedence for this in posix-aio-compat.c

Quoted 7 lines

Quoted 4 lines

So I could just do the necessary cleanup, set bs->drv to NULL and return from here ? But how do I wake up the QEMU thread that is waiting on the read side of the pipe ? W/o that, the QEMU thread that waits on the read side of the pipe is still hung.

There is no other thread. But you're right, you should probably unregister the aio_fd_handler and any other pending callbacks.

As I clarified in the other mail, this (gluster_finish_aiocb) is called from gluster thread context and hence QEMU thread that raised the original read/write request is still blocked on qemu_aio_wait().

I tried the following cleanup instead of abrupt abort:

close(read_fd); /* This will wake up the QEMU thread blocked on select(read_fd...) */ close(write_fd);
qemu_aio_set_fd_handler(read_fd, NULL, NULL, NULL, NULL); qemu_aio_release(acb);
s->qemu_aio_count--;
bs->drv = NULL;

I tested this by manually injecting faults into qemu_gluster_send_pipe(). With the above cleanup, the guest kernel crashes with IO errors.

Is there anything else that I need to do or do differently to retain the VM running w/o disk access ?

I thought of completing the aio callback by doing acb->common.cb(acb->common.opaque, -EIO);
but that would do a coroutine enter from gluster thread, which I don't think should be done.

Am 15.08.2012 07:21, schrieb Bharata B Rao:

Quoted 48 lines · Kevin Wolf

Quoted 26 lines

Quoted 26 lines

Quoted 13 lines

Quoted 13 lines

block/gluster.c new file +13 -0

+static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void *arg)
+{
+    GlusterAIOCB *acb = (GlusterAIOCB *)arg;
+    BDRVGlusterState *s = acb->common.bs->opaque;
+
+    acb->ret = ret;
+    if (qemu_gluster_send_pipe(s, acb) < 0) {
+        /*
+         * Gluster AIO callback thread failed to notify the waiting
+         * QEMU thread about IO completion. Nothing much can be done
+         * here but to abruptly abort.
+         *
+         * FIXME: Check if the read side of the fd handler can somehow

Quoted 8 lines

block/gluster.c line 487

Quoted 4 lines

block/gluster.c new file +4 -0

+         * be notified of this failure paving the way for a graceful exit.
+         */
+        error_report("Gluster failed to notify QEMU about IO completion");
+        abort();

In the extreme case you may choose to make this disk inaccessible (something like bs->drv = NULL), but abort() kills the whole VM and should only be called when there is a bug.

There have been concerns raised about this earlier too. I settled for this since I couldn't see a better way out and I could see the precedence for this in posix-aio-compat.c

Quoted 7 lines

Quoted 4 lines

So I could just do the necessary cleanup, set bs->drv to NULL and return from here ? But how do I wake up the QEMU thread that is waiting on the read side of the pipe ? W/o that, the QEMU thread that waits on the read side of the pipe is still hung.

There is no other thread. But you're right, you should probably unregister the aio_fd_handler and any other pending callbacks.

As I clarified in the other mail, this (gluster_finish_aiocb) is called from gluster thread context and hence QEMU thread that raised the original read/write request is still blocked on qemu_aio_wait().

I tried the following cleanup instead of abrupt abort:

close(read_fd); /* This will wake up the QEMU thread blocked on select(read_fd...) */ close(write_fd);
qemu_aio_set_fd_handler(read_fd, NULL, NULL, NULL, NULL); qemu_aio_release(acb);
s->qemu_aio_count--;
bs->drv = NULL;

Quoted 2 lines

I tested this by manually injecting faults into qemu_gluster_send_pipe(). With the above cleanup, the guest kernel crashes with IO errors.

What does "crash" really mean? IO errors certainly shouldn't cause a kernel to crash?

Quoted 8 lines

Is there anything else that I need to do or do differently to retain the VM running w/o disk access ?

I thought of completing the aio callback by doing acb->common.cb(acb->common.opaque, -EIO);
but that would do a coroutine enter from gluster thread, which I don't think should be done.

You would have to take the global qemu mutex at least. I agree it's not a good thing to do.

Am 14.08.2012 06:38, schrieb Bharata B Rao:

Quoted 39 lines

Kevin, Thanks for your review. I will address all of your comments in the next iteration, but have a few questions/comments on the others...

Quoted 23 lines · Kevin Wolf

Quoted 23 lines

block/gluster.c new file +23 -0

+static int parse_server(GlusterURI *uri, char *server)
+{
+    int ret = -EINVAL;
+    char *token, *saveptr;
+    char *p, *q = server;
+
+    p = strchr(server, '[');
+    if (p) {
+        /* [ipv6] */
+        if (p != server) {
+            /* [ not in the beginning */
+            goto out;
+        }
+        q++;
+        p = strrchr(p, ']');
+        if (!p) {
+            /* No matching ] */
+            goto out;
+        }
+        *p++ = '\0';
+        uri->server = g_strdup(q);
+
+        if (*p) {

Quoted 9 lines

block/gluster.c line 111

Quoted 8 lines

block/gluster.c new file +8 -0

+            if (*p != ':') {
+                /* [ipv6] followed by something other than : */
+                goto out;
+            }
+            uri->port = strtoul(++p, NULL, 0);
+            if (uri->port < 0) {
+                goto out;
+            }

In any case, let me see if I can get rid of this altogether and reuse qemu-sockets.c:inet_parse().

block/gluster.c new file +7 -0

+static void qemu_gluster_complete_aio(GlusterAIOCB *acb)
+{
+    int ret;
+
+    if (acb->canceled) {
+        qemu_aio_release(acb);
+        return;

block/gluster.c new file +26 -0

+    qemu_aio_release(acb);
+}
+
+static void qemu_gluster_aio_event_reader(void *opaque)
+{
+    BDRVGlusterState *s = opaque;
+    GlusterAIOCB *event_acb;
+    int event_reader_pos = 0;
+    ssize_t ret;
+
+    do {
+        char *p = (char *)&event_acb;
+
+        ret = read(s->fds[GLUSTER_FD_READ], p + event_reader_pos,
+                   sizeof(event_acb) - event_reader_pos);
+        if (ret > 0) {
+            event_reader_pos += ret;
+            if (event_reader_pos == sizeof(event_acb)) {
+                event_reader_pos = 0;
+                qemu_gluster_complete_aio(event_acb);
+                s->qemu_aio_count--;
+            }
+        }
+    } while (ret < 0 && errno == EINTR);
+}
+

Quoted 7 lines · Bharata B Rao

block/gluster.c new file +7 -0

+static void qemu_gluster_complete_aio(GlusterAIOCB *acb)
+{
+    int ret;
+
+    if (acb->canceled) {
+        qemu_aio_release(acb);
+        return;

block/gluster.c line 300

Quoted 10 lines

block/gluster.c new file +10 -0

+    }
+
+    if (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 */
+    }
+    acb->common.cb(acb->common.opaque, ret);

The .cb() here is bdrv_co_io_em_complete(). It does qemu_coroutine_enter(), handles the return value and comes back here.

But if the bdrv_read or bdrv_write or bdrv_flush was called from a coroutine context (as against they themselves creating a new coroutine), the above .cb() call above doesn't return to this point. Hence I won't be able to release the acb and decrement the qemu_aio_count.

What could be the issue here ? In general, how do I ensure that my aio calls get completed correctly in such scenarios where bdrv_read etc are called from coroutine context rather than from main thread context ?

Quoted 3 lines

Quoted 3 lines

What could be the issue here ? In general, how do I ensure that my aio calls get completed correctly in such scenarios where bdrv_read etc are called from coroutine context rather than from main thread context ?

One way to handle this is not to do completion from gluster thread but instead schedule a BH that does the completion. In fact I had this approach in the earlier versions, but resorted to directly calling completion from gluster thread as I didn't see the value of using a BH for completion. But I guess its indeed needed to support such scenarios (qcow2 image creation on gluster backend).

Il 05/09/2012 11:57, Bharata B Rao ha scritto:

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

What could be the issue here ? In general, how do I ensure that my aio calls get completed correctly in such scenarios where bdrv_read etc are called from coroutine context rather than from main thread context ?

One way to handle this is not to do completion from gluster thread but instead schedule a BH that does the completion. In fact I had this approach in the earlier versions, but resorted to directly calling completion from gluster thread as I didn't see the value of using a BH for completion. But I guess its indeed needed to support such scenarios (qcow2 image creation on gluster backend).

I think the problem is that we're calling bdrv_drain_all() from coroutine context. This is problematic because then the current coroutine won't yield and won't give other coroutines an occasion to run.

This could be fixed by checking whether we're in coroutine context in bdrv_drain_all(). If so, instead of draining the queues directly, schedule a bottom half that does bdrv_drain_all() followed by qemu_coroutine_enter(), and yield.

If it works, I think this change would be preferrable to using a "magic" BH in every driver.

Quoted 2 lines

Quoted 2 lines

If it works, I think this change would be preferrable to using a "magic" BH in every driver.

The way it works in posix-aio-compat is that the request is first removed from the list and then the callback is called. This way posix_aio_flush() can return 0 and bdrv_drain_all() completes.

Il 06/09/2012 11:06, Kevin Wolf ha scritto:

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

If it works, I think this change would be preferrable to using a "magic" BH in every driver.

The way it works in posix-aio-compat is that the request is first removed from the list and then the callback is called. This way posix_aio_flush() can return 0 and bdrv_drain_all() completes.

So the same could be done in gluster: first decrease qemu_aio_count, then call the callback, then call qemu_aio_release.

But in either case, wouldn't that leak the AIOCBs until the end of qcow2_create?

The AIOCB is already invalid at the time the callback is entered, so we could release it before the call. However, not all implementation of AIO are ready for that and I'm not really in the mood for large scale refactoring...

Quoted 4 lines

The AIOCB is already invalid at the time the callback is entered, so we could release it before the call. However, not all implementation of AIO are ready for that and I'm not really in the mood for large scale refactoring...

But the way, what I'd really want to see in the end is to get rid of qemu_aio_flush() and replace it by .bdrv_drain() callbacks in each BlockDriver. The way we're doing it today is a layering violation.

Doesn't change anything about this problem, though. So the options that we have are:

1. Delay the callback using a BH. Doing this in each driver is ugly. But is there actually more than one possible callback in today's coroutine world? I only see bdrv_co_io_em_complete(), which could reenter the coroutine from a BH.

2. Delay the callback by just calling it later when the cleanup has been completed and .io_flush() can return 0. You say that it's hard to implement for some drivers, except if the AIOCB are leaked until the end of functions like qcow2_create().

3. Add a delay only later in functions like bdrv_drain_all() that assume that the request has completed. Are there more of this type? AIOCBs are leaked until a bdrv_drain_all() call. Does it work with draining specific BDSes instead of everything?

Unless I forgot some important point, it almost looks like option 1 is the easiest and safest.

Il 06/09/2012 12:07, Kevin Wolf ha scritto:

Quoted 5 lines

Quoted 4 lines

The AIOCB is already invalid at the time the callback is entered, so we could release it before the call. However, not all implementation of AIO are ready for that and I'm not really in the mood for large scale refactoring...

Quoted 3 lines

But the way, what I'd really want to see in the end is to get rid of qemu_aio_flush() and replace it by .bdrv_drain() callbacks in each BlockDriver. The way we're doing it today is a layering violation.

That's quite difficult. Completion of an I/O operation can trigger another I/O operation on another block device, and so on until we go back to the first device (think of a hypothetical RAID-5 device).

Quoted 7 lines

Doesn't change anything about this problem, though. So the options that we have are:

1. Delay the callback using a BH. Doing this in each driver is ugly. But is there actually more than one possible callback in today's coroutine world? I only see bdrv_co_io_em_complete(), which could reenter the coroutine from a BH.

Easy and safe, but it feels a bit like a timebomb. Also, I'm not entirely sure of _why_ the bottom half works. :)

Quoted 4 lines

2. Delay the callback by just calling it later when the cleanup has been completed and .io_flush() can return 0. You say that it's hard to implement for some drivers, except if the AIOCB are leaked until the end of functions like qcow2_create().

... which is what we do in posix-aio-compat.c; nobody screamed so far.

Not really hard, it just has to be assessed for each driver separately.
We can just do it in gluster and refactor it later.

Quoted 7 lines

3. Add a delay only later in functions like bdrv_drain_all() that assume that the request has completed. Are there more of this type? AIOCBs are leaked until a bdrv_drain_all() call. Does it work with draining specific BDSes instead of everything?

Unless I forgot some important point, it almost looks like option 1 is the easiest and safest.

I agree with your opinion, but I would feel better if I understood better why it works. (2) can be done easily in each driver (no ugliness) and refactored later.

Quoted 3 lines

That's quite difficult. Completion of an I/O operation can trigger another I/O operation on another block device, and so on until we go back to the first device (think of a hypothetical RAID-5 device).

You always have a tree of BDSes, and children should only ever trigger completion of I/O operations in their parents. Am I missing anything?

Quoted 8 lines

Quoted 7 lines

Doesn't change anything about this problem, though. So the options that we have are:

1. Delay the callback using a BH. Doing this in each driver is ugly. But is there actually more than one possible callback in today's coroutine world? I only see bdrv_co_io_em_complete(), which could reenter the coroutine from a BH.

Quoted 2 lines

Easy and safe, but it feels a bit like a timebomb. Also, I'm not entirely sure of _why_ the bottom half works. :)

Hm, safe and time bomb is contradictory in my book. :-)

The bottom half work because we're not reentering the qcow2_create coroutine immediately, so the gluster AIO callback can complete all of its cleanup work without being interrupted by code that might wait on this particular request and create a deadlock this way.

Quoted 6 lines

Quoted 4 lines

2. Delay the callback by just calling it later when the cleanup has been completed and .io_flush() can return 0. You say that it's hard to implement for some drivers, except if the AIOCB are leaked until the end of functions like qcow2_create().

... which is what we do in posix-aio-compat.c; nobody screamed so far.

True. Would be easy to fix in posix-aio-compat, though, or can a callback expect that the AIOCB is still valid?

Quoted 2 lines

Not really hard, it just has to be assessed for each driver separately.
We can just do it in gluster and refactor it later.

Okay, so let's keep it as an option for now.

Quoted 8 lines

Quoted 7 lines

3. Add a delay only later in functions like bdrv_drain_all() that assume that the request has completed. Are there more of this type? AIOCBs are leaked until a bdrv_drain_all() call. Does it work with draining specific BDSes instead of everything?

Unless I forgot some important point, it almost looks like option 1 is the easiest and safest.

Quoted 3 lines

I agree with your opinion, but I would feel better if I understood better why it works. (2) can be done easily in each driver (no ugliness) and refactored later.

I think option 2 must be done in each driver by design, or do you see even a theoretical way how to do it generically?

Am 06.09.2012 12:18, schrieb Paolo Bonzini:

Quoted 6 lines

Il 06/09/2012 12:07, Kevin Wolf ha scritto:

Quoted 5 lines

Quoted 4 lines

The AIOCB is already invalid at the time the callback is entered, so we could release it before the call. However, not all implementation of AIO are ready for that and I'm not really in the mood for large scale refactoring...

Quoted 7 lines

Quoted 3 lines

But the way, what I'd really want to see in the end is to get rid of qemu_aio_flush() and replace it by .bdrv_drain() callbacks in each BlockDriver. The way we're doing it today is a layering violation.

That's quite difficult. Completion of an I/O operation can trigger another I/O operation on another block device, and so on until we go back to the first device (think of a hypothetical RAID-5 device).

You always have a tree of BDSes, and children should only ever trigger completion of I/O operations in their parents. Am I missing anything?

Quoted 10 lines

Quoted 7 lines

Doesn't change anything about this problem, though. So the options that we have are:

1. Delay the callback using a BH. Doing this in each driver is ugly. But is there actually more than one possible callback in today's coroutine world? I only see bdrv_co_io_em_complete(), which could reenter the coroutine from a BH.

Easy and safe, but it feels a bit like a timebomb. Also, I'm not entirely sure of _why_ the bottom half works. :)

Hm, safe and time bomb is contradictory in my book. :-)

The bottom half work because we're not reentering the qcow2_create coroutine immediately, so the gluster AIO callback can complete all of its cleanup work without being interrupted by code that might wait on this particular request and create a deadlock this way.

Quoted 6 lines

Quoted 4 lines

2. Delay the callback by just calling it later when the cleanup has been completed and .io_flush() can return 0. You say that it's hard to implement for some drivers, except if the AIOCB are leaked until the end of functions like qcow2_create().

... which is what we do in posix-aio-compat.c; nobody screamed so far.

True. Would be easy to fix in posix-aio-compat, though, or can a callback expect that the AIOCB is still valid?

Quoted 2 lines

Not really hard, it just has to be assessed for each driver separately.
We can just do it in gluster and refactor it later.

Okay, so let's keep it as an option for now.

qemu_gluster_aio_event_reader() is the node->io_read in qemu_aio_wait().

qemu_aio_wait() calls node->io_read() which calls qemu_gluster_complete_aio(). Before we return back to qemu_aio_wait(), many other things happen:

bdrv_close() gets called from qcow2_create2()
This closes the gluster connection, closes the pipe, does qemu_set_fd_hander(read_pipe_fd, NULL, NULL, NULL, NULL), which results in the AioHandler node being deleted from aio_handlers list.

Now qemu_gluster_aio_event_reader (node->io_read) which was called from qemu_aio_wait() finally completes and goes ahead and accesses "node" which has already been deleted. This causes segfault.

So I think the option 1 (scheduling a BH from node->io_read) would be better for gluster.

This is a bug that has to be fixed anyway. There are provisions in aio.c, but they are broken apparently. Can you try this:

aio.c +3 -3

index 0a9eb10..99b8b72 100644
@@ -119,7 +119,7 @@ bool qemu_aio_wait(void)
         return true;
     }
 
-    walking_handlers = 1;
+    walking_handlers++;
 
     FD_ZERO(&rdfds);
     FD_ZERO(&wrfds);

@@ -147,7 +147,7 @@ bool qemu_aio_wait(void)
         }
     }
 
-    walking_handlers = 0;
+    walking_handlers--;
 
     /* No AIO operations?  Get us out of here */
     if (!busy) {

@@ -159,7 +159,7 @@ bool qemu_aio_wait(void)
 
     /* if we have any readable fds, dispatch event */
     if (ret > 0) {
-        walking_handlers = 1;
+        walking_handlers++;
 
         /* we have to walk very carefully in case
          * qemu_aio_set_fd_handler is called while we're walking */

Quoted 12 lines

block/gluster.c new file +12 -0

+static void qemu_gluster_complete_aio(GlusterAIOCB *acb)
+{
+    int ret;
+
+    if (acb->canceled) {
+        qemu_aio_release(acb);
+        return;
+    }
+
+    if (acb->ret == acb->size) {
+        ret = 0; /* Success */
+    } else if (acb->ret < 0) {

block/gluster.c line 300

Quoted 5 lines

block/gluster.c new file +5 -0

+        ret = acb->ret; /* Read/Write failed */
+    } else {
+        ret = -EIO; /* Partial read/write - fail it */
+    }
+    acb->common.cb(acb->common.opaque, ret);

The .cb() here is bdrv_co_io_em_complete(). It does qemu_coroutine_enter(), handles the return value and comes back here.

But if the bdrv_read or bdrv_write or bdrv_flush was called from a coroutine context (as against they themselves creating a new coroutine), the above .cb() call above doesn't return to this point.

Hence I won't
be able to release the acb and decrement the qemu_aio_count.

What could be the issue here ? In general, how do I ensure that my aio calls get completed correctly in such scenarios where bdrv_read etc are called from coroutine context rather than from main thread context ?

Creating qcow2 image would lead to this scenario where ->bdrv_create (=qcow2_create) will create a coroutine and subsequently read and write are called within qcow2_create in coroutine context itself.

Am 05.09.2012 09:41, schrieb Bharata B Rao:

Quoted 13 lines · Bharata B Rao

Quoted 12 lines

block/gluster.c new file +12 -0

+static void qemu_gluster_complete_aio(GlusterAIOCB *acb)
+{
+    int ret;
+
+    if (acb->canceled) {
+        qemu_aio_release(acb);
+        return;
+    }
+
+    if (acb->ret == acb->size) {
+        ret = 0; /* Success */
+    } else if (acb->ret < 0) {

Quoted 8 lines

block/gluster.c line 300

Quoted 5 lines

block/gluster.c new file +5 -0

+        ret = acb->ret; /* Read/Write failed */
+    } else {
+        ret = -EIO; /* Partial read/write - fail it */
+    }
+    acb->common.cb(acb->common.opaque, ret);

The .cb() here is bdrv_co_io_em_complete(). It does qemu_coroutine_enter(), handles the return value and comes back here.

Right.

.cb is set by qemu_gluster_aio_rw/flush(), and the only way these can be called is through bdrv_co_io_em() and bdrv_co_flush(), which both set bdrv_co_io_em_complete as the callback.

Quoted 4 lines

But if the bdrv_read or bdrv_write or bdrv_flush was called from a coroutine context (as against they themselves creating a new coroutine), the above .cb() call above doesn't return to this point.

Why?

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://server:[port]/volname/image[?transport=socket]

'gluster' is the protocol.

'server' specifies the server where the volume file specification for the given volume resides. This can be either hostname or ipv4 address or ipv6 address. ipv6 address needs to be with in square brackets [ ].

port' is the port number on which gluster management daemon (glusterd) is listening. This is optional and if not specified, QEMU will send 0 which will make libgfapi to use the default port.

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

'image' is the path to the actual VM image in the gluster volume.

'transport' specifies the transport used to connect to glusterd. This is optional and if not specified, socket transport is used.

Examples:

file=gluster://1.2.3.4/testvol/a.img file=gluster://1.2.3.4:5000/testvol/dir/a.img?transport=socket file=gluster://[1:2:3:4:5:6:7:8]/testvol/dir/a.img file=gluster://[1:2:3:4:5:6:7:8]:5000/testvol/dir/a.img?transport=socket file=gluster://server.domain.com:5000/testvol/dir/a.img

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

Reviewed-by Stefan Hajnoczi <[email protected]>

---

 block/Makefile.objs |    1 
 block/gluster.c     |  623 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 624 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 +361 -0

new file mode 100644
index 0000000..bbbaea8
@@ -0,0 +1,623 @@
+/*
+ * GlusterFS backend for QEMU
+ *
+ * (AIO implementation is derived from block/rbd.c)
+ *
+ * Copyright (C) 2012 Bharata B Rao <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include <glusterfs/api/glfs.h>
+#include "block_int.h"
+
+typedef struct GlusterAIOCB {
+    BlockDriverAIOCB common;
+    bool canceled;
+    int64_t size;
+    int ret;
+} GlusterAIOCB;
+
+typedef struct BDRVGlusterState {
+    struct glfs *glfs;
+    int fds[2];
+    struct glfs_fd *fd;
+    int qemu_aio_count;
+} BDRVGlusterState;
+
+#define GLUSTER_FD_READ 0
+#define GLUSTER_FD_WRITE 1
+
+typedef struct GlusterURI {
+    char *server;
+    int port;
+    char *volname;
+    char *image;
+    char *transport;
+} GlusterURI;
+
+static void qemu_gluster_uri_free(GlusterURI *uri)
+{
+    g_free(uri->server);
+    g_free(uri->volname);
+    g_free(uri->image);
+    g_free(uri->transport);
+    g_free(uri);
+}
+
+/*
+ * We don't validate the transport option obtained here but
+ * instead depend on gluster to flag an error.
+ */
+static int parse_transport(GlusterURI *uri, char *transport)
+{
+    char *token, *saveptr;
+    int ret = -EINVAL;
+
+    if (!transport) {
+        uri->transport = g_strdup("socket");
+        ret = 0;
+        goto out;
+    }
+
+    token = strtok_r(transport, "=", &saveptr);
+    if (!token) {
+        goto out;
+    }
+    if (strcmp(token, "transport")) {
+        goto out;
+    }
+    token = strtok_r(NULL, "=", &saveptr);
+    if (!token) {
+        goto out;
+    }
+    uri->transport = g_strdup(token);
+    ret = 0;
+out:
+    return ret;
+}
+
+static int parse_server(GlusterURI *uri, char *server)
+{
+    int ret = -EINVAL;
+    char *token, *saveptr;
+    char *p, *q = server;
+
+    p = strchr(server, '[');
+    if (p) {
+        /* [ipv6] */
+        if (p != server) {
+            /* [ not in the beginning */
+            goto out;
+        }
+        q++;
+        p = strrchr(p, ']');
+        if (!p) {
+            /* No matching ] */
+            goto out;
+        }
+        *p++ = '\0';
+        uri->server = g_strdup(q);
+
+        if (*p) {
+            if (*p != ':') {
+                /* [ipv6] followed by something other than : */
+                goto out;
+            }
+            uri->port = strtoul(++p, NULL, 0);
+            if (uri->port < 0) {
+                goto out;
+            }
+        } else {
+            /* port not specified, use default */
+            uri->port = 0;
+        }
+
+    } else {
+        /* ipv4 or hostname */
+        if (*server == ':') {
+            /* port specified w/o a server */
+            goto out;
+        }
+        token = strtok_r(server, ":", &saveptr);
+        if (!token) {
+            goto out;
+        }
+        uri->server = g_strdup(token);
+        token = strtok_r(NULL, ":", &saveptr);
+        if (token) {
+            uri->port = strtoul(token, NULL, 0);
+            if (uri->port < 0) {
+                goto out;
+            }
+        } else {
+            uri->port = 0;
+        }
+    }
+    ret = 0;
+out:
+    return ret;
+}
+
+/*
+ * file=gluster://server:[port]/volname/image[?transport=socket]
+ *
+ * 'gluster' is the protocol.
+ *
+ * 'server' specifies the server where the volume file specification for
+ * the given volume resides. This can be either hostname or ipv4 address
+ * or ipv6 address. ipv6 address needs to be with in square brackets [ ].
+ *
+ * 'port' is the port number on which gluster management daemon (glusterd) is
+ * listening. This is optional and if not specified, QEMU will send 0 which
+ * will make libgfapi to use the default port.
+ *
+ * 'volname' is the name of the gluster volume which contains the VM image.
+ *
+ * 'image' is the path to the actual VM image in the gluster volume.
+ *
+ * 'transport' specifies the transport used to connect to glusterd. This is
+ * optional and if not specified, socket transport is used.
+ *
+ * Examples:
+ *
+ * file=gluster://1.2.3.4/testvol/a.img
+ * file=gluster://1.2.3.4:5000/testvol/dir/a.img?transport=socket
+ * file=gluster://[1:2:3:4:5:6:7:8]/testvol/dir/a.img
+ * file=gluster://[1:2:3:4:5:6:7:8]:5000/testvol/dir/a.img?transport=socket
+ * file=gluster://server.domain.com:5000/testvol/dir/a.img
+ *
+ * We just do minimal checking of the gluster options and mostly ensure
+ * that all the expected elements of the URI are present. We depend on libgfapi
+ * APIs to return appropriate errors in case of invalid arguments.
+ */
+static int qemu_gluster_parseuri(GlusterURI *uri, const char *filename)
+{
+    char *token, *saveptr;
+    char *p, *r;
+    int ret = -EINVAL;
+
+    p = r = g_strdup(filename);
+    if (strncmp(p, "gluster://", 10)) {
+        goto out;
+    }
+
+    /* Discard the protocol */
+    p += 10;
+
+    /* server */
+    token = strtok_r(p, "/", &saveptr);
+    if (!token) {
+        goto out;
+    }
+
+    ret = parse_server(uri, token);
+    if (ret < 0) {
+        goto out;
+    }
+
+    /* volname */
+    token = strtok_r(NULL, "/", &saveptr);
+    if (!token) {
+        ret = -EINVAL;
+        goto out;
+    }
+    uri->volname = g_strdup(token);
+
+    /* image */
+    token = strtok_r(NULL, "?", &saveptr);
+    if (!token) {
+        ret = -EINVAL;
+        goto out;
+    }
+    uri->image = g_strdup(token);
+
+    /* transport */
+    token = strtok_r(NULL, "?", &saveptr);
+    ret = parse_transport(uri, token);
+    if (ret < 0) {
+        goto out;
+     }
+
+    /* Flag error for extra options */
+    token = strtok_r(NULL, "?", &saveptr);
+    if (token) {
+        ret = -EINVAL;
+        goto out;
+    }
+    ret = 0;
+out:
+    g_free(r);
+    return ret;
+}
+
+static struct glfs *qemu_gluster_init(GlusterURI *uri, const char *filename)
+{
+    struct glfs *glfs = NULL;
+    int ret;
+
+    ret = qemu_gluster_parseuri(uri, filename);
+    if (ret < 0) {
+        error_report("Usage: file=gluster://server[:port]/volname/image"
+            "[?transport=socket]");
+        errno = -ret;
+        goto out;
+    }
+
+    glfs = glfs_new(uri->volname);
+    if (!glfs) {
+        goto out;
+    }
+
+    ret = glfs_set_volfile_server(glfs, uri->transport, uri->server,
+        uri->port);
+    if (ret < 0) {
+        goto out;
+    }
+
+    /*
+     * TODO: Use GF_LOG_ERROR instead of hard code value of 4 here when
+     * GlusterFS exports it in a header.
+     */
+    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", uri->server, uri->port,
+             uri->volname, uri->image, uri->transport);
+        goto out;
+    }
+    return glfs;
+
+out:
+    if (glfs) {
+        glfs_fini(glfs);
+    }
+    return NULL;
+}
+
+static void qemu_gluster_complete_aio(GlusterAIOCB *acb)
+{
+    int ret;
+
+    if (acb->canceled) {
+        qemu_aio_release(acb);
+        return;
+    }
+
+    if (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 */
+    }
+    acb->common.cb(acb->common.opaque, ret);
+    qemu_aio_release(acb);
+}
+
+static void qemu_gluster_aio_event_reader(void *opaque)
+{
+    BDRVGlusterState *s = opaque;
+    GlusterAIOCB *event_acb;
+    int event_reader_pos = 0;
+    ssize_t ret;
+
+    do {
+        char *p = (char *)&event_acb;
+
+        ret = read(s->fds[GLUSTER_FD_READ], p + event_reader_pos,
+                   sizeof(event_acb) - event_reader_pos);
+        if (ret > 0) {
+            event_reader_pos += ret;
+            if (event_reader_pos == sizeof(event_acb)) {
+                event_reader_pos = 0;
+                qemu_gluster_complete_aio(event_acb);
+                s->qemu_aio_count--;
+            }
+        }
+    } 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;
+    GlusterURI *uri = g_malloc0(sizeof(GlusterURI));
+
+    s->glfs = qemu_gluster_init(uri, filename);
+    if (!s->glfs) {
+        ret = -errno;
+        goto out;
+    }
+
+    open_flags |=  O_BINARY;
+    open_flags &= ~O_ACCMODE;
+    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, uri->image, open_flags);
+    if (!s->fd) {

block/gluster.c new file +55 -0

+    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_uri_free(uri);
+    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;
+    GlusterURI *uri = g_malloc0(sizeof(GlusterURI));
+
+    glfs = qemu_gluster_init(uri, 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, uri->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_uri_free(uri);
+    if (glfs) {
+        glfs_fini(glfs);

block/gluster.c new file +20 -0

+}
+
+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) {
+        fd_set wfd;
+        int fd = s->fds[GLUSTER_FD_WRITE];
+
+        ret = write(fd, (void *)&acb, sizeof(acb));
+        if (ret >= 0) {
+            break;
+        }
+        if (errno == EINTR) {
+            continue;

block/gluster.c new file +11 -0

+    }
+    return ret;
+}
+
+static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void *arg)
+{
+    GlusterAIOCB *acb = (GlusterAIOCB *)arg;
+    BDRVGlusterState *s = acb->common.bs->opaque;
+
+    acb->ret = ret;
+    if (qemu_gluster_send_pipe(s, acb) < 0) {

block/gluster.c new file +136 -0

+    }
+}
+
+static BlockDriverAIOCB *qemu_gluster_aio_rw(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque, int write)
+{
+    int ret;
+    GlusterAIOCB *acb;
+    BDRVGlusterState *s = bs->opaque;
+    size_t size;
+    off_t offset;
+
+    offset = sector_num * BDRV_SECTOR_SIZE;
+    size = nb_sectors * BDRV_SECTOR_SIZE;
+    s->qemu_aio_count++;
+
+    acb = qemu_aio_get(&gluster_aio_pool, bs, cb, opaque);
+    acb->size = size;
+    acb->ret = 0;
+    acb->canceled = false;
+
+    if (write) {
+        ret = glfs_pwritev_async(s->fd, qiov->iov, qiov->niov, offset, 0,
+            &gluster_finish_aiocb, acb);
+    } else {
+        ret = glfs_preadv_async(s->fd, qiov->iov, qiov->niov, offset, 0,
+            &gluster_finish_aiocb, acb);
+    }
+
+    if (ret < 0) {
+        goto out;
+    }
+    return &acb->common;
+
+out:
+    s->qemu_aio_count--;
+    qemu_aio_release(acb);
+    return NULL;
+}
+
+static BlockDriverAIOCB *qemu_gluster_aio_readv(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    return qemu_gluster_aio_rw(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
+}
+
+static BlockDriverAIOCB *qemu_gluster_aio_writev(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    return qemu_gluster_aio_rw(bs, sector_num, qiov, nb_sectors, cb, opaque, 1);
+}
+
+static BlockDriverAIOCB *qemu_gluster_aio_flush(BlockDriverState *bs,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    int ret;
+    GlusterAIOCB *acb;
+    BDRVGlusterState *s = bs->opaque;
+
+    acb = qemu_aio_get(&gluster_aio_pool, bs, cb, opaque);
+    acb->size = 0;
+    acb->ret = 0;
+    acb->canceled = false;
+    s->qemu_aio_count++;
+
+    ret = glfs_fsync_async(s->fd, &gluster_finish_aiocb, acb);
+    if (ret < 0) {
+        goto out;
+    }
+    return &acb->common;
+
+out:
+    s->qemu_aio_count--;
+    qemu_aio_release(acb);
+    return NULL;
+}
+
+static int64_t qemu_gluster_getlength(BlockDriverState *bs)
+{
+    BDRVGlusterState *s = bs->opaque;
+    struct stat st;
+    int ret;
+
+    ret = glfs_fstat(s->fd, &st);
+    if (ret < 0) {
+        return -errno;
+    } else {
+        return st.st_size;
+    }
+}
+
+static void qemu_gluster_close(BlockDriverState *bs)
+{
+    BDRVGlusterState *s = bs->opaque;
+
+    if (s->fd) {
+        glfs_close(s->fd);
+        s->fd = NULL;
+    }
+    glfs_fini(s->glfs);
+}
+
+static QEMUOptionParameter qemu_gluster_create_options[] = {
+    {
+        .name = BLOCK_OPT_SIZE,
+        .type = OPT_SIZE,
+        .help = "Virtual disk size"
+    },
+    { NULL }
+};
+
+static BlockDriver bdrv_gluster = {
+    .format_name = "gluster",
+    .protocol_name = "gluster",
+    .instance_size = sizeof(BDRVGlusterState),
+    .bdrv_file_open = qemu_gluster_open,
+    .bdrv_close = qemu_gluster_close,
+    .bdrv_create = qemu_gluster_create,
+    .bdrv_getlength = qemu_gluster_getlength,
+
+    .bdrv_aio_readv = qemu_gluster_aio_readv,
+    .bdrv_aio_writev = qemu_gluster_aio_writev,
+    .bdrv_aio_flush = qemu_gluster_aio_flush,
+
+    .create_options = qemu_gluster_create_options,
+};
+
+static void bdrv_gluster_init(void)
+{
+    bdrv_register(&bdrv_gluster);
+}
+
+block_init(bdrv_gluster_init);

Quoted 33 lines

block/gluster.c new file +33 -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;
+    GlusterURI *uri = g_malloc0(sizeof(GlusterURI));
+
+    s->glfs = qemu_gluster_init(uri, filename);
+    if (!s->glfs) {
+        ret = -errno;
+        goto out;
+    }
+
+    open_flags |=  O_BINARY;
+    open_flags &= ~O_ACCMODE;
+    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, uri->image, open_flags);
+    if (!s->fd) {
+        ret = -errno;
+        goto out;
+    }
+
+    ret = qemu_pipe(s->fds);

block/gluster.c line 371

Quoted 5 lines

block/gluster.c new file +5 -0

+    if (ret < 0) {
+        goto out;
+    }
+    fcntl(s->fds[0], F_SETFL, O_NONBLOCK);
+    fcntl(s->fds[1], F_SETFL, O_NONBLOCK);

A small thing I noticed while reviewing: since the write end of the pipe is used from the gluster thread, you do not need to make this nonblocking.

Also, please use GLUSTER_FD_{READ,WRITE} instead.

block/gluster.c line 436

Quoted 6 lines

block/gluster.c new file +6 -0

+static void qemu_gluster_aio_cancel(BlockDriverAIOCB *blockacb)
+{
+    GlusterAIOCB *acb = (GlusterAIOCB *)blockacb;
+
+    acb->common.cb(acb->common.opaque, -ECANCELED);
+    acb->canceled = true;

I think this is wrong, because the write could still complete later and undo the effects of a second write that is done by the guest. That is:

gluster QEMU guest
----------------------------------------------------------

                                             <---     write #1
                     <---   write #1
                                             <---     cancel write #1
                            write #1 canceled --->
                                             <---     write #2
                     <---   write #2
   write #2 completed --->
                            write #2 completed -->

write #1 completed --->

Now, the persistent storage recorded the effect of write #1, but the guest thinks that it recorded the effect of write #2 instead.

You can simply do qemu_aio_flush() here.

Quoted 17 lines

block/gluster.c new file +17 -0

+static int qemu_gluster_send_pipe(BDRVGlusterState *s, GlusterAIOCB *acb)
+{
+    int ret = 0;
+    while (1) {
+        fd_set wfd;
+        int fd = s->fds[GLUSTER_FD_WRITE];
+
+        ret = write(fd, (void *)&acb, sizeof(acb));
+        if (ret >= 0) {
+            break;
+        }
+        if (errno == EINTR) {
+            continue;
+        }
+        if (errno != EAGAIN) {
+            break;
+        }

block/gluster.c line 466

Quoted 6 lines

block/gluster.c new file +6 -0

+
+        FD_ZERO(&wfd);
+        FD_SET(fd, &wfd);
+        do {
+            ret = select(fd + 1, NULL, &wfd, NULL, NULL);
+        } while (ret < 0 && errno == EINTR);

If you make the fd non-blocking, you can avoid the select here.

Hi,

This is the v6 of the patchset to support GlusterFS backend from QEMU. The changes include defining a default value for glusterfs feature in the configure script and a few minor cleanups suggested by Stefan.

As I mentioned during a previous release, I have tested gluster backend with different gluster configurations like local, remote, distributed and replicated volumes. I believe that the patchset has undergone enough scrutiny now to be included in 1.2 release.