| Message ID | 57327921.9030306@sandisk.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, May 10, 2016 at 05:13:21PM -0700, Bart Van Assche wrote: > 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 <bart.vanassche@sandisk.com> > Cc: Sagi Grimberg <sai@grimberg.me> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Laurence Oberman <loberman@redhat.com> > Cc: <stable@vger.kernel.org> # 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? > + srp_fr_pool_put(ch->fr_pool, &desc, 1); > + pr_debug("%s: ib_map_mr_sg(%d) returned %d.\n", > + dev_name(&req->scmnd->device->sdev_gendev), sg_nents, > + n); > return n; > + } > > req->reg_cqe.done = srp_reg_mr_err_done; > > -- > 2.8.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/11/2016 12:31 AM, Leon Romanovsky wrote: > On Tue, May 10, 2016 at 05:13:21PM -0700, Bart Van Assche wrote: >> 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 <bart.vanassche@sandisk.com> >> Cc: Sagi Grimberg <sai@grimberg.me> >> Cc: Christoph Hellwig <hch@lst.de> >> Cc: Laurence Oberman <loberman@redhat.com> >> Cc: <stable@vger.kernel.org> # 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)) { + srp_fr_pool_put(ch->fr_pool, &desc, 1); + pr_debug("%s: ib_map_mr_sg(%d) returned %d.\n", + dev_name(&req->scmnd->device->sdev_gendev), sg_nents, + n); return n; + } req->reg_cqe.done = srp_reg_mr_err_done;
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 <bart.vanassche@sandisk.com> Cc: Sagi Grimberg <sai@grimberg.me> Cc: Christoph Hellwig <hch@lst.de> Cc: Laurence Oberman <loberman@redhat.com> Cc: <stable@vger.kernel.org> # v4.4+ --- drivers/infiniband/ulp/srp/ib_srp.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)