* [PATCH] SUNRPC: Remove redundant call to cancel_work_sync() in xprt_destroy()
@ 2017-10-11 18:01 Trond Myklebust
2017-10-12 13:47 ` Lorenzo Pieralisi
0 siblings, 1 reply; 4+ messages in thread
From: Trond Myklebust @ 2017-10-11 18:01 UTC (permalink / raw)
To: Anna Schumaker; +Cc: Lorenzo Pieralisi, linux-nfs
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);
kfree(xprt->servername);
/*
* Tear down transport state and free the rpc_xprt
--
2.13.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] SUNRPC: Remove redundant call to cancel_work_sync() in xprt_destroy()
2017-10-11 18:01 [PATCH] SUNRPC: Remove redundant call to cancel_work_sync() in xprt_destroy() Trond Myklebust
@ 2017-10-12 13:47 ` Lorenzo Pieralisi
2017-10-19 18:49 ` Trond Myklebust
0 siblings, 1 reply; 4+ messages in thread
From: Lorenzo Pieralisi @ 2017-10-12 13:47 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Anna Schumaker, linux-nfs
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
Log:
[ 6.223423] ======================================================
[ 6.229611] WARNING: possible circular locking dependency detected
[ 6.235801] 4.14.0-rc4-00001-g8ee3d7b #64 Not tainted
[ 6.240856] ------------------------------------------------------
[ 6.247044] kworker/1:0H/17 is trying to acquire lock:
[ 6.252188] ((&task->u.tk_work)){+.+.}, at: [<ffff0000080e64cc>] process_one_work+0x1cc/0x3f0
[ 6.260836]
but task is already holding lock:
[ 6.266676] ("xprtiod"){+.+.}, at: [<ffff0000080e64cc>] process_one_work+0x1cc/0x3f0
[ 6.274531]
which lock already depends on the new lock.
[ 6.282723]
the existing dependency chain (in reverse order) is:
[ 6.290217]
-> #1 ("xprtiod"){+.+.}:
[ 6.295292] lock_acquire+0x6c/0xb8
[ 6.299307] flush_work+0x188/0x270
[ 6.303321] __cancel_work_timer+0x120/0x198
[ 6.308119] cancel_work_sync+0x10/0x18
[ 6.312484] xs_destroy+0x34/0x58
[ 6.316324] xprt_destroy+0x7c/0x88
[ 6.320338] xprt_put+0x34/0x40
[ 6.324004] rpc_task_release_client+0x6c/0x80
[ 6.328976] rpc_release_resources_task+0x2c/0x38
[ 6.334210] __rpc_execute+0x9c/0x210
[ 6.338399] rpc_async_schedule+0x10/0x18
[ 6.342935] process_one_work+0x240/0x3f0
[ 6.347471] worker_thread+0x48/0x420
[ 6.351661] kthread+0x12c/0x158
[ 6.355416] ret_from_fork+0x10/0x18
[ 6.359514]
-> #0 ((&task->u.tk_work)){+.+.}:
[ 6.365372] __lock_acquire+0x12ec/0x14a8
[ 6.369908] lock_acquire+0x6c/0xb8
[ 6.373922] process_one_work+0x22c/0x3f0
[ 6.378459] worker_thread+0x48/0x420
[ 6.382648] kthread+0x12c/0x158
[ 6.386401] ret_from_fork+0x10/0x18
[ 6.390499]
other info that might help us debug this:
[ 6.398518] Possible unsafe locking scenario:
[ 6.404445] CPU0 CPU1
[ 6.408978] ---- ----
[ 6.413511] lock("xprtiod");
[ 6.416571] lock((&task->u.tk_work));
[ 6.422938] lock("xprtiod");
[ 6.428521] lock((&task->u.tk_work));
[ 6.432364]
*** DEADLOCK ***
[ 6.438295] 1 lock held by kworker/1:0H/17:
[ 6.442480] #0: ("xprtiod"){+.+.}, at: [<ffff0000080e64cc>] process_one_work+0x1cc/0x3f0
[ 6.450773]
stack backtrace:
[ 6.455138] CPU: 1 PID: 17 Comm: kworker/1:0H Not tainted 4.14.0-rc4-00001-g8ee3d7b #64
[ 6.463153] Hardware name: ARM Juno development board (r2) (DT)
[ 6.469084] Workqueue: xprtiod rpc_async_schedule
[ 6.473795] Call trace:
[ 6.476246] [<ffff000008089430>] dump_backtrace+0x0/0x3c8
[ 6.481654] [<ffff00000808980c>] show_stack+0x14/0x20
[ 6.486715] [<ffff000008a01a30>] dump_stack+0xb8/0xf0
[ 6.491776] [<ffff0000081194ac>] print_circular_bug+0x224/0x3a0
[ 6.497706] [<ffff00000811a304>] check_prev_add+0x304/0x860
[ 6.503288] [<ffff00000811c8c4>] __lock_acquire+0x12ec/0x14a8
[ 6.509043] [<ffff00000811d144>] lock_acquire+0x6c/0xb8
[ 6.514276] [<ffff0000080e652c>] process_one_work+0x22c/0x3f0
[ 6.520031] [<ffff0000080e6738>] worker_thread+0x48/0x420
[ 6.525439] [<ffff0000080ed5bc>] kthread+0x12c/0x158
[ 6.530411] [<ffff000008084d48>] ret_from_fork+0x10/0x18
[ 7.446907] systemd[1]: systemd 232 running in system mode. (+PAM +AUDIT +SELINUX +IMA +APPARMOR +SMACK +SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ +LZ4 +SECCOMP +BLKID +ELFUTILS +KMOD +IDN)
[ 7.465397] systemd[1]: Detected architecture arm64.
[ 7.487124] systemd[1]: Set hostname to <localhost.localdomain>.
[ 8.016682] systemd[1]: Listening on Journal Socket.
[ 8.034382] systemd[1]: Listening on udev Control Socket.
[ 8.053967] systemd[1]: Listening on Syslog Socket.
[ 8.069953] systemd[1]: Listening on udev Kernel Socket.
[ 8.089774] systemd[1]: Reached target Remote File Systems.
[ 8.110534] systemd[1]: Created slice User and Session Slice.
[ 8.130143] systemd[1]: Listening on Journal Audit Socket.
[ 8.781699] systemd-journald[1479]: Received request to flush runtime journal from PID 1
[ 9.685766] sky2 0000:09:00.0 enp9s0: renamed from eth4
[ 9.778209] igb 0000:07:00.2 enp7s0f2: renamed from eth2
[ 9.802564] igb 0000:07:00.1 enp7s0f1: renamed from eth1
[ 9.822138] igb 0000:07:00.3 enp7s0f3: renamed from eth3
[ 9.838393] igb 0000:07:00.0 enp7s0f0: renamed from eth0
[ 54.271348] random: crng init done
know this code path at all so I can't really comment on it, let me
know if I c
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] SUNRPC: Remove redundant call to cancel_work_sync() in xprt_destroy()
2017-10-12 13:47 ` Lorenzo Pieralisi
@ 2017-10-19 18:49 ` Trond Myklebust
2017-10-20 15:06 ` Lorenzo Pieralisi
0 siblings, 1 reply; 4+ messages in thread
From: Trond Myklebust @ 2017-10-19 18:49 UTC (permalink / raw)
To: lorenzo.pieralisi; +Cc: anna.schumaker, linux-nfs
T24gVGh1LCAyMDE3LTEwLTEyIGF0IDE0OjQ3ICswMTAwLCBMb3JlbnpvIFBpZXJhbGlzaSB3cm90
ZToNCj4gSGkgVHJvbmQsDQo+IA0KPiBPbiBXZWQsIE9jdCAxMSwgMjAxNyBhdCAwMjowMTozNFBN
IC0wNDAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6DQo+ID4gV2Uga25vdyB0aGF0IHRoZSBzb2Nr
ZXQgYXV0b2Nsb3NlIGNhbm5vdCBiZSBxdWV1ZWQgYWZ0ZXIgd2UndmUgc2V0DQo+ID4gdGhlIFhQ
UlRfTE9DS0VEIGJpdCwgc28gdGhlIGNhbGwgdG8gY2FuY2VsX3dvcmtfc3luYygpIGlzDQo+ID4g
cmVkdW5kYW50Lg0KPiA+IEluIGFkZGl0aW9uLCBpdCBpcyBjYXVzaW5nIGxvY2tkZXAgdG8gY29t
cGxhaW4gYWJvdXQgYSBmYWxzZSBBQkENCj4gPiBsb2NrIGRlcGVuZGVuY3kuDQo+ID4gDQo+ID4g
U2lnbmVkLW9mZi1ieTogVHJvbmQgTXlrbGVidXN0IDx0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRh
dGEuY29tPg0KPiA+IC0tLQ0KPiA+ICBuZXQvc3VucnBjL3hwcnQuYyB8IDEgLQ0KPiA+ICAxIGZp
bGUgY2hhbmdlZCwgMSBkZWxldGlvbigtKQ0KPiA+IA0KPiA+IGRpZmYgLS1naXQgYS9uZXQvc3Vu
cnBjL3hwcnQuYyBiL25ldC9zdW5ycGMveHBydC5jDQo+ID4gaW5kZXggZTc0MWVjMmI0ZDhlLi41
ZjEyZmUxNDVmMDIgMTAwNjQ0DQo+ID4gLS0tIGEvbmV0L3N1bnJwYy94cHJ0LmMNCj4gPiArKysg
Yi9uZXQvc3VucnBjL3hwcnQuYw0KPiA+IEBAIC0xNDY0LDcgKzE0NjQsNiBAQCBzdGF0aWMgdm9p
ZCB4cHJ0X2Rlc3Ryb3koc3RydWN0IHJwY194cHJ0DQo+ID4gKnhwcnQpDQo+ID4gIAlycGNfZGVz
dHJveV93YWl0X3F1ZXVlKCZ4cHJ0LT5wZW5kaW5nKTsNCj4gPiAgCXJwY19kZXN0cm95X3dhaXRf
cXVldWUoJnhwcnQtPnNlbmRpbmcpOw0KPiA+ICAJcnBjX2Rlc3Ryb3lfd2FpdF9xdWV1ZSgmeHBy
dC0+YmFja2xvZyk7DQo+ID4gLQljYW5jZWxfd29ya19zeW5jKCZ4cHJ0LT50YXNrX2NsZWFudXAp
Ow0KPiANCj4gVGhpcyBkb2VzIG5vdCBtYWtlIHRoZSBsb2NrZGVwIHdhcm5pbmcgZ28gYXdheSwg
YWN0dWFsbHkgdGhlIGxvY2tkZXANCj4gaXMgdHJpZ2dlcmVkIGJ5IHRoZSB4c19kZXN0cm95KCkg
Y2FuY2VsX3dvcmtfc3luYygpIGNhbGwgYnV0IEkgZG8gbm90DQo+IGtub3cgdGhpcyBjb2RlIHBh
dGggc28gSSBjYW4ndCByZWFsbHkgY29tbWVudCBvbiBpdCwgbGV0IG1lIGtub3cgaWYNCj4gdGhl
cmUgaXMgYW55IHNwZWNpZmljIHRlc3QgSSBjYW4gY2Fycnkgb3V0Lg0KPiANCj4gVGhhbmtzIGZv
ciBsb29raW5nIGludG8gdGhpcywNCj4gTG9yZW56bw0KDQpTb3JyeSBmb3IgdGhlIGRlbGF5LiBE
b2VzIHRoaXMgb25lIGhlbHA/DQoNCjg8LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LQ0KRnJvbSA1MjhmZDM1NDdiYWQwYmRkMzFjOGY5ODdlNWJkMDBjODNkZjhhZjM5IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQ0KRnJvbTogVHJvbmQgTXlrbGVidXN0IDx0cm9uZC5teWtsZWJ1c3RA
cHJpbWFyeWRhdGEuY29tPg0KRGF0ZTogVGh1LCAxOSBPY3QgMjAxNyAxMjoxMzoxMCAtMDQwMA0K
U3ViamVjdDogW1BBVENIXSBTVU5SUEM6IERlc3Ryb3kgdHJhbnNwb3J0IGZyb20gdGhlIHN5c3Rl
bSB3b3JrcXVldWUNCg0KVGhlIHRyYW5zcG9ydCBtYXkgbmVlZCB0byBmbHVzaCB0cmFuc3BvcnQg
Y29ubmVjdCBhbmQgcmVjZWl2ZSB0YXNrcw0KdGhhdCBhcmUgcnVubmluZyBvbiBycGNpb2QuIElu
IG9yZGVyIHRvIGRvIHNvIHNhZmVseSwgd2UgbmVlZCB0bw0KZW5zdXJlIHRoYXQgdGhlIGNhbGxl
ciBvZiBjYW5jZWxfd29ya19zeW5jKCkgZXRjIGlzIG5vdCBpdHNlbGYNCnJ1bm5pbmcgb24gcnBj
aW9kLg0KRG8gc28gYnkgcnVubmluZyB0aGUgZGVzdHJveSB0YXNrIGZyb20gdGhlIHN5c3RlbSB3
b3JrcXVldWUuDQoNClNpZ25lZC1vZmYtYnk6IFRyb25kIE15a2xlYnVzdCA8dHJvbmQubXlrbGVi
dXN0QHByaW1hcnlkYXRhLmNvbT4NCi0tLQ0KIG5ldC9zdW5ycGMveHBydC5jIHwgMzQgKysrKysr
KysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLQ0KIDEgZmlsZSBjaGFuZ2VkLCAyNCBpbnNlcnRp
b25zKCspLCAxMCBkZWxldGlvbnMoLSkNCg0KZGlmZiAtLWdpdCBhL25ldC9zdW5ycGMveHBydC5j
IGIvbmV0L3N1bnJwYy94cHJ0LmMNCmluZGV4IDFhMzlhZDE0YzQyZi4uODk4NDg1ZTNlY2U0IDEw
MDY0NA0KLS0tIGEvbmV0L3N1bnJwYy94cHJ0LmMNCisrKyBiL25ldC9zdW5ycGMveHBydC5jDQpA
QCAtMTQ0NSw2ICsxNDQ1LDIzIEBAIHN0cnVjdCBycGNfeHBydCAqeHBydF9jcmVhdGVfdHJhbnNw
b3J0KHN0cnVjdCB4cHJ0X2NyZWF0ZSAqYXJncykNCiAJcmV0dXJuIHhwcnQ7DQogfQ0KIA0KK3N0
YXRpYyB2b2lkIHhwcnRfZGVzdHJveV9jYihzdHJ1Y3Qgd29ya19zdHJ1Y3QgKndvcmspDQorew0K
KwlzdHJ1Y3QgcnBjX3hwcnQgKnhwcnQgPQ0KKwkJY29udGFpbmVyX29mKHdvcmssIHN0cnVjdCBy
cGNfeHBydCwgdGFza19jbGVhbnVwKTsNCisNCisJcnBjX3hwcnRfZGVidWdmc191bnJlZ2lzdGVy
KHhwcnQpOw0KKwlycGNfZGVzdHJveV93YWl0X3F1ZXVlKCZ4cHJ0LT5iaW5kaW5nKTsNCisJcnBj
X2Rlc3Ryb3lfd2FpdF9xdWV1ZSgmeHBydC0+cGVuZGluZyk7DQorCXJwY19kZXN0cm95X3dhaXRf
cXVldWUoJnhwcnQtPnNlbmRpbmcpOw0KKwlycGNfZGVzdHJveV93YWl0X3F1ZXVlKCZ4cHJ0LT5i
YWNrbG9nKTsNCisJa2ZyZWUoeHBydC0+c2VydmVybmFtZSk7DQorCS8qDQorCSAqIFRlYXIgZG93
biB0cmFuc3BvcnQgc3RhdGUgYW5kIGZyZWUgdGhlIHJwY194cHJ0DQorCSAqLw0KKwl4cHJ0LT5v
cHMtPmRlc3Ryb3koeHBydCk7DQorfQ0KKw0KIC8qKg0KICAqIHhwcnRfZGVzdHJveSAtIGRlc3Ry
b3kgYW4gUlBDIHRyYW5zcG9ydCwga2lsbGluZyBvZmYgYWxsIHJlcXVlc3RzLg0KICAqIEB4cHJ0
OiB0cmFuc3BvcnQgdG8gZGVzdHJveQ0KQEAgLTE0NTQsMjIgKzE0NzEsMTkgQEAgc3RhdGljIHZv
aWQgeHBydF9kZXN0cm95KHN0cnVjdCBycGNfeHBydCAqeHBydCkNCiB7DQogCWRwcmludGsoIlJQ
QzogICAgICAgZGVzdHJveWluZyB0cmFuc3BvcnQgJXBcbiIsIHhwcnQpOw0KIA0KLQkvKiBFeGNs
dWRlIHRyYW5zcG9ydCBjb25uZWN0L2Rpc2Nvbm5lY3QgaGFuZGxlcnMgKi8NCisJLyoNCisJICog
RXhjbHVkZSB0cmFuc3BvcnQgY29ubmVjdC9kaXNjb25uZWN0IGhhbmRsZXJzIGFuZCBhdXRvY2xv
c2UNCisJICovDQogCXdhaXRfb25fYml0X2xvY2soJnhwcnQtPnN0YXRlLCBYUFJUX0xPQ0tFRCwg
VEFTS19VTklOVEVSUlVQVElCTEUpOw0KIA0KIAlkZWxfdGltZXJfc3luYygmeHBydC0+dGltZXIp
Ow0KIA0KLQlycGNfeHBydF9kZWJ1Z2ZzX3VucmVnaXN0ZXIoeHBydCk7DQotCXJwY19kZXN0cm95
X3dhaXRfcXVldWUoJnhwcnQtPmJpbmRpbmcpOw0KLQlycGNfZGVzdHJveV93YWl0X3F1ZXVlKCZ4
cHJ0LT5wZW5kaW5nKTsNCi0JcnBjX2Rlc3Ryb3lfd2FpdF9xdWV1ZSgmeHBydC0+c2VuZGluZyk7
DQotCXJwY19kZXN0cm95X3dhaXRfcXVldWUoJnhwcnQtPmJhY2tsb2cpOw0KLQljYW5jZWxfd29y
a19zeW5jKCZ4cHJ0LT50YXNrX2NsZWFudXApOw0KLQlrZnJlZSh4cHJ0LT5zZXJ2ZXJuYW1lKTsN
CiAJLyoNCi0JICogVGVhciBkb3duIHRyYW5zcG9ydCBzdGF0ZSBhbmQgZnJlZSB0aGUgcnBjX3hw
cnQNCisJICogRGVzdHJveSBzb2NrZXRzIGV0YyBmcm9tIHRoZSBzeXN0ZW0gd29ya3F1ZXVlIHNv
IHRoZXkgY2FuDQorCSAqIHNhZmVseSBmbHVzaCByZWNlaXZlIHdvcmsgcnVubmluZyBvbiBycGNp
b2QuDQogCSAqLw0KLQl4cHJ0LT5vcHMtPmRlc3Ryb3koeHBydCk7DQorCUlOSVRfV09SSygmeHBy
dC0+dGFza19jbGVhbnVwLCB4cHJ0X2Rlc3Ryb3lfY2IpOw0KKwlzY2hlZHVsZV93b3JrKCZ4cHJ0
LT50YXNrX2NsZWFudXApOw0KIH0NCiANCiBzdGF0aWMgdm9pZCB4cHJ0X2Rlc3Ryb3lfa3JlZihz
dHJ1Y3Qga3JlZiAqa3JlZikNCi0tIA0KMi4xMy42DQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpM
aW51eCBORlMgY2xpZW50IG1haW50YWluZXIsIFByaW1hcnlEYXRhDQp0cm9uZC5teWtsZWJ1c3RA
cHJpbWFyeWRhdGEuY29tDQo=
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] SUNRPC: Remove redundant call to cancel_work_sync() in xprt_destroy()
2017-10-19 18:49 ` Trond Myklebust
@ 2017-10-20 15:06 ` Lorenzo Pieralisi
0 siblings, 0 replies; 4+ messages in thread
From: Lorenzo Pieralisi @ 2017-10-20 15:06 UTC (permalink / raw)
To: Trond Myklebust; +Cc: anna.schumaker, linux-nfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-20 15:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-11 18:01 [PATCH] SUNRPC: Remove redundant call to cancel_work_sync() in xprt_destroy() Trond Myklebust
2017-10-12 13:47 ` Lorenzo Pieralisi
2017-10-19 18:49 ` Trond Myklebust
2017-10-20 15:06 ` Lorenzo Pieralisi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.