Re: [PATCH 0/17] svcrdma: RDMA transport driver close path cleanup

Re: [PATCH 0/17] svcrdma: RDMA transport driver close path cleanup

[J. Bruce Fields]

Thanks.  I'm a little backlogged--bug me if I haven't gotten back to
these by next week.

--b.

On Fri, May 02, 2008 at 11:28:24AM -0500, Tom Tucker wrote:
> This patchset fixes a number of defects in the close path of the SVCRDMA
> transport provider. The defects were found by injecting errors on the 
> transport at random intervals while running concurrent iozone tests on
> IB and iWARP mounts. With this set of patches applied I was able to run
> the the above tests for several hours without causing a crash or leaking
> resources. The transport recovers correctly on a client reconnect.
> 
> This patchset is based on 2.6 top of tree, however, I think these fixes are
> good candidates for a 2.6.25 dot release as well. I have a set that is
> already merged for that release if you would like them. They are available in
> my git tree on linux-nfs.org in the '2.6.25' branch. Please let me know if you
> would like me to post them here.
> 
> [PATCH 1/17] svcrdma: Simplify receive buffer posting
> 
>  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |   17 +----------------
>  net/sunrpc/xprtrdma/svc_rdma_sendto.c    |   10 ++++++++++
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   19 -------------------
>  3 files changed, 11 insertions(+), 35 deletions(-)
> 
> [PATCH 2/17] svcrdma: Fix race with dto_tasklet in svc_rdma_send
> 
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> [PATCH 3/17] svcrdma: Fix return value in svc_rdma_send
> 
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> [PATCH 4/17] svcrdma: Add put of connection ESTABLISHED reference in rdma_cma_handler
> 
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> [PATCH 5/17] svcrdma: Free context on ib_post_recv error
> 
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> [PATCH 6/17] svcrdma: Free context on post_recv error in send_reply
> 
>  net/sunrpc/xprtrdma/svc_rdma_sendto.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation
> 
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> [PATCH 8/17] svcrdma: Return error from rdma_read_xdr so caller knows to free context
> 
>  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |   17 +++++++++++++----
>  1 files changed, 13 insertions(+), 4 deletions(-)
> 
> [PATCH 9/17] svcrdma: Remove unused READ_DONE context flags bit
> 
>  include/linux/sunrpc/svc_rdma.h          |    1 -
>  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |    1 -
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    1 -
>  3 files changed, 0 insertions(+), 3 deletions(-)
> 
> [PATCH 10/17] svcrdma: Simplify RDMA_READ deferral buffer management
> 
>  include/linux/sunrpc/svc_rdma.h          |    1 +
>  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |   58 ++++++------------------------
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    5 ++-
>  3 files changed, 16 insertions(+), 48 deletions(-)
> 
> [PATCH 11/17] svcrdma: Use standard Linux lists for context cache
> 
>  include/linux/sunrpc/svc_rdma.h          |    5 ++-
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   47 ++++++++++++++++-------------
>  2 files changed, 29 insertions(+), 23 deletions(-)
> 
> [PATCH 12/17] svcrdma: Relaxing locking scope on RQ CQ
> 
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> [PATCH 13/17] svcrdma: Move destroy to kernel thread
> 
>  include/linux/sunrpc/svc_rdma.h          |    1 +
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   17 ++++++++++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> [PATCH 14/17] svcrdma: Add reference for each SQ/RQ WR
> 
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
> 
> [PATCH 15/17] svcrdma: Move the QP and cm_id destruction to svc_rdma_free
> 
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   40 +++++++++++++++---------------
>  1 files changed, 20 insertions(+), 20 deletions(-)
> 
> [PATCH 16/17] svcrdma: Cleanup queued, but unprocessed I/O in svc_rdma_free
> 
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   38 +++++++++++++++++++++++++++--
>  1 files changed, 35 insertions(+), 3 deletions(-)
> 
> [PATCH 17/17] svcrdma: Use ib verbs version of dma_unmap
> 
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>

Re: [PATCH 0/17] svcrdma: RDMA transport driver close path cleanup

[Tom Tucker]

On Fri, 2008-05-02 at 18:37 -0400, J. Bruce Fields wrote:
> Thanks.  I'm a little backlogged--bug me if I haven't gotten back to
> these by next week.

No problem. 
Tom

> 
> --b.
> 
> On Fri, May 02, 2008 at 11:28:24AM -0500, Tom Tucker wrote:
> > This patchset fixes a number of defects in the close path of the SVCRDMA
> > transport provider. The defects were found by injecting errors on the 
> > transport at random intervals while running concurrent iozone tests on
> > IB and iWARP mounts. With this set of patches applied I was able to run
> > the the above tests for several hours without causing a crash or leaking
> > resources. The transport recovers correctly on a client reconnect.
> > 
> > This patchset is based on 2.6 top of tree, however, I think these fixes are
> > good candidates for a 2.6.25 dot release as well. I have a set that is
> > already merged for that release if you would like them. They are available in
> > my git tree on linux-nfs.org in the '2.6.25' branch. Please let me know if you
> > would like me to post them here.
> > 
> > [PATCH 1/17] svcrdma: Simplify receive buffer posting
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |   17 +----------------
> >  net/sunrpc/xprtrdma/svc_rdma_sendto.c    |   10 ++++++++++
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |   19 -------------------
> >  3 files changed, 11 insertions(+), 35 deletions(-)
> > 
> > [PATCH 2/17] svcrdma: Fix race with dto_tasklet in svc_rdma_send
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++++--
> >  1 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > [PATCH 3/17] svcrdma: Fix return value in svc_rdma_send
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > [PATCH 4/17] svcrdma: Add put of connection ESTABLISHED reference in rdma_cma_handler
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > [PATCH 5/17] svcrdma: Free context on ib_post_recv error
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > [PATCH 6/17] svcrdma: Free context on post_recv error in send_reply
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_sendto.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > [PATCH 8/17] svcrdma: Return error from rdma_read_xdr so caller knows to free context
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |   17 +++++++++++++----
> >  1 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > [PATCH 9/17] svcrdma: Remove unused READ_DONE context flags bit
> > 
> >  include/linux/sunrpc/svc_rdma.h          |    1 -
> >  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |    1 -
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |    1 -
> >  3 files changed, 0 insertions(+), 3 deletions(-)
> > 
> > [PATCH 10/17] svcrdma: Simplify RDMA_READ deferral buffer management
> > 
> >  include/linux/sunrpc/svc_rdma.h          |    1 +
> >  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |   58 ++++++------------------------
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |    5 ++-
> >  3 files changed, 16 insertions(+), 48 deletions(-)
> > 
> > [PATCH 11/17] svcrdma: Use standard Linux lists for context cache
> > 
> >  include/linux/sunrpc/svc_rdma.h          |    5 ++-
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |   47 ++++++++++++++++-------------
> >  2 files changed, 29 insertions(+), 23 deletions(-)
> > 
> > [PATCH 12/17] svcrdma: Relaxing locking scope on RQ CQ
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > [PATCH 13/17] svcrdma: Move destroy to kernel thread
> > 
> >  include/linux/sunrpc/svc_rdma.h          |    1 +
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |   17 ++++++++++++++---
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> > 
> > [PATCH 14/17] svcrdma: Add reference for each SQ/RQ WR
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |   18 ++++++++++++++++--
> >  1 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > [PATCH 15/17] svcrdma: Move the QP and cm_id destruction to svc_rdma_free
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |   40 +++++++++++++++---------------
> >  1 files changed, 20 insertions(+), 20 deletions(-)
> > 
> > [PATCH 16/17] svcrdma: Cleanup queued, but unprocessed I/O in svc_rdma_free
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |   38 +++++++++++++++++++++++++++--
> >  1 files changed, 35 insertions(+), 3 deletions(-)
> > 
> > [PATCH 17/17] svcrdma: Use ib verbs version of dma_unmap
> > 
> >  net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> --
> 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

Re: [PATCH 1/17] svcrdma: Simplify receive buffer posting

[J. Bruce Fields]

On Fri, May 02, 2008 at 11:28:27AM -0500, Tom Tucker wrote:
> The svcrdma transport provider currently allocates receive buffers
> to the RQ through the xpo_release_rqst method. This approach is overly
> complicated since it means that the rqstp rq_xprt_ctxt has to be
> selectively set based on whether the RPC is going to be processed
> immediately or deferred. Instead, just post the receive buffer when
> we are certain that we are replying in the send_reply function.

Makes sense to me.  But, by the way:

> index af408fc..1e0af2f 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -910,27 +910,8 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
>  	return NULL;
>  }
>  
> -/*
> - * Post an RQ WQE to the RQ when the rqst is being released. This
> - * effectively returns an RQ credit to the client. The rq_xprt_ctxt
> - * will be null if the request is deferred due to an RDMA_READ or the
> - * transport had no data ready (EAGAIN). Note that an RPC deferred in
> - * svc_process will still return the credit, this is because the data
> - * is copied and no longer consume a WQE/WC.
> - */
>  static void svc_rdma_release_rqst(struct svc_rqst *rqstp)
>  {
> -	int err;
> -	struct svcxprt_rdma *rdma =
> -		container_of(rqstp->rq_xprt, struct svcxprt_rdma, sc_xprt);
> -	if (rqstp->rq_xprt_ctxt) {
> -		BUG_ON(rqstp->rq_xprt_ctxt != rdma);
> -		err = svc_rdma_post_recv(rdma);
> -		if (err)
> -			dprintk("svcrdma: failed to post an RQ WQE error=%d\n",
> -				err);
> -	}
> -	rqstp->rq_xprt_ctxt = NULL;
>  }

Why is it that the svcsock equivalent (svc_release_skb) frees
rqstp->rq_deferred, but this doesn't?  Don't we need to free that in the
rdma case too?

--b.

Re: [PATCH 2/17] svcrdma: Fix race with dto_tasklet in svc_rdma_send

[J. Bruce Fields]

On Fri, May 02, 2008 at 11:28:34AM -0500, Tom Tucker wrote:
> The svc_rdma_send function will attempt to reap SQ WR to make room for
> a new request if it finds the SQ full. This function races with the
> dto_tasklet that also reaps SQ WR. To avoid calling the function
> unnecessarily use test_and_clear_bit with the RDMAXPRT_SQ_PENDING
> flag to serialize access to the sq_cq_reap function.

OK.  I won't pretend to understand much of this, but--would it be worth
pulling out the added code here into a helper function, since it now
exists in two different places?  (Especially if correctness depends on
the same thing happening in both the places this bit can be cleared.)

--b.

> 
> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> 
> ---
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 1e0af2f..14f83a1 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -1010,8 +1010,12 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct ib_send_wr *wr)
>  		if (xprt->sc_sq_depth == atomic_read(&xprt->sc_sq_count)) {
>  			spin_unlock_bh(&xprt->sc_lock);
>  			atomic_inc(&rdma_stat_sq_starve);
> -			/* See if we can reap some SQ WR */
> -			sq_cq_reap(xprt);
> +
> +			/* See if we can opportunistically reap SQ WR to make room */
> +			if (test_and_clear_bit(RDMAXPRT_SQ_PENDING, &xprt->sc_flags)) {
> +				ib_req_notify_cq(xprt->sc_sq_cq, IB_CQ_NEXT_COMP);
> +				sq_cq_reap(xprt);
> +			}
>  
>  			/* Wait until SQ WR available if SQ still full */
>  			wait_event(xprt->sc_send_wait,

Re: [PATCH 3/17] svcrdma: Fix return value in svc_rdma_send

[J. Bruce Fields]

On Fri, May 02, 2008 at 11:28:35AM -0500, Tom Tucker wrote:
> Fix the return value on close to -ENOTCONN so caller knows to free context.
> Also if a thread is waiting for free SQ space, check for close when waking
> to avoid posting WR to a closing transport.

A random related question: svc_rdma_send_error() seems to have only one
caller, which ignores its return value.  Should its return value be
made void?

--b.

> 
> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> 
> ---
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 14f83a1..8adb2f0 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -999,7 +999,7 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct ib_send_wr *wr)
>  	int ret;
>  
>  	if (test_bit(XPT_CLOSE, &xprt->sc_xprt.xpt_flags))
> -		return 0;
> +		return -ENOTCONN;
>  
>  	BUG_ON(wr->send_flags != IB_SEND_SIGNALED);
>  	BUG_ON(((struct svc_rdma_op_ctxt *)(unsigned long)wr->wr_id)->wr_op !=

Re: [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation

[J. Bruce Fields]

On Fri, May 02, 2008 at 11:28:39AM -0500, Tom Tucker wrote:
> A listening endpoint isn't known to the generic transport switch until
> the svc_create_xprt function returns without error. Calling
> svc_xprt_put within the xpo_create function causes the module reference
> count to be erroneously decremented.

There's some redundant code in these three error paths; would the usual
kernel-style "goto cleanup" thing help?

--b.

> 
> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> 
> ---
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 83818cf..0d9a828 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -669,7 +669,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
>  
>  	listen_id = rdma_create_id(rdma_listen_handler, cma_xprt, RDMA_PS_TCP);
>  	if (IS_ERR(listen_id)) {
> -		svc_xprt_put(&cma_xprt->sc_xprt);
> +		kfree(cma_xprt);
>  		dprintk("svcrdma: rdma_create_id failed = %ld\n",
>  			PTR_ERR(listen_id));
>  		return (void *)listen_id;
> @@ -677,7 +677,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
>  	ret = rdma_bind_addr(listen_id, sa);
>  	if (ret) {
>  		rdma_destroy_id(listen_id);
> -		svc_xprt_put(&cma_xprt->sc_xprt);
> +		kfree(cma_xprt);
>  		dprintk("svcrdma: rdma_bind_addr failed = %d\n", ret);
>  		return ERR_PTR(ret);
>  	}
> @@ -686,7 +686,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
>  	ret = rdma_listen(listen_id, RPCRDMA_LISTEN_BACKLOG);
>  	if (ret) {
>  		rdma_destroy_id(listen_id);
> -		svc_xprt_put(&cma_xprt->sc_xprt);
> +		kfree(cma_xprt);
>  		dprintk("svcrdma: rdma_listen failed = %d\n", ret);
>  		return ERR_PTR(ret);
>  	}

Re: [PATCH 8/17] svcrdma: Return error from rdma_read_xdr so caller knows to free context

[J. Bruce Fields]

On Fri, May 02, 2008 at 11:28:40AM -0500, Tom Tucker wrote:
> The rdma_read_xdr function did discriminate between no read-list and
			     ^^^
			     did not?

> an error posting the read-list. This results a leak of a page in the
> context when an asyncrhonous error occurs on the transport.
> 
> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> 
> ---
>  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |   17 +++++++++++++----
>  1 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> index f3a108a..0ac375a 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> @@ -260,11 +260,16 @@ static int rdma_read_max_sge(struct svcxprt_rdma *xprt, int sge_count)
>   * On our side, we need to read into a pagelist. The first page immediately
>   * follows the RPC header.
>   *
> - * This function returns 1 to indicate success. The data is not yet in
> + * This function returns:
> + * 0 - No error and no read-list found.
> + *
> + * 1 - Successful read-list processing. The data is not yet in
>   * the pagelist and therefore the RPC request must be deferred. The
>   * I/O completion will enqueue the transport again and
>   * svc_rdma_recvfrom will complete the request.
>   *
> + * <0 - Error processing/posting read-list.
> + *
>   * NOTE: The ctxt must not be touched after the last WR has been posted
>   * because the I/O completion processing may occur on another
>   * processor and free / modify the context. Ne touche pas!
> @@ -398,7 +403,7 @@ next_sge:
>  			svc_rdma_put_context(head, 1);
>  			head = ctxt;
>  		}
> -		return 0;
> +		return err;
>  	}
>  
>  	return 1;
> @@ -536,8 +541,12 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
>  	 * it. Not that in this case, we don't return the RQ credit
>  	 * until after the read completes.
>  	 */
> -	if (rdma_read_xdr(rdma_xprt, rmsgp, rqstp, ctxt)) {
> -		svc_xprt_received(xprt);
> +	ret = rdma_read_xdr(rdma_xprt, rmsgp, rqstp, ctxt);
> +	if (ret) {
> +		if (ret > 0)
> +			svc_xprt_received(xprt);
> +		else
> +			svc_rdma_put_context(ctxt, 1);
>  		return 0;
>  	}
>  

Possibly slightly more straightforward?:

	/* Read read-list data. */
	ret = rdma_read_xdr(rdma_xprt, rmsgp, rqstp, ctxt);
	if (ret < 0) {
		svc_rdma_put_context(ctxt, 1);
		return 0;
	}
	if (ret > 0) {
		/*
		 * We need to wait; defer the request.  Note that in
		 * this case, we don't return the RQ credit until after
		 * the read completes.
		 */
		 svc_xprt_received(xprt);
		 return 0;
	}
	...

--b.

Re: [PATCH 3/17] svcrdma: Fix return value in svc_rdma_send

[Tom Tucker]

On 5/5/08 5:08 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Fri, May 02, 2008 at 11:28:35AM -0500, Tom Tucker wrote:
>> Fix the return value on close to -ENOTCONN so caller knows to free context.
>> Also if a thread is waiting for free SQ space, check for close when waking
>> to avoid posting WR to a closing transport.
> 
> A random related question: svc_rdma_send_error() seems to have only one
> caller, which ignores its return value.  Should its return value be
> made void?

Honestly, I don't think this code path has ever been excercised. This is the
"protocol error" path. Given my experience with the "random transport error"
tests that resulted in the patchset your currently looking at, I'd be
inclined to defer this kind of thing until I've built a "protocol error" set
of tests and worked through the "evil client" scenarios.

Thoughts?
> 
> --b.
> 
>> 
>> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
>> 
>> ---
>>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> index 14f83a1..8adb2f0 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -999,7 +999,7 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct
>> ib_send_wr *wr)
>> int ret;
>>  
>> if (test_bit(XPT_CLOSE, &xprt->sc_xprt.xpt_flags))
>> -  return 0;
>> +  return -ENOTCONN;
>>  
>> BUG_ON(wr->send_flags != IB_SEND_SIGNALED);
>> BUG_ON(((struct svc_rdma_op_ctxt *)(unsigned long)wr->wr_id)->wr_op !=
> --
> 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

Re: [PATCH 8/17] svcrdma: Return error from rdma_read_xdr so caller knows to free context

[Tom Tucker]

On 5/5/08 5:52 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Fri, May 02, 2008 at 11:28:40AM -0500, Tom Tucker wrote:
>> The rdma_read_xdr function did discriminate between no read-list and
>     ^^^
>     did not?
> 
Ack.

>> an error posting the read-list. This results a leak of a page in the
>> context when an asyncrhonous error occurs on the transport.
>> 
>> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
>> 
>> ---
>>  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |   17 +++++++++++++----
>>  1 files changed, 13 insertions(+), 4 deletions(-)
>> 
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> index f3a108a..0ac375a 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> @@ -260,11 +260,16 @@ static int rdma_read_max_sge(struct svcxprt_rdma *xprt,
>> int sge_count)
>>   * On our side, we need to read into a pagelist. The first page immediately
>>   * follows the RPC header.
>>   *
>> - * This function returns 1 to indicate success. The data is not yet in
>> + * This function returns:
>> + * 0 - No error and no read-list found.
>> + *
>> + * 1 - Successful read-list processing. The data is not yet in
>>   * the pagelist and therefore the RPC request must be deferred. The
>>   * I/O completion will enqueue the transport again and
>>   * svc_rdma_recvfrom will complete the request.
>>   *
>> + * <0 - Error processing/posting read-list.
>> + *
>>   * NOTE: The ctxt must not be touched after the last WR has been posted
>>   * because the I/O completion processing may occur on another
>>   * processor and free / modify the context. Ne touche pas!
>> @@ -398,7 +403,7 @@ next_sge:
>> svc_rdma_put_context(head, 1);
>> head = ctxt;
>> }
>> -  return 0;
>> +  return err;
>> }
>>  
>> return 1;
>> @@ -536,8 +541,12 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
>> * it. Not that in this case, we don't return the RQ credit
>> * until after the read completes.
>> */
>> - if (rdma_read_xdr(rdma_xprt, rmsgp, rqstp, ctxt)) {
>> -  svc_xprt_received(xprt);
>> + ret = rdma_read_xdr(rdma_xprt, rmsgp, rqstp, ctxt);
>> + if (ret) {
>> +  if (ret > 0)
>> +   svc_xprt_received(xprt);
>> +  else
>> +   svc_rdma_put_context(ctxt, 1);
>> return 0;
>> }
>>  
> 
> Possibly slightly more straightforward?:
> 
> /* Read read-list data. */
> ret = rdma_read_xdr(rdma_xprt, rmsgp, rqstp, ctxt);
> if (ret < 0) {
> svc_rdma_put_context(ctxt, 1);
> return 0;
> }
> if (ret > 0) {
> /*
> * We need to wait; defer the request.  Note that in
> * this case, we don't return the RQ credit until after
> * the read completes.
> */
> svc_xprt_received(xprt);
> return 0;
> }
> ...
> 

Yep. I rewrote that code three different ways and never liked it. I think
you've got it right first try... :-)

> --b.
> --
> 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

Re: [PATCH 2/17] svcrdma: Fix race with dto_tasklet in svc_rdma_send

[Tom Tucker]

On 5/5/08 5:06 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Fri, May 02, 2008 at 11:28:34AM -0500, Tom Tucker wrote:
>> The svc_rdma_send function will attempt to reap SQ WR to make room for
>> a new request if it finds the SQ full. This function races with the
>> dto_tasklet that also reaps SQ WR. To avoid calling the function
>> unnecessarily use test_and_clear_bit with the RDMAXPRT_SQ_PENDING
>> flag to serialize access to the sq_cq_reap function.
> 
> OK.  I won't pretend to understand much of this, but--would it be worth
> pulling out the added code here into a helper function, since it now
> exists in two different places?  (Especially if correctness depends on
> the same thing happening in both the places this bit can be cleared.)

Yes. Good suggestions.

BTW, this code is here because the SQ is undersized for big data. Since a
single NFS_READ/WRITE can result in an attempt to fetch a large amount of
data from the client (2M) and depending on certain HW resources this can
result in a lot of WR being posted to the SQ.

That said, there is a change coming in the 2.6.27 time frame that supports
what is called Fast NSMR register. This allows the transport to effectively
DMA map the entire transfer size (32k -- 2M) all as a single SGE. This will
take a lot of pressure off the SQ and effectively make this code
unnecessary.

> 
> --b.
> 
>> 
>> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
>> 
>> ---
>>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++++--
>>  1 files changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> index 1e0af2f..14f83a1 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -1010,8 +1010,12 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct
>> ib_send_wr *wr)
>> if (xprt->sc_sq_depth == atomic_read(&xprt->sc_sq_count)) {
>> spin_unlock_bh(&xprt->sc_lock);
>> atomic_inc(&rdma_stat_sq_starve);
>> -   /* See if we can reap some SQ WR */
>> -   sq_cq_reap(xprt);
>> +
>> +   /* See if we can opportunistically reap SQ WR to make room */
>> +   if (test_and_clear_bit(RDMAXPRT_SQ_PENDING, &xprt->sc_flags)) {
>> +    ib_req_notify_cq(xprt->sc_sq_cq, IB_CQ_NEXT_COMP);
>> +    sq_cq_reap(xprt);
>> +   }
>>  
>> /* Wait until SQ WR available if SQ still full */
>> wait_event(xprt->sc_send_wait,
> --
> 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

Re: [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation

[Tom Tucker]

On 5/5/08 5:41 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Fri, May 02, 2008 at 11:28:39AM -0500, Tom Tucker wrote:
>> A listening endpoint isn't known to the generic transport switch until
>> the svc_create_xprt function returns without error. Calling
>> svc_xprt_put within the xpo_create function causes the module reference
>> count to be erroneously decremented.
> 
> There's some redundant code in these three error paths; would the usual
> kernel-style "goto cleanup" thing help?

I think code-size it's a wash, but it might look more familiar. BTW, I
noticed that the error path above returns a positive errno :-\, so that
needs to be fixed too.

> 
> --b.
> 
>> 
>> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
>> 
>> ---
>>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> index 83818cf..0d9a828 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -669,7 +669,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv
>> *serv,
>>  
>> listen_id = rdma_create_id(rdma_listen_handler, cma_xprt, RDMA_PS_TCP);
>> if (IS_ERR(listen_id)) {
>> -  svc_xprt_put(&cma_xprt->sc_xprt);
>> +  kfree(cma_xprt);
>> dprintk("svcrdma: rdma_create_id failed = %ld\n",
>> PTR_ERR(listen_id));
>> return (void *)listen_id;
>> @@ -677,7 +677,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv
>> *serv,
>> ret = rdma_bind_addr(listen_id, sa);
>> if (ret) {
>> rdma_destroy_id(listen_id);
>> -  svc_xprt_put(&cma_xprt->sc_xprt);
>> +  kfree(cma_xprt);
>> dprintk("svcrdma: rdma_bind_addr failed = %d\n", ret);
>> return ERR_PTR(ret);
>> }
>> @@ -686,7 +686,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv
>> *serv,
>> ret = rdma_listen(listen_id, RPCRDMA_LISTEN_BACKLOG);
>> if (ret) {
>> rdma_destroy_id(listen_id);
>> -  svc_xprt_put(&cma_xprt->sc_xprt);
>> +  kfree(cma_xprt);
>> dprintk("svcrdma: rdma_listen failed = %d\n", ret);
>> return ERR_PTR(ret);
>> }
> --
> 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

Re: [PATCH 3/17] svcrdma: Fix return value in svc_rdma_send

[J. Bruce Fields]

On Mon, May 05, 2008 at 09:03:02PM -0500, Tom Tucker wrote:
> 
> 
> On 5/5/08 5:08 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Fri, May 02, 2008 at 11:28:35AM -0500, Tom Tucker wrote:
> >> Fix the return value on close to -ENOTCONN so caller knows to free context.
> >> Also if a thread is waiting for free SQ space, check for close when waking
> >> to avoid posting WR to a closing transport.
> > 
> > A random related question: svc_rdma_send_error() seems to have only one
> > caller, which ignores its return value.  Should its return value be
> > made void?
> 
> Honestly, I don't think this code path has ever been excercised. This is the
> "protocol error" path. Given my experience with the "random transport error"
> tests that resulted in the patchset your currently looking at, I'd be
> inclined to defer this kind of thing until I've built a "protocol error" set
> of tests and worked through the "evil client" scenarios.
> 
> Thoughts?

None, just the observation that it's ugly to have something defined as

	int svc_rdma_send_error(struct svcxprt_rdma *xprt, ...

whose only caller is

	(void)svc_rdma_send_error(rdma_xprt, rmsgp, ERR_VERS);

But it's not urgent--if you want to wait till you're fixing something
related, OK.

--b.

> > 
> > --b.
> > 
> >> 
> >> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> >> 
> >> ---
> >>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> >> b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> >> index 14f83a1..8adb2f0 100644
> >> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> >> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> >> @@ -999,7 +999,7 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct
> >> ib_send_wr *wr)
> >> int ret;
> >>  
> >> if (test_bit(XPT_CLOSE, &xprt->sc_xprt.xpt_flags))
> >> -  return 0;
> >> +  return -ENOTCONN;
> >>  
> >> BUG_ON(wr->send_flags != IB_SEND_SIGNALED);
> >> BUG_ON(((struct svc_rdma_op_ctxt *)(unsigned long)wr->wr_id)->wr_op !=
> > --
> > 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
> 
> 

Re: [PATCH 2/17] svcrdma: Fix race with dto_tasklet in svc_rdma_send

[J. Bruce Fields]

On Mon, May 05, 2008 at 09:26:02PM -0500, Tom Tucker wrote:
> 
> 
> 
> On 5/5/08 5:06 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Fri, May 02, 2008 at 11:28:34AM -0500, Tom Tucker wrote:
> >> The svc_rdma_send function will attempt to reap SQ WR to make room for
> >> a new request if it finds the SQ full. This function races with the
> >> dto_tasklet that also reaps SQ WR. To avoid calling the function
> >> unnecessarily use test_and_clear_bit with the RDMAXPRT_SQ_PENDING
> >> flag to serialize access to the sq_cq_reap function.
> > 
> > OK.  I won't pretend to understand much of this, but--would it be worth
> > pulling out the added code here into a helper function, since it now
> > exists in two different places?  (Especially if correctness depends on
> > the same thing happening in both the places this bit can be cleared.)
> 
> Yes. Good suggestions.
> 
> BTW, this code is here because the SQ is undersized for big data. Since a
> single NFS_READ/WRITE can result in an attempt to fetch a large amount of
> data from the client (2M) and depending on certain HW resources this can
> result in a lot of WR being posted to the SQ.

WR is write request, SQ is send queue?  (In which case this happens on
nfs read operations?)  I'm behind....

> That said, there is a change coming in the 2.6.27 time frame that supports
> what is called Fast NSMR register. This allows the transport to effectively
> DMA map the entire transfer size (32k -- 2M) all as a single SGE. This will
> take a lot of pressure off the SQ and effectively make this code
> unnecessary.

OK!

Ideally we'd have patches for 2.6.27 in linux-next for a little while
first, so we should try to have that ready in a month or so, when the
-rc's start looking final.

--b.

Re: [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation

[J. Bruce Fields]

On Tue, May 06, 2008 at 09:48:06AM -0500, Tom Tucker wrote:
> 
> 
> 
> On 5/5/08 5:41 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Fri, May 02, 2008 at 11:28:39AM -0500, Tom Tucker wrote:
> >> A listening endpoint isn't known to the generic transport switch until
> >> the svc_create_xprt function returns without error. Calling
> >> svc_xprt_put within the xpo_create function causes the module reference
> >> count to be erroneously decremented.
> > 
> > There's some redundant code in these three error paths; would the usual
> > kernel-style "goto cleanup" thing help?
> 
> I think code-size it's a wash, but it might look more familiar.

Yeah, we try to stick to the same idioms as used elsewhere when it's
easy to.

Doesn't have to be done now, though.

> BTW, I noticed that the error path above returns a positive errno :-\,
> so that needs to be fixed too.

Oops, didn't catch that.  I'll expect a patch?

--b.

Re: [PATCH 11/17] svcrdma: Use standard Linux lists for context cache

[J. Bruce Fields]

On Fri, May 02, 2008 at 11:28:26AM -0500, Tom Tucker wrote:
> Replace the one-off linked list implementation used to implement the
> context cache with the standard Linux list_head lists. Add a cpmtext

cmptext == context?

Some spell checking might help those of us who are already a little lost
in the alphabet soup here.

Also, I assume that's referring to sc_ctxt_used.  It's initialized,
incremented, and decremented, but doesn't actually seem to be used
anywhere yet?

--b.

> counter to catch resource leaks.
> 
> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
> 
> ---
>  include/linux/sunrpc/svc_rdma.h          |    5 ++-
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   47 ++++++++++++++++-------------
>  2 files changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h
> index c447c41..7014390 100644
> --- a/include/linux/sunrpc/svc_rdma.h
> +++ b/include/linux/sunrpc/svc_rdma.h
> @@ -72,7 +72,7 @@ extern atomic_t rdma_stat_sq_prod;
>   */
>  struct svc_rdma_op_ctxt {
>  	struct svc_rdma_op_ctxt *read_hdr;
> -	struct svc_rdma_op_ctxt *next;
> +	struct list_head free_list;
>  	struct xdr_buf arg;
>  	struct list_head dto_q;
>  	enum ib_wr_opcode wr_op;
> @@ -104,7 +104,8 @@ struct svcxprt_rdma {
>  
>  	struct ib_pd         *sc_pd;
>  
> -	struct svc_rdma_op_ctxt  *sc_ctxt_head;
> +	atomic_t	     sc_ctxt_used;
> +	struct list_head     sc_ctxt_free;
>  	int		     sc_ctxt_cnt;
>  	int		     sc_ctxt_bump;
>  	int		     sc_ctxt_max;
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index c852fd9..c842cc2 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -103,8 +103,8 @@ static int rdma_bump_context_cache(struct svcxprt_rdma *xprt)
>  		spin_lock_bh(&xprt->sc_ctxt_lock);
>  		if (ctxt) {
>  			at_least_one = 1;
> -			ctxt->next = xprt->sc_ctxt_head;
> -			xprt->sc_ctxt_head = ctxt;
> +			INIT_LIST_HEAD(&ctxt->free_list);
> +			list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
>  		} else {
>  			/* kmalloc failed...give up for now */
>  			xprt->sc_ctxt_cnt--;
> @@ -123,7 +123,7 @@ struct svc_rdma_op_ctxt *svc_rdma_get_context(struct svcxprt_rdma *xprt)
>  
>  	while (1) {
>  		spin_lock_bh(&xprt->sc_ctxt_lock);
> -		if (unlikely(xprt->sc_ctxt_head == NULL)) {
> +		if (unlikely(list_empty(&xprt->sc_ctxt_free))) {
>  			/* Try to bump my cache. */
>  			spin_unlock_bh(&xprt->sc_ctxt_lock);
>  
> @@ -136,12 +136,15 @@ struct svc_rdma_op_ctxt *svc_rdma_get_context(struct svcxprt_rdma *xprt)
>  			schedule_timeout_uninterruptible(msecs_to_jiffies(500));
>  			continue;
>  		}
> -		ctxt = xprt->sc_ctxt_head;
> -		xprt->sc_ctxt_head = ctxt->next;
> +		ctxt = list_entry(xprt->sc_ctxt_free.next,
> +				  struct svc_rdma_op_ctxt,
> +				  free_list);
> +		list_del_init(&ctxt->free_list);
>  		spin_unlock_bh(&xprt->sc_ctxt_lock);
>  		ctxt->xprt = xprt;
>  		INIT_LIST_HEAD(&ctxt->dto_q);
>  		ctxt->count = 0;
> +		atomic_inc(&xprt->sc_ctxt_used);
>  		break;
>  	}
>  	return ctxt;
> @@ -163,10 +166,11 @@ void svc_rdma_put_context(struct svc_rdma_op_ctxt *ctxt, int free_pages)
>  				 ctxt->sge[i].addr,
>  				 ctxt->sge[i].length,
>  				 ctxt->direction);
> +
>  	spin_lock_bh(&xprt->sc_ctxt_lock);
> -	ctxt->next = xprt->sc_ctxt_head;
> -	xprt->sc_ctxt_head = ctxt;
> +	list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
>  	spin_unlock_bh(&xprt->sc_ctxt_lock);
> +	atomic_dec(&xprt->sc_ctxt_used);
>  }
>  
>  /* ib_cq event handler */
> @@ -409,28 +413,29 @@ static void create_context_cache(struct svcxprt_rdma *xprt,
>  	xprt->sc_ctxt_max = ctxt_max;
>  	xprt->sc_ctxt_bump = ctxt_bump;
>  	xprt->sc_ctxt_cnt = 0;
> -	xprt->sc_ctxt_head = NULL;
> +	atomic_set(&xprt->sc_ctxt_used, 0);
> +
> +	INIT_LIST_HEAD(&xprt->sc_ctxt_free);
>  	for (i = 0; i < ctxt_count; i++) {
>  		ctxt = kmalloc(sizeof(*ctxt), GFP_KERNEL);
>  		if (ctxt) {
> -			ctxt->next = xprt->sc_ctxt_head;
> -			xprt->sc_ctxt_head = ctxt;
> +			INIT_LIST_HEAD(&ctxt->free_list);
> +			list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
>  			xprt->sc_ctxt_cnt++;
>  		}
>  	}
>  }
>  
> -static void destroy_context_cache(struct svc_rdma_op_ctxt *ctxt)
> +static void destroy_context_cache(struct svcxprt_rdma *xprt)
>  {
> -	struct svc_rdma_op_ctxt *next;
> -	if (!ctxt)
> -		return;
> -
> -	do {
> -		next = ctxt->next;
> +	while (!list_empty(&xprt->sc_ctxt_free)) {
> +		struct svc_rdma_op_ctxt *ctxt;
> +		ctxt = list_entry(xprt->sc_ctxt_free.next,
> +				  struct svc_rdma_op_ctxt,
> +				  free_list);
> +		list_del_init(&ctxt->free_list);
>  		kfree(ctxt);
> -		ctxt = next;
> -	} while (next);
> +	}
>  }
>  
>  static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv,
> @@ -467,7 +472,7 @@ static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv,
>  				     reqs +
>  				     cma_xprt->sc_sq_depth +
>  				     RPCRDMA_MAX_THREADS + 1); /* max */
> -		if (!cma_xprt->sc_ctxt_head) {
> +		if (list_empty(&cma_xprt->sc_ctxt_free)) {
>  			kfree(cma_xprt);
>  			return NULL;
>  		}
> @@ -971,7 +976,7 @@ static void svc_rdma_free(struct svc_xprt *xprt)
>  	if (rdma->sc_pd && !IS_ERR(rdma->sc_pd))
>  		ib_dealloc_pd(rdma->sc_pd);
>  
> -	destroy_context_cache(rdma->sc_ctxt_head);
> +	destroy_context_cache(rdma);
>  	kfree(rdma);
>  }
>  

Re: [PATCH 0/17] svcrdma: RDMA transport driver close path cleanup

[J. Bruce Fields]

On Fri, May 02, 2008 at 11:28:24AM -0500, Tom Tucker wrote:
> Bruce:
> 
> This patchset fixes a number of defects in the close path of the SVCRDMA
> transport provider. The defects were found by injecting errors on the 
> transport at random intervals while running concurrent iozone tests on
> IB and iWARP mounts. With this set of patches applied I was able to run
> the the above tests for several hours without causing a crash or leaking
> resources. The transport recovers correctly on a client reconnect.
> 
> This patchset is based on 2.6 top of tree, however, I think these fixes are
> good candidates for a 2.6.25 dot release as well.

It may be a lot to try to get in to 2.6.25.

Anyway, my (unfortunately superficial) review doesn't catch anything
serious.  Up to you whether you'd like this version submitted or whether
you'd like to fix up those small things first.

--b.

Re: [PATCH 2/17] svcrdma: Fix race with dto_tasklet in svc_rdma_send

[Tom Tucker]

On 5/6/08 4:18 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, May 05, 2008 at 09:26:02PM -0500, Tom Tucker wrote:
>> 
>> 
>> 
>> On 5/5/08 5:06 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:
>> 
>>> On Fri, May 02, 2008 at 11:28:34AM -0500, Tom Tucker wrote:
>>>> The svc_rdma_send function will attempt to reap SQ WR to make room for
>>>> a new request if it finds the SQ full. This function races with the
>>>> dto_tasklet that also reaps SQ WR. To avoid calling the function
>>>> unnecessarily use test_and_clear_bit with the RDMAXPRT_SQ_PENDING
>>>> flag to serialize access to the sq_cq_reap function.
>>> 
>>> OK.  I won't pretend to understand much of this, but--would it be worth
>>> pulling out the added code here into a helper function, since it now
>>> exists in two different places?  (Especially if correctness depends on
>>> the same thing happening in both the places this bit can be cleared.)
>> 
>> Yes. Good suggestions.
>> 
>> BTW, this code is here because the SQ is undersized for big data. Since a
>> single NFS_READ/WRITE can result in an attempt to fetch a large amount of
>> data from the client (2M) and depending on certain HW resources this can
>> result in a lot of WR being posted to the SQ.
> 
> WR is write request, SQ is send queue?  (In which case this happens on
> nfs read operations?)  I'm behind....
> 
>> That said, there is a change coming in the 2.6.27 time frame that supports
>> what is called Fast NSMR register. This allows the transport to effectively
>> DMA map the entire transfer size (32k -- 2M) all as a single SGE. This will
>> take a lot of pressure off the SQ and effectively make this code
>> unnecessary.
> 
> OK!
> 
> Ideally we'd have patches for 2.6.27 in linux-next for a little while
> first, so we should try to have that ready in a month or so, when the
> -rc's start looking final.

It may be .28 then.  A lot of pieces have to come together...

> 
> --b.
> --
> 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

Re: [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation

[Tom Tucker]

On 5/6/08 4:22 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, May 06, 2008 at 09:48:06AM -0500, Tom Tucker wrote:
>> 
>> 
>> 
>> On 5/5/08 5:41 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:
>> 
>>> On Fri, May 02, 2008 at 11:28:39AM -0500, Tom Tucker wrote:
>>>> A listening endpoint isn't known to the generic transport switch until
>>>> the svc_create_xprt function returns without error. Calling
>>>> svc_xprt_put within the xpo_create function causes the module reference
>>>> count to be erroneously decremented.
>>> 
>>> There's some redundant code in these three error paths; would the usual
>>> kernel-style "goto cleanup" thing help?
>> 
>> I think code-size it's a wash, but it might look more familiar.
> 
> Yeah, we try to stick to the same idioms as used elsewhere when it's
> easy to.
> 
> Doesn't have to be done now, though.
> 
>> BTW, I noticed that the error path above returns a positive errno :-\,
>> so that needs to be fixed too.
> 
> Oops, didn't catch that.  I'll expect a patch?

I've got updates to three of the patches. I'm just retesting to make sure I
didn't inadvertently break something. I'll get the three updated patches to
you by tomorrow.

Tom

> 
> --b.

Re: [PATCH 11/17] svcrdma: Use standard Linux lists for context cache

[Tom Tucker]

On 5/6/08 4:32 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Fri, May 02, 2008 at 11:28:26AM -0500, Tom Tucker wrote:
>> Replace the one-off linked list implementation used to implement the
>> context cache with the standard Linux list_head lists. Add a cpmtext
> 
> cmptext == context?
> 
> Some spell checking might help those of us who are already a little lost
> in the alphabet soup here.
> 
> Also, I assume that's referring to sc_ctxt_used.  It's initialized,
> incremented, and decremented, but doesn't actually seem to be used
> anywhere yet?

There's a patch at the end that added a WARN_ON if it's not zero when the
transport is getting destroyed.

> 
> --b.
> 
>> counter to catch resource leaks.
>> 
>> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
>> 
>> ---
>>  include/linux/sunrpc/svc_rdma.h          |    5 ++-
>>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   47
>> ++++++++++++++++-------------
>>  2 files changed, 29 insertions(+), 23 deletions(-)
>> 
>> diff --git a/include/linux/sunrpc/svc_rdma.h
>> b/include/linux/sunrpc/svc_rdma.h
>> index c447c41..7014390 100644
>> --- a/include/linux/sunrpc/svc_rdma.h
>> +++ b/include/linux/sunrpc/svc_rdma.h
>> @@ -72,7 +72,7 @@ extern atomic_t rdma_stat_sq_prod;
>>   */
>>  struct svc_rdma_op_ctxt {
>> struct svc_rdma_op_ctxt *read_hdr;
>> - struct svc_rdma_op_ctxt *next;
>> + struct list_head free_list;
>> struct xdr_buf arg;
>> struct list_head dto_q;
>> enum ib_wr_opcode wr_op;
>> @@ -104,7 +104,8 @@ struct svcxprt_rdma {
>>  
>> struct ib_pd         *sc_pd;
>>  
>> - struct svc_rdma_op_ctxt  *sc_ctxt_head;
>> + atomic_t      sc_ctxt_used;
>> + struct list_head     sc_ctxt_free;
>> int       sc_ctxt_cnt;
>> int       sc_ctxt_bump;
>> int       sc_ctxt_max;
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> index c852fd9..c842cc2 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -103,8 +103,8 @@ static int rdma_bump_context_cache(struct svcxprt_rdma
>> *xprt)
>> spin_lock_bh(&xprt->sc_ctxt_lock);
>> if (ctxt) {
>> at_least_one = 1;
>> -   ctxt->next = xprt->sc_ctxt_head;
>> -   xprt->sc_ctxt_head = ctxt;
>> +   INIT_LIST_HEAD(&ctxt->free_list);
>> +   list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
>> } else {
>> /* kmalloc failed...give up for now */
>> xprt->sc_ctxt_cnt--;
>> @@ -123,7 +123,7 @@ struct svc_rdma_op_ctxt *svc_rdma_get_context(struct
>> svcxprt_rdma *xprt)
>>  
>> while (1) {
>> spin_lock_bh(&xprt->sc_ctxt_lock);
>> -  if (unlikely(xprt->sc_ctxt_head == NULL)) {
>> +  if (unlikely(list_empty(&xprt->sc_ctxt_free))) {
>> /* Try to bump my cache. */
>> spin_unlock_bh(&xprt->sc_ctxt_lock);
>>  
>> @@ -136,12 +136,15 @@ struct svc_rdma_op_ctxt *svc_rdma_get_context(struct
>> svcxprt_rdma *xprt)
>> schedule_timeout_uninterruptible(msecs_to_jiffies(500));
>> continue;
>> }
>> -  ctxt = xprt->sc_ctxt_head;
>> -  xprt->sc_ctxt_head = ctxt->next;
>> +  ctxt = list_entry(xprt->sc_ctxt_free.next,
>> +      struct svc_rdma_op_ctxt,
>> +      free_list);
>> +  list_del_init(&ctxt->free_list);
>> spin_unlock_bh(&xprt->sc_ctxt_lock);
>> ctxt->xprt = xprt;
>> INIT_LIST_HEAD(&ctxt->dto_q);
>> ctxt->count = 0;
>> +  atomic_inc(&xprt->sc_ctxt_used);
>> break;
>> }
>> return ctxt;
>> @@ -163,10 +166,11 @@ void svc_rdma_put_context(struct svc_rdma_op_ctxt
>> *ctxt, int free_pages)
>> ctxt->sge[i].addr,
>> ctxt->sge[i].length,
>> ctxt->direction);
>> +
>> spin_lock_bh(&xprt->sc_ctxt_lock);
>> - ctxt->next = xprt->sc_ctxt_head;
>> - xprt->sc_ctxt_head = ctxt;
>> + list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
>> spin_unlock_bh(&xprt->sc_ctxt_lock);
>> + atomic_dec(&xprt->sc_ctxt_used);
>>  }
>>  
>>  /* ib_cq event handler */
>> @@ -409,28 +413,29 @@ static void create_context_cache(struct svcxprt_rdma
>> *xprt,
>> xprt->sc_ctxt_max = ctxt_max;
>> xprt->sc_ctxt_bump = ctxt_bump;
>> xprt->sc_ctxt_cnt = 0;
>> - xprt->sc_ctxt_head = NULL;
>> + atomic_set(&xprt->sc_ctxt_used, 0);
>> +
>> + INIT_LIST_HEAD(&xprt->sc_ctxt_free);
>> for (i = 0; i < ctxt_count; i++) {
>> ctxt = kmalloc(sizeof(*ctxt), GFP_KERNEL);
>> if (ctxt) {
>> -   ctxt->next = xprt->sc_ctxt_head;
>> -   xprt->sc_ctxt_head = ctxt;
>> +   INIT_LIST_HEAD(&ctxt->free_list);
>> +   list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
>> xprt->sc_ctxt_cnt++;
>> }
>> }
>>  }
>>  
>> -static void destroy_context_cache(struct svc_rdma_op_ctxt *ctxt)
>> +static void destroy_context_cache(struct svcxprt_rdma *xprt)
>>  {
>> - struct svc_rdma_op_ctxt *next;
>> - if (!ctxt)
>> -  return;
>> -
>> - do {
>> -  next = ctxt->next;
>> + while (!list_empty(&xprt->sc_ctxt_free)) {
>> +  struct svc_rdma_op_ctxt *ctxt;
>> +  ctxt = list_entry(xprt->sc_ctxt_free.next,
>> +      struct svc_rdma_op_ctxt,
>> +      free_list);
>> +  list_del_init(&ctxt->free_list);
>> kfree(ctxt);
>> -  ctxt = next;
>> - } while (next);
>> + }
>>  }
>>  
>>  static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv,
>> @@ -467,7 +472,7 @@ static struct svcxprt_rdma *rdma_create_xprt(struct
>> svc_serv *serv,
>>     reqs +
>>     cma_xprt->sc_sq_depth +
>>     RPCRDMA_MAX_THREADS + 1); /* max */
>> -  if (!cma_xprt->sc_ctxt_head) {
>> +  if (list_empty(&cma_xprt->sc_ctxt_free)) {
>> kfree(cma_xprt);
>> return NULL;
>> }
>> @@ -971,7 +976,7 @@ static void svc_rdma_free(struct svc_xprt *xprt)
>> if (rdma->sc_pd && !IS_ERR(rdma->sc_pd))
>> ib_dealloc_pd(rdma->sc_pd);
>>  
>> - destroy_context_cache(rdma->sc_ctxt_head);
>> + destroy_context_cache(rdma);
>> kfree(rdma);
>>  }
>>  
> --
> 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

Re: [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation

[J. Bruce Fields]

On Tue, May 06, 2008 at 07:48:02PM -0500, Tom Tucker wrote:
> 
> 
> 
> On 5/6/08 4:22 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Tue, May 06, 2008 at 09:48:06AM -0500, Tom Tucker wrote:
> >> 
> >> 
> >> 
> >> On 5/5/08 5:41 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> >> 
> >>> On Fri, May 02, 2008 at 11:28:39AM -0500, Tom Tucker wrote:
> >>>> A listening endpoint isn't known to the generic transport switch until
> >>>> the svc_create_xprt function returns without error. Calling
> >>>> svc_xprt_put within the xpo_create function causes the module reference
> >>>> count to be erroneously decremented.
> >>> 
> >>> There's some redundant code in these three error paths; would the usual
> >>> kernel-style "goto cleanup" thing help?
> >> 
> >> I think code-size it's a wash, but it might look more familiar.
> > 
> > Yeah, we try to stick to the same idioms as used elsewhere when it's
> > easy to.
> > 
> > Doesn't have to be done now, though.
> > 
> >> BTW, I noticed that the error path above returns a positive errno :-\,
> >> so that needs to be fixed too.
> > 
> > Oops, didn't catch that.  I'll expect a patch?
> 
> I've got updates to three of the patches. I'm just retesting to make sure I
> didn't inadvertently break something. I'll get the three updated patches to
> you by tomorrow.

OK!  Thanks.  I'm travelling tommorow (then at connectathon for a week)
so apologies in advance if I'm slow.

--b.