[Qemu-devel] [PATCH v2 0/2] block: add suppoort for gluster reopen

[Qemu-devel] [PATCH v2 0/2] block: add suppoort for gluster reopen

[Jeff Cody]

Changes v1->v2:

    Patch 1: Removed unneeded state variables 'filename' and
             'open_flags' (Stefan)
    Patch 2: Removed unneeded reopen state variable 'open_flags'
             (Stefan)

This series provides support for bdrv_reopen() with gluster
protocol drivers, and thereby also enabling block-commit to
gluster-based images.

Jeff Cody (2):
  block: gluster - code movements, state storage changes
  block: gluster - add reopen support.

 block/gluster.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 128 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/2] block: gluster - code movements, state storage changes

[Jeff Cody]

In preparation for supporting reopen on gluster, move flag
parsing out to a function.  Also, store open_flags and filename
in the gluster state storage struct, and add a NULL check in the
gconf cleanup.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/gluster.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index a009b15..bcc9b89 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -45,11 +45,13 @@ typedef struct GlusterConf {
 
 static void qemu_gluster_gconf_free(GlusterConf *gconf)
 {
-    g_free(gconf->server);
-    g_free(gconf->volname);
-    g_free(gconf->image);
-    g_free(gconf->transport);
-    g_free(gconf);
+    if (gconf) {
+        g_free(gconf->server);
+        g_free(gconf->volname);
+        g_free(gconf->image);
+        g_free(gconf->transport);
+        g_free(gconf);
+    }
 }
 
 static int parse_volume_options(GlusterConf *gconf, char *path)
@@ -269,11 +271,28 @@ static QemuOptsList runtime_opts = {
     },
 };
 
+static void qemu_gluster_parse_flags(int bdrv_flags, int *open_flags)
+{
+    assert(open_flags != NULL);
+
+    *open_flags |= O_BINARY;
+
+    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;
+    }
+}
+
 static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
                              int bdrv_flags, Error **errp)
 {
     BDRVGlusterState *s = bs->opaque;
-    int open_flags = O_BINARY;
+    int open_flags = 0;
     int ret = 0;
     GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
     QemuOpts *opts;
@@ -297,15 +316,7 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
         goto out;
     }
 
-    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;
-    }
+    qemu_gluster_parse_flags(bdrv_flags, &open_flags);
 
     s->fd = glfs_open(s->glfs, gconf->image, open_flags);
     if (!s->fd) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 1/2] block: gluster - code movements, state storage changes

[Stefan Hajnoczi]

On Mon, Feb 17, 2014 at 11:11:11AM -0500, Jeff Cody wrote:
> In preparation for supporting reopen on gluster, move flag
> parsing out to a function.  Also, store open_flags and filename
> in the gluster state storage struct, and add a NULL check in the
> gconf cleanup.

It no longer stores open_flags and filename in the gluster state storage
struct.  I can fix this up when merging.

Re: [Qemu-devel] [PATCH v2 1/2] block: gluster - code movements, state storage changes

[Jeff Cody]

On Wed, Feb 26, 2014 at 03:58:40PM +0100, Stefan Hajnoczi wrote:
> On Mon, Feb 17, 2014 at 11:11:11AM -0500, Jeff Cody wrote:
> > In preparation for supporting reopen on gluster, move flag
> > parsing out to a function.  Also, store open_flags and filename
> > in the gluster state storage struct, and add a NULL check in the
> > gconf cleanup.
> 
> It no longer stores open_flags and filename in the gluster state storage
> struct.  I can fix this up when merging.

Ah, yes.  Thanks.

[Qemu-devel] [PATCH v2 2/2] block: gluster - add reopen support.

[Jeff Cody]

Gluster does parse open flags in its .bdrv_open() implementation,
and the .bdrv_reopen_* implementations need to do the same.

A new gluster connection to the image file to be created is established
in the .bdrv_reopen_prepare(), and the image file opened with the new
flags.

If this is successful, then the old image file is closed, and the
old connection torn down. The relevant structure pointers in the gluster
state structure are updated to the new connection.

If it is not successful, then the new file handle and connection is
abandoned (if it exists), while the old connection is not modified at
all.

With reopen supported, block-commit (and offline commit) is now also
supported for image files whose base image uses the native gluster
protocol driver.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/gluster.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/block/gluster.c b/block/gluster.c
index bcc9b89..f4756b8 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -338,6 +338,96 @@ out:
     return ret;
 }
 
+typedef struct BDRVGlusterReopenState {
+    struct glfs *glfs;
+    struct glfs_fd *fd;
+} BDRVGlusterReopenState;
+
+
+static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
+                                       BlockReopenQueue *queue, Error **errp)
+{
+    int ret = 0;
+    BDRVGlusterReopenState *reop_s;
+    GlusterConf *gconf = NULL;
+    int open_flags = 0;
+
+    assert(state != NULL);
+    assert(state->bs != NULL);
+
+    state->opaque = g_malloc0(sizeof(BDRVGlusterReopenState));
+    reop_s = state->opaque;
+
+    qemu_gluster_parse_flags(state->flags, &open_flags);
+
+    gconf = g_malloc0(sizeof(GlusterConf));
+
+    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename);
+    if (reop_s->glfs == NULL) {
+        ret = -errno;
+        goto exit;
+    }
+
+    reop_s->fd = glfs_open(reop_s->glfs, gconf->image, open_flags);
+    if (reop_s->fd == NULL) {
+        /* reops->glfs will be cleaned up in _abort */
+        ret = -errno;
+        goto exit;
+    }
+
+exit:
+    /* state->opaque will be freed in either the _abort or _commit */
+    qemu_gluster_gconf_free(gconf);
+    return ret;
+}
+
+static void qemu_gluster_reopen_commit(BDRVReopenState *state)
+{
+    BDRVGlusterReopenState *reop_s = state->opaque;
+    BDRVGlusterState *s = state->bs->opaque;
+
+
+    /* close the old */
+    if (s->fd) {
+        glfs_close(s->fd);
+    }
+    if (s->glfs) {
+        glfs_fini(s->glfs);
+    }
+
+    /* use the newly opened image / connection */
+    s->fd         = reop_s->fd;
+    s->glfs       = reop_s->glfs;
+
+    g_free(state->opaque);
+    state->opaque = NULL;
+
+    return;
+}
+
+
+static void qemu_gluster_reopen_abort(BDRVReopenState *state)
+{
+    BDRVGlusterReopenState *reop_s = state->opaque;
+
+    if (reop_s == NULL) {
+        return;
+    }
+
+    if (reop_s->fd) {
+        glfs_close(reop_s->fd);
+    }
+
+    if (reop_s->glfs) {
+        glfs_fini(reop_s->glfs);
+    }
+
+    g_free(state->opaque);
+    state->opaque = NULL;
+
+    return;
+}
+
 #ifdef CONFIG_GLUSTERFS_ZEROFILL
 static coroutine_fn int qemu_gluster_co_write_zeroes(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
@@ -628,6 +718,9 @@ static BlockDriver bdrv_gluster = {
     .instance_size                = sizeof(BDRVGlusterState),
     .bdrv_needs_filename          = true,
     .bdrv_file_open               = qemu_gluster_open,
+    .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
+    .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
+    .bdrv_reopen_abort            = qemu_gluster_reopen_abort,
     .bdrv_close                   = qemu_gluster_close,
     .bdrv_create                  = qemu_gluster_create,
     .bdrv_getlength               = qemu_gluster_getlength,
@@ -652,6 +745,9 @@ static BlockDriver bdrv_gluster_tcp = {
     .instance_size                = sizeof(BDRVGlusterState),
     .bdrv_needs_filename          = true,
     .bdrv_file_open               = qemu_gluster_open,
+    .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
+    .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
+    .bdrv_reopen_abort            = qemu_gluster_reopen_abort,
     .bdrv_close                   = qemu_gluster_close,
     .bdrv_create                  = qemu_gluster_create,
     .bdrv_getlength               = qemu_gluster_getlength,
@@ -676,6 +772,9 @@ static BlockDriver bdrv_gluster_unix = {
     .instance_size                = sizeof(BDRVGlusterState),
     .bdrv_needs_filename          = true,
     .bdrv_file_open               = qemu_gluster_open,
+    .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
+    .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
+    .bdrv_reopen_abort            = qemu_gluster_reopen_abort,
     .bdrv_close                   = qemu_gluster_close,
     .bdrv_create                  = qemu_gluster_create,
     .bdrv_getlength               = qemu_gluster_getlength,
@@ -700,6 +799,9 @@ static BlockDriver bdrv_gluster_rdma = {
     .instance_size                = sizeof(BDRVGlusterState),
     .bdrv_needs_filename          = true,
     .bdrv_file_open               = qemu_gluster_open,
+    .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
+    .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
+    .bdrv_reopen_abort            = qemu_gluster_reopen_abort,
     .bdrv_close                   = qemu_gluster_close,
     .bdrv_create                  = qemu_gluster_create,
     .bdrv_getlength               = qemu_gluster_getlength,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 2/2] block: gluster - add reopen support.

[Stefan Hajnoczi]

On Mon, Feb 17, 2014 at 11:11:12AM -0500, Jeff Cody wrote:
> +static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
> +                                       BlockReopenQueue *queue, Error **errp)
> +{
> +    int ret = 0;
> +    BDRVGlusterReopenState *reop_s;
> +    GlusterConf *gconf = NULL;
> +    int open_flags = 0;
> +
> +    assert(state != NULL);
> +    assert(state->bs != NULL);
> +
> +    state->opaque = g_malloc0(sizeof(BDRVGlusterReopenState));

Not worth respinning but if you use the type name, then the more concise
notation is:

state->opaque = g_new0(BDRVGlusterReopenState, 1);

Re: [Qemu-devel] [PATCH v2 0/2] block: add suppoort for gluster reopen

[Stefan Hajnoczi]

On Mon, Feb 17, 2014 at 11:11:10AM -0500, Jeff Cody wrote:
> Changes v1->v2:
> 
>     Patch 1: Removed unneeded state variables 'filename' and
>              'open_flags' (Stefan)
>     Patch 2: Removed unneeded reopen state variable 'open_flags'
>              (Stefan)
> 
> This series provides support for bdrv_reopen() with gluster
> protocol drivers, and thereby also enabling block-commit to
> gluster-based images.
> 
> Jeff Cody (2):
>   block: gluster - code movements, state storage changes
>   block: gluster - add reopen support.
> 
>  block/gluster.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 128 insertions(+), 15 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan