Re: [PATCH 2/6] IB/srp: Fix a memory descriptor leak in an error path

On 05/11/2016 12:31 AM, Leon Romanovsky wrote:

On Tue, May 10, 2016 at 05:13:21PM -0700, Bart Van Assche wrote:

quoted

If an error occurs after srp_fr_pool_get() succeeded and before the
descriptor is stored in srp_map_state (*state->fr.next++ = desc)
then srp_unmap_data() won't free the newly allocated memory
descriptor. Hence free the descriptor explicitly.

Fixes: f7f7aab1a5c0 ("IB/srp: Convert to new registration API")
Signed-off-by: Bart Van Assche <redacted>
Cc: Sagi Grimberg <redacted>
Cc: Christoph Hellwig <redacted>
Cc: Laurence Oberman <redacted>
Cc: <redacted> # v4.4+
---
  drivers/infiniband/ulp/srp/ib_srp.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index e088a49..74e3ec8 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c

@@ -1330,8 +1330,13 @@ static int srp_map_finish_fr(struct srp_map_state *state,
  	ib_update_fast_reg_key(desc->mr, rkey);

  	n = ib_map_mr_sg(desc->mr, state->sg, sg_nents, 0, dev->mr_page_size);
-	if (unlikely(n < 0))
+	if (unlikely(n < 0)) {

The ib_map_mr_sg can return 0 which is not error, but still pretty
useless number of mapped SGE. I didn't look on the srp code close enough, but will
the code handle this case correctly?

Hello Leon,

ib_map_mr_sg() can only return 0 after patch 4/6 of this series has been 
applied. xfstests triggers I/O requests that are large enough to make 
ib_map_mr_sg() return 0 when enabling register_always and when using 
fast registration with an mlx4 HCA. This is because for mlx4 the number 
of pages per fast registration request is limited to 511. 511 * 4096 = 
2044 KB. This is less than the value I used for max_sectors in my tests 
(4 MB).

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html