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

On Tue, Jun 8, 2021 at 5:06 PM Chuck Lever III [off-list ref] wrote:

quoted

On Jun 8, 2021, at 4:56 PM, Olga Kornievskaia [off-list ref] wrote:

On Tue, Jun 8, 2021 at 4:41 PM Chuck Lever III [off-list ref] wrote:

quoted

quoted

On Jun 8, 2021, at 2:45 PM, Olga Kornievskaia [off-list ref] 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->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?

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.

To both Trond and Chuck,

If we were to cap the max number transports how to manage nconnect
transports and trunking transports. Are they the "same" transports? So
for instance, if somebody specified nconnect=10,max_connect=2 which
one matters more. Should nconnect be cut at 2? And that would mean all
the allowable transports are used up by nconnect.

But we've seen that there is benefit from creating multiple
connections for a single NIC. But in a multi-NIC environment, it means
multiple connections on each NIC, wouldn't it?So should we say allow
nconnect * max_connect value of transports instead? Or does the admin
need to calculate that value to make sure that max_connect reflects
that?

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