Return-Path: Received: from aserp1040.oracle.com ([141.146.126.69]:47135 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751898AbcCVWCL convert rfc822-to-8bit (ORCPT ); Tue, 22 Mar 2016 18:02:11 -0400 Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 9.2 \(3112\)) Subject: Re: [PATCH] nfs: add nfs IPv6 rdma6 mount option support From: Chuck Lever In-Reply-To: Date: Tue, 22 Mar 2016 18:01:58 -0400 Cc: Anna Schumaker , Shirley Ma , "J. Bruce Fields" , Leon Romanovsky , Linux NFS Mailing List Message-Id: <4A9EA080-30CC-4DD8-9BD8-28D54F13C582@oracle.com> References: <56E9A650.7060409@oracle.com> <56F19F28.9020504@Netapp.com> <9414CE65-D41C-4497-9111-BCAB54FB6C7B@oracle.com> To: Trond Myklebust Sender: linux-nfs-owner@vger.kernel.org List-ID: > On Mar 22, 2016, at 5:55 PM, Trond Myklebust wrote: > > On Tue, Mar 22, 2016 at 5:52 PM, Chuck Lever wrote: >> >>> On Mar 22, 2016, at 5:46 PM, Trond Myklebust wrote: >>> >>> On Tue, Mar 22, 2016 at 5:24 PM, Chuck Lever wrote: >>>> >>>>> On Mar 22, 2016, at 3:38 PM, Anna Schumaker wrote: >>>>> >>>>> Hi Shirley, >>>>> >>>>> Sorry for the delay in looking at this patch. Comments are below: >>>>> >>>>> On 03/16/2016 02:30 PM, Shirley Ma wrote: >>>>>> Add rdma6 option to support NFS/RDMA IPv6. >>>>>> >>>>>> Signed-off-by: Shirley Ma >>>>> >>>>> Can you add a little more to the patch description to describe when RDMA with IPv6 would be used? >>>>> >>>>>> --- >>>>>> >>>>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c >>>>>> index f126828..62a55d0 100644 >>>>>> --- a/fs/nfs/super.c >>>>>> +++ b/fs/nfs/super.c >>>>>> @@ -191,6 +191,7 @@ static const match_table_t nfs_mount_option_tokens = { >>>>>> >>>>>> enum { >>>>>> Opt_xprt_udp, Opt_xprt_udp6, Opt_xprt_tcp, Opt_xprt_tcp6, Opt_xprt_rdma, >>>>>> + Opt_xprt_rdma6, >>>>>> >>>>>> Opt_xprt_err >>>>>> }; >>>>>> @@ -201,6 +202,7 @@ static const match_table_t nfs_xprt_protocol_tokens = { >>>>>> { Opt_xprt_tcp, "tcp" }, >>>>>> { Opt_xprt_tcp6, "tcp6" }, >>>>>> { Opt_xprt_rdma, "rdma" }, >>>>>> + { Opt_xprt_rdma6, "rdma6" }, >>>>>> >>>>>> { Opt_xprt_err, NULL } >>>>>> }; >>>>>> @@ -1456,6 +1458,8 @@ static int nfs_parse_mount_options(char *raw, >>>>>> mnt->flags |= NFS_MOUNT_TCP; >>>>>> mnt->nfs_server.protocol = XPRT_TRANSPORT_TCP; >>>>>> break; >>>>>> + case Opt_xprt_rdma6: >>>>>> + protofamily = AF_INET6; >>>>>> case Opt_xprt_rdma: >>>>>> /* vector side protocols to TCP */ >>>>>> mnt->flags |= NFS_MOUNT_TCP; >>>>>> @@ -1490,6 +1494,8 @@ static int nfs_parse_mount_options(char *raw, >>>>>> case Opt_xprt_tcp: >>>>>> mnt->mount_server.protocol = XPRT_TRANSPORT_TCP; >>>>>> break; >>>>>> + case Opt_xprt_rdma6: >>>>>> + mountfamily = AF_INET6; >>>>>> case Opt_xprt_rdma: /* not used for side protocols */ >>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>> Do we need to be setting mountfamily here? The comment next to Opt_xprt_rdma makes it sound like this code doesn't apply to RDMA. >>>> >>>> I wondered this too. >>>> >>>> But what's going on is that for NFSv3 on RDMA, when IPv6 is >>>> used for the main protocol, IPv6 needs to be used for the >>>> mount protocol as well, even though it is going over TCP. >>> >>> It causes nfs_mount_parse_options to immediately stop further parsing >>> and return '0' == error encountered. The mount is supposed to fail in >>> that case. >> >> Ah, a break; is needed there too. >> > > I'm saying that makes no sense to allow "rdma6" here in the first > place, since we've already banned the use of "rdma" through that same > mechanism. I see, this is the parsing logic just for "mountproto=". This hunk should be dropped, and maybe the "case Opt_xprt_rdma:" can be removed, too. -- Chuck Lever