DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DD6A620685
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\))
Subject: Re: [PATCH v1 4/4] xprtrdma: Plant XID in on-the-wire RDMA offset
 (FRWR)
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <f1d83adeef9acf690c523df43c2f542b97a0bc33.camel@gmail.com>
Date:   Tue, 20 Nov 2018 13:07:34 -0500
Cc:     linux-rdma <linux-rdma@vger.kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <08D50748-8C07-4EF1-B2BB-3DA30A3598BF@oracle.com>
References: <20181119153707.10832.42881.stgit@manet.1015granger.net>
 <20181119154607.10832.92558.stgit@manet.1015granger.net>
 <f1d83adeef9acf690c523df43c2f542b97a0bc33.camel@gmail.com>
To:     Anna Schumaker <schumaker.anna@gmail.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Nov 20, 2018, at 1:02 PM, Anna Schumaker <schumaker.anna@gmail.com> =
wrote:
>=20
> Hi Chuck,
>=20
> On Mon, 2018-11-19 at 10:46 -0500, Chuck Lever wrote:
>> Place the associated RPC transaction's XID in the upper 32 bits of
>> each RDMA segment's rdma_offset field. These bits are currently
>> always zero.
>>=20
>> There are two reasons to do this:
>>=20
>> - The R_key only has 8 bits that are different from registration to
>>  registration. The XID adds more uniqueness to each RDMA segment to
>>  reduce the likelihood of a software bug on the server reading from
>>  or writing into memory it's not supposed to.
>>=20
>> - On-the-wire RDMA Read and Write operations do not otherwise carry
>>  any identifier that matches them up to an RPC. The XID in the
>>  upper 32 bits will act as an eye-catcher in network captures.
>>=20
>> Suggested-by: Tom Talpey <ttalpey@microsoft.com>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> net/sunrpc/xprtrdma/frwr_ops.c  |    3 ++-
>> net/sunrpc/xprtrdma/rpc_rdma.c  |    6 +++---
>> net/sunrpc/xprtrdma/xprt_rdma.h |    2 +-
>> 3 files changed, 6 insertions(+), 5 deletions(-)
>>=20
>> diff --git a/net/sunrpc/xprtrdma/frwr_ops.c =
b/net/sunrpc/xprtrdma/frwr_ops.c
>> index 49b314d..3b260d2 100644
>> --- a/net/sunrpc/xprtrdma/frwr_ops.c
>> +++ b/net/sunrpc/xprtrdma/frwr_ops.c
>> @@ -344,7 +344,7 @@
>>  */
>> static struct rpcrdma_mr_seg *
>> frwr_op_map(struct rpcrdma_xprt *r_xprt, struct rpcrdma_mr_seg *seg,
>> -	    int nsegs, bool writing, struct rpcrdma_mr **out)
>> +	    int nsegs, bool writing, u32 xid, struct rpcrdma_mr **out)
>> {
>> 	struct rpcrdma_ia *ia =3D &r_xprt->rx_ia;
>> 	bool holes_ok =3D ia->ri_mrtype =3D=3D IB_MR_TYPE_SG_GAPS;
>> @@ -398,6 +398,7 @@
>> 	if (unlikely(n !=3D mr->mr_nents))
>> 		goto out_mapmr_err;
>>=20
>> +	ibmr->iova |=3D ((u64)cpu_to_be32(xid)) << 32;
>=20
> My mount command hangs once we make this change (I got bored and =
killed it after
> about 5 minutes).  This is with NFS over soft-RoCE in a kvm virtual =
machine, and
> I see the behavior with all NFS versions.
>=20
> I hope this helps!

OK. Can you capture ftrace output to show us where the hang occurs?
I use this on the NFS client:

 # trace-cmd record -e sunrpc -e rpcrdma
 ^C
 # trace-cmd report | less      (or > /tmp/trace.log)

Thanks!


> Anna
>=20
>> 	key =3D (u8)(ibmr->rkey & 0x000000FF);
>> 	ib_update_fast_reg_key(ibmr, ++key);
>>=20
>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c =
b/net/sunrpc/xprtrdma/rpc_rdma.c
>> index 9f53e02..89a2db2 100644
>> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
>> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
>> @@ -357,7 +357,7 @@ static bool rpcrdma_results_inline(struct =
rpcrdma_xprt
>> *r_xprt,
>>=20
>> 	do {
>> 		seg =3D r_xprt->rx_ia.ri_ops->ro_map(r_xprt, seg, nsegs,
>> -						   false, &mr);
>> +						   false, rqst->rq_xid, =
&mr);
>> 		if (IS_ERR(seg))
>> 			return PTR_ERR(seg);
>> 		rpcrdma_mr_push(mr, &req->rl_registered);
>> @@ -415,7 +415,7 @@ static bool rpcrdma_results_inline(struct =
rpcrdma_xprt
>> *r_xprt,
>> 	nchunks =3D 0;
>> 	do {
>> 		seg =3D r_xprt->rx_ia.ri_ops->ro_map(r_xprt, seg, nsegs,
>> -						   true, &mr);
>> +						   true, rqst->rq_xid, =
&mr);
>> 		if (IS_ERR(seg))
>> 			return PTR_ERR(seg);
>> 		rpcrdma_mr_push(mr, &req->rl_registered);
>> @@ -473,7 +473,7 @@ static bool rpcrdma_results_inline(struct =
rpcrdma_xprt
>> *r_xprt,
>> 	nchunks =3D 0;
>> 	do {
>> 		seg =3D r_xprt->rx_ia.ri_ops->ro_map(r_xprt, seg, nsegs,
>> -						   true, &mr);
>> +						   true, rqst->rq_xid, =
&mr);
>> 		if (IS_ERR(seg))
>> 			return PTR_ERR(seg);
>> 		rpcrdma_mr_push(mr, &req->rl_registered);
>> diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h =
b/net/sunrpc/xprtrdma/xprt_rdma.h
>> index a13ccb6..2ae1ee2 100644
>> --- a/net/sunrpc/xprtrdma/xprt_rdma.h
>> +++ b/net/sunrpc/xprtrdma/xprt_rdma.h
>> @@ -472,7 +472,7 @@ struct rpcrdma_memreg_ops {
>> 	struct rpcrdma_mr_seg *
>> 			(*ro_map)(struct rpcrdma_xprt *,
>> 				  struct rpcrdma_mr_seg *, int, bool,
>> -				  struct rpcrdma_mr **);
>> +				  u32, struct rpcrdma_mr **);
>> 	int		(*ro_send)(struct rpcrdma_ia *ia,
>> 				   struct rpcrdma_req *req);
>> 	void		(*ro_reminv)(struct rpcrdma_rep *rep,

--
Chuck Lever