NFSv4: Fix deadlock during the running of state manager

[PATCH] NFSv4: Fix deadlock during the running of state manager

Posted by Li Lingfeng 1 year ago

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@huawei.com>
---
 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
-- 
2.31.1

Re: [PATCH] NFSv4: Fix deadlock during the running of state manager

Posted by Trond Myklebust 12 months ago

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@huawei.com>
> ---
>  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.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] NFSv4: Fix deadlock during the running of state manager

Posted by Li Lingfeng 10 months, 3 weeks ago

在 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@huawei.com>
>> ---
>>   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.

>