Re: [PATCH 3/3] NFSD: Fix server reboot hang problem when callback workqueue is stuck

[dai.ngo@oracle.com]

On 12/15/23 11:54 AM, Jeff Layton wrote:
> On Fri, 2023-12-15 at 11:15 -0800, Dai Ngo wrote:
>> If the callback workqueue is stuck, nfsd4_deleg_getattr_conflict will
>> also stuck waiting for the callback request to be executed. This causes
>> the client to hang waiting for the reply of the GETATTR and also causes
>> the reboot of the NFS server to hang due to the pending NFS request.
>>
>> Fix by replacing wait_on_bit with wait_on_bit_timeout with 20 seconds
>> time out.
>>
>> Reported-by: Wolfgang Walter <linux-nfs@stwm.de>
>> Fixes: 6c41d9a9bd02 ("NFSD: handle GETATTR conflict with write delegation")
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4state.c | 6 +++++-
>>   fs/nfsd/state.h     | 2 ++
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 175f3e9f5822..0cc7d4953807 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -2948,6 +2948,9 @@ void nfs4_cb_getattr(struct nfs4_cb_fattr *ncf)
>>   	if (test_and_set_bit(CB_GETATTR_BUSY, &ncf->ncf_cb_flags))
>>   		return;
>>   
>> +	/* set to proper status when nfsd4_cb_getattr_done runs */
>> +	ncf->ncf_cb_status = NFS4ERR_IO;
>> +
>>   	refcount_inc(&dp->dl_stid.sc_count);
>>   	if (!nfsd4_run_cb(&ncf->ncf_getattr)) {
>>   		refcount_dec(&dp->dl_stid.sc_count);
>> @@ -8558,7 +8561,8 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
>>   			nfs4_cb_getattr(&dp->dl_cb_fattr);
>>   			spin_unlock(&ctx->flc_lock);
>>   
>> -			wait_on_bit(&ncf->ncf_cb_flags, CB_GETATTR_BUSY, TASK_INTERRUPTIBLE);
>> +			wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
>> +				TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
> The RPC won't necessarily have timed out at this point, and it looks
> like ncf_cb_status won't have been set to anything (and is probably
> still 0?).

The timeout was added to handle the case where the callback request
did not get queued to the workqueue; nfsd4_run_cb fails. In this case
RPC is not involved and we don't want to hang here. Note that this patch
sets ncf_cb_status to NFS4ERR_IO before calling nfsd4_run_cb so we can
detect this error condition.

>
> Don't you need to check whether the wait timed out or was successful?

ncf_cb_status is set to tk_status by nfsd4_cb_getattr_done. If the request
was successful then ncf_cb_status is 0.

> What happens now when this times out?

Then we go through the normal logic of nfsd_open_break_lease which will
also get timed out but eventually the lease, delegation state, will be
removed by __break_lease after 45 secs (lease_break_time).

-Dai

>
>
>>   			if (ncf->ncf_cb_status) {
>>   				status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
>>   				if (status != nfserr_jukebox ||
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index f96eaa8e9413..94563a6813a6 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -135,6 +135,8 @@ struct nfs4_cb_fattr {
>>   /* bits for ncf_cb_flags */
>>   #define	CB_GETATTR_BUSY		0
>>   
>> +#define	NFSD_CB_GETATTR_TIMEOUT	msecs_to_jiffies(20000) /* 20 secs */
>> +
> Why 20s?

RPC will time out after 9 secs if it does not receive a callback reply.
This time out value needs to be greater than 9 secs. I just be generous
here, we can reduce it to any value > 9 secs.

-Dai

>
>>   /*
>>    * Represents a delegation stateid. The nfs4_client holds references to these
>>    * and they are put when it is being destroyed or when the delegation is
>