| Message ID | 1438038086-20522-1-git-send-email-devesh.sharma@avagotech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 7/28/2015 2:01 AM, Devesh Sharma wrote: > Thanks Chuck Lever for the valuable feedback and suggestions. > > This is a rework of the following patch sent almost a year back: > http://www.mail-archive.com/linux-rdma%40vger.kernel.org/msg20730.html > > In presence of active mount if someone tries to rmmod vendor-driver, the > command remains stuck forever waiting for destruction of all rdma-cm-id. > in worst case client can crash during shutdown with active mounts. Ouch, taking a reference on the module preventing it from unloading is not very well behaved (putting it nicely). That's also breaking the layering of ULPs <-> core <-> provider scheme. Why not just cleanup everything upon DEVICE_REMOVAL? > > The existing code assumes that ia->ri_id->device cannot change during > the lifetime of a transport. Lifting that assumption is a long chain > of work, and is in plan. > > The community decided that preventing the hang right now is more > important than waiting for architectural changes. Well, if you are putting a bandage here - the code should be documented with a proper FIXME. -- 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 Jul 28, 2015, at 4:46 AM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote: > On 7/28/2015 2:01 AM, Devesh Sharma wrote: >> Thanks Chuck Lever for the valuable feedback and suggestions. >> >> This is a rework of the following patch sent almost a year back: >> http://www.mail-archive.com/linux-rdma%40vger.kernel.org/msg20730.html >> >> In presence of active mount if someone tries to rmmod vendor-driver, the >> command remains stuck forever waiting for destruction of all rdma-cm-id. >> in worst case client can crash during shutdown with active mounts. > > Ouch, taking a reference on the module preventing it from unloading is > not very well behaved (putting it nicely). That's also breaking the > layering of ULPs <-> core <-> provider scheme. > > Why not just cleanup everything upon DEVICE_REMOVAL? xprtrdma does not support DEVICE_REMOVAL yet. That's why we are taking this temporary approach. >> The existing code assumes that ia->ri_id->device cannot change during >> the lifetime of a transport. Lifting that assumption is a long chain >> of work, and is in plan. >> >> The community decided that preventing the hang right now is more >> important than waiting for architectural changes. > > Well, if you are putting a bandage here - the code should be documented > with a proper FIXME. That's a good suggestion. -- Chuck Lever -- 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, Jul 28, 2015 at 7:43 PM, Chuck Lever <chuck.lever@oracle.com> wrote: > > On Jul 28, 2015, at 4:46 AM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote: > >> On 7/28/2015 2:01 AM, Devesh Sharma wrote: >>> Thanks Chuck Lever for the valuable feedback and suggestions. >>> >>> This is a rework of the following patch sent almost a year back: >>> http://www.mail-archive.com/linux-rdma%40vger.kernel.org/msg20730.html >>> >>> In presence of active mount if someone tries to rmmod vendor-driver, the >>> command remains stuck forever waiting for destruction of all rdma-cm-id. >>> in worst case client can crash during shutdown with active mounts. >> >> Ouch, taking a reference on the module preventing it from unloading is >> not very well behaved (putting it nicely). That's also breaking the >> layering of ULPs <-> core <-> provider scheme. >> >> Why not just cleanup everything upon DEVICE_REMOVAL? > > xprtrdma does not support DEVICE_REMOVAL yet. That's why we are > taking this temporary approach. > > >>> The existing code assumes that ia->ri_id->device cannot change during >>> the lifetime of a transport. Lifting that assumption is a long chain >>> of work, and is in plan. >>> >>> The community decided that preventing the hang right now is more >>> important than waiting for architectural changes. >> >> Well, if you are putting a bandage here - the code should be documented >> with a proper FIXME. > > That's a good suggestion. I will put this appropriatly in my next post. > > > -- > Chuck Lever > > > -- 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/verbs.c b/net/sunrpc/xprtrdma/verbs.c index 891c4ed..d16f599 100644 --- a/net/sunrpc/xprtrdma/verbs.c +++ b/net/sunrpc/xprtrdma/verbs.c @@ -52,6 +52,7 @@ #include <linux/prefetch.h> #include <linux/sunrpc/addr.h> #include <asm/bitops.h> +#include <linux/module.h> /* try_module_get()/module_put() */ #include "xprt_rdma.h" @@ -414,6 +415,14 @@ connected: return 0; } +static void rpcrdma_destroy_id(struct rdma_cm_id *id) +{ + if (id) { + module_put(id->device->owner); + rdma_destroy_id(id); + } +} + static struct rdma_cm_id * rpcrdma_create_id(struct rpcrdma_xprt *xprt, struct rpcrdma_ia *ia, struct sockaddr *addr) @@ -440,6 +449,11 @@ rpcrdma_create_id(struct rpcrdma_xprt *xprt, } wait_for_completion_interruptible_timeout(&ia->ri_done, msecs_to_jiffies(RDMA_RESOLVE_TIMEOUT) + 1); + if (!ia->ri_async_rc && !try_module_get(id->device->owner)) { + dprintk("RPC: %s: Failed to get device module\n", + __func__); + ia->ri_async_rc = -ENODEV; + } rc = ia->ri_async_rc; if (rc) goto out; @@ -449,16 +463,17 @@ rpcrdma_create_id(struct rpcrdma_xprt *xprt, if (rc) { dprintk("RPC: %s: rdma_resolve_route() failed %i\n", __func__, rc); - goto out; + goto put; } wait_for_completion_interruptible_timeout(&ia->ri_done, msecs_to_jiffies(RDMA_RESOLVE_TIMEOUT) + 1); rc = ia->ri_async_rc; if (rc) - goto out; + goto put; return id; - +put: + module_put(id->device->owner); out: rdma_destroy_id(id); return ERR_PTR(rc); @@ -592,7 +607,7 @@ out3: ib_dealloc_pd(ia->ri_pd); ia->ri_pd = NULL; out2: - rdma_destroy_id(ia->ri_id); + rpcrdma_destroy_id(ia->ri_id); ia->ri_id = NULL; out1: return rc; @@ -618,7 +633,7 @@ rpcrdma_ia_close(struct rpcrdma_ia *ia) if (ia->ri_id != NULL && !IS_ERR(ia->ri_id)) { if (ia->ri_id->qp) rdma_destroy_qp(ia->ri_id); - rdma_destroy_id(ia->ri_id); + rpcrdma_destroy_id(ia->ri_id); ia->ri_id = NULL; } @@ -825,7 +840,7 @@ retry: if (ia->ri_device != id->device) { printk("RPC: %s: can't reconnect on " "different device!\n", __func__); - rdma_destroy_id(id); + rpcrdma_destroy_id(id); rc = -ENETUNREACH; goto out; } @@ -834,7 +849,7 @@ retry: if (rc) { dprintk("RPC: %s: rdma_create_qp failed %i\n", __func__, rc); - rdma_destroy_id(id); + rpcrdma_destroy_id(id); rc = -ENETUNREACH; goto out; } @@ -845,7 +860,7 @@ retry: write_unlock(&ia->ri_qplock); rdma_destroy_qp(old); - rdma_destroy_id(old); + rpcrdma_destroy_id(old); } else { dprintk("RPC: %s: connecting...\n", __func__); rc = rdma_create_qp(ia->ri_id, ia->ri_pd, &ep->rep_attr);
Thanks Chuck Lever for the valuable feedback and suggestions. This is a rework of the following patch sent almost a year back: http://www.mail-archive.com/linux-rdma%40vger.kernel.org/msg20730.html In presence of active mount if someone tries to rmmod vendor-driver, the command remains stuck forever waiting for destruction of all rdma-cm-id. in worst case client can crash during shutdown with active mounts. The existing code assumes that ia->ri_id->device cannot change during the lifetime of a transport. Lifting that assumption is a long chain of work, and is in plan. The community decided that preventing the hang right now is more important than waiting for architectural changes. Signed-off-by: Devesh Sharma <devesh.sharma@avagotech.com> --- net/sunrpc/xprtrdma/verbs.c | 31 +++++++++++++++++++++++-------- 1 files changed, 23 insertions(+), 8 deletions(-)