Re: [PATCH v1 4/8] xprtrdma: Properly handle RDMA_ERROR replies

[Anna Schumaker <Anna.Schumaker@netapp.com>]

On 02/17/2016 04:21 PM, Chuck Lever wrote:
> 
>> On Feb 17, 2016, at 4:19 PM, Anna Schumaker <Anna.Schumaker@netapp.com> wrote:
>>
>> Hi Chuck,
>>
>> Can you combine this patch with 3/8, that way we're using RPCRDMA_HDRLEN_ERR in the same patch it's introduced?  It's only a one line difference :)
> 
> I have to submit 3/8 also in the server series, to prevent
> merge conflicts, and only RPCRDMA_HDRLEN_ERR is needed there.
> 
> So 3/8 has to remain separate.

Got it.  Thanks for the explanation!

Anna
> 
> 
>> Thanks,
>> Anna
>>
>> On 02/12/2016 04:06 PM, Chuck Lever wrote:
>>> These are shorter than RPCRDMA_HDRLEN_MIN, and they need to
>>> complete the waiting RPC.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>> include/linux/sunrpc/rpc_rdma.h |   11 ++++++-----
>>> net/sunrpc/xprtrdma/rpc_rdma.c  |   38 +++++++++++++++++++++++++++++++++-----
>>> 2 files changed, 39 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/linux/sunrpc/rpc_rdma.h b/include/linux/sunrpc/rpc_rdma.h
>>> index 8c6d23c..3b1ff38 100644
>>> --- a/include/linux/sunrpc/rpc_rdma.h
>>> +++ b/include/linux/sunrpc/rpc_rdma.h
>>> @@ -93,6 +93,12 @@ struct rpcrdma_msg {
>>> 			__be32 rm_pempty[3];	/* 3 empty chunk lists */
>>> 		} rm_padded;
>>>
>>> +		struct {
>>> +			__be32 rm_err;
>>> +			__be32 rm_vers_low;
>>> +			__be32 rm_vers_high;
>>> +		} rm_error;
>>> +
>>> 		__be32 rm_chunks[0];	/* read, write and reply chunks */
>>>
>>> 	} rm_body;
>>> @@ -109,11 +115,6 @@ enum rpcrdma_errcode {
>>> 	ERR_CHUNK = 2
>>> };
>>>
>>> -struct rpcrdma_err_vers {
>>> -	uint32_t rdma_vers_low;	/* Version range supported by peer */
>>> -	uint32_t rdma_vers_high;
>>> -};
>>> -
>>> enum rpcrdma_proc {
>>> 	RDMA_MSG = 0,		/* An RPC call or reply msg */
>>> 	RDMA_NOMSG = 1,		/* An RPC call or reply msg - separate body */
>>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
>>> index add1f98..c341225 100644
>>> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
>>> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
>>> @@ -795,7 +795,7 @@ rpcrdma_reply_handler(struct rpcrdma_rep *rep)
>>> 	struct rpcrdma_xprt *r_xprt = rep->rr_rxprt;
>>> 	struct rpc_xprt *xprt = &r_xprt->rx_xprt;
>>> 	__be32 *iptr;
>>> -	int rdmalen, status;
>>> +	int rdmalen, status, rmerr;
>>> 	unsigned long cwnd;
>>> 	u32 credits;
>>>
>>> @@ -803,12 +803,10 @@ rpcrdma_reply_handler(struct rpcrdma_rep *rep)
>>>
>>> 	if (rep->rr_len == RPCRDMA_BAD_LEN)
>>> 		goto out_badstatus;
>>> -	if (rep->rr_len < RPCRDMA_HDRLEN_MIN)
>>> +	if (rep->rr_len < RPCRDMA_HDRLEN_ERR)
>>> 		goto out_shortreply;
>>>
>>> 	headerp = rdmab_to_msg(rep->rr_rdmabuf);
>>> -	if (headerp->rm_vers != rpcrdma_version)
>>> -		goto out_badversion;
>>> #if defined(CONFIG_SUNRPC_BACKCHANNEL)
>>> 	if (rpcrdma_is_bcall(headerp))
>>> 		goto out_bcall;
>>> @@ -840,6 +838,9 @@ rpcrdma_reply_handler(struct rpcrdma_rep *rep)
>>> 	req->rl_reply = rep;
>>> 	xprt->reestablish_timeout = 0;
>>>
>>> +	if (headerp->rm_vers != rpcrdma_version)
>>> +		goto out_badversion;
>>> +
>>> 	/* check for expected message types */
>>> 	/* The order of some of these tests is important. */
>>> 	switch (headerp->rm_type) {
>>> @@ -900,6 +901,9 @@ rpcrdma_reply_handler(struct rpcrdma_rep *rep)
>>> 		status = rdmalen;
>>> 		break;
>>>
>>> +	case rdma_error:
>>> +		goto out_rdmaerr;
>>> +
>>> badheader:
>>> 	default:
>>> 		dprintk("%s: invalid rpcrdma reply header (type %d):"
>>> @@ -915,6 +919,7 @@ badheader:
>>> 		break;
>>> 	}
>>>
>>> +out:
>>> 	/* Invalidate and flush the data payloads before waking the
>>> 	 * waiting application. This guarantees the memory region is
>>> 	 * properly fenced from the server before the application
>>> @@ -957,6 +962,27 @@ out_bcall:
>>> 	return;
>>> #endif
>>>
>>> +out_rdmaerr:
>>> +	rmerr = be32_to_cpu(headerp->rm_body.rm_error.rm_err);
>>> +	switch (rmerr) {
>>> +	case ERR_VERS:
>>> +		pr_err("%s: server reports header version error (%u-%u)\n",
>>> +		       __func__,
>>> +		       be32_to_cpu(headerp->rm_body.rm_error.rm_vers_low),
>>> +		       be32_to_cpu(headerp->rm_body.rm_error.rm_vers_high));
>>> +		break;
>>> +	case ERR_CHUNK:
>>> +		pr_err("%s: server reports header decoding error\n",
>>> +		       __func__);
>>> +		break;
>>> +	default:
>>> +		pr_err("%s: server reports unknown error %d\n",
>>> +		       __func__, rmerr);
>>> +	}
>>> +	status = -EREMOTEIO;
>>> +	r_xprt->rx_stats.bad_reply_count++;
>>> +	goto out;
>>> +
>>> out_shortreply:
>>> 	dprintk("RPC:       %s: short/invalid reply\n", __func__);
>>> 	goto repost;
>>> @@ -964,7 +990,9 @@ out_shortreply:
>>> out_badversion:
>>> 	dprintk("RPC:       %s: invalid version %d\n",
>>> 		__func__, be32_to_cpu(headerp->rm_vers));
>>> -	goto repost;
>>> +	status = -EIO;
>>> +	r_xprt->rx_stats.bad_reply_count++;
>>> +	goto out;
>>>
>>> out_nomatch:
>>> 	spin_unlock_bh(&xprt->transport_lock);
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 
> --
> Chuck Lever
> 
> 
> 
>