diff mbox

[v3,1/1] block/gluster: add support for multiple gluster backup volfile servers

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

Commit Message

Prasanna Kumar Kalever Sept. 21, 2015, 11:24 a.m. UTC
This patch adds a way to specify multiple backup 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]://server1[:port]/testvol/a.img

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

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

Solution:

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

Basic command line syntax looks like:

Pattern I:
 -drive driver=gluster,
        volname=testvol,file.image-path=/path/a.raw,
        backup-volfile-servers.0.server=1.2.3.4,
       [backup-volfile-servers.0.port=24007,]
       [backup-volfile-servers.0.transport=tcp,]
        backup-volfile-servers.1.server=5.6.7.8,
       [backup-volfile-servers.1.port=24008,]
       [backup-volfile-servers.1.transport=rdma,] ...

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

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

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

   server       => server 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:
1.
 -drive driver=qcow2,file.driver=gluster,
        file.volname=testvol,file.image-path=/path/a.qcow2,
        file.backup-volfile-servers.0.server=1.2.3.4,
        file.backup-volfile-servers.0.port=24007,
        file.backup-volfile-servers.0.transport=tcp,
        file.backup-volfile-servers.1.server=5.6.7.8,
        file.backup-volfile-servers.1.port=24008,
        file.backup-volfile-servers.1.transport=rdma
2.
 'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
         "image-path":"/path/a.qcow2","backup-volfile-servers":
         [{"server":"1.2.3.4","port":"24007","transport":"tcp"},
          {"server":"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 server1 is down VM can still boot from any of the
active servers.

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/ (not merged yet)

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

v1:
multiple server addresses but common port number and transport type
pattern: URI syntax with query (?) delimitor
syntax:
    file=gluster[+transport-type]://server1:24007/testvol/a.img\
         ?backup-volfile-servers=server2&backup-volfile-servers=server3

v2:
multiple server addresses each have their own port number, but all use
common transport type
pattern: URI syntax  with query (?) delimiter
syntax:
    file=gluster[+transport-type]://[server[:port]]/testvol/a.img\
         [?backup-volfile-servers=server1[:port]\
          &backup-volfile-servers=server2[:port]]

v3:
multiple server addresses each have their own port number and transport type
pattern: changed to json
syntax:
    'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
           "image-path":"/path/a.qcow2","backup-volfile-servers":
         [{"server":"1.2.3.4","port":"24007","transport":"tcp"},
          {"server":"4.5.6.7","port":"24008","transport":"rdma"},] } }'

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
---
 block/gluster.c      | 412 +++++++++++++++++++++++++++++++++++++++++++++------
 qapi/block-core.json |  40 ++++-
 2 files changed, 405 insertions(+), 47 deletions(-)

Comments

Eric Blake Sept. 21, 2015, 3:41 p.m. UTC | #1
On 09/21/2015 05:24 AM, Prasanna Kumar Kalever wrote:
> This patch adds a way to specify multiple backup volfile servers to the gluster
> block backend of QEMU with tcp|rdma transport types and their port numbers.
> 

> 
> This patch gives a mechanism to provide all the server addresses which are in
> replica set, so in case server1 is down VM can still boot from any of the
> active servers.
> 
> 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/ (not merged yet)
> 

It would be nice to get that merged before we take this in qemu.

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

Up to here is good.

> v1:
> multiple server addresses but common port number and transport type
> pattern: URI syntax with query (?) delimitor
> syntax:
>     file=gluster[+transport-type]://server1:24007/testvol/a.img\
>          ?backup-volfile-servers=server2&backup-volfile-servers=server3

But the changelog information here should instead occur...


> 
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> ---

...here, so that it is helpful to reviewers but doesn't clog qemu.git
history (a year from now, we won't care how many iterations a patch went
through, only what got committed).


> +++ b/qapi/block-core.json
> @@ -1382,7 +1382,7 @@
>    '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',
> +            'qcow', 'qcow2', 'qed', 'quorum', 'gluster', 'raw', 'tftp', 'vdi', 'vhdx',
>              'vmdk', 'vpc', 'vvfat' ] }

Please keep this list sorted.  Also, missing documentation that
'gluster' was added in 2.5.

>  
>  ##
> @@ -1794,6 +1794,43 @@
>              '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> +# @GlusterTuplePattern

Name is long. Something like 'GlusterHost' would be nicer.

> +#
> +# Gluster tuple pattern
> +#
> +# @transport:    #transport type used to connect to gluster management daemon
> +#                 it can be tcp|rdma (default 'tcp')
> +#
> +# @port:         port number on which glusterd is listening. (default 24007)

To have a default, the parameter needs to be optional ('*port').

> +#
> +# @server:       server address (hostname/ipv4/ipv6 addresses)
> +#
> +# Since: 2.4+

s/2.4+/2.5/ (there is no 2.4+ release; the release that this will appear
in is 2.5)

> +##
> +{ 'struct': 'GlusterTuplePattern',
> +  'data': { 'server': 'str',
> +            'transport': 'str',

Yuck. Please don't open-code this as a 'str', but instead define it as
an enum:

{ 'enum': 'GlusterTransport', 'data': [ 'tcp', 'rdma' ] }

> +            'port': 'str' } }

Should port be an integer instead of a string? I guess we already allow
named ports in other network-related interfaces, but it would still be
nice to have an 'alternate' type that allows both integer and string.

It would be nice if your documentation of the parameters occurred in the
same order as the parameters appear in the 'struct'.

> +
> +##
> +# @BlockdevOptionsGluster
> +#
> +# Driver specific block device options for Gluster
> +#
> +# @volname:      name of gluster volume where our VM image resides
> +#
> +# @image-path:   absolute path to image file in gluster volume
> +#
> +# @backup-volfile-servers: holds multiple tuples of {server, transport, port}
> +#
> +# Since: 2.4+

s/2.4+/2.5/

> +##
> +{ 'struct': 'BlockdevOptionsGluster',
> +  'data': { 'volname': 'str',
> +            'image-path': 'str',
> +            'backup-volfile-servers': [ 'GlusterTuplePattern' ] } }

Shouldn't this be simply 'volfile-servers', as you are including the
primary server in addition to the backup servers?

> +
> +##
>  # @BlockdevOptions
>  #
>  # Options for creating a block device.
> @@ -1814,6 +1851,7 @@
>        'ftp':        'BlockdevOptionsFile',
>        'ftps':       'BlockdevOptionsFile',
>  # TODO gluster: Wait for structured options
> +      'gluster':    'BlockdevOptionsGluster',
>        'host_cdrom': 'BlockdevOptionsFile',
>        'host_device':'BlockdevOptionsFile',
>        'host_floppy':'BlockdevOptionsFile',
> 

There's also work on the list to expose NBD in a structured fashion; I'm
a bit worried that the two proposals might need to be sharing some
common functionality, rather than independently inventing slightly
different syntax.
Prasanna Kalever Sept. 22, 2015, 8:06 a.m. UTC | #2
> On 09/21/2015 05:24 AM, Prasanna Kumar Kalever wrote:
> > This patch adds a way to specify multiple backup volfile servers to the
> > gluster
> > block backend of QEMU with tcp|rdma transport types and their port numbers.
> > 
> 
> > 
> > This patch gives a mechanism to provide all the server addresses which are
> > in
> > replica set, so in case server1 is down VM can still boot from any of the
> > active servers.
> > 
> > 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/ (not merged yet)
> > 
> 
> It would be nice to get that merged before we take this in qemu.
> 
> > Credits: Sincere thanks to Kevin Wolf <kwolf@redhat.com> and
> > "Deepak C Shetty" <deepakcs@redhat.com> for inputs and all their support
> > 
> 
> Up to here is good.

thank you

> 
> > v1:
> > multiple server addresses but common port number and transport type
> > pattern: URI syntax with query (?) delimitor
> > syntax:
> >     file=gluster[+transport-type]://server1:24007/testvol/a.img\
> >          ?backup-volfile-servers=server2&backup-volfile-servers=server3
> 
> But the changelog information here should instead occur...

Agree, I will correct myself

> 
> 
> > 
> > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> > ---
> 
> ...here, so that it is helpful to reviewers but doesn't clog qemu.git
> history (a year from now, we won't care how many iterations a patch went
> through, only what got committed).
> 
> 
> > +++ b/qapi/block-core.json
> > @@ -1382,7 +1382,7 @@
> >    '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',
> > +            'qcow', 'qcow2', 'qed', 'quorum', 'gluster', 'raw', 'tftp',
> > 'vdi', 'vhdx',
> >              'vmdk', 'vpc', 'vvfat' ] }
> 
> Please keep this list sorted.  Also, missing documentation that
> 'gluster' was added in 2.5.
> 

Agree, I will change it

> >  
> >  ##
> > @@ -1794,6 +1794,43 @@
> >              '*read-pattern': 'QuorumReadPattern' } }
> >  
> >  ##
> > +# @GlusterTuplePattern
> 
> Name is long. Something like 'GlusterHost' would be nicer.
>

Hmm, I think rather than length, readability is important, Even 'QuorumReadPattern' is also long, but gives  completeness.
Okay, If you still thing its better to have short name I can change it to "GlusterTuple"
 
> > +#
> > +# Gluster tuple pattern
> > +#
> > +# @transport:    #transport type used to connect to gluster management
> > daemon
> > +#                 it can be tcp|rdma (default 'tcp')
> > +#
> > +# @port:         port number on which glusterd is listening. (default
> > 24007)
> 
> To have a default, the parameter needs to be optional ('*port').
>

Thanks for correcting here, I really didn't knew this.
 
> > +#
> > +# @server:       server address (hostname/ipv4/ipv6 addresses)
> > +#
> > +# Since: 2.4+
> 
> s/2.4+/2.5/ (there is no 2.4+ release; the release that this will appear
> in is 2.5)
>

Okay.
 
> > +##
> > +{ 'struct': 'GlusterTuplePattern',
> > +  'data': { 'server': 'str',
> > +            'transport': 'str',
> 
> Yuck. Please don't open-code this as a 'str', but instead define it as
> an enum:
> 
> { 'enum': 'GlusterTransport', 'data': [ 'tcp', 'rdma' ] }
> 
> > +            'port': 'str' } }
> 
> Should port be an integer instead of a string? I guess we already allow
> named ports in other network-related interfaces, but it would still be
> nice to have an 'alternate' type that allows both integer and string.
> 
> It would be nice if your documentation of the parameters occurred in the
> same order as the parameters appear in the 'struct'.
> 

I expected, If I take it as an integer, while passing in json format
'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
             "image-path":"/path/a.qcow2","backup-volfile-servers":
             [{"server":"1.2.3.4","port":"24007","transport":"tcp"},
              {"server":"4.5.6.7","port":"24008","transport":"rdma"},] } }'
I should specify only "port":"24007" => "port":24007 (as integer)
Thanks for correcting me again.

I think the below should be enough

+##
+{ 'enum': 'GTransport', 'data': [ 'tcp', 'rdma' ] }

 ##
 # @GlusterTuplePattern
 #
@@ -1809,8 +1813,8 @@
 ##
 { 'struct': 'GlusterTuplePattern',
   'data': { 'server': 'str',
-            'transport': 'str',
-            'port': 'str' } }
+            '*transport': 'GTransport',
+            '*port': 'int' } }

Will do this.

> > +
> > +##
> > +# @BlockdevOptionsGluster
> > +#
> > +# Driver specific block device options for Gluster
> > +#
> > +# @volname:      name of gluster volume where our VM image resides
> > +#
> > +# @image-path:   absolute path to image file in gluster volume
> > +#
> > +# @backup-volfile-servers: holds multiple tuples of {server, transport,
> > port}
> > +#
> > +# Since: 2.4+
> 
> s/2.4+/2.5/
> 
Okay.
> > +##
> > +{ 'struct': 'BlockdevOptionsGluster',
> > +  'data': { 'volname': 'str',
> > +            'image-path': 'str',
> > +            'backup-volfile-servers': [ 'GlusterTuplePattern' ] } }
> 
> Shouldn't this be simply 'volfile-servers', as you are including the
> primary server in addition to the backup servers?
>

Again I want to maintain naming as mount.glusterfs do for fuse.
 
> > +
> > +##
> >  # @BlockdevOptions
> >  #
> >  # Options for creating a block device.
> > @@ -1814,6 +1851,7 @@
> >        'ftp':        'BlockdevOptionsFile',
> >        'ftps':       'BlockdevOptionsFile',
> >  # TODO gluster: Wait for structured options
> > +      'gluster':    'BlockdevOptionsGluster',
> >        'host_cdrom': 'BlockdevOptionsFile',
> >        'host_device':'BlockdevOptionsFile',
> >        'host_floppy':'BlockdevOptionsFile',
> > 
> 
> There's also work on the list to expose NBD in a structured fashion; I'm
> a bit worried that the two proposals might need to be sharing some
> common functionality, rather than independently inventing slightly
> different syntax.

I have gone through patch, Still NDB support a slight different transport types,
and naming say volname etc..
So I think for now lets make them independent.

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

Thanks for your detailed comments Eric Blake :)

Best Regards,
Prasanna Kumar Kalever.
Peter Krempa Sept. 23, 2015, 3:04 p.m. UTC | #3
On Tue, Sep 22, 2015 at 04:06:54 -0400, Prasanna Kalever wrote:
> 
> > On 09/21/2015 05:24 AM, Prasanna Kumar Kalever wrote:
> > > This patch adds a way to specify multiple backup volfile servers to the
> > > gluster
> > > block backend of QEMU with tcp|rdma transport types and their port numbers.
> > > 
> > 

...

> 
> > > +##
> > > +{ 'struct': 'BlockdevOptionsGluster',
> > > +  'data': { 'volname': 'str',
> > > +            'image-path': 'str',
> > > +            'backup-volfile-servers': [ 'GlusterTuplePattern' ] } }
> > 
> > Shouldn't this be simply 'volfile-servers', as you are including the
> > primary server in addition to the backup servers?
> >
> 
> Again I want to maintain naming as mount.glusterfs do for fuse.

Well, I have to agree with Eric here. I think the option name doesn't
need to be kept in sync with the gluster implementation since they don't
share anything with qemu and since the array contains also the primary
server to be queried the word backup doesn't make snese there.

Peter
Prasanna Kalever Sept. 26, 2015, 4:47 a.m. UTC | #4
> On Tue, Sep 22, 2015 at 04:06:54 -0400, Prasanna Kalever wrote:
> > 
> > > On 09/21/2015 05:24 AM, Prasanna Kumar Kalever wrote:
> > > > This patch adds a way to specify multiple backup volfile servers to the
> > > > gluster
> > > > block backend of QEMU with tcp|rdma transport types and their port
> > > > numbers.
> > > > 
> > > 
> ter
> ...
> 
> > 
> > > > +##
> > > > +{ 'struct': 'BlockdevOptionsGluster',
> > > > +  'data': { 'volname': 'str',
> > > > +            'image-path': 'str',
> > > > +            'backup-volfile-servers': [ 'GlusterTuplePattern' ] } }
> > > 
> > > Shouldn't this be simply 'volfile-servers', as you are including the
> > > primary server in addition to the backup servers?
> > >
> > 
> > Again I want to maintain naming as mount.glusterfs do for fuse.
> 
> Well, I have to agree with Eric here. I think the option name doesn't
> need to be kept in sync with the gluster implementation since they don't
> share anything with qemu and since the array contains also the primary
> server to be queried the word backup doesn't make snese there.

Yes Peter, I have to agree with you and Eric.
I will rectify this changes in next patch-set, just waiting for 'gfapi' patch to get merged.

Thank you,

Best Regards, 
Prasanna Kumar Kalever.

> 
> Peter
>
diff mbox

Patch

diff --git a/block/gluster.c b/block/gluster.c
index 1eb3a8c..48ae62e 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -11,6 +11,17 @@ 
 #include "block/block_int.h"
 #include "qemu/uri.h"
 
+#define MAX_PORTNUM_SIZE  6       /* max port num 65535 */
+
+#define GLUSTER_OPT_FILENAME      "filename"
+#define GLUSTER_OPT_VOLNAME       "volname"
+#define GLUSTER_OPT_IMAGE_PATH    "image-path"
+#define GLUSTER_OPT_SERVER        "server"
+#define GLUSTER_OPT_PORT          "port"
+#define GLUSTER_OPT_TRANSPORT     "transport"
+#define GLUSTER_OPT_READ_PATTERN  "backup-volfile-servers."
+
+
 typedef struct GlusterAIOCB {
     int64_t size;
     int ret;
@@ -26,7 +37,7 @@  typedef struct BDRVGlusterState {
 
 typedef struct GlusterConf {
     char *server;
-    int port;
+    char *port;
     char *volname;
     char *image;
     char *transport;
@@ -39,10 +50,65 @@  static void qemu_gluster_gconf_free(GlusterConf *gconf)
         g_free(gconf->volname);
         g_free(gconf->image);
         g_free(gconf->transport);
+        g_free(gconf->port);
         g_free(gconf);
     }
 }
 
+static QemuOptsList runtime_opts = {
+    .name = "gluster",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = GLUSTER_OPT_FILENAME,
+            .type = QEMU_OPT_STRING,
+            .help = "URL to the gluster image",
+        },
+        { /* end of list */ }
+    },
+};
+
+static QemuOptsList runtime_json_opts = {
+    .name = "gluster_json",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
+    .desc = {
+        {
+            .name = GLUSTER_OPT_VOLNAME,
+            .type = QEMU_OPT_STRING,
+            .help = "name of gluster volume where our VM image resides",
+        },
+        {
+            .name = GLUSTER_OPT_IMAGE_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_SERVER,
+            .type = QEMU_OPT_STRING,
+            .help = "server address (hostname/ipv4/ipv6 addresses)",
+        },
+        {
+            .name = GLUSTER_OPT_PORT,
+            .type = QEMU_OPT_STRING,
+            .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 int parse_volume_options(GlusterConf *gconf, char *path)
 {
     char *p, *q;
@@ -105,6 +171,7 @@  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)
 {
     URI *uri;
@@ -155,7 +222,8 @@  static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
         gconf->server = g_strdup(qp->p[0].value);
     } else {
         gconf->server = g_strdup(uri->server ? uri->server : "localhost");
-        gconf->port = uri->port;
+        gconf->port = g_malloc(MAX_PORTNUM_SIZE);
+        snprintf(gconf->port, MAX_PORTNUM_SIZE, "%d", uri->port);
     }
 
 out:
@@ -166,30 +234,25 @@  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 ret = 0;
+    int i = 0;
 
     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[i].transport, gconf[i].server,
+                atoi(gconf[i].port));
+        if (ret < 0) {
+            goto out;
+        }
     }
 
     /*
@@ -204,15 +267,12 @@  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);
+                "Gluster connection failed for volname=%s image=%s",
+                gconf->volname, gconf->image);
 
         /* glfs_init sometimes doesn't set errno although docs suggest that */
         if (errno == 0)
             errno = EINVAL;
-
         goto out;
     }
     return glfs;
@@ -226,6 +286,273 @@  out:
     return NULL;
 }
 
+
+static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
+                                      Error **errp)
+{
+    struct glfs *glfs = NULL;
+    int ret;
+
+    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;
+    }
+
+    glfs = qemu_gluster_glfs_init(gconf, 1, errp);
+    if (!glfs) {
+        goto out;
+    }
+    return glfs;
+
+out:
+    return NULL;
+}
+
+/*
+*
+*  Basic command line syntax looks like:
+*
+* Pattern I:
+* -drive driver=gluster,
+*        volname=testvol,file.image-path=/path/a.raw,
+*        backup-volfile-servers.0.server=1.2.3.4,
+*       [backup-volfile-servers.0.port=24007,]
+*       [backup-volfile-servers.0.transport=tcp,]
+*        backup-volfile-servers.1.server=5.6.7.8,
+*       [backup-volfile-servers.1.port=24008,]
+*       [backup-volfile-servers.1.transport=rdma,] ...
+*
+* Pattern II:
+* 'json:{"driver":"qcow2","file":{"driver":"gluster",
+*       "volname":"testvol","image-path":"/path/a.qcow2",
+*       "backup-volfile-servers":[{tuple0},{tuple1}, ...{tupleN}]}}'
+*
+*
+*   driver       => 'gluster' (protocol name)
+*   volname      => name of gluster volume where our VM image resides
+*   image-path   => is the absolute path of image in gluster volume
+*
+*  {tuple}       => {"server":"1.2.3.4"[,"port":"24007","transport":"tcp"]}
+*
+*   server       => server 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.volname=testvol,file.image-path=/path/a.qcow2,
+*        file.backup-volfile-servers.0.server=1.2.3.4,
+*        file.backup-volfile-servers.0.port=24007,
+*        file.backup-volfile-servers.0.transport=tcp,
+*        file.backup-volfile-servers.1.server=5.6.7.8,
+*        file.backup-volfile-servers.1.port=24008,
+*        file.backup-volfile-servers.1.transport=rdma, ...
+*
+* -drive driver=qcow2,file.driver=gluster,
+*        file.volname=testvol,file.image-path=/path/a.qcow2,
+*        file.backup-volfile-servers.0.server=1.2.3.4,
+*        file.backup-volfile-servers.1.server=5.6.7.8, ...
+*
+* -drive driver=qcow2,file.driver=gluster,
+*        file.volname=testvol,file.image-path=/path/a.qcow2,
+*        file.backup-volfile-servers.0.server=1.2.3.4,
+*        file.backup-volfile-servers.0.port=24007,
+*        file.backup-volfile-servers.1.server=5.6.7.8,
+*        file.backup-volfile-servers.1.port=24008, ...
+*
+* -drive driver=qcow2,file.driver=gluster,
+*        file.volname=testvol,file.image-path=/path/a.qcow2,
+*        file.backup-volfile-servers.0.server=1.2.3.4,
+*        file.backup-volfile-servers.0.transport=tcp,
+*        file.backup-volfile-servers.1.server=5.6.7.8,
+*        file.backup-volfile-servers.1.transport=rdma, ...
+*
+* Pattern II:
+* 'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
+*         "image-path":"/path/a.qcow2","backup-volfile-servers":
+*         [{"server":"1.2.3.4","port":"24007","transport":"tcp"},
+*          {"server":"4.5.6.7","port":"24008","transport":"rdma"}, ...]}}'
+*
+* 'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
+*         "image-path":"/path/a.qcow2","backup-volfile-servers":
+*                                    [{"server":"1.2.3.4"},
+*                                     {"server":"4.5.6.7"}, ...]}}'
+*
+*
+* 'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
+*         "image-path":"/path/a.qcow2","backup-volfile-servers":
+*                                  [{"server":"1.2.3.4","port":"24007"},
+*                                   {"server":"4.5.6.7","port":"24008"}, ...]}}'
+*
+*
+* 'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
+*        "image-path":"/path/a.qcow2","backup-volfile-servers":
+*                              [{"server":"1.2.3.4","transport":"tcp"},
+*                               {"server":"4.5.6.7","transport":"rdma"}, ...]}}'
+*
+* Just for better readability pattern II is kept as:
+* json:
+* {
+*    "driver":"qcow2",
+*    "file":{
+*       "driver":"gluster",
+*       "volname":"testvol",
+*       "image-path":"/path/a.qcow2",
+*       "backup-volfile-servers":[
+*          {
+*             "server":"1.2.3.4",
+*             "port":"24007",
+*             "transport":"tcp"
+*          },
+*          {
+*             "server":"5.6.7.8",
+*             "port":"24008",
+*             "transport":"rdma"
+*          }
+*       ]
+*    }
+* }
+*
+*/
+
+static int qemu_gluster_parseopts(GlusterConf **pgconf, QDict *options,
+                                  Error **errp)
+{
+    QemuOpts *opts;
+    GlusterConf *gconf = NULL;
+    QDict *backing_options;
+    Error *local_err = NULL;
+    char *str = NULL;
+    int num_servers = 0;
+    int ret = 0;
+    int i = 0;
+
+    /* parse options in dict */
+    opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto out;
+    }
+    num_servers = qdict_array_entries(options, GLUSTER_OPT_READ_PATTERN);
+    if (num_servers < 1) {
+        error_setg(&local_err, "\n\n ********* qemu_gluster: "
+                "please provide 'backup-volfile-server' option "
+                "with valid fields in array of tuples *********\n");
+        error_report_err(local_err);
+        goto out;
+    }
+
+    gconf = g_new0(GlusterConf, num_servers);
+
+    gconf->volname = (char *) qemu_opt_get(opts, GLUSTER_OPT_VOLNAME);
+    if (!gconf->volname) {
+        error_setg(&local_err, "\n\n ********* qemu_gluster: "
+                "please provide 'volname'option *********\n");
+        error_report_err(local_err);
+        goto out;
+    }
+    gconf->image = (char *) qemu_opt_get(opts, GLUSTER_OPT_IMAGE_PATH);
+    if (!gconf->image) {
+        error_setg(&local_err, "\n\n ********* qemu_gluster: "
+                "please provide 'image-path' option *********\n");
+        error_report_err(local_err);
+        goto out;
+    }
+    opts = qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error_abort);
+    for (i = 0; i < num_servers; i++) {
+        if (i > 0) {
+            gconf[i].volname = gconf->volname;
+            gconf[i].image = gconf->image;
+        }
+        str = g_malloc(40);
+        snprintf(str, 40, GLUSTER_OPT_READ_PATTERN"%d.", i);
+        qdict_extract_subqdict(options, &backing_options, str);
+        g_free(str);
+        qemu_opts_absorb_qdict(opts, backing_options, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            goto out;
+        }
+        gconf[i].server = (char *) qemu_opt_get(opts, GLUSTER_OPT_SERVER);
+        if (!gconf[i].server) {
+            error_setg(&local_err, "\n\n ********* qemu_gluster: "
+               "backup-volfile-servers.{tuple.%d} requires 'server' "
+               "option *********\n", i);
+            error_report_err(local_err);
+            goto out;
+        }
+        gconf[i].transport = (char *) qemu_opt_get(opts, GLUSTER_OPT_TRANSPORT);
+        if (!gconf[i].transport) {
+            gconf[i].transport = (char *)"tcp";
+        }
+        gconf[i].port = (char *) qemu_opt_get(opts, GLUSTER_OPT_PORT);
+        if (!gconf[i].port) {
+            gconf[i].port = (char *)"24007";
+        }
+
+        /*
+         * reset current tuple opts to NULL, so that in case if the next tuple
+         * misses any of (server, tranport, port) options there is no chance of
+         * copying from current set.
+         */
+        qemu_opt_set(opts, GLUSTER_OPT_PORT, NULL, &error_abort);
+        qemu_opt_set(opts, GLUSTER_OPT_TRANSPORT, NULL, &error_abort);
+        qemu_opt_set(opts, GLUSTER_OPT_SERVER, NULL, &error_abort);
+    }
+    *pgconf = gconf;
+    return num_servers;
+
+out:
+    ret = -EINVAL;
+    errno = -ret;
+    qemu_opts_del(opts);
+    return ret;
+}
+
+static struct glfs  *qemu_gluster_opts_init(GlusterConf **pgconf,
+                                            QDict *options, Error **errp)
+{
+    struct glfs *glfs = NULL;
+    int num_servers = 0;
+
+    num_servers = qemu_gluster_parseopts(pgconf, options, errp);
+    if (num_servers < 1) {
+        error_setg(errp, "\n"
+                        "\n#Usage1:\n"
+                        "-drive driver=qcow2,file.driver=gluster,"
+                        "file.volname=testvol,file.image-path=/path/a.qcow2,"
+                        "file.backup-volfile-servers.0.server=1.2.3.4,"
+                        "[file.backup-volfile-servers.0.port=24007,]"
+                        "[file.backup-volfile-servers.0.transport=tcp,]"
+                        "file.backup-volfile-servers.1.server=5.6.7.8,"
+                        "[file.backup-volfile-servers.1.port=24008,]"
+                        "[file.backup-volfile-servers.1.transport=rdma,] ...\n"
+                        "\n#Usage2:\n'json:{\"driver\":\"qcow2\",\"file\":"
+                        "{\"driver\":\"gluster\",\"volname\":\""
+                        "testvol\",\"image-path\":\"/path/a.qcow2\","
+                        "\"backup-volfile-servers\":[{\"server\":\"1.2.3.4\","
+                        "\"port\":\"24007\",\"transport\":\"tcp\"},"
+                        "{\"server\":\"4.5.6.7\",\"port\":\"24007\","
+                        "\"transport\":\"rdma\"}, ...]}}'\n");
+        goto out;
+    }
+    glfs = qemu_gluster_glfs_init(*pgconf, num_servers, errp);
+    if (!glfs) {
+        goto out;
+    }
+    return glfs;
+
+out:
+    return NULL;
+}
+
 static void qemu_gluster_complete_aio(void *opaque)
 {
     GlusterAIOCB *acb = (GlusterAIOCB *)opaque;
@@ -254,20 +581,6 @@  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",
-            .type = QEMU_OPT_STRING,
-            .help = "URL to the gluster image",
-        },
-        { /* end of list */ }
-    },
-};
-
 static void qemu_gluster_parse_flags(int bdrv_flags, int *open_flags)
 {
     assert(open_flags != NULL);
@@ -291,11 +604,12 @@  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;
+    const char *filename = NULL;
 
+    qdict_flatten(options);
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (local_err) {
@@ -305,8 +619,12 @@  static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
     }
 
     filename = qemu_opt_get(opts, "filename");
-
-    s->glfs = qemu_gluster_init(gconf, filename, errp);
+    if (filename) {
+        gconf = g_new0(GlusterConf, 1);
+        s->glfs = qemu_gluster_init(gconf, filename, errp);
+    } else {
+        s->glfs = qemu_gluster_opts_init(&gconf, options, errp);
+    }
     if (!s->glfs) {
         ret = -errno;
         goto out;
@@ -321,7 +639,11 @@  static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
 
 out:
     qemu_opts_del(opts);
-    qemu_gluster_gconf_free(gconf);
+    if (filename) {
+        qemu_gluster_gconf_free(gconf);
+    } else {
+        g_free(gconf);
+    }
     if (!ret) {
         return ret;
     }
@@ -339,7 +661,6 @@  typedef struct BDRVGlusterReopenState {
     struct glfs_fd *fd;
 } BDRVGlusterReopenState;
 
-
 static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
                                        BlockReopenQueue *queue, Error **errp)
 {
@@ -401,7 +722,6 @@  static void qemu_gluster_reopen_commit(BDRVReopenState *state)
     return;
 }
 
-
 static void qemu_gluster_reopen_abort(BDRVReopenState *state)
 {
     BDRVGlusterReopenState *reop_s = state->opaque;
@@ -718,7 +1038,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 +1065,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 +1119,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..490a2f2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1382,7 +1382,7 @@ 
   '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',
+            'qcow', 'qcow2', 'qed', 'quorum', 'gluster', 'raw', 'tftp', 'vdi', 'vhdx',
             'vmdk', 'vpc', 'vvfat' ] }
 
 ##
@@ -1794,6 +1794,43 @@ 
             '*read-pattern': 'QuorumReadPattern' } }
 
 ##
+# @GlusterTuplePattern
+#
+# Gluster tuple pattern
+#
+# @transport:    #transport type used to connect to gluster management daemon
+#                 it can be tcp|rdma (default 'tcp')
+#
+# @port:         port number on which glusterd is listening. (default 24007)
+#
+# @server:       server address (hostname/ipv4/ipv6 addresses)
+#
+# Since: 2.4+
+##
+{ 'struct': 'GlusterTuplePattern',
+  'data': { 'server': 'str',
+            'transport': 'str',
+            'port': 'str' } }
+
+##
+# @BlockdevOptionsGluster
+#
+# Driver specific block device options for Gluster
+#
+# @volname:      name of gluster volume where our VM image resides
+#
+# @image-path:   absolute path to image file in gluster volume
+#
+# @backup-volfile-servers: holds multiple tuples of {server, transport, port}
+#
+# Since: 2.4+
+##
+{ 'struct': 'BlockdevOptionsGluster',
+  'data': { 'volname': 'str',
+            'image-path': 'str',
+            'backup-volfile-servers': [ 'GlusterTuplePattern' ] } }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.
@@ -1814,6 +1851,7 @@ 
       'ftp':        'BlockdevOptionsFile',
       'ftps':       'BlockdevOptionsFile',
 # TODO gluster: Wait for structured options
+      'gluster':    'BlockdevOptionsGluster',
       'host_cdrom': 'BlockdevOptionsFile',
       'host_device':'BlockdevOptionsFile',
       'host_floppy':'BlockdevOptionsFile',