Re: [Qemu-devel] [PATCH v19 3/5] block/gluster: remove rdma transport

[Markus Armbruster]

Prasanna Kalever <address@hidden> writes:

> On Mon, Jul 18, 2016 at 2:23 PM, Markus Armbruster <address@hidden> wrote:
>> Prasanna Kumar Kalever <address@hidden> 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 <address@hidden>
>>> ---
>>>  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.