[PATCH 00/10] Improve 9P/RDMA

[PATCH 00/10] Improve 9P/RDMA

[Simon Derr]

Hi,

This is a series of patches which aim to make 9P/RDMA better.

Eric Van Hensbergen asked that I send them to netdev to get a wider review.

They deal mainly with fcall allocation/deallocation, RQ locking,
error handling, and above all buffer management.
This has been tested intensively with the nfs-ganesha server.

Regards,

	Simon

Simon Derr (10):
  9P: Fix fcall allocation for rdma
  9P/RDMA: rdma_request() needs not allocate req->rc
  9pnet: refactor struct p9_fcall alloc code
  9P/RDMA: increase P9_RDMA_MAXSIZE to 1MB
  9P/RDMA: Protect against duplicate replies
  9P/RDMA: Use a semaphore to protect the RQ
  9P/RDMA: Do not free req->rc in error handling in rdma_request()
  9P/RDMA: Improve error handling in rdma_request
  9P/RDMA: count posted buffers without a pending request
  9P: Add cancelled() to the transport functions.

 include/net/9p/transport.h |    3 +
 net/9p/client.c            |   65 +++++++++++++---------
 net/9p/trans_rdma.c        |  132 ++++++++++++++++++++++++++++----------------
 3 files changed, 126 insertions(+), 74 deletions(-)

-- 
1.7.2.2

[PATCH 01/10] 9P: Fix fcall allocation for rdma

[Simon Derr]

The current code assumes that when a request in the request array
does have a tc, it also has a rc.

This is normally true, but not always : when using RDMA, req->rc
will temporarily be set to NULL after the request has been sent.
That is usually OK though, as when the reply arrives, req->rc will be
reassigned to a sane value before the request is recycled.

But there is a catch : if the request is flushed, the reply will never
arrive, and req->rc will be NULL, but not req->tc.

This patch fixes p9_tag_alloc to take this into account.

Signed-off-by: Simon Derr <simon.derr@bull.net>
---
 net/9p/client.c |   39 +++++++++++++++++++++++----------------
 1 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index 8eb7542..47cd7d0 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -258,27 +258,25 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
 	req = &c->reqs[row][col];
 	if (!req->tc) {
 		req->wq = kmalloc(sizeof(wait_queue_head_t), GFP_NOFS);
-		if (!req->wq) {
-			pr_err("Couldn't grow tag array\n");
-			return ERR_PTR(-ENOMEM);
-		}
+		if (!req->wq) 
+			goto grow_failed;
+
 		init_waitqueue_head(req->wq);
 		req->tc = kmalloc(sizeof(struct p9_fcall) + alloc_msize,
 				  GFP_NOFS);
+		if (!req->tc)
+			goto grow_failed;
+
+		req->tc->capacity = alloc_msize;
+		req->tc->sdata = (char *) req->tc + sizeof(struct p9_fcall);
+	}
+	if (!req->rc) {
 		req->rc = kmalloc(sizeof(struct p9_fcall) + alloc_msize,
 				  GFP_NOFS);
-		if ((!req->tc) || (!req->rc)) {
-			pr_err("Couldn't grow tag array\n");
-			kfree(req->tc);
-			kfree(req->rc);
-			kfree(req->wq);
-			req->tc = req->rc = NULL;
-			req->wq = NULL;
-			return ERR_PTR(-ENOMEM);
-		}
-		req->tc->capacity = alloc_msize;
+		if (!req->rc)
+			goto grow_failed;
+
 		req->rc->capacity = alloc_msize;
-		req->tc->sdata = (char *) req->tc + sizeof(struct p9_fcall);
 		req->rc->sdata = (char *) req->rc + sizeof(struct p9_fcall);
 	}
 
@@ -288,7 +286,16 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
 	req->tc->tag = tag-1;
 	req->status = REQ_STATUS_ALLOC;
 
-	return &c->reqs[row][col];
+	return req;
+
+grow_failed:
+	pr_err("Couldn't grow tag array\n");
+	kfree(req->tc);
+	kfree(req->rc);
+	kfree(req->wq);
+	req->tc = req->rc = NULL;
+	req->wq = NULL;
+	return ERR_PTR(-ENOMEM);
 }
 
 /**
-- 
1.7.2.2

[PATCH 02/10] 9P/RDMA: rdma_request() needs not allocate req->rc

[Simon Derr]

p9_tag_alloc() takes care of that.

Signed-off-by: Simon Derr <simon.derr@bull.net>
---
 net/9p/trans_rdma.c |   19 -------------------
 1 files changed, 0 insertions(+), 19 deletions(-)

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 2c69ddd..b1dfdf2 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -427,26 +427,7 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
 		err = -ENOMEM;
 		goto err_close;
 	}
-
-	/*
-	 * If the request has a buffer, steal it, otherwise
-	 * allocate a new one.  Typically, requests should already
-	 * have receive buffers allocated and just swap them around
-	 */
-	if (!req->rc) {
-		req->rc = kmalloc(sizeof(struct p9_fcall)+client->msize,
-				  GFP_NOFS);
-		if (req->rc) {
-			req->rc->sdata = (char *) req->rc +
-						sizeof(struct p9_fcall);
-			req->rc->capacity = client->msize;
-		}
-	}
 	rpl_context->rc = req->rc;
-	if (!rpl_context->rc) {
-		err = -ENOMEM;
-		goto err_free2;
-	}
 
 	/*
 	 * Post a receive buffer for this request. We need to ensure
-- 
1.7.2.2

[PATCH 03/10] 9pnet: refactor struct p9_fcall alloc code

[Simon Derr]

Signed-off-by: Simon Derr <simon.derr@bull.net>
---
 net/9p/client.c |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index 47cd7d0..44691b9 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -204,6 +204,17 @@ free_and_return:
 	return ret;
 }
 
+struct p9_fcall *p9_fcall_alloc(int alloc_msize)
+{
+	struct p9_fcall *fc;
+	fc = kmalloc(sizeof(struct p9_fcall) + alloc_msize, GFP_NOFS);
+	if (!fc)
+		return NULL;
+	fc->capacity = alloc_msize;
+	fc->sdata = (char *) fc + sizeof(struct p9_fcall);
+	return fc;
+}
+
 /**
  * p9_tag_alloc - lookup/allocate a request by tag
  * @c: client session to lookup tag within
@@ -256,29 +267,19 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
 	col = tag % P9_ROW_MAXTAG;
 
 	req = &c->reqs[row][col];
-	if (!req->tc) {
+	if (!req->wq) {
 		req->wq = kmalloc(sizeof(wait_queue_head_t), GFP_NOFS);
 		if (!req->wq) 
 			goto grow_failed;
-
 		init_waitqueue_head(req->wq);
-		req->tc = kmalloc(sizeof(struct p9_fcall) + alloc_msize,
-				  GFP_NOFS);
-		if (!req->tc)
-			goto grow_failed;
-
-		req->tc->capacity = alloc_msize;
-		req->tc->sdata = (char *) req->tc + sizeof(struct p9_fcall);
 	}
-	if (!req->rc) {
-		req->rc = kmalloc(sizeof(struct p9_fcall) + alloc_msize,
-				  GFP_NOFS);
-		if (!req->rc)
-			goto grow_failed;
 
-		req->rc->capacity = alloc_msize;
-		req->rc->sdata = (char *) req->rc + sizeof(struct p9_fcall);
-	}
+	if (!req->tc)
+		req->tc = p9_fcall_alloc(alloc_msize);
+	if (!req->rc)
+		req->rc = p9_fcall_alloc(alloc_msize);
+	if (!req->tc || !req->rc)
+		goto grow_failed;
 
 	p9pdu_reset(req->tc);
 	p9pdu_reset(req->rc);
-- 
1.7.2.2

Re: [PATCH 03/10] 9pnet: refactor struct p9_fcall alloc code

[Sergei Shtylyov]

Hello.

On 02-07-2013 17:11, Simon Derr wrote:

> Signed-off-by: Simon Derr <simon.derr@bull.net>
> ---
>   net/9p/client.c |   35 ++++++++++++++++++-----------------
>   1 files changed, 18 insertions(+), 17 deletions(-)

> diff --git a/net/9p/client.c b/net/9p/client.c
> index 47cd7d0..44691b9 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -204,6 +204,17 @@ free_and_return:
>   	return ret;
>   }
>
> +struct p9_fcall *p9_fcall_alloc(int alloc_msize)
> +{
> +	struct p9_fcall *fc;

    Empty line wouldn't hurt here, after declaration.

> +	fc = kmalloc(sizeof(struct p9_fcall) + alloc_msize, GFP_NOFS);
> +	if (!fc)
> +		return NULL;
> +	fc->capacity = alloc_msize;
> +	fc->sdata = (char *) fc + sizeof(struct p9_fcall);
> +	return fc;
> +}
> +
[...]

WBR, Sergei

[PATCH 04/10] 9P/RDMA: increase P9_RDMA_MAXSIZE to 1MB

[Simon Derr]

The current value is too low to get good performance.

Signed-off-by: Simon Derr <simon.derr@bull.net>
---
 net/9p/trans_rdma.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index b1dfdf2..b8b66d3 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -57,9 +57,7 @@
 #define P9_RDMA_IRD		0
 #define P9_RDMA_ORD		0
 #define P9_RDMA_TIMEOUT		30000		/* 30 seconds */
-#define P9_RDMA_MAXSIZE		(4*4096)	/* Min SGE is 4, so we can
-						 * safely advertise a maxsize
-						 * of 64k */
+#define P9_RDMA_MAXSIZE		(1024*1024)	/* 1MB */
 
 /**
  * struct p9_trans_rdma - RDMA transport instance
-- 
1.7.2.2

[PATCH 05/10] 9P/RDMA: Protect against duplicate replies

[Simon Derr]

A well-behaved server would not send twice the reply to a request.
But if it ever happens...
This additional check prevents the kernel from leaking memory
and possibly more nasty consequences in that unlikely event.

Signed-off-by: Simon Derr <simon.derr@bull.net>
---
 net/9p/trans_rdma.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index b8b66d3..274a9c1 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -294,6 +294,13 @@ handle_recv(struct p9_client *client, struct p9_trans_rdma *rdma,
 	if (!req)
 		goto err_out;
 
+	/* Check that we have not yet received a reply for this request.
+	 */
+	if (unlikely(req->rc)) {
+		pr_err("Duplicate reply for request %d", tag);
+		goto err_out;
+	}
+
 	req->rc = c->rc;
 	req->status = REQ_STATUS_RCVD;
 	p9_client_cb(client, req);
-- 
1.7.2.2

[PATCH 06/10] 9P/RDMA: Use a semaphore to protect the RQ

[Simon Derr]

The current code keeps track of the number of buffers posted in the RQ,
and will prevent it from overflowing. But it does so by simply dropping
post requests (And leaking memory in the process).
When this happens there will actually be too few buffers posted, and
soon the 9P server will complain about 'RNR retry counter exceeded'
errors.

Instead, use a semaphore, and block until the RQ is ready for another
buffer to be posted.

Signed-off-by: Simon Derr <simon.derr@bull.net>
---
 net/9p/trans_rdma.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 274a9c1..ad8dc33 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -73,7 +73,7 @@
  * @sq_depth: The depth of the Send Queue
  * @sq_sem: Semaphore for the SQ
  * @rq_depth: The depth of the Receive Queue.
- * @rq_count: Count of requests in the Receive Queue.
+ * @rq_sem: Semaphore for the RQ
  * @addr: The remote peer's address
  * @req_lock: Protects the active request list
  * @cm_done: Completion event for connection management tracking
@@ -98,7 +98,7 @@ struct p9_trans_rdma {
 	int sq_depth;
 	struct semaphore sq_sem;
 	int rq_depth;
-	atomic_t rq_count;
+	struct semaphore rq_sem;
 	struct sockaddr_in addr;
 	spinlock_t req_lock;
 
@@ -341,8 +341,8 @@ static void cq_comp_handler(struct ib_cq *cq, void *cq_context)
 
 		switch (c->wc_op) {
 		case IB_WC_RECV:
-			atomic_dec(&rdma->rq_count);
 			handle_recv(client, rdma, c, wc.status, wc.byte_len);
+			up(&rdma->rq_sem);
 			break;
 
 		case IB_WC_SEND:
@@ -441,12 +441,14 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
 	 * outstanding request, so we must keep a count to avoid
 	 * overflowing the RQ.
 	 */
-	if (atomic_inc_return(&rdma->rq_count) <= rdma->rq_depth) {
-		err = post_recv(client, rpl_context);
-		if (err)
-			goto err_free1;
-	} else
-		atomic_dec(&rdma->rq_count);
+	if (down_interruptible(&rdma->rq_sem))
+		goto error; /* FIXME : -EINTR instead */
+
+	err = post_recv(client, rpl_context);
+	if (err) {
+		p9_debug(P9_DEBUG_FCALL, "POST RECV failed\n");
+		goto err_free1;
+	}
 
 	/* remove posted receive buffer from request structure */
 	req->rc = NULL;
@@ -537,7 +539,7 @@ static struct p9_trans_rdma *alloc_rdma(struct p9_rdma_opts *opts)
 	spin_lock_init(&rdma->req_lock);
 	init_completion(&rdma->cm_done);
 	sema_init(&rdma->sq_sem, rdma->sq_depth);
-	atomic_set(&rdma->rq_count, 0);
+	sema_init(&rdma->rq_sem, rdma->rq_depth);
 
 	return rdma;
 }
-- 
1.7.2.2

[PATCH 07/10] 9P/RDMA: Do not free req->rc in error handling in rdma_request()

[Simon Derr]

rdma_request() should never be in charge of freeing rc.

When an error occurs:
* Either the rc buffer has been recv_post()'ed.
  then kfree()'ing it certainly is a bad idea.
* Or is has not, and in that case req->rc still points to it,
  hence it needs not be freed.

Signed-off-by: Simon Derr <simon.derr@bull.net>
---
 net/9p/trans_rdma.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index ad8dc33..1bd4c71 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -447,7 +447,7 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
 	err = post_recv(client, rpl_context);
 	if (err) {
 		p9_debug(P9_DEBUG_FCALL, "POST RECV failed\n");
-		goto err_free1;
+		goto err_free;
 	}
 
 	/* remove posted receive buffer from request structure */
@@ -457,7 +457,7 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
 	c = kmalloc(sizeof *c, GFP_NOFS);
 	if (!c) {
 		err = -ENOMEM;
-		goto err_free1;
+		goto err_free;
 	}
 	c->req = req;
 
@@ -486,13 +486,10 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
 
  error:
 	kfree(c);
-	kfree(rpl_context->rc);
 	kfree(rpl_context);
 	p9_debug(P9_DEBUG_ERROR, "EIO\n");
 	return -EIO;
- err_free1:
-	kfree(rpl_context->rc);
- err_free2:
+ err_free:
 	kfree(rpl_context);
  err_close:
 	spin_lock_irqsave(&rdma->req_lock, flags);
-- 
1.7.2.2

[PATCH 08/10] 9P/RDMA: Improve error handling in rdma_request

[Simon Derr]

Most importantly:
- do not free the recv context (rpl_context) after a successful post_recv()
- but do free the send context (c) after a failed send.

Signed-off-by: Simon Derr <simon.derr@bull.net>
---
 net/9p/trans_rdma.c |   44 ++++++++++++++++++++++++++++----------------
 1 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 1bd4c71..926e72d 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -430,7 +430,7 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
 	rpl_context = kmalloc(sizeof *rpl_context, GFP_NOFS);
 	if (!rpl_context) {
 		err = -ENOMEM;
-		goto err_close;
+		goto recv_error;
 	}
 	rpl_context->rc = req->rc;
 
@@ -441,13 +441,15 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
 	 * outstanding request, so we must keep a count to avoid
 	 * overflowing the RQ.
 	 */
-	if (down_interruptible(&rdma->rq_sem))
-		goto error; /* FIXME : -EINTR instead */
+	if (down_interruptible(&rdma->rq_sem)) {
+		err = -EINTR;
+		goto recv_error;
+	}
 
 	err = post_recv(client, rpl_context);
 	if (err) {
 		p9_debug(P9_DEBUG_FCALL, "POST RECV failed\n");
-		goto err_free;
+		goto recv_error;
 	}
 
 	/* remove posted receive buffer from request structure */
@@ -457,15 +459,17 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
 	c = kmalloc(sizeof *c, GFP_NOFS);
 	if (!c) {
 		err = -ENOMEM;
-		goto err_free;
+		goto send_error;
 	}
 	c->req = req;
 
 	c->busa = ib_dma_map_single(rdma->cm_id->device,
 				    c->req->tc->sdata, c->req->tc->size,
 				    DMA_TO_DEVICE);
-	if (ib_dma_mapping_error(rdma->cm_id->device, c->busa))
-		goto error;
+	if (ib_dma_mapping_error(rdma->cm_id->device, c->busa)) {
+		err = -EIO;
+		goto send_error;
+	}
 
 	sge.addr = c->busa;
 	sge.length = c->req->tc->size;
@@ -479,19 +483,27 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
 	wr.sg_list = &sge;
 	wr.num_sge = 1;
 
-	if (down_interruptible(&rdma->sq_sem))
-		goto error;
+	if (down_interruptible(&rdma->sq_sem)) {
+		err = -EINTR;
+		goto send_error;
+	}
 
-	return ib_post_send(rdma->qp, &wr, &bad_wr);
+	err = ib_post_send(rdma->qp, &wr, &bad_wr);
+	if (err)
+		goto send_error;
 
- error:
+	/* Success */
+	return 0;
+
+ /* Handle errors that happened during or while preparing the send: */
+ send_error:
 	kfree(c);
+	p9_debug(P9_DEBUG_ERROR, "Error %d in rdma_request()\n", err);
+	return err;
+
+ /* Handle errors that happened during or while preparing post_recv(): */
+ recv_error:
 	kfree(rpl_context);
-	p9_debug(P9_DEBUG_ERROR, "EIO\n");
-	return -EIO;
- err_free:
-	kfree(rpl_context);
- err_close:
 	spin_lock_irqsave(&rdma->req_lock, flags);
 	if (rdma->state < P9_RDMA_CLOSING) {
 		rdma->state = P9_RDMA_CLOSING;
-- 
1.7.2.2

[PATCH 09/10] 9P/RDMA: count posted buffers without a pending request

[Simon Derr]

In rdma_request():

If an error occurs between posting the recv and the send,
there will be a reply context posted without a pending
request.
Since there is no way to "un-post" it, we remember it and
skip post_recv() for the next request.

Signed-off-by: Simon Derr <simon.derr@bull.net>
---
 net/9p/trans_rdma.c |   31 ++++++++++++++++++++++++++++++-
 1 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 926e72d..8f68df5 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -74,6 +74,8 @@
  * @sq_sem: Semaphore for the SQ
  * @rq_depth: The depth of the Receive Queue.
  * @rq_sem: Semaphore for the RQ
+ * @excess_rc : Amount of posted Receive Contexts without a pending request.
+ *		See rdma_request()
  * @addr: The remote peer's address
  * @req_lock: Protects the active request list
  * @cm_done: Completion event for connection management tracking
@@ -99,6 +101,7 @@ struct p9_trans_rdma {
 	struct semaphore sq_sem;
 	int rq_depth;
 	struct semaphore rq_sem;
+	atomic_t excess_rc;
 	struct sockaddr_in addr;
 	spinlock_t req_lock;
 
@@ -426,6 +429,26 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
 	struct p9_rdma_context *c = NULL;
 	struct p9_rdma_context *rpl_context = NULL;
 
+	/* When an error occurs between posting the recv and the send,
+	 * there will be a receive context posted without a pending request.
+	 * Since there is no way to "un-post" it, we remember it and skip
+	 * post_recv() for the next request.
+	 * So here,
+	 * see if we are this `next request' and need to absorb an excess rc.
+	 * If yes, then drop and free our own, and do not recv_post().
+	 **/
+	if (unlikely(atomic_read(&rdma->excess_rc) > 0)) {
+		if ((atomic_sub_return(1, &rdma->excess_rc) >= 0)) {
+			/* Got one ! */
+			kfree(req->rc);
+			req->rc = NULL;
+			goto dont_need_post_recv;
+		} else {
+			/* We raced and lost. */
+			atomic_inc(&rdma->excess_rc);
+		}
+	}
+
 	/* Allocate an fcall for the reply */
 	rpl_context = kmalloc(sizeof *rpl_context, GFP_NOFS);
 	if (!rpl_context) {
@@ -451,10 +474,10 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
 		p9_debug(P9_DEBUG_FCALL, "POST RECV failed\n");
 		goto recv_error;
 	}
-
 	/* remove posted receive buffer from request structure */
 	req->rc = NULL;
 
+dont_need_post_recv:
 	/* Post the request */
 	c = kmalloc(sizeof *c, GFP_NOFS);
 	if (!c) {
@@ -499,6 +522,11 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
  send_error:
 	kfree(c);
 	p9_debug(P9_DEBUG_ERROR, "Error %d in rdma_request()\n", err);
+
+	/* Ach.
+	 *  We did recv_post(), but not send. We have one recv_post in excess.
+	 */
+	atomic_inc(&rdma->excess_rc);
 	return err;
 
  /* Handle errors that happened during or while preparing post_recv(): */
@@ -549,6 +577,7 @@ static struct p9_trans_rdma *alloc_rdma(struct p9_rdma_opts *opts)
 	init_completion(&rdma->cm_done);
 	sema_init(&rdma->sq_sem, rdma->sq_depth);
 	sema_init(&rdma->rq_sem, rdma->rq_depth);
+	atomic_set(&rdma->excess_rc, 0);
 
 	return rdma;
 }
-- 
1.7.2.2

[PATCH 10/10] 9P: Add cancelled() to the transport functions.

[Simon Derr]

RDMA needs to post a buffer for each incoming reply.
Hence it needs to keep count of these and needs to be
aware of whether a flushed request has received a reply
or not.

This patch adds the cancelled() callback to the transport modules.
It is called when RFLUSH has been received and that the corresponding
request will never receive a reply.

Signed-off-by: Simon Derr <simon.derr@bull.net>
---
 include/net/9p/transport.h |    3 +++
 net/9p/client.c            |   11 ++++++++---
 net/9p/trans_rdma.c        |   10 ++++++++++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/include/net/9p/transport.h b/include/net/9p/transport.h
index adcbb20..77a0578 100644
--- a/include/net/9p/transport.h
+++ b/include/net/9p/transport.h
@@ -37,6 +37,8 @@
  * @close: member function to discard a connection on this transport
  * @request: member function to issue a request to the transport
  * @cancel: member function to cancel a request (if it hasn't been sent)
+ * @cancelled: member function to notify that a cancelled request will not
+ *             not receive a reply	
  *
  * This is the basic API for a transport module which is registered by the
  * transport module with the 9P core network module and used by the client
@@ -55,6 +57,7 @@ struct p9_trans_module {
 	void (*close) (struct p9_client *);
 	int (*request) (struct p9_client *, struct p9_req_t *req);
 	int (*cancel) (struct p9_client *, struct p9_req_t *req);
+	int (*cancelled) (struct p9_client *, struct p9_req_t *req);
 	int (*zc_request)(struct p9_client *, struct p9_req_t *,
 			  char *, char *, int , int, int, int);
 };
diff --git a/net/9p/client.c b/net/9p/client.c
index 44691b9..61e0455 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -676,11 +676,16 @@ static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq)
 
 
 	/* if we haven't received a response for oldreq,
-	   remove it from the list. */
+	   remove it from the list, and notify the transport
+	   layer that the reply will never arrive. */
 	spin_lock(&c->lock);
-	if (oldreq->status == REQ_STATUS_FLSH)
+	if (oldreq->status == REQ_STATUS_FLSH) {
 		list_del(&oldreq->req_list);
-	spin_unlock(&c->lock);
+		spin_unlock(&c->lock);
+		if (c->trans_mod->cancelled)
+			c->trans_mod->cancelled(c, req);
+	} else
+		spin_unlock(&c->lock);
 
 	p9_free_req(c, req);
 	return 0;
diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 8f68df5..9e6c220 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -588,6 +588,16 @@ static int rdma_cancel(struct p9_client *client, struct p9_req_t *req)
 	return 1;
 }
 
+/* A request has been fully flushed without a reply.
+ * That means we have posted one buffer in excess.
+ */
+static int rdma_cancelled(struct p9_client *client, struct p9_req_t *req)
+{
+	struct p9_trans_rdma *rdma = client->trans;
+	atomic_inc(&rdma->excess_rc);
+	return 0;
+}
+
 /**
  * trans_create_rdma - Transport method for creating atransport instance
  * @client: client instance
-- 
1.7.2.2

Re: [PATCH 10/10] 9P: Add cancelled() to the transport functions.

[Sergei Shtylyov]

Hello.

On 02-07-2013 17:11, Simon Derr wrote:

> RDMA needs to post a buffer for each incoming reply.
> Hence it needs to keep count of these and needs to be
> aware of whether a flushed request has received a reply
> or not.

> This patch adds the cancelled() callback to the transport modules.
> It is called when RFLUSH has been received and that the corresponding
> request will never receive a reply.

> Signed-off-by: Simon Derr <simon.derr@bull.net>
> ---
>   include/net/9p/transport.h |    3 +++
>   net/9p/client.c            |   11 ++++++++---
>   net/9p/trans_rdma.c        |   10 ++++++++++
>   3 files changed, 21 insertions(+), 3 deletions(-)

> diff --git a/include/net/9p/transport.h b/include/net/9p/transport.h
> index adcbb20..77a0578 100644
> --- a/include/net/9p/transport.h
> +++ b/include/net/9p/transport.h
[...]
> @@ -55,6 +57,7 @@ struct p9_trans_module {
>   	void (*close) (struct p9_client *);
>   	int (*request) (struct p9_client *, struct p9_req_t *req);
>   	int (*cancel) (struct p9_client *, struct p9_req_t *req);
> +	int (*cancelled) (struct p9_client *, struct p9_req_t *req);

    There should be no space before ( opening the parameter list.
scripts/checkpatch.pl should have warned you here.

>   	int (*zc_request)(struct p9_client *, struct p9_req_t *,
>   			  char *, char *, int , int, int, int);
>   };
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 44691b9..61e0455 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -676,11 +676,16 @@ static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq)
>
>
>   	/* if we haven't received a response for oldreq,
> -	   remove it from the list. */
> +	   remove it from the list, and notify the transport
> +	   layer that the reply will never arrive. */

    The preferred style of the multi-line commnets in the networking 
code is:

	/* bla
	 * bla
	 */

>   	spin_lock(&c->lock);
> -	if (oldreq->status == REQ_STATUS_FLSH)
> +	if (oldreq->status == REQ_STATUS_FLSH) {
>   		list_del(&oldreq->req_list);
> -	spin_unlock(&c->lock);
> +		spin_unlock(&c->lock);
> +		if (c->trans_mod->cancelled)
> +			c->trans_mod->cancelled(c, req);
> +	} else
> +		spin_unlock(&c->lock);

    According to Documentation/CodingStyle chapter 3, both arms of the 
*if* statment should have {} if one arm has it.

> diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
> index 8f68df5..9e6c220 100644
> --- a/net/9p/trans_rdma.c
> +++ b/net/9p/trans_rdma.c
> @@ -588,6 +588,16 @@ static int rdma_cancel(struct p9_client *client, struct p9_req_t *req)
>   	return 1;
>   }
>
> +/* A request has been fully flushed without a reply.
> + * That means we have posted one buffer in excess.
> + */
> +static int rdma_cancelled(struct p9_client *client, struct p9_req_t *req)
> +{
> +	struct p9_trans_rdma *rdma = client->trans;

    Empty line wouldn't hurt here, after declaration.

> +	atomic_inc(&rdma->excess_rc);
> +	return 0;
> +}
> +

WBR, Sergei