Skip to contentSkip to conversation

NFS deadlock between 'sync' and commit after unmount....

DISCUSSION NeilBrown 3 participants 9 messages Apr 7, 2014 – Apr 10, 2014

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

NeilBrown

author
#0 lore raw

Hi,
I've just hit a deadlock in NFS that seems very strange.
The kernel is 3.14-rc8 which some local changes which shouldn't affect the deadlocking code.

Shortly after umounting the NFS filesystem with "umount -f" (though I don't think the -f is important), I ran "sync".

The sync is now stuck in

[<ffffffff81197fc1>] sync_inodes_sb+0xa1/0x1c0
[<ffffffff8119cd99>] sync_inodes_one_sb+0x19/0x20
[<ffffffff81173372>] iterate_supers+0xb2/0x110
[<ffffffff8119cfd0>] sys_sync+0x30/0x90
[<ffffffff81aa4622>] system_call_fastpath+0x16/0x1b

[<ffffffffffffffff>] 0xffffffffffffffff

while kworker/u16:1 is stuck:

[<ffffffff815420b3>] call_rwsem_down_write_failed+0x13/0x20
[<ffffffff81172889>] deactivate_super+0x39/0x60
[<ffffffff812d56f1>] nfs_sb_deactive+0x21/0x30
[<ffffffff812d2ef9>] __put_nfs_open_context+0xc9/0x100
[<ffffffff812d2f3b>] put_nfs_open_context+0xb/0x10
[<ffffffff812ddd14>] nfs_commitdata_release+0x14/0x30
[<ffffffff812ddd4a>] nfs_commit_release+0x1a/0x20
[<ffffffff81a45a05>] rpc_free_task+0x25/0x70
[<ffffffff81a45fd8>] rpc_do_put_task+0x78/0x80
[<ffffffff81a45feb>] rpc_put_task+0xb/0x10
[<ffffffff812de3fe>] nfs_initiate_commit+0xce/0x110
[<ffffffff812df112>] nfs_commit_list+0x62/0x90
[<ffffffff812dfd26>] nfs_commit_inode+0xa6/0x170
[<ffffffff812dfe4d>] nfs_write_inode+0x5d/0xa0
[<ffffffff81300d69>] nfs4_write_inode+0x9/0x10
[<ffffffff811978ec>] __writeback_single_inode+0x10c/0x2c0
[<ffffffff811987ea>] writeback_sb_inodes+0x2ca/0x450
[<ffffffff81198b2c>] wb_writeback+0xec/0x320
[<ffffffff81199365>] bdi_writeback_workfn+0x115/0x4c0
[<ffffffff810a595b>] process_one_work+0x16b/0x430
[<ffffffff810a6619>] worker_thread+0x119/0x3a0
[<ffffffff810ac2bd>] kthread+0xcd/0xf0
[<ffffffff81aa457c>] ret_from_fork+0x7c/0xb0

[<ffffffffffffffff>] 0xffffffffffffffff

So sync is holding sb->s_umount, queued some bdi work on the filesystem and is waiting for it to complete.
Mean while, that work has (I think) submitted a 'commit' (via ->write_inode) and that commit wants to deactivate_super and so needs to get ->s_umount.

I suspect this could happen even more easily with a lazy unmount.

It seems that this commit request is that last thing that is keeping ->s_active elevated and it deadlocks trying to drop the last s_active.

I have no idea how to fix it.... help?

NeilBrown

#1 lore raw
Quoted 52 lines · NeilBrown

Hi,
I've just hit a deadlock in NFS that seems very strange.
The kernel is 3.14-rc8 which some local changes which shouldn't affect the deadlocking code.

Shortly after umounting the NFS filesystem with "umount -f" (though I don't think the -f is important), I ran "sync".

The sync is now stuck in

[<ffffffff81197fc1>] sync_inodes_sb+0xa1/0x1c0
[<ffffffff8119cd99>] sync_inodes_one_sb+0x19/0x20
[<ffffffff81173372>] iterate_supers+0xb2/0x110
[<ffffffff8119cfd0>] sys_sync+0x30/0x90
[<ffffffff81aa4622>] system_call_fastpath+0x16/0x1b

[<ffffffffffffffff>] 0xffffffffffffffff

while kworker/u16:1 is stuck:

[<ffffffff815420b3>] call_rwsem_down_write_failed+0x13/0x20
[<ffffffff81172889>] deactivate_super+0x39/0x60
[<ffffffff812d56f1>] nfs_sb_deactive+0x21/0x30
[<ffffffff812d2ef9>] __put_nfs_open_context+0xc9/0x100
[<ffffffff812d2f3b>] put_nfs_open_context+0xb/0x10
[<ffffffff812ddd14>] nfs_commitdata_release+0x14/0x30
[<ffffffff812ddd4a>] nfs_commit_release+0x1a/0x20
[<ffffffff81a45a05>] rpc_free_task+0x25/0x70
[<ffffffff81a45fd8>] rpc_do_put_task+0x78/0x80
[<ffffffff81a45feb>] rpc_put_task+0xb/0x10
[<ffffffff812de3fe>] nfs_initiate_commit+0xce/0x110
[<ffffffff812df112>] nfs_commit_list+0x62/0x90
[<ffffffff812dfd26>] nfs_commit_inode+0xa6/0x170
[<ffffffff812dfe4d>] nfs_write_inode+0x5d/0xa0
[<ffffffff81300d69>] nfs4_write_inode+0x9/0x10
[<ffffffff811978ec>] __writeback_single_inode+0x10c/0x2c0
[<ffffffff811987ea>] writeback_sb_inodes+0x2ca/0x450
[<ffffffff81198b2c>] wb_writeback+0xec/0x320
[<ffffffff81199365>] bdi_writeback_workfn+0x115/0x4c0
[<ffffffff810a595b>] process_one_work+0x16b/0x430
[<ffffffff810a6619>] worker_thread+0x119/0x3a0
[<ffffffff810ac2bd>] kthread+0xcd/0xf0
[<ffffffff81aa457c>] ret_from_fork+0x7c/0xb0

[<ffffffffffffffff>] 0xffffffffffffffff

So sync is holding sb->s_umount, queued some bdi work on the filesystem and is waiting for it to complete.
Mean while, that work has (I think) submitted a 'commit' (via ->write_inode) and that commit wants to deactivate_super and so needs to get ->s_umount.

Quoted 7 lines

I suspect this could happen even more easily with a lazy unmount.

It seems that this commit request is that last thing that is keeping ->s_active elevated and it deadlocks trying to drop the last s_active.

I have no idea how to fix it.... help?

The problem seems to be the use of iterate_supers(), which grabs a passive reference, and conflicts with our use of an active reference in the open context.

Jan, any suggestions?

Cheers
Trond

_________________________________
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]

#2 lore raw

Hello,

Quoted 55 lines · Trond Myklebust
Quoted 54 lines · NeilBrown

I've just hit a deadlock in NFS that seems very strange.
The kernel is 3.14-rc8 which some local changes which shouldn't affect the deadlocking code.

Shortly after umounting the NFS filesystem with "umount -f" (though I don't think the -f is important), I ran "sync".

The sync is now stuck in

[<ffffffff81197fc1>] sync_inodes_sb+0xa1/0x1c0
[<ffffffff8119cd99>] sync_inodes_one_sb+0x19/0x20
[<ffffffff81173372>] iterate_supers+0xb2/0x110
[<ffffffff8119cfd0>] sys_sync+0x30/0x90
[<ffffffff81aa4622>] system_call_fastpath+0x16/0x1b

[<ffffffffffffffff>] 0xffffffffffffffff

while kworker/u16:1 is stuck:

[<ffffffff815420b3>] call_rwsem_down_write_failed+0x13/0x20
[<ffffffff81172889>] deactivate_super+0x39/0x60
[<ffffffff812d56f1>] nfs_sb_deactive+0x21/0x30
[<ffffffff812d2ef9>] __put_nfs_open_context+0xc9/0x100
[<ffffffff812d2f3b>] put_nfs_open_context+0xb/0x10
[<ffffffff812ddd14>] nfs_commitdata_release+0x14/0x30
[<ffffffff812ddd4a>] nfs_commit_release+0x1a/0x20
[<ffffffff81a45a05>] rpc_free_task+0x25/0x70
[<ffffffff81a45fd8>] rpc_do_put_task+0x78/0x80
[<ffffffff81a45feb>] rpc_put_task+0xb/0x10
[<ffffffff812de3fe>] nfs_initiate_commit+0xce/0x110
[<ffffffff812df112>] nfs_commit_list+0x62/0x90
[<ffffffff812dfd26>] nfs_commit_inode+0xa6/0x170
[<ffffffff812dfe4d>] nfs_write_inode+0x5d/0xa0
[<ffffffff81300d69>] nfs4_write_inode+0x9/0x10
[<ffffffff811978ec>] __writeback_single_inode+0x10c/0x2c0
[<ffffffff811987ea>] writeback_sb_inodes+0x2ca/0x450
[<ffffffff81198b2c>] wb_writeback+0xec/0x320
[<ffffffff81199365>] bdi_writeback_workfn+0x115/0x4c0
[<ffffffff810a595b>] process_one_work+0x16b/0x430
[<ffffffff810a6619>] worker_thread+0x119/0x3a0
[<ffffffff810ac2bd>] kthread+0xcd/0xf0
[<ffffffff81aa457c>] ret_from_fork+0x7c/0xb0

[<ffffffffffffffff>] 0xffffffffffffffff

So sync is holding sb->s_umount, queued some bdi work on the filesystem and is waiting for it to complete. Mean while, that work has (I think) submitted a 'commit' (via ->write_inode) and that commit wants to deactivate_super and so needs to get ->s_umount.

I suspect this could happen even more easily with a lazy unmount.

It seems that this commit request is that last thing that is keeping ->s_active elevated and it deadlocks trying to drop the last s_active.

Quoted 6 lines
Quoted 2 lines

I have no idea how to fix it.... help?

The problem seems to be the use of iterate_supers(), which grabs a passive reference, and conflicts with our use of an active reference in the open context.

Yeah, we cannot really do otherwise in iterate_supers() - we have to grab
some superblock reference and we don't really want to get an active one since that would result in spurious EBUSY returns from umount.

Cannot we just punt the deactivate_super() call to a workqueue to avoid this deadlock? It's a bit ugly but it should do the trick. Or is nfs_sb_deactive() called too often and we'd see some adverse effects for that? We could also offload it to workqueue only in the special case where sb->s_active == 1. That should be really rare then but it's a bit ugly poking in VFS internals. BTW why is nfs_sb_active() safe doing just atomic_inc() on s_active? grab_super() is more careful...

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
#3 lore raw
Quoted 68 lines · Jan Kara

Hello,

Quoted 55 lines · Trond Myklebust
Quoted 54 lines · NeilBrown

I've just hit a deadlock in NFS that seems very strange.
The kernel is 3.14-rc8 which some local changes which shouldn't affect the deadlocking code.

Shortly after umounting the NFS filesystem with "umount -f" (though I don't think the -f is important), I ran "sync".

The sync is now stuck in

[<ffffffff81197fc1>] sync_inodes_sb+0xa1/0x1c0
[<ffffffff8119cd99>] sync_inodes_one_sb+0x19/0x20
[<ffffffff81173372>] iterate_supers+0xb2/0x110
[<ffffffff8119cfd0>] sys_sync+0x30/0x90
[<ffffffff81aa4622>] system_call_fastpath+0x16/0x1b

[<ffffffffffffffff>] 0xffffffffffffffff

while kworker/u16:1 is stuck:

[<ffffffff815420b3>] call_rwsem_down_write_failed+0x13/0x20
[<ffffffff81172889>] deactivate_super+0x39/0x60
[<ffffffff812d56f1>] nfs_sb_deactive+0x21/0x30
[<ffffffff812d2ef9>] __put_nfs_open_context+0xc9/0x100
[<ffffffff812d2f3b>] put_nfs_open_context+0xb/0x10
[<ffffffff812ddd14>] nfs_commitdata_release+0x14/0x30
[<ffffffff812ddd4a>] nfs_commit_release+0x1a/0x20
[<ffffffff81a45a05>] rpc_free_task+0x25/0x70
[<ffffffff81a45fd8>] rpc_do_put_task+0x78/0x80
[<ffffffff81a45feb>] rpc_put_task+0xb/0x10
[<ffffffff812de3fe>] nfs_initiate_commit+0xce/0x110
[<ffffffff812df112>] nfs_commit_list+0x62/0x90
[<ffffffff812dfd26>] nfs_commit_inode+0xa6/0x170
[<ffffffff812dfe4d>] nfs_write_inode+0x5d/0xa0
[<ffffffff81300d69>] nfs4_write_inode+0x9/0x10
[<ffffffff811978ec>] __writeback_single_inode+0x10c/0x2c0
[<ffffffff811987ea>] writeback_sb_inodes+0x2ca/0x450
[<ffffffff81198b2c>] wb_writeback+0xec/0x320
[<ffffffff81199365>] bdi_writeback_workfn+0x115/0x4c0
[<ffffffff810a595b>] process_one_work+0x16b/0x430
[<ffffffff810a6619>] worker_thread+0x119/0x3a0
[<ffffffff810ac2bd>] kthread+0xcd/0xf0
[<ffffffff81aa457c>] ret_from_fork+0x7c/0xb0

[<ffffffffffffffff>] 0xffffffffffffffff

So sync is holding sb->s_umount, queued some bdi work on the filesystem and is waiting for it to complete. Mean while, that work has (I think) submitted a 'commit' (via ->write_inode) and that commit wants to deactivate_super and so needs to get ->s_umount.

I suspect this could happen even more easily with a lazy unmount.

It seems that this commit request is that last thing that is keeping ->s_active elevated and it deadlocks trying to drop the last s_active.

Quoted 6 lines
Quoted 2 lines

I have no idea how to fix it.... help?

The problem seems to be the use of iterate_supers(), which grabs a passive reference, and conflicts with our use of an active reference in the open context.

Yeah, we cannot really do otherwise in iterate_supers() - we have to grab
some superblock reference and we don't really want to get an active one since that would result in spurious EBUSY returns from umount.

Quoted 6 lines

Cannot we just punt the deactivate_super() call to a workqueue to avoid this deadlock? It's a bit ugly but it should do the trick. Or is nfs_sb_deactive() called too often and we'd see some adverse effects for that? We could also offload it to workqueue only in the special case where sb->s_active == 1. That should be really rare then but it's a bit ugly poking in VFS internals.

The activate/deactivate super is basically there to save our bacon when NFS file state extends beyond the usual VFS path walk, open() and close(). Examples include sillyrename and NFSv4 delegations. Even ordinary read and write state can extend beyond close() if the user decides to 'kill -9' in the wrong places.
In most of these situations, we need to keep a dentry around until we're finished, which means that we want to keep the super block alive too.

Quoted 2 lines

BTW why is nfs_sb_active() safe doing just
atomic_inc() on s_active? grab_super() is more careful...

We're never grabbing a reference without already holding a reference via a struct path or a struct file passed down to us by the VFS. However we may want to keep the struct dentry around for longer than the lifetime of that struct path/file.

Signature · 3 lines
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
#7 lore raw
In reply to Jan Kara
Quoted 10 lines · Jan Kara

Hello,

Quoted 3 lines · Trond Myklebust

The problem seems to be the use of iterate_supers(), which grabs a passive reference, and conflicts with our use of an active reference in the open context.

Yeah, we cannot really do otherwise in iterate_supers() - we have to grab
some superblock reference and we don't really want to get an active one since that would result in spurious EBUSY returns from umount.

BTW: By what mechanism does an active reference lead to EBUSY issues here?

Signature · 3 lines
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
#8 lore raw
Quoted 8 lines · Trond Myklebust
Quoted 7 lines · Jan Kara

Hello,

Quoted 3 lines · Trond Myklebust

The problem seems to be the use of iterate_supers(), which grabs a passive reference, and conflicts with our use of an active reference in the open context.

Quoted 6 lines
Quoted 3 lines

Yeah, we cannot really do otherwise in iterate_supers() - we have to grab
some superblock reference and we don't really want to get an active one since that would result in spurious EBUSY returns from umount.

BTW: By what mechanism does an active reference lead to EBUSY issues here?

Ah, sorry. I was wrong. We use mount usecount for this. But still using
active reference for iterate_supers() seems wrong as that could result in handling of destruction of superblock from it...

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
#4 lore raw
In reply to Trond Myklebust
Quoted 74 lines · Trond Myklebust
Quoted 66 lines · Jan Kara
Quoted 55 lines · Trond Myklebust
Quoted 54 lines · NeilBrown

I've just hit a deadlock in NFS that seems very strange.
The kernel is 3.14-rc8 which some local changes which shouldn't affect the deadlocking code.

Shortly after umounting the NFS filesystem with "umount -f" (though I don't think the -f is important), I ran "sync".

The sync is now stuck in

[<ffffffff81197fc1>] sync_inodes_sb+0xa1/0x1c0
[<ffffffff8119cd99>] sync_inodes_one_sb+0x19/0x20
[<ffffffff81173372>] iterate_supers+0xb2/0x110
[<ffffffff8119cfd0>] sys_sync+0x30/0x90
[<ffffffff81aa4622>] system_call_fastpath+0x16/0x1b

[<ffffffffffffffff>] 0xffffffffffffffff

while kworker/u16:1 is stuck:

[<ffffffff815420b3>] call_rwsem_down_write_failed+0x13/0x20
[<ffffffff81172889>] deactivate_super+0x39/0x60
[<ffffffff812d56f1>] nfs_sb_deactive+0x21/0x30
[<ffffffff812d2ef9>] __put_nfs_open_context+0xc9/0x100
[<ffffffff812d2f3b>] put_nfs_open_context+0xb/0x10
[<ffffffff812ddd14>] nfs_commitdata_release+0x14/0x30
[<ffffffff812ddd4a>] nfs_commit_release+0x1a/0x20
[<ffffffff81a45a05>] rpc_free_task+0x25/0x70
[<ffffffff81a45fd8>] rpc_do_put_task+0x78/0x80
[<ffffffff81a45feb>] rpc_put_task+0xb/0x10
[<ffffffff812de3fe>] nfs_initiate_commit+0xce/0x110
[<ffffffff812df112>] nfs_commit_list+0x62/0x90
[<ffffffff812dfd26>] nfs_commit_inode+0xa6/0x170
[<ffffffff812dfe4d>] nfs_write_inode+0x5d/0xa0
[<ffffffff81300d69>] nfs4_write_inode+0x9/0x10
[<ffffffff811978ec>] __writeback_single_inode+0x10c/0x2c0
[<ffffffff811987ea>] writeback_sb_inodes+0x2ca/0x450
[<ffffffff81198b2c>] wb_writeback+0xec/0x320
[<ffffffff81199365>] bdi_writeback_workfn+0x115/0x4c0
[<ffffffff810a595b>] process_one_work+0x16b/0x430
[<ffffffff810a6619>] worker_thread+0x119/0x3a0
[<ffffffff810ac2bd>] kthread+0xcd/0xf0
[<ffffffff81aa457c>] ret_from_fork+0x7c/0xb0

[<ffffffffffffffff>] 0xffffffffffffffff

So sync is holding sb->s_umount, queued some bdi work on the filesystem and is waiting for it to complete. Mean while, that work has (I think) submitted a 'commit' (via ->write_inode) and that commit wants to deactivate_super and so needs to get ->s_umount.

I suspect this could happen even more easily with a lazy unmount.

It seems that this commit request is that last thing that is keeping ->s_active elevated and it deadlocks trying to drop the last s_active.

Quoted 6 lines
Quoted 2 lines

I have no idea how to fix it.... help?

The problem seems to be the use of iterate_supers(), which grabs a passive reference, and conflicts with our use of an active reference in the open context.

Yeah, we cannot really do otherwise in iterate_supers() - we have to grab
some superblock reference and we don't really want to get an active one since that would result in spurious EBUSY returns from umount.

Quoted 6 lines

Cannot we just punt the deactivate_super() call to a workqueue to avoid this deadlock? It's a bit ugly but it should do the trick. Or is nfs_sb_deactive() called too often and we'd see some adverse effects for that? We could also offload it to workqueue only in the special case where sb->s_active == 1. That should be really rare then but it's a bit ugly poking in VFS internals.

Quoted 7 lines

The activate/deactivate super is basically there to save our bacon when NFS file state extends beyond the usual VFS path walk, open() and close(). Examples include sillyrename and NFSv4 delegations. Even ordinary read and write state can extend beyond close() if the user decides to 'kill -9' in the wrong places.
In most of these situations, we need to keep a dentry around until we're finished, which means that we want to keep the super block alive too.

Yeah, that makes sense. But offloading dropping of sb reference to a
workqueue would work then, wouldn't it?

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
#5 lore raw
Quoted 82 lines · Jan Kara
Quoted 74 lines · Trond Myklebust
Quoted 66 lines · Jan Kara
Quoted 55 lines · Trond Myklebust
Quoted 54 lines · NeilBrown

I've just hit a deadlock in NFS that seems very strange.
The kernel is 3.14-rc8 which some local changes which shouldn't affect the deadlocking code.

Shortly after umounting the NFS filesystem with "umount -f" (though I don't think the -f is important), I ran "sync".

The sync is now stuck in

[<ffffffff81197fc1>] sync_inodes_sb+0xa1/0x1c0
[<ffffffff8119cd99>] sync_inodes_one_sb+0x19/0x20
[<ffffffff81173372>] iterate_supers+0xb2/0x110
[<ffffffff8119cfd0>] sys_sync+0x30/0x90
[<ffffffff81aa4622>] system_call_fastpath+0x16/0x1b

[<ffffffffffffffff>] 0xffffffffffffffff

while kworker/u16:1 is stuck:

[<ffffffff815420b3>] call_rwsem_down_write_failed+0x13/0x20
[<ffffffff81172889>] deactivate_super+0x39/0x60
[<ffffffff812d56f1>] nfs_sb_deactive+0x21/0x30
[<ffffffff812d2ef9>] __put_nfs_open_context+0xc9/0x100
[<ffffffff812d2f3b>] put_nfs_open_context+0xb/0x10
[<ffffffff812ddd14>] nfs_commitdata_release+0x14/0x30
[<ffffffff812ddd4a>] nfs_commit_release+0x1a/0x20
[<ffffffff81a45a05>] rpc_free_task+0x25/0x70
[<ffffffff81a45fd8>] rpc_do_put_task+0x78/0x80
[<ffffffff81a45feb>] rpc_put_task+0xb/0x10
[<ffffffff812de3fe>] nfs_initiate_commit+0xce/0x110
[<ffffffff812df112>] nfs_commit_list+0x62/0x90
[<ffffffff812dfd26>] nfs_commit_inode+0xa6/0x170
[<ffffffff812dfe4d>] nfs_write_inode+0x5d/0xa0
[<ffffffff81300d69>] nfs4_write_inode+0x9/0x10
[<ffffffff811978ec>] __writeback_single_inode+0x10c/0x2c0
[<ffffffff811987ea>] writeback_sb_inodes+0x2ca/0x450
[<ffffffff81198b2c>] wb_writeback+0xec/0x320
[<ffffffff81199365>] bdi_writeback_workfn+0x115/0x4c0
[<ffffffff810a595b>] process_one_work+0x16b/0x430
[<ffffffff810a6619>] worker_thread+0x119/0x3a0
[<ffffffff810ac2bd>] kthread+0xcd/0xf0
[<ffffffff81aa457c>] ret_from_fork+0x7c/0xb0

[<ffffffffffffffff>] 0xffffffffffffffff

So sync is holding sb->s_umount, queued some bdi work on the filesystem and is waiting for it to complete. Mean while, that work has (I think) submitted a 'commit' (via ->write_inode) and that commit wants to deactivate_super and so needs to get ->s_umount.

I suspect this could happen even more easily with a lazy unmount.

It seems that this commit request is that last thing that is keeping ->s_active elevated and it deadlocks trying to drop the last s_active.

Quoted 6 lines
Quoted 2 lines

I have no idea how to fix it.... help?

The problem seems to be the use of iterate_supers(), which grabs a passive reference, and conflicts with our use of an active reference in the open context.

Yeah, we cannot really do otherwise in iterate_supers() - we have to grab
some superblock reference and we don't really want to get an active one since that would result in spurious EBUSY returns from umount.

Quoted 6 lines

Cannot we just punt the deactivate_super() call to a workqueue to avoid this deadlock? It's a bit ugly but it should do the trick. Or is nfs_sb_deactive() called too often and we'd see some adverse effects for that? We could also offload it to workqueue only in the special case where sb->s_active == 1. That should be really rare then but it's a bit ugly poking in VFS internals.

Quoted 7 lines

The activate/deactivate super is basically there to save our bacon when NFS file state extends beyond the usual VFS path walk, open() and close(). Examples include sillyrename and NFSv4 delegations. Even ordinary read and write state can extend beyond close() if the user decides to 'kill -9' in the wrong places.
In most of these situations, we need to keep a dentry around until we're finished, which means that we want to keep the super block alive too.

Quoted 2 lines

Yeah, that makes sense. But offloading dropping of sb reference to a
workqueue would work then, wouldn't it?

Could we perhaps have a helper in the VFS that can optimise away the case where s->s_active > 1?

_________________________________
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]

#6 lore raw
Quoted 84 lines · Trond Myklebust
Quoted 82 lines · Jan Kara
Quoted 81 lines · Trond Myklebust
Quoted 66 lines · Jan Kara
Quoted 55 lines · Trond Myklebust
Quoted 54 lines · NeilBrown

I've just hit a deadlock in NFS that seems very strange.
The kernel is 3.14-rc8 which some local changes which shouldn't affect the deadlocking code.

Shortly after umounting the NFS filesystem with "umount -f" (though I don't think the -f is important), I ran "sync".

The sync is now stuck in

[<ffffffff81197fc1>] sync_inodes_sb+0xa1/0x1c0
[<ffffffff8119cd99>] sync_inodes_one_sb+0x19/0x20
[<ffffffff81173372>] iterate_supers+0xb2/0x110
[<ffffffff8119cfd0>] sys_sync+0x30/0x90
[<ffffffff81aa4622>] system_call_fastpath+0x16/0x1b

[<ffffffffffffffff>] 0xffffffffffffffff

while kworker/u16:1 is stuck:

[<ffffffff815420b3>] call_rwsem_down_write_failed+0x13/0x20
[<ffffffff81172889>] deactivate_super+0x39/0x60
[<ffffffff812d56f1>] nfs_sb_deactive+0x21/0x30
[<ffffffff812d2ef9>] __put_nfs_open_context+0xc9/0x100
[<ffffffff812d2f3b>] put_nfs_open_context+0xb/0x10
[<ffffffff812ddd14>] nfs_commitdata_release+0x14/0x30
[<ffffffff812ddd4a>] nfs_commit_release+0x1a/0x20
[<ffffffff81a45a05>] rpc_free_task+0x25/0x70
[<ffffffff81a45fd8>] rpc_do_put_task+0x78/0x80
[<ffffffff81a45feb>] rpc_put_task+0xb/0x10
[<ffffffff812de3fe>] nfs_initiate_commit+0xce/0x110
[<ffffffff812df112>] nfs_commit_list+0x62/0x90
[<ffffffff812dfd26>] nfs_commit_inode+0xa6/0x170
[<ffffffff812dfe4d>] nfs_write_inode+0x5d/0xa0
[<ffffffff81300d69>] nfs4_write_inode+0x9/0x10
[<ffffffff811978ec>] __writeback_single_inode+0x10c/0x2c0
[<ffffffff811987ea>] writeback_sb_inodes+0x2ca/0x450
[<ffffffff81198b2c>] wb_writeback+0xec/0x320
[<ffffffff81199365>] bdi_writeback_workfn+0x115/0x4c0
[<ffffffff810a595b>] process_one_work+0x16b/0x430
[<ffffffff810a6619>] worker_thread+0x119/0x3a0
[<ffffffff810ac2bd>] kthread+0xcd/0xf0
[<ffffffff81aa457c>] ret_from_fork+0x7c/0xb0

[<ffffffffffffffff>] 0xffffffffffffffff

So sync is holding sb->s_umount, queued some bdi work on the filesystem and is waiting for it to complete. Mean while, that work has (I think) submitted a 'commit' (via ->write_inode) and that commit wants to deactivate_super and so needs to get ->s_umount.

I suspect this could happen even more easily with a lazy unmount.

It seems that this commit request is that last thing that is keeping ->s_active elevated and it deadlocks trying to drop the last s_active.

Quoted 6 lines
Quoted 2 lines

I have no idea how to fix it.... help?

The problem seems to be the use of iterate_supers(), which grabs a passive reference, and conflicts with our use of an active reference in the open context.

Yeah, we cannot really do otherwise in iterate_supers() - we have to grab
some superblock reference and we don't really want to get an active one since that would result in spurious EBUSY returns from umount.

Quoted 6 lines

Cannot we just punt the deactivate_super() call to a workqueue to avoid this deadlock? It's a bit ugly but it should do the trick. Or is nfs_sb_deactive() called too often and we'd see some adverse effects for that? We could also offload it to workqueue only in the special case where sb->s_active == 1. That should be really rare then but it's a bit ugly poking in VFS internals.

The activate/deactivate super is basically there to save our bacon when NFS file state extends beyond the usual VFS path walk, open() and close(). Examples include sillyrename and NFSv4 delegations. Even ordinary read and write state can extend beyond close() if the user decides to 'kill -9' in the wrong places.
In most of these situations, we need to keep a dentry around until we're finished, which means that we want to keep the super block alive too.

Quoted 5 lines
Quoted 2 lines

Yeah, that makes sense. But offloading dropping of sb reference to a
workqueue would work then, wouldn't it?

Could we perhaps have a helper in the VFS that can optimise away the case where s->s_active > 1?

I'm not sure how you'd imagine the optimisation in VFS. But what I had in
mind was something like:

void nfs_deactivate_super(struct super_block *sb)

{
	if (!atomic_add_unless(&sb->s_active, -1, 1)) {
		/*
		 * Postpone deactivation to workqueue to avoid deadlocking
		 * on s_umount semaphore - we can get here when trying to
		 * complete sync(2) request for forcefully unmounted
		 * filesystem.
		 */
		schedule_work(&NFS_SB(sb)->deactivate_work);
	}
}

static void nfs_deactivate_sb_work(struct work_struct *work)

{
	struct super_block *sb = container_of(work, struct nfs_server,
					      deactivate_work)->super;

	deactivate_super(sb);
}

in nfs_initialise_sb():

	INIT_WORK(&NFS_SB(sb)->deactivate_work, nfs_deactivate_sb_work);

and then use nfs_deactive_super() instead of deactivate_super(). That should do the trick and do the offloading only if we are really dropping the last reference.

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR