Re: [PATCH 1/1] NFSv4.1+ add trunking when server trunking detected

On Tue, 2021-06-08 at 18:18 -0400, Olga Kornievskaia wrote:

On Tue, Jun 8, 2021 at 6:13 PM Trond Myklebust < 
trondmy@hammerspace.com> wrote:

quoted

On Tue, 2021-06-08 at 17:40 -0400, Olga Kornievskaia wrote:

quoted

On Tue, Jun 8, 2021 at 5:06 PM Chuck Lever III <
chuck.lever@oracle.com> wrote:

quoted

quoted

On Jun 8, 2021, at 4:56 PM, Olga Kornievskaia <
olga.kornievskaia@gmail.com> wrote:

On Tue, Jun 8, 2021 at 4:41 PM Chuck Lever III <
chuck.lever@oracle.com> wrote:

quoted

quoted

On Jun 8, 2021, at 2:45 PM, Olga Kornievskaia <
olga.kornievskaia@gmail.com> wrote:

From: Olga Kornievskaia <redacted>

After trunking is discovered in
nfs4_discover_server_trunking(),
add the transport to the old client structure before
destroying
the new client structure (along with its transport).

Signed-off-by: Olga Kornievskaia <redacted>
---
fs/nfs/nfs4client.c | 40
++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)

diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 42719384e25f..984c851844d8 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c

@@ -361,6 +361,44 @@ static int

nfs4_init_client_minor_version(struct nfs_client *clp)
     return nfs4_init_callback(clp);
}

+static void nfs4_add_trunk(struct nfs_client *clp,
struct
nfs_client *old)
+{
+     struct sockaddr_storage clp_addr, old_addr;
+     struct sockaddr *clp_sap = (struct sockaddr
*)&clp_addr;
+     struct sockaddr *old_sap = (struct sockaddr
*)&old_addr;
+     size_t clp_salen, old_salen;
+     struct xprt_create xprt_args = {
+             .ident = old->cl_proto,
+             .net = old->cl_net,
+             .servername = old->cl_hostname,
+     };
+     struct nfs4_add_xprt_data xprtdata = {
+             .clp = old,
+     };
+     struct rpc_add_xprt_test rpcdata = {
+             .add_xprt_test = old->cl_mvops-

quoted

session_trunk,

+             .data = &xprtdata,
+     };
+
+     if (clp->cl_proto != old->cl_proto)
+             return;
+     clp_salen = rpc_peeraddr(clp->cl_rpcclient,
clp_sap,
sizeof(clp_addr));
+     old_salen = rpc_peeraddr(old->cl_rpcclient,
old_sap,
sizeof(old_addr));
+
+     if (clp_addr.ss_family != old_addr.ss_family)
+             return;
+
+     xprt_args.dstaddr = clp_sap;
+     xprt_args.addrlen = clp_salen;
+
+     xprtdata.cred = nfs4_get_clid_cred(old);
+     rpc_clnt_add_xprt(old->cl_rpcclient, &xprt_args,
+                       rpc_clnt_setup_test_and_add_xprt,
&rpcdata);

Is there an upper bound on the number of transports that
are added to the NFS client's switch?

I don't believe any limits exist right now. Why should there
be a
limit? Are you saying that the client should limit trunking?
While
this is not what's happening here, but say FS_LOCATION
returned
100
ips for the server. Are you saying the client should be
limiting
how
many trunkable connections it would be establishing and
picking
just a
few addresses to try? What's happening with this patch is
that
say
there are 100mounts to 100 ips (each representing the same
server
or
trunkable server(s)), without this patch a single connection
is
kept,
with this patch we'll have 100 connections.

The patch description needs to make this behavior more clear.
It
needs to explain "why" -- the body of the patch already
explains
"what". Can you include your last sentence in the description
as
a use case?

I sure can.

quoted

As for the behavior... Seems to me that there are going to be
only
infinitesimal gains after the first few connections, and after
that, it's going to be a lot for both sides to manage for no
real
gain. I think you do want to cap the total number of
connections
at a reasonable number, even in the FS_LOCATIONS case.

Do you want to cap it at 16 like we do for nconnect? I'm ok with
that
for now.

I don't understand why a setup where there X NICs on each side
(client/server) and X mounts are done, there won't be throughput
of
close to X * throughput of a single NIC.

That still does not make the patch acceptable. There are already
server
vendors seeing problems with supporting nconnect for various
reasons.

Why are there servers presenting multiple IP addresses and returning
the same server identity if they can not support trunking? That seems
to be going against the spec?

That's not a question I can answer, but I'm not thinking of our server
or the Linux server.

quoted

There needs to be a way to ensure that we can keep the current
client
behaviour without requiring changes to server DNS entries, etc.

Ok, how about we introduce a mount option, "enable_trunking", and if
that's present we would not collapse transports?

I'd suggest making it 'max_connect=X' so that we can actually set a
limit like we do for nconnect. That limit probably needs to be lower
bounded by the value of nconnect.

quoted

quoted

quoted

quoted

quoted

quoted

+
+     if (xprtdata.cred)
+             put_cred(xprtdata.cred);
+}
+
/**
* nfs4_init_client - Initialise an NFS4 client record
*

@@ -434,6 +472,8 @@ struct nfs_client

*nfs4_init_client(struct nfs_client *clp,
              * won't try to use it.
              */
             nfs_mark_client_ready(clp, -EPERM);
+             if (old->cl_mvops->session_trunk)
+                     nfs4_add_trunk(clp, old);
     }
     clear_bit(NFS_CS_TSM_POSSIBLE, &clp->cl_flags);
     nfs_put_client(clp);
--
2.27.0

--
Chuck Lever

--
Chuck Lever

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com