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

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

[Jeff Cody]

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 | 154 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 138 insertions(+), 16 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 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 | 48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index a009b15..79af3fd 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -30,6 +30,8 @@ typedef struct GlusterAIOCB {
 typedef struct BDRVGlusterState {
     struct glfs *glfs;
     struct glfs_fd *fd;
+    int open_flags;
+    char *filename;
 } BDRVGlusterState;
 
 #define GLUSTER_FD_READ  0
@@ -45,11 +47,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 +273,27 @@ 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 ret = 0;
     GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
     QemuOpts *opts;
@@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
 
     filename = qemu_opt_get(opts, "filename");
 
+    s->filename = g_strdup(filename);
+
     s->glfs = qemu_gluster_init(gconf, filename);
     if (!s->glfs) {
         ret = -errno;
         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, &s->open_flags);
 
-    s->fd = glfs_open(s->glfs, gconf->image, open_flags);
+    s->fd = glfs_open(s->glfs, gconf->image, s->open_flags);
     if (!s->fd) {
         ret = -errno;
     }
@@ -324,6 +338,7 @@ out:
     if (s->glfs) {
         glfs_fini(s->glfs);
     }
+    g_free(s->filename);
     return ret;
 }
 
@@ -589,6 +604,7 @@ static void qemu_gluster_close(BlockDriverState *bs)
         s->fd = NULL;
     }
     glfs_fini(s->glfs);
+    g_free(s->filename);
 }
 
 static int qemu_gluster_has_zero_init(BlockDriverState *bs)
-- 
1.8.3.1

[Qemu-devel] [PATCH 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 | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/block/gluster.c b/block/gluster.c
index 79af3fd..f813ac8 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -342,6 +342,100 @@ out:
     return ret;
 }
 
+typedef struct BDRVGlusterReopenState {
+    struct glfs *glfs;
+    struct glfs_fd *fd;
+    int open_flags;
+} BDRVGlusterReopenState;
+
+
+static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
+                                       BlockReopenQueue *queue, Error **errp)
+{
+    int ret = 0;
+    BDRVGlusterState *s;
+    BDRVGlusterReopenState *reop_s;
+    GlusterConf *gconf = NULL;
+
+    assert(state != NULL);
+    assert(state->bs != NULL);
+
+    s = state->bs->opaque;
+
+    state->opaque = g_malloc0(sizeof(BDRVGlusterReopenState));
+    reop_s = state->opaque;
+
+    qemu_gluster_parse_flags(state->flags, &reop_s->open_flags);
+
+    gconf = g_malloc0(sizeof(GlusterConf));
+
+    reop_s->glfs = qemu_gluster_init(gconf, s->filename);
+    if (reop_s->glfs == NULL) {
+        ret = -errno;
+        goto exit;
+    }
+
+    reop_s->fd = glfs_open(reop_s->glfs, gconf->image, reop_s->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;
+    s->open_flags = reop_s->open_flags;
+
+    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)
@@ -633,6 +727,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,
@@ -657,6 +754,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,
@@ -681,6 +781,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,
@@ -705,6 +808,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 1/2] block: gluster - code movements, state storage changes

[Benoît Canet]

Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit :
> 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 | 48 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index a009b15..79af3fd 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -30,6 +30,8 @@ typedef struct GlusterAIOCB {
>  typedef struct BDRVGlusterState {
>      struct glfs *glfs;
>      struct glfs_fd *fd;
> +    int open_flags;
> +    char *filename;
>  } BDRVGlusterState;
>  
>  #define GLUSTER_FD_READ  0
> @@ -45,11 +47,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 +273,27 @@ 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;
> +    }
> +}

I saw the enable-O_SYNC option here.
http://www.gluster.org/community/documentation/index.php/Translators/performance
Why the gluster driver does not allow to enable O_SYNC ?

> +
>  static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>                               int bdrv_flags, Error **errp)
>  {
>      BDRVGlusterState *s = bs->opaque;
> -    int open_flags = O_BINARY;
>      int ret = 0;
>      GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
>      QemuOpts *opts;
> @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>  
>      filename = qemu_opt_get(opts, "filename");
>  
> +    s->filename = g_strdup(filename);
> +
>      s->glfs = qemu_gluster_init(gconf, filename);
>      if (!s->glfs) {
>          ret = -errno;
>          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, &s->open_flags);
>  
> -    s->fd = glfs_open(s->glfs, gconf->image, open_flags);
> +    s->fd = glfs_open(s->glfs, gconf->image, s->open_flags);
>      if (!s->fd) {
>          ret = -errno;
>      }
> @@ -324,6 +338,7 @@ out:
>      if (s->glfs) {
>          glfs_fini(s->glfs);
>      }
> +    g_free(s->filename);
>      return ret;
>  }
>  
> @@ -589,6 +604,7 @@ static void qemu_gluster_close(BlockDriverState *bs)
>          s->fd = NULL;
>      }
>      glfs_fini(s->glfs);
> +    g_free(s->filename);
>  }
>  
>  static int qemu_gluster_has_zero_init(BlockDriverState *bs)
> -- 
> 1.8.3.1
> 
> 

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

[Bharata B Rao]

On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote:
> Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit :
> >  
> > +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;
> > +    }
> > +}
> 
> I saw the enable-O_SYNC option here.
> http://www.gluster.org/community/documentation/index.php/Translators/performance
> Why the gluster driver does not allow to enable O_SYNC ?

I am not aware of any option in QEMU (like cache= etc) that will force
block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ?

Turning off write-behind for the entire gluster volume isn't an option ?

Regards,
Bharata.

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

[Benoît Canet]

Le Friday 07 Feb 2014 à 09:14:50 (+0530), Bharata B Rao a écrit :
> On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote:
> > Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit :
> > >  
> > > +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;
> > > +    }
> > > +}
> > 
> > I saw the enable-O_SYNC option here.
> > http://www.gluster.org/community/documentation/index.php/Translators/performance
> > Why the gluster driver does not allow to enable O_SYNC ?
> 
> I am not aware of any option in QEMU (like cache= etc) that will force
> block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ?

[,cache=writethrough|writeback|none|directsync|unsafe][

I think writethough is O_SYNC and directsync is O_DIRECT|O_SYNC.

Best regards

Benoît

> 
> Turning off write-behind for the entire gluster volume isn't an option ?
> 
> Regards,
> Bharata.
> 

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

[Bharata B Rao]

On Fri, Feb 07, 2014 at 03:22:29PM +0100, Benoît Canet wrote:
> Le Friday 07 Feb 2014 à 09:14:50 (+0530), Bharata B Rao a écrit :
> > On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote:
> > > Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit :
> > > >  
> > > > +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;
> > > > +    }
> > > > +}
> > > 
> > > I saw the enable-O_SYNC option here.
> > > http://www.gluster.org/community/documentation/index.php/Translators/performance
> > > Why the gluster driver does not allow to enable O_SYNC ?
> > 
> > I am not aware of any option in QEMU (like cache= etc) that will force
> > block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ?
> 
> [,cache=writethrough|writeback|none|directsync|unsafe][
> 
> I think writethough is O_SYNC and directsync is O_DIRECT|O_SYNC.

May be I am missing something, but I checked the flags with which
raw protocol driver opens the file image for different cache options
and this is what I found by looking at open flags in util/osdep.c:qemu_open()

(Ignoring O_CLOEXEC which is common for all the cases)

writethrough	02	O_RDWR
writeback	02	O_RDWR	
none		040002	O_DIRECT|O_RDWR
directsync	040002	O_DIRECT|O_RDWR
unsafe		02	O_RDWR

I do see the below comment in block/raw-posix.c:raw_parse_flags()

    /* Use O_DSYNC for write-through caching, no flags for write-back caching,
     * and O_DIRECT for no caching. */
    if ((bdrv_flags & BDRV_O_NOCACHE)) {
        *open_flags |= O_DIRECT;
    }

But I don't see the driver actually setting O_DSYNC anywhere.

Regards,
Bharata.

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

[Jeff Cody]

On Fri, Feb 07, 2014 at 08:57:35PM +0530, Bharata B Rao wrote:
> On Fri, Feb 07, 2014 at 03:22:29PM +0100, Benoît Canet wrote:
> > Le Friday 07 Feb 2014 à 09:14:50 (+0530), Bharata B Rao a écrit :
> > > On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote:
> > > > Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit :
> > > > >  
> > > > > +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;
> > > > > +    }
> > > > > +}
> > > > 
> > > > I saw the enable-O_SYNC option here.
> > > > http://www.gluster.org/community/documentation/index.php/Translators/performance
> > > > Why the gluster driver does not allow to enable O_SYNC ?
> > > 
> > > I am not aware of any option in QEMU (like cache= etc) that will force
> > > block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ?
> > 
> > [,cache=writethrough|writeback|none|directsync|unsafe][
> > 
> > I think writethough is O_SYNC and directsync is O_DIRECT|O_SYNC.
> 
> May be I am missing something, but I checked the flags with which
> raw protocol driver opens the file image for different cache options
> and this is what I found by looking at open flags in util/osdep.c:qemu_open()
> 
> (Ignoring O_CLOEXEC which is common for all the cases)
> 
> writethrough	02	O_RDWR
> writeback	02	O_RDWR	
> none		040002	O_DIRECT|O_RDWR
> directsync	040002	O_DIRECT|O_RDWR
> unsafe		02	O_RDWR
> 
> I do see the below comment in block/raw-posix.c:raw_parse_flags()
> 
>     /* Use O_DSYNC for write-through caching, no flags for write-back caching,
>      * and O_DIRECT for no caching. */
>     if ((bdrv_flags & BDRV_O_NOCACHE)) {
>         *open_flags |= O_DIRECT;
>     }
> 
> But I don't see the driver actually setting O_DSYNC anywhere.
>

You are not mistaken.  For qemu writethrough cache (the default), it
is not via O_SYNC/DSYNC directly in the protocol drivers, but
essentially done inside block.c (e.g., bdrv_flush()).

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

[Benoît Canet]

Le Friday 07 Feb 2014 à 20:57:35 (+0530), Bharata B Rao a écrit :
> On Fri, Feb 07, 2014 at 03:22:29PM +0100, Benoît Canet wrote:
> > Le Friday 07 Feb 2014 à 09:14:50 (+0530), Bharata B Rao a écrit :
> > > On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote:
> > > > Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit :
> > > > >  
> > > > > +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;
> > > > > +    }
> > > > > +}
> > > > 
> > > > I saw the enable-O_SYNC option here.
> > > > http://www.gluster.org/community/documentation/index.php/Translators/performance
> > > > Why the gluster driver does not allow to enable O_SYNC ?
> > > 
> > > I am not aware of any option in QEMU (like cache= etc) that will force
> > > block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ?
> > 
> > [,cache=writethrough|writeback|none|directsync|unsafe][
> > 
> > I think writethough is O_SYNC and directsync is O_DIRECT|O_SYNC.
> 
> May be I am missing something, but I checked the flags with which
> raw protocol driver opens the file image for different cache options
> and this is what I found by looking at open flags in util/osdep.c:qemu_open()
> 
> (Ignoring O_CLOEXEC which is common for all the cases)
> 
> writethrough	02	O_RDWR
> writeback	02	O_RDWR	
> none		040002	O_DIRECT|O_RDWR
> directsync	040002	O_DIRECT|O_RDWR
> unsafe		02	O_RDWR
> 
> I do see the below comment in block/raw-posix.c:raw_parse_flags()
> 
>     /* Use O_DSYNC for write-through caching, no flags for write-back caching,
>      * and O_DIRECT for no caching. */
>     if ((bdrv_flags & BDRV_O_NOCACHE)) {
>         *open_flags |= O_DIRECT;
>     }
> 
> But I don't see the driver actually setting O_DSYNC anywhere.

True, Maybe it has changed over time.

Best regards

Benoît

> 
> Regards,
> Bharata.
> 
> 

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

[Benoît Canet]

Le Friday 07 Feb 2014 à 10:57:33 (-0500), Jeff Cody a écrit :
> On Fri, Feb 07, 2014 at 08:57:35PM +0530, Bharata B Rao wrote:
> > On Fri, Feb 07, 2014 at 03:22:29PM +0100, Benoît Canet wrote:
> > > Le Friday 07 Feb 2014 à 09:14:50 (+0530), Bharata B Rao a écrit :
> > > > On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote:
> > > > > Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit :
> > > > > >  
> > > > > > +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;
> > > > > > +    }
> > > > > > +}
> > > > > 
> > > > > I saw the enable-O_SYNC option here.
> > > > > http://www.gluster.org/community/documentation/index.php/Translators/performance
> > > > > Why the gluster driver does not allow to enable O_SYNC ?
> > > > 
> > > > I am not aware of any option in QEMU (like cache= etc) that will force
> > > > block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ?
> > > 
> > > [,cache=writethrough|writeback|none|directsync|unsafe][
> > > 
> > > I think writethough is O_SYNC and directsync is O_DIRECT|O_SYNC.
> > 
> > May be I am missing something, but I checked the flags with which
> > raw protocol driver opens the file image for different cache options
> > and this is what I found by looking at open flags in util/osdep.c:qemu_open()
> > 
> > (Ignoring O_CLOEXEC which is common for all the cases)
> > 
> > writethrough	02	O_RDWR
> > writeback	02	O_RDWR	
> > none		040002	O_DIRECT|O_RDWR
> > directsync	040002	O_DIRECT|O_RDWR
> > unsafe		02	O_RDWR
> > 
> > I do see the below comment in block/raw-posix.c:raw_parse_flags()
> > 
> >     /* Use O_DSYNC for write-through caching, no flags for write-back caching,
> >      * and O_DIRECT for no caching. */
> >     if ((bdrv_flags & BDRV_O_NOCACHE)) {
> >         *open_flags |= O_DIRECT;
> >     }
> > 
> > But I don't see the driver actually setting O_DSYNC anywhere.
> >
> 
> You are not mistaken.  For qemu writethrough cache (the default), it
> is not via O_SYNC/DSYNC directly in the protocol drivers, but
> essentially done inside block.c (e.g., bdrv_flush()).
> 
> 

Hmm sorry for spreading fud. It was not intentional.

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

[Stefan Hajnoczi]

On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote:
> diff --git a/block/gluster.c b/block/gluster.c
> index a009b15..79af3fd 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -30,6 +30,8 @@ typedef struct GlusterAIOCB {
>  typedef struct BDRVGlusterState {
>      struct glfs *glfs;
>      struct glfs_fd *fd;
> +    int open_flags;

Is this field used?  I only see stores to this field but no loads.

Seems unnecessary since the block layer already provides us with QEMU
BDRV_* flags and qemu_gluster_parse_flags() can be used to produce POSIX
open(2) flags from them.

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

[Stefan Hajnoczi]

On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote:
> @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>  
>      filename = qemu_opt_get(opts, "filename");
>  
> +    s->filename = g_strdup(filename);

It's not obvious to me that copying the filename is necessary.
block/raw-posix.c does this:

  raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags);

Why didn't you use bs->filename?

Stefan

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

[Jeff Cody]

On Fri, Feb 14, 2014 at 03:21:28PM +0100, Stefan Hajnoczi wrote:
> On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote:
> > @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
> >  
> >      filename = qemu_opt_get(opts, "filename");
> >  
> > +    s->filename = g_strdup(filename);
> 
> It's not obvious to me that copying the filename is necessary.
> block/raw-posix.c does this:
> 
>   raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags);
> 
> Why didn't you use bs->filename?
>

Back when the raw-posix reopen was implemented, the .bdrv_file_open
used the char * filename instead of options parameters.  Now that the
filename is explicitly parsed via the options, I thought it made
logical sense to cache the filename into the protocol state variable,
for later use by reopen.

That way if the bs->filename was changed in some manner, the protocol
would have the variable as originally intended to be passed to the
.bdrv_file_open() method.

In reality, I should be able to just use bs->filename instead, as it
does not get modified; this just seemed to make a bit more sense.

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

[Jeff Cody]

On Fri, Feb 14, 2014 at 03:12:41PM +0100, Stefan Hajnoczi wrote:
> On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote:
> > diff --git a/block/gluster.c b/block/gluster.c
> > index a009b15..79af3fd 100644
> > --- a/block/gluster.c
> > +++ b/block/gluster.c
> > @@ -30,6 +30,8 @@ typedef struct GlusterAIOCB {
> >  typedef struct BDRVGlusterState {
> >      struct glfs *glfs;
> >      struct glfs_fd *fd;
> > +    int open_flags;
> 
> Is this field used?  I only see stores to this field but no loads.
> 
> Seems unnecessary since the block layer already provides us with QEMU
> BDRV_* flags and qemu_gluster_parse_flags() can be used to produce POSIX
> open(2) flags from them.

It is not currently used, I cached it for later use, but never used
it.  I will purge it in a v2 (same thing with the sister variable in
the reopen state struct in patch 2).

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

[Stefan Hajnoczi]

On Fri, Feb 14, 2014 at 09:41:46AM -0500, Jeff Cody wrote:
> On Fri, Feb 14, 2014 at 03:21:28PM +0100, Stefan Hajnoczi wrote:
> > On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote:
> > > @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
> > >  
> > >      filename = qemu_opt_get(opts, "filename");
> > >  
> > > +    s->filename = g_strdup(filename);
> > 
> > It's not obvious to me that copying the filename is necessary.
> > block/raw-posix.c does this:
> > 
> >   raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags);
> > 
> > Why didn't you use bs->filename?
> >
> 
> Back when the raw-posix reopen was implemented, the .bdrv_file_open
> used the char * filename instead of options parameters.  Now that the
> filename is explicitly parsed via the options, I thought it made
> logical sense to cache the filename into the protocol state variable,
> for later use by reopen.
> 
> That way if the bs->filename was changed in some manner, the protocol
> would have the variable as originally intended to be passed to the
> .bdrv_file_open() method.
> 
> In reality, I should be able to just use bs->filename instead, as it
> does not get modified; this just seemed to make a bit more sense.

I thought about driver-specific options that are not part of 'filename',
too.  IMO it's simpler to use bs->filename until we actually use
driver-specific options, and then switch over to using bs->options
instead of copying.

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

[Jeff Cody]

On Fri, Feb 14, 2014 at 04:38:03PM +0100, Stefan Hajnoczi wrote:
> On Fri, Feb 14, 2014 at 09:41:46AM -0500, Jeff Cody wrote:
> > On Fri, Feb 14, 2014 at 03:21:28PM +0100, Stefan Hajnoczi wrote:
> > > On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote:
> > > > @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
> > > >  
> > > >      filename = qemu_opt_get(opts, "filename");
> > > >  
> > > > +    s->filename = g_strdup(filename);
> > > 
> > > It's not obvious to me that copying the filename is necessary.
> > > block/raw-posix.c does this:
> > > 
> > >   raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags);
> > > 
> > > Why didn't you use bs->filename?
> > >
> > 
> > Back when the raw-posix reopen was implemented, the .bdrv_file_open
> > used the char * filename instead of options parameters.  Now that the
> > filename is explicitly parsed via the options, I thought it made
> > logical sense to cache the filename into the protocol state variable,
> > for later use by reopen.
> > 
> > That way if the bs->filename was changed in some manner, the protocol
> > would have the variable as originally intended to be passed to the
> > .bdrv_file_open() method.
> > 
> > In reality, I should be able to just use bs->filename instead, as it
> > does not get modified; this just seemed to make a bit more sense.
> 
> I thought about driver-specific options that are not part of 'filename',
> too.  IMO it's simpler to use bs->filename until we actually use
> driver-specific options, and then switch over to using bs->options
> instead of copying.
>

OK, I'll make that change for v2 as well.

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

[Kevin Wolf]

Am 14.02.2014 um 15:41 hat Jeff Cody geschrieben:
> On Fri, Feb 14, 2014 at 03:21:28PM +0100, Stefan Hajnoczi wrote:
> > On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote:
> > > @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
> > >  
> > >      filename = qemu_opt_get(opts, "filename");
> > >  
> > > +    s->filename = g_strdup(filename);
> > 
> > It's not obvious to me that copying the filename is necessary.
> > block/raw-posix.c does this:
> > 
> >   raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags);
> > 
> > Why didn't you use bs->filename?
> 
> Back when the raw-posix reopen was implemented, the .bdrv_file_open
> used the char * filename instead of options parameters.  Now that the
> filename is explicitly parsed via the options, I thought it made
> logical sense to cache the filename into the protocol state variable,
> for later use by reopen.
> 
> That way if the bs->filename was changed in some manner, the protocol
> would have the variable as originally intended to be passed to the
> .bdrv_file_open() method.
> 
> In reality, I should be able to just use bs->filename instead, as it
> does not get modified; this just seemed to make a bit more sense.

FWIW, I think we want bs->filename to go away eventually and get it
replaced by a callback (or perhaps two, a short summary one for humans
and a complete one) so that the callers work with setups that are driven
by options and don't have a filename.

Kevin