Return-Path: Received: from mail-la0-f53.google.com ([209.85.215.53]:35022 "EHLO mail-la0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752874AbbEHPdZ (ORCPT ); Fri, 8 May 2015 11:33:25 -0400 Received: by labbd9 with SMTP id bd9so55286785lab.2 for ; Fri, 08 May 2015 08:33:23 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <554B402F.3000604@dev.mellanox.co.il> References: <20150504174626.3483.97639.stgit@manet.1015granger.net> <20150504175818.3483.22408.stgit@manet.1015granger.net> <554B402F.3000604@dev.mellanox.co.il> Date: Fri, 8 May 2015 21:03:23 +0530 Message-ID: Subject: Re: [PATCH v1 10/14] xprtrdma: Remove ->ro_reset From: Devesh Sharma To: Sagi Grimberg Cc: Chuck Lever , linux-rdma@vger.kernel.org, Linux NFS Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-nfs-owner@vger.kernel.org List-ID: Reviewed-by: Devesh Sharma On Thu, May 7, 2015 at 4:06 PM, Sagi Grimberg 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 >> --- >> 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 > > -- > 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 -- -Regards Devesh