| Message ID | 20150504175818.3483.22408.stgit@manet.1015granger.net (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On 5/4/2015 8:58 PM, Chuck Lever wrote: > An RPC can exit at any time. When it does so, xprt_rdma_free() is > called, and it calls ->op_unmap(). > > If ->ro_reset() is running due to a transport disconnect, the two > methods can race while processing the same rpcrdma_mw. The results > are unpredictable. > > Because of this, in previous patches I've replaced the ->ro_reset() > methods with a recovery workqueue. ->ro_reset() is no longer used > and can be removed. > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > net/sunrpc/xprtrdma/fmr_ops.c | 11 ----------- > net/sunrpc/xprtrdma/frwr_ops.c | 16 ---------------- > net/sunrpc/xprtrdma/physical_ops.c | 6 ------ > net/sunrpc/xprtrdma/verbs.c | 2 -- > net/sunrpc/xprtrdma/xprt_rdma.h | 1 - > 5 files changed, 36 deletions(-) > > diff --git a/net/sunrpc/xprtrdma/fmr_ops.c b/net/sunrpc/xprtrdma/fmr_ops.c > index ad0055b..5dd77da 100644 > --- a/net/sunrpc/xprtrdma/fmr_ops.c > +++ b/net/sunrpc/xprtrdma/fmr_ops.c > @@ -197,16 +197,6 @@ out_err: > return nsegs; > } > > -/* After a disconnect, unmap all FMRs. > - * > - * This is invoked only in the transport connect worker in order > - * to serialize with rpcrdma_register_fmr_external(). > - */ > -static void > -fmr_op_reset(struct rpcrdma_xprt *r_xprt) > -{ > -} > - > static void > fmr_op_destroy(struct rpcrdma_buffer *buf) > { > @@ -230,7 +220,6 @@ const struct rpcrdma_memreg_ops rpcrdma_fmr_memreg_ops = { > .ro_open = fmr_op_open, > .ro_maxpages = fmr_op_maxpages, > .ro_init = fmr_op_init, > - .ro_reset = fmr_op_reset, > .ro_destroy = fmr_op_destroy, > .ro_displayname = "fmr", > }; > diff --git a/net/sunrpc/xprtrdma/frwr_ops.c b/net/sunrpc/xprtrdma/frwr_ops.c > index 6f93a89..3fb609a 100644 > --- a/net/sunrpc/xprtrdma/frwr_ops.c > +++ b/net/sunrpc/xprtrdma/frwr_ops.c > @@ -430,21 +430,6 @@ out_err: > return nsegs; > } > > -/* After a disconnect, a flushed FAST_REG_MR can leave an FRMR in > - * an unusable state. Find FRMRs in this state and dereg / reg > - * each. FRMRs that are VALID and attached to an rpcrdma_req are > - * also torn down. > - * > - * This gives all in-use FRMRs a fresh rkey and leaves them INVALID. > - * > - * This is invoked only in the transport connect worker in order > - * to serialize with rpcrdma_register_frmr_external(). > - */ > -static void > -frwr_op_reset(struct rpcrdma_xprt *r_xprt) > -{ > -} > - > static void > frwr_op_destroy(struct rpcrdma_buffer *buf) > { > @@ -464,7 +449,6 @@ const struct rpcrdma_memreg_ops rpcrdma_frwr_memreg_ops = { > .ro_open = frwr_op_open, > .ro_maxpages = frwr_op_maxpages, > .ro_init = frwr_op_init, > - .ro_reset = frwr_op_reset, > .ro_destroy = frwr_op_destroy, > .ro_displayname = "frwr", > }; > diff --git a/net/sunrpc/xprtrdma/physical_ops.c b/net/sunrpc/xprtrdma/physical_ops.c > index da149e8..41985d0 100644 > --- a/net/sunrpc/xprtrdma/physical_ops.c > +++ b/net/sunrpc/xprtrdma/physical_ops.c > @@ -69,11 +69,6 @@ physical_op_unmap(struct rpcrdma_xprt *r_xprt, struct rpcrdma_mr_seg *seg) > } > > static void > -physical_op_reset(struct rpcrdma_xprt *r_xprt) > -{ > -} > - > -static void > physical_op_destroy(struct rpcrdma_buffer *buf) > { > } > @@ -84,7 +79,6 @@ const struct rpcrdma_memreg_ops rpcrdma_physical_memreg_ops = { > .ro_open = physical_op_open, > .ro_maxpages = physical_op_maxpages, > .ro_init = physical_op_init, > - .ro_reset = physical_op_reset, > .ro_destroy = physical_op_destroy, > .ro_displayname = "physical", > }; > diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c > index 5120a8e..eaf0b9d 100644 > --- a/net/sunrpc/xprtrdma/verbs.c > +++ b/net/sunrpc/xprtrdma/verbs.c > @@ -897,8 +897,6 @@ retry: > rpcrdma_flush_cqs(ep); > > xprt = container_of(ia, struct rpcrdma_xprt, rx_ia); > - ia->ri_ops->ro_reset(xprt); > - > id = rpcrdma_create_id(xprt, ia, > (struct sockaddr *)&xprt->rx_data.addr); > if (IS_ERR(id)) { > diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h b/net/sunrpc/xprtrdma/xprt_rdma.h > index 98227d6..6a1e565 100644 > --- a/net/sunrpc/xprtrdma/xprt_rdma.h > +++ b/net/sunrpc/xprtrdma/xprt_rdma.h > @@ -353,7 +353,6 @@ struct rpcrdma_memreg_ops { > struct rpcrdma_create_data_internal *); > size_t (*ro_maxpages)(struct rpcrdma_xprt *); > int (*ro_init)(struct rpcrdma_xprt *); > - void (*ro_reset)(struct rpcrdma_xprt *); > void (*ro_destroy)(struct rpcrdma_buffer *); > const char *ro_displayname; > }; > Looks good, Reviewed-by: Sagi Grimberg <sagig@mellanox.com> -- 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
Reviewed-by: Devesh Sharma <devesh.sharma@avagotech.com> On Thu, May 7, 2015 at 4:06 PM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote: > On 5/4/2015 8:58 PM, Chuck Lever wrote: >> >> An RPC can exit at any time. When it does so, xprt_rdma_free() is >> called, and it calls ->op_unmap(). >> >> If ->ro_reset() is running due to a transport disconnect, the two >> methods can race while processing the same rpcrdma_mw. The results >> are unpredictable. >> >> Because of this, in previous patches I've replaced the ->ro_reset() >> methods with a recovery workqueue. ->ro_reset() is no longer used >> and can be removed. >> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> net/sunrpc/xprtrdma/fmr_ops.c | 11 ----------- >> net/sunrpc/xprtrdma/frwr_ops.c | 16 ---------------- >> net/sunrpc/xprtrdma/physical_ops.c | 6 ------ >> net/sunrpc/xprtrdma/verbs.c | 2 -- >> net/sunrpc/xprtrdma/xprt_rdma.h | 1 - >> 5 files changed, 36 deletions(-) >> >> diff --git a/net/sunrpc/xprtrdma/fmr_ops.c b/net/sunrpc/xprtrdma/fmr_ops.c >> index ad0055b..5dd77da 100644 >> --- a/net/sunrpc/xprtrdma/fmr_ops.c >> +++ b/net/sunrpc/xprtrdma/fmr_ops.c >> @@ -197,16 +197,6 @@ out_err: >> return nsegs; >> } >> >> -/* After a disconnect, unmap all FMRs. >> - * >> - * This is invoked only in the transport connect worker in order >> - * to serialize with rpcrdma_register_fmr_external(). >> - */ >> -static void >> -fmr_op_reset(struct rpcrdma_xprt *r_xprt) >> -{ >> -} >> - >> static void >> fmr_op_destroy(struct rpcrdma_buffer *buf) >> { >> @@ -230,7 +220,6 @@ const struct rpcrdma_memreg_ops rpcrdma_fmr_memreg_ops >> = { >> .ro_open = fmr_op_open, >> .ro_maxpages = fmr_op_maxpages, >> .ro_init = fmr_op_init, >> - .ro_reset = fmr_op_reset, >> .ro_destroy = fmr_op_destroy, >> .ro_displayname = "fmr", >> }; >> diff --git a/net/sunrpc/xprtrdma/frwr_ops.c >> b/net/sunrpc/xprtrdma/frwr_ops.c >> index 6f93a89..3fb609a 100644 >> --- a/net/sunrpc/xprtrdma/frwr_ops.c >> +++ b/net/sunrpc/xprtrdma/frwr_ops.c >> @@ -430,21 +430,6 @@ out_err: >> return nsegs; >> } >> >> -/* After a disconnect, a flushed FAST_REG_MR can leave an FRMR in >> - * an unusable state. Find FRMRs in this state and dereg / reg >> - * each. FRMRs that are VALID and attached to an rpcrdma_req are >> - * also torn down. >> - * >> - * This gives all in-use FRMRs a fresh rkey and leaves them INVALID. >> - * >> - * This is invoked only in the transport connect worker in order >> - * to serialize with rpcrdma_register_frmr_external(). >> - */ >> -static void >> -frwr_op_reset(struct rpcrdma_xprt *r_xprt) >> -{ >> -} >> - >> static void >> frwr_op_destroy(struct rpcrdma_buffer *buf) >> { >> @@ -464,7 +449,6 @@ const struct rpcrdma_memreg_ops >> rpcrdma_frwr_memreg_ops = { >> .ro_open = frwr_op_open, >> .ro_maxpages = frwr_op_maxpages, >> .ro_init = frwr_op_init, >> - .ro_reset = frwr_op_reset, >> .ro_destroy = frwr_op_destroy, >> .ro_displayname = "frwr", >> }; >> diff --git a/net/sunrpc/xprtrdma/physical_ops.c >> b/net/sunrpc/xprtrdma/physical_ops.c >> index da149e8..41985d0 100644 >> --- a/net/sunrpc/xprtrdma/physical_ops.c >> +++ b/net/sunrpc/xprtrdma/physical_ops.c >> @@ -69,11 +69,6 @@ physical_op_unmap(struct rpcrdma_xprt *r_xprt, struct >> rpcrdma_mr_seg *seg) >> } >> >> static void >> -physical_op_reset(struct rpcrdma_xprt *r_xprt) >> -{ >> -} >> - >> -static void >> physical_op_destroy(struct rpcrdma_buffer *buf) >> { >> } >> @@ -84,7 +79,6 @@ const struct rpcrdma_memreg_ops >> rpcrdma_physical_memreg_ops = { >> .ro_open = physical_op_open, >> .ro_maxpages = physical_op_maxpages, >> .ro_init = physical_op_init, >> - .ro_reset = physical_op_reset, >> .ro_destroy = physical_op_destroy, >> .ro_displayname = "physical", >> }; >> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c >> index 5120a8e..eaf0b9d 100644 >> --- a/net/sunrpc/xprtrdma/verbs.c >> +++ b/net/sunrpc/xprtrdma/verbs.c >> @@ -897,8 +897,6 @@ retry: >> rpcrdma_flush_cqs(ep); >> >> xprt = container_of(ia, struct rpcrdma_xprt, rx_ia); >> - ia->ri_ops->ro_reset(xprt); >> - >> id = rpcrdma_create_id(xprt, ia, >> (struct sockaddr *)&xprt->rx_data.addr); >> if (IS_ERR(id)) { >> diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h >> b/net/sunrpc/xprtrdma/xprt_rdma.h >> index 98227d6..6a1e565 100644 >> --- a/net/sunrpc/xprtrdma/xprt_rdma.h >> +++ b/net/sunrpc/xprtrdma/xprt_rdma.h >> @@ -353,7 +353,6 @@ struct rpcrdma_memreg_ops { >> struct rpcrdma_create_data_internal *); >> size_t (*ro_maxpages)(struct rpcrdma_xprt *); >> int (*ro_init)(struct rpcrdma_xprt *); >> - void (*ro_reset)(struct rpcrdma_xprt *); >> void (*ro_destroy)(struct rpcrdma_buffer *); >> const char *ro_displayname; >> }; >> > > Looks good, > > Reviewed-by: Sagi Grimberg <sagig@mellanox.com> > > -- > 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/net/sunrpc/xprtrdma/fmr_ops.c b/net/sunrpc/xprtrdma/fmr_ops.c index ad0055b..5dd77da 100644 --- a/net/sunrpc/xprtrdma/fmr_ops.c +++ b/net/sunrpc/xprtrdma/fmr_ops.c @@ -197,16 +197,6 @@ out_err: return nsegs; } -/* After a disconnect, unmap all FMRs. - * - * This is invoked only in the transport connect worker in order - * to serialize with rpcrdma_register_fmr_external(). - */ -static void -fmr_op_reset(struct rpcrdma_xprt *r_xprt) -{ -} - static void fmr_op_destroy(struct rpcrdma_buffer *buf) { @@ -230,7 +220,6 @@ const struct rpcrdma_memreg_ops rpcrdma_fmr_memreg_ops = { .ro_open = fmr_op_open, .ro_maxpages = fmr_op_maxpages, .ro_init = fmr_op_init, - .ro_reset = fmr_op_reset, .ro_destroy = fmr_op_destroy, .ro_displayname = "fmr", }; diff --git a/net/sunrpc/xprtrdma/frwr_ops.c b/net/sunrpc/xprtrdma/frwr_ops.c index 6f93a89..3fb609a 100644 --- a/net/sunrpc/xprtrdma/frwr_ops.c +++ b/net/sunrpc/xprtrdma/frwr_ops.c @@ -430,21 +430,6 @@ out_err: return nsegs; } -/* After a disconnect, a flushed FAST_REG_MR can leave an FRMR in - * an unusable state. Find FRMRs in this state and dereg / reg - * each. FRMRs that are VALID and attached to an rpcrdma_req are - * also torn down. - * - * This gives all in-use FRMRs a fresh rkey and leaves them INVALID. - * - * This is invoked only in the transport connect worker in order - * to serialize with rpcrdma_register_frmr_external(). - */ -static void -frwr_op_reset(struct rpcrdma_xprt *r_xprt) -{ -} - static void frwr_op_destroy(struct rpcrdma_buffer *buf) { @@ -464,7 +449,6 @@ const struct rpcrdma_memreg_ops rpcrdma_frwr_memreg_ops = { .ro_open = frwr_op_open, .ro_maxpages = frwr_op_maxpages, .ro_init = frwr_op_init, - .ro_reset = frwr_op_reset, .ro_destroy = frwr_op_destroy, .ro_displayname = "frwr", }; diff --git a/net/sunrpc/xprtrdma/physical_ops.c b/net/sunrpc/xprtrdma/physical_ops.c index da149e8..41985d0 100644 --- a/net/sunrpc/xprtrdma/physical_ops.c +++ b/net/sunrpc/xprtrdma/physical_ops.c @@ -69,11 +69,6 @@ physical_op_unmap(struct rpcrdma_xprt *r_xprt, struct rpcrdma_mr_seg *seg) } static void -physical_op_reset(struct rpcrdma_xprt *r_xprt) -{ -} - -static void physical_op_destroy(struct rpcrdma_buffer *buf) { } @@ -84,7 +79,6 @@ const struct rpcrdma_memreg_ops rpcrdma_physical_memreg_ops = { .ro_open = physical_op_open, .ro_maxpages = physical_op_maxpages, .ro_init = physical_op_init, - .ro_reset = physical_op_reset, .ro_destroy = physical_op_destroy, .ro_displayname = "physical", }; diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c index 5120a8e..eaf0b9d 100644 --- a/net/sunrpc/xprtrdma/verbs.c +++ b/net/sunrpc/xprtrdma/verbs.c @@ -897,8 +897,6 @@ retry: rpcrdma_flush_cqs(ep); xprt = container_of(ia, struct rpcrdma_xprt, rx_ia); - ia->ri_ops->ro_reset(xprt); - id = rpcrdma_create_id(xprt, ia, (struct sockaddr *)&xprt->rx_data.addr); if (IS_ERR(id)) { diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h b/net/sunrpc/xprtrdma/xprt_rdma.h index 98227d6..6a1e565 100644 --- a/net/sunrpc/xprtrdma/xprt_rdma.h +++ b/net/sunrpc/xprtrdma/xprt_rdma.h @@ -353,7 +353,6 @@ struct rpcrdma_memreg_ops { struct rpcrdma_create_data_internal *); size_t (*ro_maxpages)(struct rpcrdma_xprt *); int (*ro_init)(struct rpcrdma_xprt *); - void (*ro_reset)(struct rpcrdma_xprt *); void (*ro_destroy)(struct rpcrdma_buffer *); const char *ro_displayname; };
An RPC can exit at any time. When it does so, xprt_rdma_free() is called, and it calls ->op_unmap(). If ->ro_reset() is running due to a transport disconnect, the two methods can race while processing the same rpcrdma_mw. The results are unpredictable. Because of this, in previous patches I've replaced the ->ro_reset() methods with a recovery workqueue. ->ro_reset() is no longer used and can be removed. Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- net/sunrpc/xprtrdma/fmr_ops.c | 11 ----------- net/sunrpc/xprtrdma/frwr_ops.c | 16 ---------------- net/sunrpc/xprtrdma/physical_ops.c | 6 ------ net/sunrpc/xprtrdma/verbs.c | 2 -- net/sunrpc/xprtrdma/xprt_rdma.h | 1 - 5 files changed, 36 deletions(-) -- 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