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

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

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

On Tue, 2021-06-08 at 21:19 +0000, Trond Myklebust wrote:

quoted

On Tue, 2021-06-08 at 21:06 +0000, Chuck Lever III 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 < 
kolga@netapp.com>
---
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?

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.

I'd tend to agree. If you want to scale I/O then pNFS is
the
way
to
go,
not vast numbers of connections to a single server.

BTW: AFAICS this patch will end up adding another
connection
every
time
we mount a new filesystem, whether or not a connection
already
exists
to that IP address. That's unacceptable.

Not in my testing. Mounts to the same server (same IP)
different
export volumes lead to use of a single connection.

quoted

Ah... Never mind. The comparison is made by
rpc_clnt_setup_test_and_add_xprt(), and the address is
discarded if
it
is already present.

However why are you running trunking detection a second time on
the
address you've just run trunking detection on and have decided
to
add?

Because that's where we determined that these are different
clients
and are trunkable? But I guess also for the ease of re-using
existing
code. There is no hook to create an xprt and add it to an
existing
RPC
client. One can be created.

Why not rpc_clnt_add_xprt()? That's what pNFS uses for this case.

That's what this code is using.

Sorry. I should have written: 'rpc_clnt_add_xprt() with
rpc_clnt_setup_test_and_add_xprt()', which is what pNFS uses when
connecting to NFSv3 servers.
i.e.

   rpc_clnt_add_xprt(clp->cl_rpcclient, &xprt_args,
                     rpc_clnt_test_and_add_xprt, NULL);

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