* [PATCH] sunrpc: Add xprt after nfs4_test_session_trunk()
@ 2018-12-11 9:58 Santosh kumar pradhan
2018-12-11 10:10 ` Suresh Jayaraman
2018-12-14 21:32 ` Trond Myklebust
0 siblings, 2 replies; 3+ messages in thread
From: Santosh kumar pradhan @ 2018-12-11 9:58 UTC (permalink / raw)
To: trond.myklebust, anna.schumaker; +Cc: linux-nfs, Santosh Kumar Pradhan
From: Santosh Kumar Pradhan <santoshkumar.pradhan@wdc.com>
Multipathing: In case of NFSv3, rpc_clnt_test_and_add_xprt() adds
the xprt to xprt switch (i.e. xps) if rpc_call_null_helper() returns
success. But in case of NFSv4.1, it needs to do EXCHANGEID to verify
the path along with check for session trunking.
Add the xprt once nfs4_test_session_trunk() returns success.
Also release refcount hold by rpc_clnt_setup_test_and_add_xprt().
Signed-off-by: Santosh Kumar Pradhan <santoshkumar.pradhan@wdc.com>
Tested-by: Suresh Jayaraman <suresh.jayaraman@wdc.com>
---
net/sunrpc/clnt.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index d839c33ae7d9..053f594cc144 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -2654,10 +2654,14 @@ int rpc_clnt_setup_test_and_add_xprt(struct rpc_clnt *clnt,
goto out_err;
/* rpc_xprt_switch and rpc_xprt are deferrenced by add_xprt_test() */
- xtest->add_xprt_test(clnt, xprt, xtest->data);
+ status = xtest->add_xprt_test(clnt, xprt, xtest->data);
+ if (status != 0)
+ goto out_err;
- /* so that rpc_clnt_add_xprt does not call rpc_xprt_switch_add_xprt */
- return 1;
+ xprt_put(xprt);
+ xprt_switch_put(xps);
+ /* so that rpc_clnt_add_xprt calls rpc_xprt_switch_add_xprt */
+ return 0;
out_err:
xprt_put(xprt);
xprt_switch_put(xps);
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] sunrpc: Add xprt after nfs4_test_session_trunk()
2018-12-11 9:58 [PATCH] sunrpc: Add xprt after nfs4_test_session_trunk() Santosh kumar pradhan
@ 2018-12-11 10:10 ` Suresh Jayaraman
2018-12-14 21:32 ` Trond Myklebust
1 sibling, 0 replies; 3+ messages in thread
From: Suresh Jayaraman @ 2018-12-11 10:10 UTC (permalink / raw)
To: Trond Myklebust, anna.schumaker@netapp.com
Cc: linux-nfs@vger.kernel.org, Santosh Kumar Pradhan
Santosh Kumar Pradhan <SantoshKumar.Pradhan@wdc.com> wrote:
> From: Santosh Kumar Pradhan <santoshkumar.pradhan@wdc.com>
>
> Multipathing: In case of NFSv3, rpc_clnt_test_and_add_xprt() adds
> the xprt to xprt switch (i.e. xps) if rpc_call_null_helper() returns
> success. But in case of NFSv4.1, it needs to do EXCHANGEID to verify
> the path along with check for session trunking.
>
> Add the xprt once nfs4_test_session_trunk() returns success.
> Also release refcount hold by rpc_clnt_setup_test_and_add_xprt().
>
> Signed-off-by: Santosh Kumar Pradhan <santoshkumar.pradhan@wdc.com>
> Tested-by: Suresh Jayaraman <suresh.jayaraman@wdc.com>
> ---
> net/sunrpc/clnt.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index d839c33ae7d9..053f594cc144 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2654,10 +2654,14 @@ int rpc_clnt_setup_test_and_add_xprt(struct rpc_clnt *clnt,
> goto out_err;
>
> /* rpc_xprt_switch and rpc_xprt are deferrenced by add_xprt_test() */
> - xtest->add_xprt_test(clnt, xprt, xtest->data);
> + status = xtest->add_xprt_test(clnt, xprt, xtest->data);
> + if (status != 0)
> + goto out_err;
>
> - /* so that rpc_clnt_add_xprt does not call rpc_xprt_switch_add_xprt */
> - return 1;
> + xprt_put(xprt);
> + xprt_switch_put(xps);
> + /* so that rpc_clnt_add_xprt calls rpc_xprt_switch_add_xprt */
> + return 0;
> out_err:
> xprt_put(xprt);
> xprt_switch_put(xps);
> --
> 2.17.1
Tested this patch on 4.20-rc3 kernel with Tegile/Western Digital pNFS server and
the client is able to use multipathing without issues.
Setup:
The configuration has a MetaData server (which also acts as a Data server) and a
Dedicated Data server. Four paths between client and MDS/DS as show below:
client MDS+DS DS
192.168.54.25 — —> 192.168.54.20
| - - - - - - - - - - - - - -> 192.168.54.30
192.168.55.25 — —> 192.168.55.20
| - - - - - - - - - - - - - -> 192.168.55.30
192.168.56.25 — —> 192.168.56.20
| - - - - - - - - - - - - - -> 192.168.56.30
192.168.57.25 — —> 192.168.57.20
| - - - - - - - - - - - - - -> 192.168.57.30
Workload: fio with sequential read profile
Note: ens192 is the management interface and hence there wouldn’t be any NFS traffic.
Without patch
--------------
Traffic is only seen on only one interface/path (ens256)
# ifstat;sleep 2;ifstat
#kernel
Interface RX Pkts/Rate TX Pkts/Rate RX Data/Rate TX Data/Rate
RX Errs/Drop TX Errs/Drop RX Over/Rate TX Coll/Rate
lo 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0
ens161 29 0 0 0 1900 0 0 0
0 0 0 0 0 0 0 0
ens192 72 0 39 0 4941 0 6926 0
0 0 0 0 0 0 0 0
ens193 29 0 0 0 1900 0 0 0
0 0 0 0 0 0 0 0
ens224 35 0 6 0 2464 0 684 0
0 0 0 0 0 0 0 0
ens256 978898 0 728595 0 18446744073336M 0 181491K 0
0 0 0 0 0 0 0 0
#kernel
Interface RX Pkts/Rate TX Pkts/Rate RX Data/Rate TX Data/Rate
RX Errs/Drop TX Errs/Drop RX Over/Rate TX Coll/Rate
lo 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0
ens161 2 0 0 0 120 0 0 0
0 0 0 0 0 0 0 0
ens192 15 0 12 0 900 0 3112 0
0 0 0 0 0 0 0 0
ens193 2 0 0 0 120 0 0 0
0 0 0 0 0 0 0 0
ens224 2 0 0 0 120 0 0 0
0 0 0 0 0 0 0 0
ens256 80314 0 59560 0 1024M 0 14827K 0
0 0 0 0 0 0 0 0
With patch
-----------
Client is using all the paths and traffic is seen on all the 4 paths
# ifstat;sleep 2;ifstat
#6710.1804289383 sampling_interval=2 time_const=60
Interface RX Pkts/Rate TX Pkts/Rate RX Data/Rate TX Data/Rate
RX Errs/Drop TX Errs/Drop RX Over/Rate TX Coll/Rate
lo 0 0 0 0 0 32 0 32
0 0 0 0 0 0 0 0
ens161 72920 9K 62228 7K 814126K 100M 12649K 1M
0 0 0 0 0 0 0 0
ens192 56 4 33 2 3826 316 5746 396
0 0 0 0 0 0 0 0
ens193 74411 9K 64024 7K 822219K 100M 12768K 1M
0 0 0 0 0 0 0 0
ens224 74494 9K 63606 7K 823831K 101M 12740K 1M
0 0 0 0 0 0 0 0
ens256 73970 9K 62759 7K 815506K 100M 12685K 1M
0 0 0 0 0 0 0 0
#6710.1804289383 sampling_interval=2 time_const=60
Interface RX Pkts/Rate TX Pkts/Rate RX Data/Rate TX Data/Rate
RX Errs/Drop TX Errs/Drop RX Over/Rate TX Coll/Rate
lo 0 0 0 0 0 29 0 29
0 0 0 0 0 0 0 0
ens161 19029 9K 16178 7K 204156K 100M 3200K 1M
0 0 0 0 0 0 0 0
ens192 21 4 15 2 1500 348 2734 468
0 0 0 0 0 0 0 0
ens193 18486 9K 15749 7K 205332K 100M 3171K 1M
0 0 0 0 0 0 0 0
ens224 18481 9K 15741 7K 207855K 101M 3169K 1M
0 0 0 0 0 0 0 0
ens256 18390 9K 15693 7K 204801K 101M 3167K 1M
0 0 0 0 0 0 0 0
--
Suresh Jayaraman
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] sunrpc: Add xprt after nfs4_test_session_trunk()
2018-12-11 9:58 [PATCH] sunrpc: Add xprt after nfs4_test_session_trunk() Santosh kumar pradhan
2018-12-11 10:10 ` Suresh Jayaraman
@ 2018-12-14 21:32 ` Trond Myklebust
1 sibling, 0 replies; 3+ messages in thread
From: Trond Myklebust @ 2018-12-14 21:32 UTC (permalink / raw)
To: anna.schumaker@netapp.com, santoshkumar.pradhan@wdc.com
Cc: linux-nfs@vger.kernel.org
Hi Santosh,
On Tue, 2018-12-11 at 15:28 +0530, Santosh kumar pradhan wrote:
> From: Santosh Kumar Pradhan <santoshkumar.pradhan@wdc.com>
>
> Multipathing: In case of NFSv3, rpc_clnt_test_and_add_xprt() adds
> the xprt to xprt switch (i.e. xps) if rpc_call_null_helper() returns
> success. But in case of NFSv4.1, it needs to do EXCHANGEID to verify
> the path along with check for session trunking.
>
> Add the xprt once nfs4_test_session_trunk() returns success.
> Also release refcount hold by rpc_clnt_setup_test_and_add_xprt().
>
> Signed-off-by: Santosh Kumar Pradhan <santoshkumar.pradhan@wdc.com>
> Tested-by: Suresh Jayaraman <suresh.jayaraman@wdc.com>
> ---
> net/sunrpc/clnt.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index d839c33ae7d9..053f594cc144 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2654,10 +2654,14 @@ int rpc_clnt_setup_test_and_add_xprt(struct
> rpc_clnt *clnt,
> goto out_err;
>
> /* rpc_xprt_switch and rpc_xprt are deferrenced by
> add_xprt_test() */
> - xtest->add_xprt_test(clnt, xprt, xtest->data);
> + status = xtest->add_xprt_test(clnt, xprt, xtest->data);
> + if (status != 0)
> + goto out_err;
>
> - /* so that rpc_clnt_add_xprt does not call
> rpc_xprt_switch_add_xprt */
> - return 1;
> + xprt_put(xprt);
> + xprt_switch_put(xps);
> + /* so that rpc_clnt_add_xprt calls rpc_xprt_switch_add_xprt */
> + return 0;
> out_err:
> xprt_put(xprt);
> xprt_switch_put(xps);
I'd really prefer to have callbacks that don't return values here so
that they can be made asynchronous at some point in the future. For
that reason, I'd prefer a fix in the NFS code, not the RPC code.
Can you therefore please change struct rpc_add_xprt_test to something
like the following:
struct rpc_add_xprt_test {
void (*add_xprt_test)(struct rpc_clnt *,
struct rpc_xprt *,
void *calldata);
void *data;
};
and then simply have nfs4_test_session_trunk() call
rpc_clnt_xprt_switch_add_xprt() if the call to
nfs4_detect_session_trunking() is successful?
Thanks!
Trond
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-12-14 21:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-11 9:58 [PATCH] sunrpc: Add xprt after nfs4_test_session_trunk() Santosh kumar pradhan
2018-12-11 10:10 ` Suresh Jayaraman
2018-12-14 21:32 ` Trond Myklebust
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox