On Mon, Jul 18, 2016 at 7:05 PM, Raghavendra Talur <[email protected]> wrote: > > > On Mon, Jul 18, 2016 at 5:48 PM, Markus Armbruster <[email protected]> > wrote: >> >> Prasanna Kalever <[email protected]> writes: >> >> > On Mon, Jul 18, 2016 at 2:23 PM, Markus Armbruster <[email protected]> >> > wrote: >> >> Prasanna Kumar Kalever <[email protected]> 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 <[email protected]> >> >>> --- >> >>> 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. > >
