| Message ID | 20171126133104.22710-1-sagi@grimberg.me (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Oops, This should go via the target tree, CCing target-devel and nab On 11/26/2017 03:31 PM, Sagi Grimberg wrote: > In case we fail to establish the connection we must drain our pre-posted > login recieve work request before continuing safely with connection > teardown. > > Reported-by: Amrani, Ram <Ram.Amrani@cavium.com> > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > --- > drivers/infiniband/ulp/isert/ib_isert.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c > index ceabdb85df8b..9d4785ba24cb 100644 > --- a/drivers/infiniband/ulp/isert/ib_isert.c > +++ b/drivers/infiniband/ulp/isert/ib_isert.c > @@ -741,6 +741,7 @@ isert_connect_error(struct rdma_cm_id *cma_id) > { > struct isert_conn *isert_conn = cma_id->qp->qp_context; > > + ib_drain_qp(isert_conn->qp); > list_del_init(&isert_conn->node); > isert_conn->cm_id = NULL; > isert_put_conn(isert_conn); > -- 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 11/26/2017 03:31 PM, Sagi Grimberg wrote: > > In case we fail to establish the connection we must drain our pre-posted > > login recieve work request before continuing safely with connection > > teardown. > > > > Reported-by: Amrani, Ram <Ram.Amrani@cavium.com> > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > > --- > > drivers/infiniband/ulp/isert/ib_isert.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c > > index ceabdb85df8b..9d4785ba24cb 100644 > > --- a/drivers/infiniband/ulp/isert/ib_isert.c > > +++ b/drivers/infiniband/ulp/isert/ib_isert.c > > @@ -741,6 +741,7 @@ isert_connect_error(struct rdma_cm_id *cma_id) > > { > > struct isert_conn *isert_conn = cma_id->qp->qp_context; > > > > + ib_drain_qp(isert_conn->qp); > > list_del_init(&isert_conn->node); > > isert_conn->cm_id = NULL; > > isert_put_conn(isert_conn); With this patch our test behaves as expected. Thanks, Ram
Hi Sagi, > > Reported-by: Amrani, Ram <Ram.Amrani@cavium.com> > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > > --- > > drivers/infiniband/ulp/isert/ib_isert.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c > > index ceabdb85df8b..9d4785ba24cb 100644 > > --- a/drivers/infiniband/ulp/isert/ib_isert.c > > +++ b/drivers/infiniband/ulp/isert/ib_isert.c > > @@ -741,6 +741,7 @@ isert_connect_error(struct rdma_cm_id *cma_id) > > { > > struct isert_conn *isert_conn = cma_id->qp->qp_context; > > > > + ib_drain_qp(isert_conn->qp); > > list_del_init(&isert_conn->node); > > isert_conn->cm_id = NULL; > > isert_put_conn(isert_conn); > > > -- If it's not already committed, can you add a "Fixes:" so stable maintainers will catch it? Thanks, Ram
> If it's not already committed, can you add a "Fixes:" so stable maintainers will catch it?
I'm not sure at all that there is a clear cut commit that this fixes,
both the target code and the rdma interface code changed a lot so I
can't find any clear indication which broke this.
I can send a separate patch to stable from when we introduced
ib_drain_qp though.
--
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
hi Guys, did we forget taking this fix to mainline/stable or we found another solution ? On 11/30/2017 12:46 AM, Sagi Grimberg wrote: > >> If it's not already committed, can you add a "Fixes:" so stable >> maintainers will catch it? > > I'm not sure at all that there is a clear cut commit that this fixes, > both the target code and the rdma interface code changed a lot so I > can't find any clear indication which broke this. > > I can send a separate patch to stable from when we introduced > ib_drain_qp though. > -- > To unsubscribe from this list: send the line "unsubscribe target-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.kernel.org%2Fmajordomo-info.html&data=02%7C01%7Cmaxg%40mellanox.com%7C18cac1be99bb4d99661c08d5377b08c6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636475923920075321&sdata=WDJ6wr2o8jFyyGdHcLo06EhE18GTz9wUbF%2BezWzkGbc%3D&reserved=0 > -Max. -- 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 Tue, Jan 09, 2018 at 07:04:16PM +0200, Max Gurtovoy wrote: > hi Guys, > did we forget taking this fix to mainline/stable or we found another > solution ? I thought it was going to NAB's tree.. My mistake, I put it back into patchworks and it will get into for-next Jason -- 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
> I thought it was going to NAB's tree.. My mistake, I put it back into > patchworks and it will get into for-next > > Jason Thanks. BTW, you can use for-rc, as it is a bug fix. Ram -- 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 Tue, 2018-01-09 at 10:17 -0700, Jason Gunthorpe wrote: > On Tue, Jan 09, 2018 at 07:04:16PM +0200, Max Gurtovoy wrote: > > hi Guys, > > did we forget taking this fix to mainline/stable or we found another > > solution ? > > I thought it was going to NAB's tree.. My mistake, I put it back into > patchworks and it will get into for-next In fairness, the email thread said exactly that. But, it never got picked up by NAB. And being that it's in our tree and calling an rdma- core function, it can easily enough go through our tree, so I applied it to for-rc.
On Wed, 2018-01-10 at 16:54 -0500, Doug Ledford wrote: > On Tue, 2018-01-09 at 10:17 -0700, Jason Gunthorpe wrote: > > On Tue, Jan 09, 2018 at 07:04:16PM +0200, Max Gurtovoy wrote: > > > hi Guys, > > > did we forget taking this fix to mainline/stable or we found another > > > solution ? > > > > I thought it was going to NAB's tree.. My mistake, I put it back into > > patchworks and it will get into for-next > > In fairness, the email thread said exactly that. But, it never got > picked up by NAB. And being that it's in our tree and calling an rdma- > core function, it can easily enough go through our tree, so I applied it > to for-rc. Thanks Sagi + Ram, and Doug for picking this up. Per the earlier discussion, it looks like this needs a stable CC, right..? As Sagi mentioned, it's not clear if this is a regression vs. generic RDMA READ/WRITE API logic, or a day one issue. So how about adding a 4.7+ stable tag, to match commit a060b5629..? -- 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 Fri, Jan 12, 2018 at 09:46:05PM -0800, Nicholas A. Bellinger wrote: > On Wed, 2018-01-10 at 16:54 -0500, Doug Ledford wrote: > > On Tue, 2018-01-09 at 10:17 -0700, Jason Gunthorpe wrote: > > > On Tue, Jan 09, 2018 at 07:04:16PM +0200, Max Gurtovoy wrote: > > > > hi Guys, > > > > did we forget taking this fix to mainline/stable or we found another > > > > solution ? > > > > > > I thought it was going to NAB's tree.. My mistake, I put it back into > > > patchworks and it will get into for-next > > > > In fairness, the email thread said exactly that. But, it never got > > picked up by NAB. And being that it's in our tree and calling an rdma- > > core function, it can easily enough go through our tree, so I applied it > > to for-rc. > > Thanks Sagi + Ram, and Doug for picking this up. > > Per the earlier discussion, it looks like this needs a stable CC, > right..? As Sagi mentioned, it's not clear if this is a regression vs. > generic RDMA READ/WRITE API logic, or a day one issue. > > So how about adding a 4.7+ stable tag, to match commit a060b5629..? That can be done: Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API") Cc: <stable@vger.kernel.org> # 4.7+ Reported-by: Amrani, Ram <Ram.Amrani@cavium.com> Signed-off-by: Sagi Grimberg <sagi@grimberg.me> Signed-off-by: Doug Ledford <dledford@redhat.com> Doug: I took your wip/dl-for-rc branch, revised the commit message and pushed it to for-rc. Jason -- 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
Thanks Jason for picking it up, and sorry for the late reply on this. > That can be done: > > Fixes: a060b5629ab0 ("IB/core: generic RDMA READ/WRITE API") This patch is not the offending patch, if at all its this one: Fixes: 572a143489a1 ("iser-target: Use ib_drain_qp") But, the missing qp drain existed way before this commit. I think any fixes tag would be confusing as there isn't a clear culprit for this one... -- 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/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c index ceabdb85df8b..9d4785ba24cb 100644 --- a/drivers/infiniband/ulp/isert/ib_isert.c +++ b/drivers/infiniband/ulp/isert/ib_isert.c @@ -741,6 +741,7 @@ isert_connect_error(struct rdma_cm_id *cma_id) { struct isert_conn *isert_conn = cma_id->qp->qp_context; + ib_drain_qp(isert_conn->qp); list_del_init(&isert_conn->node); isert_conn->cm_id = NULL; isert_put_conn(isert_conn);
In case we fail to establish the connection we must drain our pre-posted login recieve work request before continuing safely with connection teardown. Reported-by: Amrani, Ram <Ram.Amrani@cavium.com> Signed-off-by: Sagi Grimberg <sagi@grimberg.me> --- drivers/infiniband/ulp/isert/ib_isert.c | 1 + 1 file changed, 1 insertion(+)