| Message ID | 1508438956.72950.1.camel@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 19, 2017 at 06:49:18PM +0000, Trond Myklebust wrote: > On Thu, 2017-10-12 at 14:47 +0100, Lorenzo Pieralisi wrote: > > Hi Trond, > > > > On Wed, Oct 11, 2017 at 02:01:34PM -0400, Trond Myklebust wrote: > > > We know that the socket autoclose cannot be queued after we've set > > > the XPRT_LOCKED bit, so the call to cancel_work_sync() is > > > redundant. > > > In addition, it is causing lockdep to complain about a false ABA > > > lock dependency. > > > > > > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> > > > --- > > > net/sunrpc/xprt.c | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c > > > index e741ec2b4d8e..5f12fe145f02 100644 > > > --- a/net/sunrpc/xprt.c > > > +++ b/net/sunrpc/xprt.c > > > @@ -1464,7 +1464,6 @@ static void xprt_destroy(struct rpc_xprt > > > *xprt) > > > rpc_destroy_wait_queue(&xprt->pending); > > > rpc_destroy_wait_queue(&xprt->sending); > > > rpc_destroy_wait_queue(&xprt->backlog); > > > - cancel_work_sync(&xprt->task_cleanup); > > > > This does not make the lockdep warning go away, actually the lockdep > > is triggered by the xs_destroy() cancel_work_sync() call but I do not > > know this code path so I can't really comment on it, let me know if > > there is any specific test I can carry out. > > > > Thanks for looking into this, > > Lorenzo > > Sorry for the delay. Does this one help? Thank you. Yes it does (on top of -rc5) - I get no lockdep warning anymore. Lorenzo > 8<---------------------------------- > From 528fd3547bad0bdd31c8f987e5bd00c83df8af39 Mon Sep 17 00:00:00 2001 > From: Trond Myklebust <trond.myklebust@primarydata.com> > Date: Thu, 19 Oct 2017 12:13:10 -0400 > Subject: [PATCH] SUNRPC: Destroy transport from the system workqueue > > The transport may need to flush transport connect and receive tasks > that are running on rpciod. In order to do so safely, we need to > ensure that the caller of cancel_work_sync() etc is not itself > running on rpciod. > Do so by running the destroy task from the system workqueue. > > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> > --- > net/sunrpc/xprt.c | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c > index 1a39ad14c42f..898485e3ece4 100644 > --- a/net/sunrpc/xprt.c > +++ b/net/sunrpc/xprt.c > @@ -1445,6 +1445,23 @@ struct rpc_xprt *xprt_create_transport(struct xprt_create *args) > return xprt; > } > > +static void xprt_destroy_cb(struct work_struct *work) > +{ > + struct rpc_xprt *xprt = > + container_of(work, struct rpc_xprt, task_cleanup); > + > + rpc_xprt_debugfs_unregister(xprt); > + rpc_destroy_wait_queue(&xprt->binding); > + rpc_destroy_wait_queue(&xprt->pending); > + rpc_destroy_wait_queue(&xprt->sending); > + rpc_destroy_wait_queue(&xprt->backlog); > + kfree(xprt->servername); > + /* > + * Tear down transport state and free the rpc_xprt > + */ > + xprt->ops->destroy(xprt); > +} > + > /** > * xprt_destroy - destroy an RPC transport, killing off all requests. > * @xprt: transport to destroy > @@ -1454,22 +1471,19 @@ static void xprt_destroy(struct rpc_xprt *xprt) > { > dprintk("RPC: destroying transport %p\n", xprt); > > - /* Exclude transport connect/disconnect handlers */ > + /* > + * Exclude transport connect/disconnect handlers and autoclose > + */ > wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_UNINTERRUPTIBLE); > > del_timer_sync(&xprt->timer); > > - rpc_xprt_debugfs_unregister(xprt); > - rpc_destroy_wait_queue(&xprt->binding); > - rpc_destroy_wait_queue(&xprt->pending); > - rpc_destroy_wait_queue(&xprt->sending); > - rpc_destroy_wait_queue(&xprt->backlog); > - cancel_work_sync(&xprt->task_cleanup); > - kfree(xprt->servername); > /* > - * Tear down transport state and free the rpc_xprt > + * Destroy sockets etc from the system workqueue so they can > + * safely flush receive work running on rpciod. > */ > - xprt->ops->destroy(xprt); > + INIT_WORK(&xprt->task_cleanup, xprt_destroy_cb); > + schedule_work(&xprt->task_cleanup); > } > > static void xprt_destroy_kref(struct kref *kref) > -- > 2.13.6 > > -- > Trond Myklebust > Linux NFS client maintainer, PrimaryData > trond.myklebust@primarydata.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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/xprt.c b/net/sunrpc/xprt.c index 1a39ad14c42f..898485e3ece4 100644 --- a/net/sunrpc/xprt.c +++ b/net/sunrpc/xprt.c @@ -1445,6 +1445,23 @@ struct rpc_xprt *xprt_create_transport(struct xprt_create *args) return xprt; } +static void xprt_destroy_cb(struct work_struct *work) +{ + struct rpc_xprt *xprt = + container_of(work, struct rpc_xprt, task_cleanup); + + rpc_xprt_debugfs_unregister(xprt); + rpc_destroy_wait_queue(&xprt->binding); + rpc_destroy_wait_queue(&xprt->pending); + rpc_destroy_wait_queue(&xprt->sending); + rpc_destroy_wait_queue(&xprt->backlog); + kfree(xprt->servername); + /* + * Tear down transport state and free the rpc_xprt + */ + xprt->ops->destroy(xprt); +} + /** * xprt_destroy - destroy an RPC transport, killing off all requests. * @xprt: transport to destroy @@ -1454,22 +1471,19 @@ static void xprt_destroy(struct rpc_xprt *xprt) { dprintk("RPC: destroying transport %p\n", xprt); - /* Exclude transport connect/disconnect handlers */ + /* + * Exclude transport connect/disconnect handlers and autoclose + */ wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_UNINTERRUPTIBLE); del_timer_sync(&xprt->timer); - rpc_xprt_debugfs_unregister(xprt); - rpc_destroy_wait_queue(&xprt->binding); - rpc_destroy_wait_queue(&xprt->pending); - rpc_destroy_wait_queue(&xprt->sending); - rpc_destroy_wait_queue(&xprt->backlog); - cancel_work_sync(&xprt->task_cleanup); - kfree(xprt->servername); /* - * Tear down transport state and free the rpc_xprt + * Destroy sockets etc from the system workqueue so they can + * safely flush receive work running on rpciod. */ - xprt->ops->destroy(xprt); + INIT_WORK(&xprt->task_cleanup, xprt_destroy_cb); + schedule_work(&xprt->task_cleanup); } static void xprt_destroy_kref(struct kref *kref)