diff mbox

[1/3] block/gluster: add support for multiple gluster servers

Message ID 1445256783-28558-1-git-send-email-prasanna.kalever@redhat.com
State New
Headers show

Commit Message

Prasanna Kumar Kalever Oct. 19, 2015, 12:13 p.m. UTC 
This patch adds a way to specify multiple volfile servers to the gluster
block backend of QEMU with tcp|rdma transport types and their port numbers.

Problem:

Currenly VM Image on gluster volume is specified like this:

file=gluster[+tcp]://host[:port]/testvol/a.img

Assuming we have three hosts in trustred pool with replica 3 volume
in action and unfortunately host (mentioned in the command above) went down
for some reason, since the volume is replica 3 we now have other 2 hosts
active from which we can boot the VM.

But currently there is no mechanism to pass the other 2 gluster host
addresses to qemu.

Solution:

New way of specifying VM Image on gluster volume with volfile servers:
(We still support old syntax to maintain backward compatibility)

Basic command line syntax looks like:

Pattern I:
 -drive driver=gluster,
        volume=testvol,path=/path/a.raw,
        servers.0.host=1.2.3.4,
       [servers.0.port=24007,]
       [servers.0.transport=tcp,]
        servers.1.host=5.6.7.8,
       [servers.1.port=24008,]
       [servers.1.transport=rdma,] ...

Pattern II:
 'json:{"driver":"qcow2","file":{"driver":"gluster",
       "volume":"testvol","path":"/path/a.qcow2",
       "servers":[{tuple0},{tuple1}, ...{tupleN}]}}'

   driver      => 'gluster' (protocol name)
   volume      => name of gluster volume where our VM image resides
   path        => absolute path of image in gluster volume

  {tuple}      => {"host":"1.2.3.4"[,"port":"24007","transport":"tcp"]}

   host        => host address (hostname/ipv4/ipv6 addresses)
   port        => port number on which glusterd is listening. (default 24007)
   transport   => transport type used to connect to gluster management daemon,
                   it can be tcp|rdma (default 'tcp')

Examples:
1.
 -drive driver=qcow2,file.driver=gluster,
        file.volume=testvol,file.path=/path/a.qcow2,
        file.servers.0.host=1.2.3.4,
        file.servers.0.port=24007,
        file.servers.0.transport=tcp,
        file.servers.1.host=5.6.7.8,
        file.servers.1.port=24008,
        file.servers.1.transport=rdma
2.
 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
         "path":"/path/a.qcow2","servers":
         [{"host":"1.2.3.4","port":"24007","transport":"tcp"},
          {"host":"4.5.6.7","port":"24008","transport":"rdma"}] } }'

This patch gives a mechanism to provide all the server addresses, which are in
replica set, so in case host1 is down VM can still boot from any of the
active hosts.

This is equivalent to the backup-volfile-servers option supported by
mount.glusterfs (FUSE way of mounting gluster volume)

This patch depends on a recent fix in libgfapi raised as part of this work:
http://review.gluster.org/#/c/12114/

Credits: Sincere thanks to Kevin Wolf <kwolf@redhat.com> and
"Deepak C Shetty" <deepakcs@redhat.com> for inputs and all their support

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
---
This series of patches are sent based on "Peter Krempa" <pkrempa@redhat.com>
review comments on v8 sent earlier.
---
 block/gluster.c      | 414 +++++++++++++++++++++++++++++++++++++++++++++------
 qapi/block-core.json |  60 +++++++-
 2 files changed, 426 insertions(+), 48 deletions(-)

Comments

Eric Blake Oct. 19, 2015, 8:08 p.m. UTC | #1 
On 10/19/2015 06:13 AM, Prasanna Kumar Kalever wrote:
> This patch adds a way to specify multiple volfile servers to the gluster
> block backend of QEMU with tcp|rdma transport types and their port numbers.

When sending a multi-patch series, it is best to also include a 0/3
cover letter.  Git can be configured to do this automatically with:
git config format.coverLetter auto

> 
> Problem:
> 
> Currenly VM Image on gluster volume is specified like this:
> 
> file=gluster[+tcp]://host[:port]/testvol/a.img
> 
> Assuming we have three hosts in trustred pool with replica 3 volume

s/trustred/trusted/ ?

> in action and unfortunately host (mentioned in the command above) went down
> for some reason, since the volume is replica 3 we now have other 2 hosts
> active from which we can boot the VM.
> 
> ---
> This series of patches are sent based on "Peter Krempa" <pkrempa@redhat.com>
> review comments on v8 sent earlier.

Even though that was a single patch and you now have a series, it is
still probably worth including v9 in the subject line (git send-email
-v9) to make it obvious how to find the earlier review.


> +++ b/block/gluster.c
> @@ -11,6 +11,16 @@
>  #include "block/block_int.h"
>  #include "qemu/uri.h"
>  
> +#define GLUSTER_OPT_FILENAME       "filename"
> +#define GLUSTER_OPT_VOLUME         "volume"
> +#define GLUSTER_OPT_PATH           "path"
> +#define GLUSTER_OPT_HOST           "host"
> +#define GLUSTER_OPT_PORT           "port"
> +#define GLUSTER_OPT_TRANSPORT      "transport"
> +#define GLUSTER_OPT_SERVER_PATTERN "servers."

Should the macro name be GLUSTER_OPT_SERVERS_PATTERN?

> +
> +#define GLUSTER_DEFAULT_PORT       24007
> +
>  typedef struct GlusterAIOCB {
>      int64_t size;
>      int ret;
> @@ -24,22 +34,72 @@ typedef struct BDRVGlusterState {
>      struct glfs_fd *fd;
>  } BDRVGlusterState;
>  
> -typedef struct GlusterConf {
> +typedef struct GlusterServerConf {
>      char *server;
>      int port;
> +    char *transport;

How do you know how many transport tuples are present? I'd expect a size
argument somewhere.

> +} GlusterServerConf;

It looks like 2/3 fixes confusing naming in preparation for this patch,
which means your series should probably be reordered to do the trivial
code cleanups and renames first, and then the new feature last.

> +
> +typedef struct GlusterConf {
>      char *volname;
>      char *image;
> -    char *transport;
> +    GlusterServerConf *gsconf;
>  } GlusterConf;
>  
> +static QemuOptsList runtime_json_opts = {
> +    .name = "gluster_json",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
> +    .desc = {
> +        {
> +            .name = GLUSTER_OPT_VOLUME,
> +            .type = QEMU_OPT_STRING,
> +            .help = "name of gluster volume where our VM image resides",

s/our //

> +        },
> +        {
> +            .name = GLUSTER_OPT_PATH,
> +            .type = QEMU_OPT_STRING,
> +            .help = "absolute path to image file in gluster volume",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +static QemuOptsList runtime_tuple_opts = {
> +    .name = "gluster_tuple",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_tuple_opts.head),
> +    .desc = {
> +        {
> +            .name = GLUSTER_OPT_HOST,
> +            .type = QEMU_OPT_STRING,
> +            .help = "host address (hostname/ipv4/ipv6 addresses)",
> +        },
> +        {
> +            .name = GLUSTER_OPT_PORT,
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "port number on which glusterd is listening(default 24007)",

Space before ( in English.

> +        },
> +        {
> +            .name = GLUSTER_OPT_TRANSPORT,
> +            .type = QEMU_OPT_STRING,
> +            .help = "transport type used to connect to glusterd(default tcp)",

And again.  This should not be an open-coded string, but a finite set of
enum values, so you may want to list all the valid possibilities rather
than just the default of tcp.

/me reads ahead

Aha, the only other valid value is rdma.

> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  static void qemu_gluster_gconf_free(GlusterConf *gconf)
>  {
>      if (gconf) {
> -        g_free(gconf->server);
>          g_free(gconf->volname);
>          g_free(gconf->image);
> -        g_free(gconf->transport);
> +        if (gconf->gsconf) {
> +            g_free(gconf->gsconf[0].server);
> +            g_free(gconf->gsconf[0].transport);
> +            g_free(gconf->gsconf);
> +            gconf->gsconf = NULL;

Dead assignment, given that...

> +        }
>          g_free(gconf);

...you are just about to free gconf itself.

> +        gconf = NULL;

Dead assignment, given that...

>      }
>  }

...gconf is about to go out of scope.


> @@ -117,16 +178,19 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> return -EINVAL;
> }
> 
> + gconf = g_new0(GlusterConf, 1);
> + gconf->gsconf = g_new0(GlusterServerConf, 1);

Wow - you are hard-coding things to exactly one server.  The subject
line of the patch claims multiple gluster servers, but I don't see
anything that supports more than one.  So something needs to be fixed up
(if this patch is just renaming things, and a later patch adds support
for more than one, that's okay - but it needs to be described that way).

> +static int qemu_gluster_parsejson(GlusterConf **pgconf, QDict *options)
> +{

> +    /* create opts info from runtime_tuple_opts list */
> +    str = g_malloc(40);

That looks fishy.  For something that small, stack-allocation rather
than malloc may suffice.  Or...

> +    for (i = 0; i < num_servers; i++) {
> +        opts = qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error_abort);
> +        snprintf(str, 40, GLUSTER_OPT_SERVER_PATTERN"%d.", i);

If you stick with snprintf(), it would probably be worth at least an
assert() that you didn't overflow.

...just use g_string_printf() in the first place, and then you don't
have to worry about checking whether your malloc was big enough.  But
then you'd have to be careful to not leak it on iterations.

> +static struct glfs *qemu_gluster_init(GlusterConf **gconf, const char *filename,
> +                                      QDict *options, Error **errp)
> +{
> +    int ret;
> +    int num_servers = 1;
> +
> +    if (filename) {
> +        ret = qemu_gluster_parseuri(gconf, filename);
> +        if (ret < 0) {
> +            error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
> +                             "volume/path[?socket=...]");
> +            errno = -ret;
> +            return NULL;
> +        }
> +    } else {
> +        num_servers = qemu_gluster_parsejson(gconf, options);
> +        if (num_servers < 1) {
> +            error_setg(errp, "Usage1: "
> +                             "-drive driver=qcow2,file.driver=gluster,"
> +                             "file.volume=testvol,file.path=/path/a.qcow2,"
> +                             "file.servers.0.host=1.2.3.4,"
> +                             "[file.servers.0.port=24007,]"
> +                             "[file.servers.0.transport=tcp,]"
> +                             "file.servers.1.host=5.6.7.8,"
> +                             "[file.servers.1.port=24008,]"
> +                             "[file.servers.1.transport=rdma,] ..."
> +                             "\nUsage2: "
> +                             "'json:{\"driver\":\"qcow2\",\"file\":"
> +                             "{\"driver\":\"gluster\",\"volume\":\""
> +                             "testvol\",\"path\":\"/path/a.qcow2\","
> +                             "\"servers\":[{\"host\":\"1.2.3.4\","
> +                             "\"port\":\"24007\",\"transport\":\"tcp\"},"
> +                             "{\"host\":\"4.5.6.7\",\"port\":\"24007\","
> +                             "\"transport\":\"rdma\"}, ...]}}'");

That's a bit long for an error message; it might be better to split
things into a short error, plus optional informative messages using
error_append_hint().


> +++ b/qapi/block-core.json
> @@ -1380,10 +1380,10 @@
>  ##
>  { 'enum': 'BlockdevDriver',
>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
> -            'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> -            'host_floppy', 'http', 'https', 'null-aio', 'null-co', 'parallels',
> -            'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
> -            'vmdk', 'vpc', 'vvfat' ] }
> +            'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> +            'host_device', 'host_floppy', 'http', 'https', 'null-aio',
> +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> +            'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }

Missing documentation that 'gluster' was supported since 2.5.

>  
>  ##
>  # @BlockdevOptionsBase
> @@ -1794,6 +1794,56 @@
>              '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> +# @GlusterTransport
> +#
> +# An enumeration of Gluster transport type
> +#
> +# @tcp:   TCP  - Transmission Control Protocol
> +#
> +# @rdma:  RDMA - Remote direct memory access
> +#
> +# Since: 2.5
> +##
> +{ 'enum': 'GlusterTransport', 'data': [ 'tcp', 'rdma' ] }
> +
> +##
> +# @GlusterTuple
> +#
> +# Gluster tuple set
> +#
> +# @host:       host address (hostname/ipv4/ipv6 addresses)
> +#
> +# @port:       port number on which glusterd is listening. (default 24007)

Missing an #optional tag.

> +#
> +# @transport:  #transport type used to connect to gluster management daemon

s/#transport/#optional/

> +#               it can be tcp|rdma (default 'tcp')

Mentioning tcp|rdma is redundant with the fact that you typed this
parameter as GlusterTransport (looking at the enum already tells you the
valid values).

> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'GlusterTuple',
> +  'data': { 'host': 'str',
> +            '*port': 'int',
> +            '*transport': 'GlusterTransport' } }
> +
> +##
> +# @BlockdevOptionsGluster
> +#
> +# Driver specific block device options for Gluster
> +#
> +# @volume:   name of gluster volume where our VM image resides
> +#
> +# @path:     absolute path to image file in gluster volume
> +#
> +# @servers:  holds multiple tuples of {host, transport, port}

For this patch, it looks like it holds exactly one tuple.  But it looks
like you plan to support multiple tuples later on; maybe a better
wording is:

@servers: one or more gluster host descriptions (host, port, and transport)

> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'BlockdevOptionsGluster',
> +  'data': { 'volume': 'str',
> +            'path': 'str',
> +            'servers': [ 'GlusterTuple' ] } }

I'm not necessarily sold that GlusterTuple is the best name (maybe
GlusterServer is better?)  But at least the naming is not part of the
ABI, so I'm not too fussed.  Overall, the API looks like it is starting
to converge to something usable.

> +
> +##
>  # @BlockdevOptions
>  #
>  # Options for creating a block device.
> @@ -1813,7 +1863,7 @@
>        'file':       'BlockdevOptionsFile',
>        'ftp':        'BlockdevOptionsFile',
>        'ftps':       'BlockdevOptionsFile',
> -# TODO gluster: Wait for structured options
> +      'gluster':    'BlockdevOptionsGluster',
>        'host_cdrom': 'BlockdevOptionsFile',
>        'host_device':'BlockdevOptionsFile',
>        'host_floppy':'BlockdevOptionsFile',
>
Prasanna Kalever Oct. 20, 2015, 7:23 a.m. UTC | #2 
On  Tuesday, October 20, 2015 1:38:37 AM, Eric Blake wrote: 
> On 10/19/2015 06:13 AM, Prasanna Kumar Kalever wrote:
> > This patch adds a way to specify multiple volfile servers to the gluster
> > block backend of QEMU with tcp|rdma transport types and their port numbers.
> 
> When sending a multi-patch series, it is best to also include a 0/3
> cover letter.  Git can be configured to do this automatically with:
> git config format.coverLetter auto
> 

Thanks for the tip Eric :)

[...]
> > +#define GLUSTER_DEFAULT_PORT       24007
> > +
> >  typedef struct GlusterAIOCB {
> >      int64_t size;
> >      int ret;
> > @@ -24,22 +34,72 @@ typedef struct BDRVGlusterState {
> >      struct glfs_fd *fd;
> >  } BDRVGlusterState;
> >  
> > -typedef struct GlusterConf {
> > +typedef struct GlusterServerConf {
> >      char *server;
> >      int port;
> > +    char *transport;
> 
> How do you know how many transport tuples are present? I'd expect a size
> argument somewhere.
> 
Its based on users choice I don't want to make it static

> > +} GlusterServerConf;

[...]

> > @@ -117,16 +178,19 @@ static int qemu_gluster_parseuri(GlusterConf *gconf,
> > const char *filename)
> > return -EINVAL;
> > }
> > 
> > + gconf = g_new0(GlusterConf, 1);
> > + gconf->gsconf = g_new0(GlusterServerConf, 1);
> 
> Wow - you are hard-coding things to exactly one server.  The subject
> line of the patch claims multiple gluster servers, but I don't see
> anything that supports more than one.  So something needs to be fixed up
> (if this patch is just renaming things, and a later patch adds support
> for more than one, that's okay - but it needs to be described that way).
> 

[1] I think you need to check 'qemu_gluster_parsejson' function for multiple gluster servers
usage which parse JSON syntax with multiple tuples, 'qemu_gluster_parseuri' function is to
parse URI syntax only and that supports single server usage only (kept for compatibility)

> > +static int qemu_gluster_parsejson(GlusterConf **pgconf, QDict *options)
> > +{

[...]

> > +#
> > +# Since: 2.5
> > +##
> > +{ 'struct': 'GlusterTuple',
> > +  'data': { 'host': 'str',
> > +            '*port': 'int',
> > +            '*transport': 'GlusterTransport' } }
> > +
> > +##
> > +# @BlockdevOptionsGluster
> > +#
> > +# Driver specific block device options for Gluster
> > +#
> > +# @volume:   name of gluster volume where our VM image resides
> > +#
> > +# @path:     absolute path to image file in gluster volume
> > +#
> > +# @servers:  holds multiple tuples of {host, transport, port}
> 
> For this patch, it looks like it holds exactly one tuple.  But it looks
> like you plan to support multiple tuples later on; maybe a better
> wording is:
> 
> @servers: one or more gluster host descriptions (host, port, and transport)
> 
... [1] should clarify your understanding, but yes still I will bind to your comment 

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

-Prasanna
diff mbox

Patch

diff --git a/block/gluster.c b/block/gluster.c
index 1eb3a8c..dd076fe 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -11,6 +11,16 @@ 
 #include "block/block_int.h"
 #include "qemu/uri.h"
 
+#define GLUSTER_OPT_FILENAME       "filename"
+#define GLUSTER_OPT_VOLUME         "volume"
+#define GLUSTER_OPT_PATH           "path"
+#define GLUSTER_OPT_HOST           "host"
+#define GLUSTER_OPT_PORT           "port"
+#define GLUSTER_OPT_TRANSPORT      "transport"
+#define GLUSTER_OPT_SERVER_PATTERN "servers."
+
+#define GLUSTER_DEFAULT_PORT       24007
+
 typedef struct GlusterAIOCB {
     int64_t size;
     int ret;
@@ -24,22 +34,72 @@  typedef struct BDRVGlusterState {
     struct glfs_fd *fd;
 } BDRVGlusterState;
 
-typedef struct GlusterConf {
+typedef struct GlusterServerConf {
     char *server;
     int port;
+    char *transport;
+} GlusterServerConf;
+
+typedef struct GlusterConf {
     char *volname;
     char *image;
-    char *transport;
+    GlusterServerConf *gsconf;
 } GlusterConf;
 
+static QemuOptsList runtime_json_opts = {
+    .name = "gluster_json",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
+    .desc = {
+        {
+            .name = GLUSTER_OPT_VOLUME,
+            .type = QEMU_OPT_STRING,
+            .help = "name of gluster volume where our VM image resides",
+        },
+        {
+            .name = GLUSTER_OPT_PATH,
+            .type = QEMU_OPT_STRING,
+            .help = "absolute path to image file in gluster volume",
+        },
+        { /* end of list */ }
+    },
+};
+
+static QemuOptsList runtime_tuple_opts = {
+    .name = "gluster_tuple",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_tuple_opts.head),
+    .desc = {
+        {
+            .name = GLUSTER_OPT_HOST,
+            .type = QEMU_OPT_STRING,
+            .help = "host address (hostname/ipv4/ipv6 addresses)",
+        },
+        {
+            .name = GLUSTER_OPT_PORT,
+            .type = QEMU_OPT_NUMBER,
+            .help = "port number on which glusterd is listening(default 24007)",
+        },
+        {
+            .name = GLUSTER_OPT_TRANSPORT,
+            .type = QEMU_OPT_STRING,
+            .help = "transport type used to connect to glusterd(default tcp)",
+        },
+        { /* end of list */ }
+    },
+};
+
 static void qemu_gluster_gconf_free(GlusterConf *gconf)
 {
     if (gconf) {
-        g_free(gconf->server);
         g_free(gconf->volname);
         g_free(gconf->image);
-        g_free(gconf->transport);
+        if (gconf->gsconf) {
+            g_free(gconf->gsconf[0].server);
+            g_free(gconf->gsconf[0].transport);
+            g_free(gconf->gsconf);
+            gconf->gsconf = NULL;
+        }
         g_free(gconf);
+        gconf = NULL;
     }
 }
 
@@ -105,8 +165,9 @@  static int parse_volume_options(GlusterConf *gconf, char *path)
  * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
  * file=gluster+rdma://1.2.3.4:24007/testvol/a.img
  */
-static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
+static int qemu_gluster_parseuri(GlusterConf **pgconf, const char *filename)
 {
+    GlusterConf *gconf;
     URI *uri;
     QueryParams *qp = NULL;
     bool is_unix = false;
@@ -117,16 +178,19 @@  static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
         return -EINVAL;
     }
 
+    gconf = g_new0(GlusterConf, 1);
+    gconf->gsconf = g_new0(GlusterServerConf, 1);
+
     /* transport */
     if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
-        gconf->transport = g_strdup("tcp");
+        gconf->gsconf[0].transport = g_strdup("tcp");
     } else if (!strcmp(uri->scheme, "gluster+tcp")) {
-        gconf->transport = g_strdup("tcp");
+        gconf->gsconf[0].transport = g_strdup("tcp");
     } else if (!strcmp(uri->scheme, "gluster+unix")) {
-        gconf->transport = g_strdup("unix");
+        gconf->gsconf[0].transport = g_strdup("unix");
         is_unix = true;
     } else if (!strcmp(uri->scheme, "gluster+rdma")) {
-        gconf->transport = g_strdup("rdma");
+        gconf->gsconf[0].transport = g_strdup("rdma");
     } else {
         ret = -EINVAL;
         goto out;
@@ -152,13 +216,22 @@  static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
             ret = -EINVAL;
             goto out;
         }
-        gconf->server = g_strdup(qp->p[0].value);
+        gconf->gsconf[0].server = g_strdup(qp->p[0].value);
     } else {
-        gconf->server = g_strdup(uri->server ? uri->server : "localhost");
-        gconf->port = uri->port;
+        gconf->gsconf[0].server = g_strdup(uri->server ? uri->server : "localhost");
+        if (uri->port) {
+            gconf->gsconf[0].port = uri->port;
+        } else {
+            gconf->gsconf[0].port = GLUSTER_DEFAULT_PORT;
+        }
     }
 
+    *pgconf = gconf;
+
 out:
+    if (ret < 0) {
+        qemu_gluster_gconf_free(gconf);
+    }
     if (qp) {
         query_params_free(qp);
     }
@@ -166,30 +239,26 @@  out:
     return ret;
 }
 
-static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
-                                      Error **errp)
+static struct glfs *qemu_gluster_glfs_init(GlusterConf *gconf, int num_servers,
+                                           Error **errp)
 {
     struct glfs *glfs = NULL;
     int ret;
     int old_errno;
-
-    ret = qemu_gluster_parseuri(gconf, filename);
-    if (ret < 0) {
-        error_setg(errp, "Usage: file=gluster[+transport]://[server[:port]]/"
-                   "volname/image[?socket=...]");
-        errno = -ret;
-        goto out;
-    }
+    int i;
 
     glfs = glfs_new(gconf->volname);
     if (!glfs) {
         goto out;
     }
 
-    ret = glfs_set_volfile_server(glfs, gconf->transport, gconf->server,
-            gconf->port);
-    if (ret < 0) {
-        goto out;
+    for (i = 0; i < num_servers; i++) {
+        ret = glfs_set_volfile_server(glfs, gconf->gsconf[i].transport,
+                                      gconf->gsconf[i].server,
+                                      gconf->gsconf[i].port);
+        if (ret < 0) {
+            goto out;
+        }
     }
 
     /*
@@ -204,10 +273,9 @@  static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
     ret = glfs_init(glfs);
     if (ret) {
         error_setg_errno(errp, errno,
-                         "Gluster connection failed for server=%s port=%d "
-                         "volume=%s image=%s transport=%s", gconf->server,
-                         gconf->port, gconf->volname, gconf->image,
-                         gconf->transport);
+                         "Error: Gluster connection failed for given hosts "
+                         "volume:'%s' path:'%s' host1: %s", gconf->volname,
+                         gconf->image, gconf->gsconf[0].server);
 
         /* glfs_init sometimes doesn't set errno although docs suggest that */
         if (errno == 0)
@@ -226,6 +294,270 @@  out:
     return NULL;
 }
 
+static int parse_transport_option(const char *opt)
+{
+    int i;
+
+    if (!opt) {
+        /* Set tcp as default */
+        return GLUSTER_TRANSPORT_TCP;
+    }
+
+    for (i = 0; i < GLUSTER_TRANSPORT_MAX; i++) {
+        if (!strcmp(opt, GlusterTransport_lookup[i])) {
+            return i;
+        }
+    }
+
+    return -EINVAL;
+}
+
+/*
+*
+*  Basic command line syntax looks like:
+*
+* Pattern I:
+* -drive driver=gluster,
+*        volume=testvol,file.path=/path/a.raw,
+*        servers.0.host=1.2.3.4,
+*       [servers.0.port=24007,]
+*       [servers.0.transport=tcp,]
+*        servers.1.host=5.6.7.8,
+*       [servers.1.port=24008,]
+*       [servers.1.transport=rdma,] ...
+*
+* Pattern II:
+* 'json:{"driver":"qcow2","file":{"driver":"gluster",
+*       "volume":"testvol","path":"/path/a.qcow2",
+*       "servers":[{tuple0},{tuple1}, ...{tupleN}]}}'
+*
+*
+*   driver    => 'gluster' (protocol name)
+*   volume    => name of gluster volume where our VM image resides
+*   path      => absolute path of image in gluster volume
+*
+*  {tuple}    => {"host":"1.2.3.4"[,"port":"24007","transport":"tcp"]}
+*
+*   host      => host address (hostname/ipv4/ipv6 addresses)
+*   port      => port number on which glusterd is listening. (default 24007)
+*   tranport  => transport type used to connect to gluster management daemon,
+*                    it can be tcp|rdma (default 'tcp')
+*
+*
+* Examples:
+* Pattern I:
+* -drive driver=qcow2,file.driver=gluster,
+*        file.volume=testvol,file.path=/path/a.qcow2,
+*        file.servers.0.host=1.2.3.4,
+*        file.servers.0.port=24007,
+*        file.servers.0.transport=tcp,
+*        file.servers.1.host=5.6.7.8,
+*        file.servers.1.port=24008,
+*        file.servers.1.transport=rdma, ...
+*
+* -drive driver=qcow2,file.driver=gluster,
+*        file.volume=testvol,file.path=/path/a.qcow2,
+*        file.servers.0.host=1.2.3.4,
+*        file.servers.1.host=5.6.7.8, ...
+*
+* -drive driver=qcow2,file.driver=gluster,
+*        file.volume=testvol,file.path=/path/a.qcow2,
+*        file.servers.0.host=1.2.3.4,
+*        file.servers.0.port=24007,
+*        file.servers.1.host=5.6.7.8,
+*        file.servers.1.port=24008, ...
+*
+* -drive driver=qcow2,file.driver=gluster,
+*        file.volume=testvol,file.path=/path/a.qcow2,
+*        file.servers.0.host=1.2.3.4,
+*        file.servers.0.transport=tcp,
+*        file.servers.1.host=5.6.7.8,
+*        file.servers.1.transport=rdma, ...
+*
+* Pattern II:
+* 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
+*         "path":"/path/a.qcow2","servers":
+*         [{"host":"1.2.3.4","port":"24007","transport":"tcp"},
+*          {"host":"4.5.6.7","port":"24008","transport":"rdma"}, ...]}}'
+*
+* 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
+*         "path":"/path/a.qcow2","servers":
+*                                    [{"host":"1.2.3.4"},
+*                                     {"host":"4.5.6.7"}, ...]}}'
+*
+*
+* 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
+*         "path":"/path/a.qcow2","servers":
+*                                  [{"host":"1.2.3.4","port":"24007"},
+*                                   {"host":"4.5.6.7","port":"24008"}, ...]}}'
+*
+*
+* 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
+*        "path":"/path/a.qcow2","servers":
+*                              [{"host":"1.2.3.4","transport":"tcp"},
+*                               {"host":"4.5.6.7","transport":"rdma"}, ...]}}'
+*
+* Just for better readability pattern II is kept as:
+* json:
+* {
+*    "driver":"qcow2",
+*    "file":{
+*       "driver":"gluster",
+*       "volume":"testvol",
+*       "path":"/path/a.qcow2",
+*       "servers":[
+*          {
+*             "host":"1.2.3.4",
+*             "port":"24007",
+*             "transport":"tcp"
+*          },
+*          {
+*             "host":"5.6.7.8",
+*             "port":"24008",
+*             "transport":"rdma"
+*          }
+*       ]
+*    }
+* }
+*
+*/
+static int qemu_gluster_parsejson(GlusterConf **pgconf, QDict *options)
+{
+    QemuOpts *opts;
+    GlusterConf *gconf = NULL;
+    QDict *backing_options = NULL;
+    Error *local_err = NULL;
+    char *str = NULL;
+    const char *ptr = NULL;
+    int num_servers;
+    int i;
+
+    /* create opts info from runtime_json_opts list */
+    opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN);
+    if (num_servers < 1) {
+        error_setg(&local_err, "Error: qemu_gluster: please provide 'servers' "
+                               "option with valid fields in array of tuples");
+        goto out;
+    }
+
+    gconf = g_new0(GlusterConf, 1);
+    gconf->gsconf = g_new0(GlusterServerConf, num_servers);
+
+    ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
+    if (!ptr) {
+        error_setg(&local_err, "Error: qemu_gluster: please provide 'volume' "
+                               "option");
+        goto out;
+    }
+    gconf->volname = g_strdup(ptr);
+
+    ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
+    if (!ptr) {
+        error_setg(&local_err, "Error: qemu_gluster: please provide 'path' "
+                               "option");
+        goto out;
+    }
+    gconf->image = g_strdup(ptr);
+
+    qemu_opts_del(opts);
+
+    /* create opts info from runtime_tuple_opts list */
+    str = g_malloc(40);
+    for (i = 0; i < num_servers; i++) {
+        opts = qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error_abort);
+        snprintf(str, 40, GLUSTER_OPT_SERVER_PATTERN"%d.", i);
+        qdict_extract_subqdict(options, &backing_options, str);
+        qemu_opts_absorb_qdict(opts, backing_options, &local_err);
+        if (local_err) {
+            goto out;
+        }
+        qdict_del(backing_options, str);
+
+        ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
+        if (!ptr) {
+            error_setg(&local_err, "Error: qemu_gluster: servers.{tuple.%d} "
+                                   "requires 'host' option", i);
+            goto out;
+        }
+        gconf->gsconf[i].server = g_strdup(ptr);
+
+        ptr = qemu_opt_get(opts, GLUSTER_OPT_TRANSPORT);
+        /* check whether transport type specified in json command is valid */
+        if (parse_transport_option(ptr) < 0) {
+            error_setg(&local_err, "Error: qemu_gluster: please set 'transport'"
+                                   " type in tuple.%d as tcp or rdma", i);
+            goto out;
+        }
+        /* only if valid transport i.e. either of tcp|rdma is specified */
+        gconf->gsconf[i].transport = g_strdup(ptr);
+
+        gconf->gsconf[i].port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT,
+                                                    GLUSTER_DEFAULT_PORT);
+
+        qemu_opts_del(opts);
+    }
+
+    *pgconf = gconf;
+    g_free(str);
+    return num_servers;
+
+out:
+    error_report_err(local_err);
+    qemu_gluster_gconf_free(gconf);
+    qemu_opts_del(opts);
+    qdict_del(backing_options, str);
+    g_free(str);
+    errno = EINVAL;
+    return -errno;
+}
+
+static struct glfs *qemu_gluster_init(GlusterConf **gconf, const char *filename,
+                                      QDict *options, Error **errp)
+{
+    int ret;
+    int num_servers = 1;
+
+    if (filename) {
+        ret = qemu_gluster_parseuri(gconf, filename);
+        if (ret < 0) {
+            error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
+                             "volume/path[?socket=...]");
+            errno = -ret;
+            return NULL;
+        }
+    } else {
+        num_servers = qemu_gluster_parsejson(gconf, options);
+        if (num_servers < 1) {
+            error_setg(errp, "Usage1: "
+                             "-drive driver=qcow2,file.driver=gluster,"
+                             "file.volume=testvol,file.path=/path/a.qcow2,"
+                             "file.servers.0.host=1.2.3.4,"
+                             "[file.servers.0.port=24007,]"
+                             "[file.servers.0.transport=tcp,]"
+                             "file.servers.1.host=5.6.7.8,"
+                             "[file.servers.1.port=24008,]"
+                             "[file.servers.1.transport=rdma,] ..."
+                             "\nUsage2: "
+                             "'json:{\"driver\":\"qcow2\",\"file\":"
+                             "{\"driver\":\"gluster\",\"volume\":\""
+                             "testvol\",\"path\":\"/path/a.qcow2\","
+                             "\"servers\":[{\"host\":\"1.2.3.4\","
+                             "\"port\":\"24007\",\"transport\":\"tcp\"},"
+                             "{\"host\":\"4.5.6.7\",\"port\":\"24007\","
+                             "\"transport\":\"rdma\"}, ...]}}'");
+            errno = -num_servers;
+            return NULL;
+        }
+    }
+
+    return qemu_gluster_glfs_init(*gconf, num_servers, errp);
+}
+
 static void qemu_gluster_complete_aio(void *opaque)
 {
     GlusterAIOCB *acb = (GlusterAIOCB *)opaque;
@@ -254,13 +586,12 @@  static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void *arg)
     qemu_bh_schedule(acb->bh);
 }
 
-/* TODO Convert to fine grained options */
 static QemuOptsList runtime_opts = {
     .name = "gluster",
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
     .desc = {
         {
-            .name = "filename",
+            .name = GLUSTER_OPT_FILENAME,
             .type = QEMU_OPT_STRING,
             .help = "URL to the gluster image",
         },
@@ -291,7 +622,7 @@  static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
     BDRVGlusterState *s = bs->opaque;
     int open_flags = 0;
     int ret = 0;
-    GlusterConf *gconf = g_new0(GlusterConf, 1);
+    GlusterConf *gconf = NULL;
     QemuOpts *opts;
     Error *local_err = NULL;
     const char *filename;
@@ -305,8 +636,7 @@  static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
     }
 
     filename = qemu_opt_get(opts, "filename");
-
-    s->glfs = qemu_gluster_init(gconf, filename, errp);
+    s->glfs = qemu_gluster_init(&gconf, filename, options, errp);
     if (!s->glfs) {
         ret = -errno;
         goto out;
@@ -356,9 +686,7 @@  static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
 
     qemu_gluster_parse_flags(state->flags, &open_flags);
 
-    gconf = g_new0(GlusterConf, 1);
-
-    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp);
+    reop_s->glfs = qemu_gluster_init(&gconf, state->bs->filename, NULL, errp);
     if (reop_s->glfs == NULL) {
         ret = -errno;
         goto exit;
@@ -480,15 +808,15 @@  static inline int qemu_gluster_zerofill(struct glfs_fd *fd, int64_t offset,
 static int qemu_gluster_create(const char *filename,
                                QemuOpts *opts, Error **errp)
 {
+    GlusterConf *gconf = NULL;
     struct glfs *glfs;
     struct glfs_fd *fd;
     int ret = 0;
     int prealloc = 0;
     int64_t total_size = 0;
     char *tmp = NULL;
-    GlusterConf *gconf = g_new0(GlusterConf, 1);
 
-    glfs = qemu_gluster_init(gconf, filename, errp);
+    glfs = qemu_gluster_init(&gconf, filename, NULL, errp);
     if (!glfs) {
         ret = -errno;
         goto out;
@@ -718,7 +1046,7 @@  static BlockDriver bdrv_gluster = {
     .format_name                  = "gluster",
     .protocol_name                = "gluster",
     .instance_size                = sizeof(BDRVGlusterState),
-    .bdrv_needs_filename          = true,
+    .bdrv_needs_filename          = false,
     .bdrv_file_open               = qemu_gluster_open,
     .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
     .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
@@ -745,7 +1073,7 @@  static BlockDriver bdrv_gluster_tcp = {
     .format_name                  = "gluster",
     .protocol_name                = "gluster+tcp",
     .instance_size                = sizeof(BDRVGlusterState),
-    .bdrv_needs_filename          = true,
+    .bdrv_needs_filename          = false,
     .bdrv_file_open               = qemu_gluster_open,
     .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
     .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
@@ -799,7 +1127,7 @@  static BlockDriver bdrv_gluster_rdma = {
     .format_name                  = "gluster",
     .protocol_name                = "gluster+rdma",
     .instance_size                = sizeof(BDRVGlusterState),
-    .bdrv_needs_filename          = true,
+    .bdrv_needs_filename          = false,
     .bdrv_file_open               = qemu_gluster_open,
     .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
     .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index bb2189e..8bc69e0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1380,10 +1380,10 @@ 
 ##
 { 'enum': 'BlockdevDriver',
   'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
-            'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
-            'host_floppy', 'http', 'https', 'null-aio', 'null-co', 'parallels',
-            'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
-            'vmdk', 'vpc', 'vvfat' ] }
+            'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
+            'host_device', 'host_floppy', 'http', 'https', 'null-aio',
+            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
+            'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
 
 ##
 # @BlockdevOptionsBase
@@ -1794,6 +1794,56 @@ 
             '*read-pattern': 'QuorumReadPattern' } }
 
 ##
+# @GlusterTransport
+#
+# An enumeration of Gluster transport type
+#
+# @tcp:   TCP  - Transmission Control Protocol
+#
+# @rdma:  RDMA - Remote direct memory access
+#
+# Since: 2.5
+##
+{ 'enum': 'GlusterTransport', 'data': [ 'tcp', 'rdma' ] }
+
+##
+# @GlusterTuple
+#
+# Gluster tuple set
+#
+# @host:       host address (hostname/ipv4/ipv6 addresses)
+#
+# @port:       port number on which glusterd is listening. (default 24007)
+#
+# @transport:  #transport type used to connect to gluster management daemon
+#               it can be tcp|rdma (default 'tcp')
+#
+# Since: 2.5
+##
+{ 'struct': 'GlusterTuple',
+  'data': { 'host': 'str',
+            '*port': 'int',
+            '*transport': 'GlusterTransport' } }
+
+##
+# @BlockdevOptionsGluster
+#
+# Driver specific block device options for Gluster
+#
+# @volume:   name of gluster volume where our VM image resides
+#
+# @path:     absolute path to image file in gluster volume
+#
+# @servers:  holds multiple tuples of {host, transport, port}
+#
+# Since: 2.5
+##
+{ 'struct': 'BlockdevOptionsGluster',
+  'data': { 'volume': 'str',
+            'path': 'str',
+            'servers': [ 'GlusterTuple' ] } }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.
@@ -1813,7 +1863,7 @@ 
       'file':       'BlockdevOptionsFile',
       'ftp':        'BlockdevOptionsFile',
       'ftps':       'BlockdevOptionsFile',
-# TODO gluster: Wait for structured options
+      'gluster':    'BlockdevOptionsGluster',
       'host_cdrom': 'BlockdevOptionsFile',
       'host_device':'BlockdevOptionsFile',
       'host_floppy':'BlockdevOptionsFile',