| Message ID | 20181211095843.5338-1-santoshkumar.pradhan@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sunrpc: Add xprt after nfs4_test_session_trunk() | expand |
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
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
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);