在 2024/12/18 1:00, Trond Myklebust 写道:
On Fri, 2024-12-13 at 11:59 +0800, Li Lingfeng wrote:
Unlinking file may cause the following deadlock in state manager:
[root@localhost test]# cat /proc/2943/stack
[<0>] rpc_wait_bit_killable+0x1a/0x90
[<0>] _nfs4_proc_delegreturn+0x60f/0x760
[<0>] nfs4_proc_delegreturn+0x13d/0x2a0
[<0>] nfs_do_return_delegation+0xba/0x110
[<0>] nfs_end_delegation_return+0x32c/0x620
[<0>] nfs_complete_unlink+0xc7/0x290
[<0>] nfs_dentry_iput+0x36/0x50
[<0>] __dentry_kill+0xaa/0x250
[<0>] dput.part.0+0x26c/0x4d0
[<0>] __put_nfs_open_context+0x1d9/0x260
[<0>] nfs4_open_reclaim+0x77/0xa0
[<0>] nfs4_do_reclaim+0x385/0xf40
[<0>] nfs4_state_manager+0x762/0x14e0
[<0>] nfs4_run_state_manager+0x181/0x330
[<0>] kthread+0x1a7/0x1f0
[<0>] ret_from_fork+0x34/0x60
[<0>] ret_from_fork_asm+0x1a/0x30
[root@localhost test]#
It can be reproduced by following steps:
1) client: open file
2) client: unlink file
3) server: service restart(trigger state manager in client)
4) client: close file(in nfs4_open_reclaim, between
nfs4_do_open_reclaim
and put_nfs_open_context)
Since the file has been open, unlinking will just set
DCACHE_NFSFS_RENAMED
for the dentry like this:
nfs_unlink
nfs_sillyrename
nfs_async_unlink
// set DCACHE_NFSFS_RENAMED
Restarting service will trigger state manager in client.
(1) NFS4_SLOT_TBL_DRAINING will be set to nfs4_slot_table since
session
has been reset.
(2) DCACHE_NFSFS_RENAMED is detected in nfs_dentry_iput. Therefore,
nfs_complete_unlink is called to trigger delegation return.
(3) Due to the slot table being in draining state and sa_privileged
being
0, the delegation return will be queued and wait.
nfs4_state_manager
nfs4_reset_session
nfs4_begin_drain_session
nfs4_drain_slot_tbl
// set NFS4_SLOT_TBL_DRAINING (1)
nfs4_do_reclaim
nfs4_open_reclaim
__put_nfs_open_context
__dentry_kill
nfs_dentry_iput // check DCACHE_NFSFS_RENAMED (2)
nfs_complete_unlink
nfs_end_delegation_return
nfs_do_return_delegation
nfs4_proc_delegreturn
_nfs4_proc_delegreturn
rpc_run_task
...
nfs4_delegreturn_prepare
nfs4_setup_sequence
nfs4_slot_tbl_draining // check NFS4_SLOT_TBL_DRAINING
// and sa_privileged is 0 (3)
rpc_sleep_on // set queued and add to slot_tbl_waitq
// rpc_task is async and wait in __rpc_execute
rpc_wait_for_completion_task
__rpc_wait_for_completion_task
out_of_line_wait_on_bit
rpc_wait_bit_killable // wait for rpc_task to complete
<-------- can not get here to wake up rpc_task -------->
nfs4_end_drain_session
nfs4_end_drain_slot_table
nfs41_wake_slot_table
On the one hand, the state manager is blocked by the unfinished
delegation
return. As a result, nfs4_end_drain_session cannot be invoked to
clear
NFS4_SLOT_TBL_DRAINING and wake up waiting tasks.
On the other hand, since NFS4_SLOT_TBL_DRAINING is not cleared,
delegation return can only wait in the queue, resulting in a
deadlock.
Fix it by turning the delegation return into a privileged operation
for
the case where the nfs_client is in NFS4CLNT_RECLAIM_REBOOT state.
Fixes: 977fcc2b0b41 ("NFS: Add a delegation return into
nfs4_proc_unlink_setup()")
Signed-off-by: Li Lingfeng <lilingfeng3@xxxxxxxxxx>
---
fs/nfs/nfs4proc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 405f17e6e0b4..f3b1f2725c4e 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6878,7 +6878,7 @@ static int _nfs4_proc_delegreturn(struct inode
*inode, const struct cred *cred,
data->res.sattr_res = true;
}
- if (!data->inode)
+ if (!data->inode || test_bit(NFS4CLNT_RECLAIM_REBOOT,
&server->nfs_client->cl_state))
nfs4_init_sequence(&data->args.seq_args, &data-
res.seq_res, 1,
1);
else
Rather than make the delegreturn be privileged, it seems better to make
that delegreturn be asynchronous, just like the unlink itself.
Hi,
In my understanding, the rpc_task in delegreturn has the RPC_TASK_ASYNC
flag set, which means it is already asynchronous.
In the described scenario, delegreturn waiting for this asynchronous
rpc_task to complete caused the deadlock. I'm not entirely clear on what
you mean by 'make that delegreturn be asynchronous.' Could you please
elaborate a bit more on that for better clarity?
Thanks.
