| Message ID | 1468594858-26889-4-git-send-email-prasanna.kalever@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: > gluster volfile server fetch happens through unix and/or tcp, it doesn't > support volfile fetch over rdma, hence removing the dead code > > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> > --- > block/gluster.c | 35 +---------------------------------- > 1 file changed, 1 insertion(+), 34 deletions(-) > > diff --git a/block/gluster.c b/block/gluster.c > index 40ee852..59f77bb 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -134,8 +134,7 @@ static int parse_volume_options(GlusterConf *gconf, char *path) > * > * 'transport' specifies the transport type used to connect to gluster > * management daemon (glusterd). Valid transport types are > - * tcp, unix and rdma. If a transport type isn't specified, then tcp > - * type is assumed. > + * tcp, unix. If a transport type isn't specified, then tcp type is assumed. > * > * 'host' specifies the host where the volume file specification for > * the given volume resides. This can be either hostname, ipv4 address > @@ -162,7 +161,6 @@ static int parse_volume_options(GlusterConf *gconf, char *path) > * file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir/a.img > * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img > * 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) > { > @@ -184,8 +182,6 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename) > } else if (!strcmp(uri->scheme, "gluster+unix")) { > gconf->transport = g_strdup("unix"); Outside this patch's scope: string literals would be just fine for gconf->transport. > is_unix = true; > - } else if (!strcmp(uri->scheme, "gluster+rdma")) { > - gconf->transport = g_strdup("rdma"); > } else { > ret = -EINVAL; > goto out; > @@ -1048,37 +1044,8 @@ static BlockDriver bdrv_gluster_unix = { > .create_opts = &qemu_gluster_create_opts, > }; > > -static BlockDriver bdrv_gluster_rdma = { > - .format_name = "gluster", > - .protocol_name = "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, > - .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size, > - .bdrv_truncate = qemu_gluster_truncate, > - .bdrv_co_readv = qemu_gluster_co_readv, > - .bdrv_co_writev = qemu_gluster_co_writev, > - .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, > - .bdrv_has_zero_init = qemu_gluster_has_zero_init, > -#ifdef CONFIG_GLUSTERFS_DISCARD > - .bdrv_co_discard = qemu_gluster_co_discard, > -#endif > -#ifdef CONFIG_GLUSTERFS_ZEROFILL > - .bdrv_co_pwrite_zeroes = qemu_gluster_co_pwrite_zeroes, > -#endif > - .bdrv_co_get_block_status = qemu_gluster_co_get_block_status, > - .create_opts = &qemu_gluster_create_opts, > -}; > - > static void bdrv_gluster_init(void) > { > - bdrv_register(&bdrv_gluster_rdma); > bdrv_register(&bdrv_gluster_unix); > bdrv_register(&bdrv_gluster_tcp); > bdrv_register(&bdrv_gluster); This is fine if gluster+rdma never actually worked. I tried to find out at https://github.com/gluster/glusterfs/blob/master/api/src/glfs.h. Transport rdma is mentioned there. Does it work?
On Mon, Jul 18, 2016 at 2:23 PM, Markus Armbruster <armbru@redhat.com> wrote: > Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: > >> gluster volfile server fetch happens through unix and/or tcp, it doesn't >> support volfile fetch over rdma, hence removing the dead code >> >> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> >> --- >> block/gluster.c | 35 +---------------------------------- >> 1 file changed, 1 insertion(+), 34 deletions(-) >> >> diff --git a/block/gluster.c b/block/gluster.c >> index 40ee852..59f77bb 100644 >> --- a/block/gluster.c >> +++ b/block/gluster.c >> @@ -134,8 +134,7 @@ static int parse_volume_options(GlusterConf *gconf, char *path) >> * >> * 'transport' specifies the transport type used to connect to gluster >> * management daemon (glusterd). Valid transport types are >> - * tcp, unix and rdma. If a transport type isn't specified, then tcp >> - * type is assumed. >> + * tcp, unix. If a transport type isn't specified, then tcp type is assumed. >> * >> * 'host' specifies the host where the volume file specification for >> * the given volume resides. This can be either hostname, ipv4 address >> @@ -162,7 +161,6 @@ static int parse_volume_options(GlusterConf *gconf, char *path) >> * file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir/a.img >> * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img >> * 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) >> { >> @@ -184,8 +182,6 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename) >> } else if (!strcmp(uri->scheme, "gluster+unix")) { >> gconf->transport = g_strdup("unix"); > > Outside this patch's scope: string literals would be just fine for > gconf->transport. If we remove rdma support, again comments here may drag people into confusion. Do you recommend to have this as a separate patch ? > >> is_unix = true; >> - } else if (!strcmp(uri->scheme, "gluster+rdma")) { >> - gconf->transport = g_strdup("rdma"); >> } else { >> ret = -EINVAL; >> goto out; >> @@ -1048,37 +1044,8 @@ static BlockDriver bdrv_gluster_unix = { >> .create_opts = &qemu_gluster_create_opts, >> }; >> >> -static BlockDriver bdrv_gluster_rdma = { >> - .format_name = "gluster", >> - .protocol_name = "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, >> - .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size, >> - .bdrv_truncate = qemu_gluster_truncate, >> - .bdrv_co_readv = qemu_gluster_co_readv, >> - .bdrv_co_writev = qemu_gluster_co_writev, >> - .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, >> - .bdrv_has_zero_init = qemu_gluster_has_zero_init, >> -#ifdef CONFIG_GLUSTERFS_DISCARD >> - .bdrv_co_discard = qemu_gluster_co_discard, >> -#endif >> -#ifdef CONFIG_GLUSTERFS_ZEROFILL >> - .bdrv_co_pwrite_zeroes = qemu_gluster_co_pwrite_zeroes, >> -#endif >> - .bdrv_co_get_block_status = qemu_gluster_co_get_block_status, >> - .create_opts = &qemu_gluster_create_opts, >> -}; >> - >> static void bdrv_gluster_init(void) >> { >> - bdrv_register(&bdrv_gluster_rdma); >> bdrv_register(&bdrv_gluster_unix); >> bdrv_register(&bdrv_gluster_tcp); >> bdrv_register(&bdrv_gluster); > > This is fine if gluster+rdma never actually worked. I tried to find out > at https://github.com/gluster/glusterfs/blob/master/api/src/glfs.h. > Transport rdma is mentioned there. Does it work? this is transport used for fetching the volume file from the nodes. Which is of type tcp previously and then now it also supports the unix transport. More response from gluster community @http://www.gluster.org/pipermail/gluster-devel/2016-July/050112.html Thanks Markus! -- Prasanna
Prasanna Kalever <pkalever@redhat.com> writes: > On Mon, Jul 18, 2016 at 2:23 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: >> >>> gluster volfile server fetch happens through unix and/or tcp, it doesn't >>> support volfile fetch over rdma, hence removing the dead code >>> >>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> >>> --- >>> block/gluster.c | 35 +---------------------------------- >>> 1 file changed, 1 insertion(+), 34 deletions(-) >>> >>> diff --git a/block/gluster.c b/block/gluster.c >>> index 40ee852..59f77bb 100644 >>> --- a/block/gluster.c >>> +++ b/block/gluster.c >>> @@ -134,8 +134,7 @@ static int parse_volume_options(GlusterConf *gconf, char *path) >>> * >>> * 'transport' specifies the transport type used to connect to gluster >>> * management daemon (glusterd). Valid transport types are >>> - * tcp, unix and rdma. If a transport type isn't specified, then tcp >>> - * type is assumed. >>> + * tcp, unix. If a transport type isn't specified, then tcp type is assumed. >>> * >>> * 'host' specifies the host where the volume file specification for >>> * the given volume resides. This can be either hostname, ipv4 address >>> @@ -162,7 +161,6 @@ static int parse_volume_options(GlusterConf *gconf, char *path) >>> * file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir/a.img >>> * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img >>> * 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) >>> { >>> @@ -184,8 +182,6 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename) >>> } else if (!strcmp(uri->scheme, "gluster+unix")) { >>> gconf->transport = g_strdup("unix"); >> >> Outside this patch's scope: string literals would be just fine for >> gconf->transport. > > If we remove rdma support, again comments here may drag people into confusion. > Do you recommend to have this as a separate patch ? I'm afraid we're talking about totally different things. But it doesn't actually matter, because I now see that I got confused. Simply ignore this comment. >> >>> is_unix = true; >>> - } else if (!strcmp(uri->scheme, "gluster+rdma")) { >>> - gconf->transport = g_strdup("rdma"); >>> } else { >>> ret = -EINVAL; >>> goto out; >>> @@ -1048,37 +1044,8 @@ static BlockDriver bdrv_gluster_unix = { >>> .create_opts = &qemu_gluster_create_opts, >>> }; >>> >>> -static BlockDriver bdrv_gluster_rdma = { >>> - .format_name = "gluster", >>> - .protocol_name = "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, >>> - .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size, >>> - .bdrv_truncate = qemu_gluster_truncate, >>> - .bdrv_co_readv = qemu_gluster_co_readv, >>> - .bdrv_co_writev = qemu_gluster_co_writev, >>> - .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, >>> - .bdrv_has_zero_init = qemu_gluster_has_zero_init, >>> -#ifdef CONFIG_GLUSTERFS_DISCARD >>> - .bdrv_co_discard = qemu_gluster_co_discard, >>> -#endif >>> -#ifdef CONFIG_GLUSTERFS_ZEROFILL >>> - .bdrv_co_pwrite_zeroes = qemu_gluster_co_pwrite_zeroes, >>> -#endif >>> - .bdrv_co_get_block_status = qemu_gluster_co_get_block_status, >>> - .create_opts = &qemu_gluster_create_opts, >>> -}; >>> - >>> static void bdrv_gluster_init(void) >>> { >>> - bdrv_register(&bdrv_gluster_rdma); >>> bdrv_register(&bdrv_gluster_unix); >>> bdrv_register(&bdrv_gluster_tcp); >>> bdrv_register(&bdrv_gluster); >> >> This is fine if gluster+rdma never actually worked. I tried to find out >> at https://github.com/gluster/glusterfs/blob/master/api/src/glfs.h. >> Transport rdma is mentioned there. Does it work? > > this is transport used for fetching the volume file from the nodes. > Which is of type tcp previously and then now it also supports the unix > transport. > > More response from gluster community > @http://www.gluster.org/pipermail/gluster-devel/2016-July/050112.html Quote Raghavendra Talur's reply: > My understanding is that @transport argumet in > glfs_set_volfile_server() is meant for specifying transport used in > fetching volfile server, > Yes, @transport arg here is transport to use for fetching volfile. > IIRC which currently supports tcp and unix only... > Yes, this is correct too. Here, Raghavendra seems to confirm that only tcp and unix are supported. > > The doc here > https://github.com/gluster/glusterfs/blob/master/api/src/glfs.h > +166 shows the rdma as well, which is something I cannot digest. > This is doc written with assumption that rdma would work too. > > > Can someone correct me ? > > Have we ever supported volfile fetch over rdma ? > I think no. To test, you would have to set rdma as only transport option in glusterd.vol and see what happens in volfile fetch. But here, it sounds like it might work anyway! IMO, fetching volfile over rdma is an overkill and would not be required. RDMA should be kept only for IO operations. We should just remove it from the docs. Don't misunderstand me, I'm very much in favor of removing the rdma transport here. All I'm trying to do is figure out what backward compatibility ramifications that might have. If protocol gluster+rdma has never actually worked, we can safely remove it. But if it happens to work even though it isn't really supported, the situation is complicated. Dropping it might break working setups. Could perhaps be justified by "your setup works, but it's unsupported, and I'm forcing you to switch to a supported setup now, before you come to grief." If it used to work but no more, or if it will stop working, it's differently complicated. Dropping it would still break working setups, but they'd be bound to break anyway. Thus, my questions: does protocol gluster+rdma work before your patch? If no, did it ever work? "I don't know" is an acceptable answer to the latter question, but not so much to the former, because that one is easily testable. Once we have answers to these questions, we can decide what needs to be done for compatibility, if anything.
On Mon, Jul 18, 2016 at 5:48 PM, Markus Armbruster <armbru@redhat.com> wrote: > Prasanna Kalever <pkalever@redhat.com> writes: > > > On Mon, Jul 18, 2016 at 2:23 PM, Markus Armbruster <armbru@redhat.com> > wrote: > >> Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: > >> > >>> gluster volfile server fetch happens through unix and/or tcp, it > doesn't > >>> support volfile fetch over rdma, hence removing the dead code > >>> > >>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> > >>> --- > >>> block/gluster.c | 35 +---------------------------------- > >>> 1 file changed, 1 insertion(+), 34 deletions(-) > >>> > >>> diff --git a/block/gluster.c b/block/gluster.c > >>> index 40ee852..59f77bb 100644 > >>> --- a/block/gluster.c > >>> +++ b/block/gluster.c > >>> @@ -134,8 +134,7 @@ static int parse_volume_options(GlusterConf > *gconf, char *path) > >>> * > >>> * 'transport' specifies the transport type used to connect to gluster > >>> * management daemon (glusterd). Valid transport types are > >>> - * tcp, unix and rdma. If a transport type isn't specified, then tcp > >>> - * type is assumed. > >>> + * tcp, unix. If a transport type isn't specified, then tcp type is > assumed. > >>> * > >>> * 'host' specifies the host where the volume file specification for > >>> * the given volume resides. This can be either hostname, ipv4 address > >>> @@ -162,7 +161,6 @@ static int parse_volume_options(GlusterConf > *gconf, char *path) > >>> * file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir/a.img > >>> * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img > >>> * 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) > >>> { > >>> @@ -184,8 +182,6 @@ static int qemu_gluster_parseuri(GlusterConf > *gconf, const char *filename) > >>> } else if (!strcmp(uri->scheme, "gluster+unix")) { > >>> gconf->transport = g_strdup("unix"); > >> > >> Outside this patch's scope: string literals would be just fine for > >> gconf->transport. > > > > If we remove rdma support, again comments here may drag people into > confusion. > > Do you recommend to have this as a separate patch ? > > I'm afraid we're talking about totally different things. But it doesn't > actually matter, because I now see that I got confused. Simply ignore > this comment. > > >> > >>> is_unix = true; > >>> - } else if (!strcmp(uri->scheme, "gluster+rdma")) { > >>> - gconf->transport = g_strdup("rdma"); > >>> } else { > >>> ret = -EINVAL; > >>> goto out; > >>> @@ -1048,37 +1044,8 @@ static BlockDriver bdrv_gluster_unix = { > >>> .create_opts = &qemu_gluster_create_opts, > >>> }; > >>> > >>> -static BlockDriver bdrv_gluster_rdma = { > >>> - .format_name = "gluster", > >>> - .protocol_name = "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, > >>> - .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size, > >>> - .bdrv_truncate = qemu_gluster_truncate, > >>> - .bdrv_co_readv = qemu_gluster_co_readv, > >>> - .bdrv_co_writev = qemu_gluster_co_writev, > >>> - .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, > >>> - .bdrv_has_zero_init = qemu_gluster_has_zero_init, > >>> -#ifdef CONFIG_GLUSTERFS_DISCARD > >>> - .bdrv_co_discard = qemu_gluster_co_discard, > >>> -#endif > >>> -#ifdef CONFIG_GLUSTERFS_ZEROFILL > >>> - .bdrv_co_pwrite_zeroes = qemu_gluster_co_pwrite_zeroes, > >>> -#endif > >>> - .bdrv_co_get_block_status = qemu_gluster_co_get_block_status, > >>> - .create_opts = &qemu_gluster_create_opts, > >>> -}; > >>> - > >>> static void bdrv_gluster_init(void) > >>> { > >>> - bdrv_register(&bdrv_gluster_rdma); > >>> bdrv_register(&bdrv_gluster_unix); > >>> bdrv_register(&bdrv_gluster_tcp); > >>> bdrv_register(&bdrv_gluster); > >> > >> This is fine if gluster+rdma never actually worked. I tried to find out > >> at https://github.com/gluster/glusterfs/blob/master/api/src/glfs.h. > >> Transport rdma is mentioned there. Does it work? > > > > this is transport used for fetching the volume file from the nodes. > > Which is of type tcp previously and then now it also supports the unix > > transport. > > > > More response from gluster community > > @http://www.gluster.org/pipermail/gluster-devel/2016-July/050112.html > > Quote Raghavendra Talur's reply: > > > My understanding is that @transport argumet in > > glfs_set_volfile_server() is meant for specifying transport used in > > fetching volfile server, > > > > Yes, @transport arg here is transport to use for fetching volfile. > > > > IIRC which currently supports tcp and unix only... > > > Yes, this is correct too. > > Here, Raghavendra seems to confirm that only tcp and unix are supported. > > > > > The doc here > > https://github.com/gluster/glusterfs/blob/master/api/src/glfs.h > > +166 shows the rdma as well, which is something I cannot digest. > > > This is doc written with assumption that rdma would work too. > > > > > > > > Can someone correct me ? > > > > Have we ever supported volfile fetch over rdma ? > > > > I think no. To test, you would have to set rdma as only transport > option in > glusterd.vol and see what happens in volfile fetch. > > But here, it sounds like it might work anyway! > Prasanna, Rafi and I tested this. When rdma option is specified, gluster defaults to tcp silently. I do not like this behavior. > IMO, fetching volfile over rdma is an overkill and would not be > required. > RDMA should be kept only for IO operations. > > We should just remove it from the docs. > > Don't misunderstand me, I'm very much in favor of removing the rdma > transport here. All I'm trying to do is figure out what backward > compatibility ramifications that might have. > > If protocol gluster+rdma has never actually worked, we can safely remove > it. > > But if it happens to work even though it isn't really supported, the > situation is complicated. Dropping it might break working setups. > Could perhaps be justified by "your setup works, but it's unsupported, > and I'm forcing you to switch to a supported setup now, before you come > to grief." > > If it used to work but no more, or if it will stop working, it's > differently complicated. Dropping it would still break working setups, > but they'd be bound to break anyway. > > Thus, my questions: does protocol gluster+rdma work before your patch? > If no, did it ever work? "I don't know" is an acceptable answer to the > latter question, but not so much to the former, because that one is > easily testable. > Yes, it appeared to user that the option worked and removing the option would break such setups. I agree with Markus that removing the option right away would hurt users and we should follow a deprecation strategy for backward compatibility. > > Once we have answers to these questions, we can decide what needs to be > done for compatibility, if anything. >
On Mon, Jul 18, 2016 at 7:05 PM, Raghavendra Talur <rtalur@redhat.com> wrote: > > > On Mon, Jul 18, 2016 at 5:48 PM, Markus Armbruster <armbru@redhat.com> > wrote: >> >> Prasanna Kalever <pkalever@redhat.com> writes: >> >> > On Mon, Jul 18, 2016 at 2:23 PM, Markus Armbruster <armbru@redhat.com> >> > wrote: >> >> Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: >> >> >> >>> gluster volfile server fetch happens through unix and/or tcp, it >> >>> doesn't >> >>> support volfile fetch over rdma, hence removing the dead code >> >>> >> >>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> >> >>> --- >> >>> block/gluster.c | 35 +---------------------------------- >> >>> 1 file changed, 1 insertion(+), 34 deletions(-) >> >>> >> >>> diff --git a/block/gluster.c b/block/gluster.c >> >>> index 40ee852..59f77bb 100644 >> >>> --- a/block/gluster.c >> >>> +++ b/block/gluster.c >> >>> @@ -134,8 +134,7 @@ static int parse_volume_options(GlusterConf >> >>> *gconf, char *path) >> >>> * >> >>> * 'transport' specifies the transport type used to connect to >> >>> gluster >> >>> * management daemon (glusterd). Valid transport types are >> >>> - * tcp, unix and rdma. If a transport type isn't specified, then tcp >> >>> - * type is assumed. >> >>> + * tcp, unix. If a transport type isn't specified, then tcp type is >> >>> assumed. >> >>> * >> >>> * 'host' specifies the host where the volume file specification for >> >>> * the given volume resides. This can be either hostname, ipv4 >> >>> address >> >>> @@ -162,7 +161,6 @@ static int parse_volume_options(GlusterConf >> >>> *gconf, char *path) >> >>> * file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir/a.img >> >>> * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img >> >>> * 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) >> >>> { >> >>> @@ -184,8 +182,6 @@ static int qemu_gluster_parseuri(GlusterConf >> >>> *gconf, const char *filename) >> >>> } else if (!strcmp(uri->scheme, "gluster+unix")) { >> >>> gconf->transport = g_strdup("unix"); >> >> >> >> Outside this patch's scope: string literals would be just fine for >> >> gconf->transport. >> > >> > If we remove rdma support, again comments here may drag people into >> > confusion. >> > Do you recommend to have this as a separate patch ? >> >> I'm afraid we're talking about totally different things. But it doesn't >> actually matter, because I now see that I got confused. Simply ignore >> this comment. >> >> >> >> >>> is_unix = true; >> >>> - } else if (!strcmp(uri->scheme, "gluster+rdma")) { >> >>> - gconf->transport = g_strdup("rdma"); >> >>> } else { >> >>> ret = -EINVAL; >> >>> goto out; >> >>> @@ -1048,37 +1044,8 @@ static BlockDriver bdrv_gluster_unix = { >> >>> .create_opts = &qemu_gluster_create_opts, >> >>> }; >> >>> >> >>> -static BlockDriver bdrv_gluster_rdma = { >> >>> - .format_name = "gluster", >> >>> - .protocol_name = "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, >> >>> - .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size, >> >>> - .bdrv_truncate = qemu_gluster_truncate, >> >>> - .bdrv_co_readv = qemu_gluster_co_readv, >> >>> - .bdrv_co_writev = qemu_gluster_co_writev, >> >>> - .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, >> >>> - .bdrv_has_zero_init = qemu_gluster_has_zero_init, >> >>> -#ifdef CONFIG_GLUSTERFS_DISCARD >> >>> - .bdrv_co_discard = qemu_gluster_co_discard, >> >>> -#endif >> >>> -#ifdef CONFIG_GLUSTERFS_ZEROFILL >> >>> - .bdrv_co_pwrite_zeroes = qemu_gluster_co_pwrite_zeroes, >> >>> -#endif >> >>> - .bdrv_co_get_block_status = qemu_gluster_co_get_block_status, >> >>> - .create_opts = &qemu_gluster_create_opts, >> >>> -}; >> >>> - >> >>> static void bdrv_gluster_init(void) >> >>> { >> >>> - bdrv_register(&bdrv_gluster_rdma); >> >>> bdrv_register(&bdrv_gluster_unix); >> >>> bdrv_register(&bdrv_gluster_tcp); >> >>> bdrv_register(&bdrv_gluster); >> >> >> >> This is fine if gluster+rdma never actually worked. I tried to find >> >> out >> >> at https://github.com/gluster/glusterfs/blob/master/api/src/glfs.h. >> >> Transport rdma is mentioned there. Does it work? >> > >> > this is transport used for fetching the volume file from the nodes. >> > Which is of type tcp previously and then now it also supports the unix >> > transport. >> > >> > More response from gluster community >> > @http://www.gluster.org/pipermail/gluster-devel/2016-July/050112.html >> >> Quote Raghavendra Talur's reply: >> >> > My understanding is that @transport argumet in >> > glfs_set_volfile_server() is meant for specifying transport used in >> > fetching volfile server, >> > >> >> Yes, @transport arg here is transport to use for fetching volfile. >> >> >> > IIRC which currently supports tcp and unix only... >> > >> Yes, this is correct too. >> >> Here, Raghavendra seems to confirm that only tcp and unix are supported. >> >> > >> > The doc here >> > https://github.com/gluster/glusterfs/blob/master/api/src/glfs.h >> > +166 shows the rdma as well, which is something I cannot digest. >> > >> This is doc written with assumption that rdma would work too. >> >> >> > >> > >> > Can someone correct me ? >> > >> > Have we ever supported volfile fetch over rdma ? >> > >> >> I think no. To test, you would have to set rdma as only transport >> option in >> glusterd.vol and see what happens in volfile fetch. >> >> But here, it sounds like it might work anyway! > > > Prasanna, Rafi and I tested this. When rdma option is specified, gluster > defaults to tcp silently. I do not like this behavior. > >> >> IMO, fetching volfile over rdma is an overkill and would not be >> required. >> RDMA should be kept only for IO operations. >> >> We should just remove it from the docs. >> >> Don't misunderstand me, I'm very much in favor of removing the rdma >> transport here. All I'm trying to do is figure out what backward >> compatibility ramifications that might have. >> >> If protocol gluster+rdma has never actually worked, we can safely remove >> it. >> >> But if it happens to work even though it isn't really supported, the >> situation is complicated. Dropping it might break working setups. >> Could perhaps be justified by "your setup works, but it's unsupported, >> and I'm forcing you to switch to a supported setup now, before you come >> to grief." >> >> If it used to work but no more, or if it will stop working, it's >> differently complicated. Dropping it would still break working setups, >> but they'd be bound to break anyway. >> >> Thus, my questions: does protocol gluster+rdma work before your patch? >> If no, did it ever work? "I don't know" is an acceptable answer to the >> latter question, but not so much to the former, because that one is >> easily testable. > > > Yes, it appeared to user that the option worked and removing the option > would break such setups. I agree with Markus that removing the option right > away would hurt users and we should follow a deprecation strategy for > backward compatibility. I agree with Raghavendra and Markus here. Hence, just for backward compatibility I would like to redirect the rdma transport to tcp may be with a warning, which says this is deprecated and something gluster won't even support after its 4.0 (strict checks) Markus thanks for pointing me the docs links there, I shall right away remove it from glfs.h In any regards I am not in favor of providing rdma as part of Gluster transport schema. Any suggestions are deeply appreciable. Thanks, -- Prasanna > >> >> >> Once we have answers to these questions, we can decide what needs to be >> done for compatibility, if anything. > >
Raghavendra Talur <rtalur@redhat.com> writes: > On Mon, Jul 18, 2016 at 5:48 PM, Markus Armbruster <armbru@redhat.com> > wrote: > >> Prasanna Kalever <pkalever@redhat.com> writes: >> >> > On Mon, Jul 18, 2016 at 2:23 PM, Markus Armbruster <armbru@redhat.com> [...] >> >> This is fine if gluster+rdma never actually worked. I tried to find out >> >> at https://github.com/gluster/glusterfs/blob/master/api/src/glfs.h. >> >> Transport rdma is mentioned there. Does it work? >> > >> > this is transport used for fetching the volume file from the nodes. >> > Which is of type tcp previously and then now it also supports the unix >> > transport. >> > >> > More response from gluster community >> > @http://www.gluster.org/pipermail/gluster-devel/2016-July/050112.html >> >> Quote Raghavendra Talur's reply: >> >> > My understanding is that @transport argumet in >> > glfs_set_volfile_server() is meant for specifying transport used in >> > fetching volfile server, >> > >> >> Yes, @transport arg here is transport to use for fetching volfile. >> >> >> > IIRC which currently supports tcp and unix only... >> > >> Yes, this is correct too. >> >> Here, Raghavendra seems to confirm that only tcp and unix are supported. >> >> > >> > The doc here >> > https://github.com/gluster/glusterfs/blob/master/api/src/glfs.h >> > +166 shows the rdma as well, which is something I cannot digest. >> > >> This is doc written with assumption that rdma would work too. >> >> >> > >> > >> > Can someone correct me ? >> > >> > Have we ever supported volfile fetch over rdma ? >> > >> >> I think no. To test, you would have to set rdma as only transport >> option in >> glusterd.vol and see what happens in volfile fetch. >> >> But here, it sounds like it might work anyway! >> > > Prasanna, Rafi and I tested this. When rdma option is specified, gluster > defaults to tcp silently. Good to know. Thanks! > I do not like this behavior. Understandable :) >> IMO, fetching volfile over rdma is an overkill and would not be >> required. >> RDMA should be kept only for IO operations. >> >> We should just remove it from the docs. >> >> Don't misunderstand me, I'm very much in favor of removing the rdma >> transport here. All I'm trying to do is figure out what backward >> compatibility ramifications that might have. >> >> If protocol gluster+rdma has never actually worked, we can safely remove >> it. >> >> But if it happens to work even though it isn't really supported, the >> situation is complicated. Dropping it might break working setups. >> Could perhaps be justified by "your setup works, but it's unsupported, >> and I'm forcing you to switch to a supported setup now, before you come >> to grief." >> >> If it used to work but no more, or if it will stop working, it's >> differently complicated. Dropping it would still break working setups, >> but they'd be bound to break anyway. >> >> Thus, my questions: does protocol gluster+rdma work before your patch? >> If no, did it ever work? "I don't know" is an acceptable answer to the >> latter question, but not so much to the former, because that one is >> easily testable. >> > > Yes, it appeared to user that the option worked and removing the option > would break such setups. I agree with Markus that removing the option right > away would hurt users and we should follow a deprecation strategy for > backward compatibility. Since Gluster maps rdma to tcp, I think the following should do: * If the user asks for file=gluster+rdma:..., print a warning and use file=gluster+tcp:... instead. Deprecate this usage. * Don't add transport 'rdma' to the QAPI schema. What do you think? [...]
diff --git a/block/gluster.c b/block/gluster.c index 40ee852..59f77bb 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -134,8 +134,7 @@ static int parse_volume_options(GlusterConf *gconf, char *path) * * 'transport' specifies the transport type used to connect to gluster * management daemon (glusterd). Valid transport types are - * tcp, unix and rdma. If a transport type isn't specified, then tcp - * type is assumed. + * tcp, unix. If a transport type isn't specified, then tcp type is assumed. * * 'host' specifies the host where the volume file specification for * the given volume resides. This can be either hostname, ipv4 address @@ -162,7 +161,6 @@ static int parse_volume_options(GlusterConf *gconf, char *path) * file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir/a.img * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img * 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) { @@ -184,8 +182,6 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename) } else if (!strcmp(uri->scheme, "gluster+unix")) { gconf->transport = g_strdup("unix"); is_unix = true; - } else if (!strcmp(uri->scheme, "gluster+rdma")) { - gconf->transport = g_strdup("rdma"); } else { ret = -EINVAL; goto out; @@ -1048,37 +1044,8 @@ static BlockDriver bdrv_gluster_unix = { .create_opts = &qemu_gluster_create_opts, }; -static BlockDriver bdrv_gluster_rdma = { - .format_name = "gluster", - .protocol_name = "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, - .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size, - .bdrv_truncate = qemu_gluster_truncate, - .bdrv_co_readv = qemu_gluster_co_readv, - .bdrv_co_writev = qemu_gluster_co_writev, - .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, - .bdrv_has_zero_init = qemu_gluster_has_zero_init, -#ifdef CONFIG_GLUSTERFS_DISCARD - .bdrv_co_discard = qemu_gluster_co_discard, -#endif -#ifdef CONFIG_GLUSTERFS_ZEROFILL - .bdrv_co_pwrite_zeroes = qemu_gluster_co_pwrite_zeroes, -#endif - .bdrv_co_get_block_status = qemu_gluster_co_get_block_status, - .create_opts = &qemu_gluster_create_opts, -}; - static void bdrv_gluster_init(void) { - bdrv_register(&bdrv_gluster_rdma); bdrv_register(&bdrv_gluster_unix); bdrv_register(&bdrv_gluster_tcp); bdrv_register(&bdrv_gluster);
gluster volfile server fetch happens through unix and/or tcp, it doesn't support volfile fetch over rdma, hence removing the dead code Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- block/gluster.c | 35 +---------------------------------- 1 file changed, 1 insertion(+), 34 deletions(-)