Received-SPF: pass (google.com: domain of linux-nfs+bounces-645-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <eb949f89-6133-4922-9015-0dced4ee5c06@oracle.com>
Date: Fri, 15 Dec 2023 12:18:59 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/3] NFSD: Fix server reboot hang problem when callback
 workqueue is stuck
Content-Language: en-US
To: Jeff Layton <jlayton@kernel.org>, chuck.lever@oracle.com
Cc: linux-nfs@vger.kernel.org, linux-nfs@stwm.de
References: <1702667703-17978-1-git-send-email-dai.ngo@oracle.com>
 <1702667703-17978-4-git-send-email-dai.ngo@oracle.com>
 <c9e8608d0cb2562c645d1f31043fcbc90cf53d52.camel@kernel.org>
From: dai.ngo@oracle.com
In-Reply-To: <c9e8608d0cb2562c645d1f31043fcbc90cf53d52.camel@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?eGZtZzlRVEpuUWdMV1JweG9LeklXVmRWZVUzc2J4UXV2cDlwUmZ3NmU3cVJo?=
 =?utf-8?B?YnhyWW1QUTZZdUNScVd6bDNNZFZBUnZHWWxkTDZYdlBnUGVKRVhFd0txZWx2?=
 =?utf-8?B?SVkwdmpEWUVLSWgrYlZBNElmNTVHMEFqTVFhNmpJekliMER3SWpwMWVXTm4y?=
 =?utf-8?B?cFAxTzJ4NTcrZTBSZ1hDaWVwbU1GNzRNbGpEcEFXL0k4N0Z6dTBQZmtSREV2?=
 =?utf-8?B?MVNOTVFlSGpoTzd1UU1Ha01DZlFadVQyRXhDQ29FaHpXNnYrVkc0SjA3WnNH?=
 =?utf-8?B?NURydGllMlVZdENKL3Z3Y0lDQ3FVQmVQWWFYNXhQbm12RzllR2Z3NytucHV4?=
 =?utf-8?B?RU8zSVIwT050MGFmclNaNFZEdlYxZGJKZXZMUk9yNitobTJPOTZHaHhXK05r?=
 =?utf-8?B?dXJxbURvejMzNklONVhhWWRxSnkxTVJtU1ViS010OXU3c1FCT3Y4RHV4ZnVG?=
 =?utf-8?B?NkhkMDQ2Qkg2Qm5Jcyt6ZjV5cXF0bThtMHJVYjUrOGdwM3ppWlE1WlpwWTV5?=
 =?utf-8?B?MTNCNVQzOVBMMzNEY0pzRUNwWU1Vb09LbXYzZHRub0gzZTZFTU1EN21NK2Nt?=
 =?utf-8?B?QkFYbzNDR1k2YjQ1dE1sSFpIK1FZYUVaRWxvZEJ4eTI0ZGtsL0l0RG8rQi9m?=
 =?utf-8?B?Yk9zSjQzaGVHYW5hRFFzVlhhSVlwSndNSDY1aVBWS3BrUG0rL2VFOWF0aXM2?=
 =?utf-8?B?d2hqWmlSSll4ejFCVHFsUDlYTC95Yk5HOEdTV2liS3dqN2tlT0gvQXM3Ulpz?=
 =?utf-8?B?b1Qyd1dwMjVsMnVqSUJxYW1RVTJDN1hoaVNPUFYxSzZ0SGIrbEd1REhMcnpE?=
 =?utf-8?B?dEI4MkRvajFRTFVOV2k2R04vWCs3Y1M3OVh1SThITzc1ektYV3V0NWp2aU1L?=
 =?utf-8?B?TmMrY2VieXpJMVhiTDlLcFFzaDhRUE01aFFjdzJhbXZKRjkyaXd4SFNsSUlZ?=
 =?utf-8?B?eE5WU2ZGWnVGbXVBNEJ0d3kvbng5d1NmN2NRdlpMcGc4T1NwZDE4T0FRdExy?=
 =?utf-8?B?am9mUFJJbitJQUUvMHRhcVMvSDk3MWlzUTlCbG5aZ1hwYlJXbExBUUR0WVNU?=
 =?utf-8?B?aG9hZGQrTDJmMU5uTW45enY0NUpMWTRKb09ENTluTDEzWmhDSWhjaEVQSlF4?=
 =?utf-8?B?YW5ZM3lna3Eza2hwMlJRWmR0VXd0SzZvNEtYYVZ3eHNRcWZEdkZxU2NWeDkv?=
 =?utf-8?B?d3VzempTOWxCdUFNZEpLbTZ4NVBpNWlRVlNLTVowWjhyNFQwN2lhRlFMYVFI?=
 =?utf-8?B?TnNOc1d1ZUNyK3ZXQmtOdXVhSWJFVDNETVRySjYwUmloNXFuK2VpREwwVW9o?=
 =?utf-8?B?ZzRxQmVWUjMwM0xBd0E3bnQ3Z2NqTlNFOENSR0hhUFRWZjN5ZGZneDNVZmdS?=
 =?utf-8?B?WDlWQWs0dFB6Rk5vQ05XU3BYUWZZMTIrM3NkcndPV3BxbW5ZWkt4eGtXUktQ?=
 =?utf-8?B?MWh3L1pER3doTmVBaG5JNlY4K25lL1pBQWJLRXFWTlhlcjhabGlBampEYVRv?=
 =?utf-8?B?bmcvRHFPbURDcUF5WG9obExNL3VRUG0xSXJDRUh2ZFd6bldzZkltdFJJN0c2?=
 =?utf-8?B?MU1XakhnT01kRGlQMzU4K2phREN6T0RnZmJ5cEtWQ0FMajJrbU9zT2x3Skx5?=
 =?utf-8?B?REdQN1JBQnQzd0JzV0tPSEdKa25QM3ZobUtmbDV3aEQyekt6a0crU256a3Zp?=
 =?utf-8?B?YURXN21tRVVheTFMbERoYmNiaEVpeXQrTW9Gc3YyZlFYY0RFRzZpUjNGMUM1?=
 =?utf-8?B?dytlYVp0OG5zWFJDZ2ZEcWt3WjhWWTdwd3IrNEtKRzg0TjRFb2VhVXVRUkNF?=
 =?utf-8?B?OEZrWHV4SXlyQlRmekdBVUhyRGF1VEx6ZzlWUW9pQVFhVlIyWEs2ZFZOQUhG?=
 =?utf-8?B?RlIvcFVVaDBrMDNKeWVCNVB0cCtQUDQyU1ViRVZUVFg2aHU5ZWoyR3Z6ZHdt?=
 =?utf-8?B?R2hNRUl0VkhrdlV5MEdMaDFTN1BjUVBRTUlPUHBoVEV0MTEvMnYvRmZrbFZH?=
 =?utf-8?B?WlRFU0wrQ3dmaklwa0s5eTZkcURUNTVPR2NUVUtvVUtrZTh3dE9TZ0xNM0N4?=
 =?utf-8?B?SjVjaUtWYkoyS3dGdDVpMU5oc1pZVko5L3JBaXJNRE1RNkthTHQ0SElIWkFw?=
 =?utf-8?Q?xPxso8XaXXCcV+XZyXtYh4+f2?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	lB8S0heCYclFQcauedDcAOqeDyFPksLwIvCKTAcIfiQBZiPqQ6Fd+tSVfjDQ9TGSnA1XnbVLwOCm8LzSJwDX5MeqEo3drtSDwWhqKrqEq+gsFlZmYF33RlvDsFUbwiUn1lJ7148sCVfjRtKC0abrdMakYP+YbERdQo2RK15w0mySZnen+Q+mHK/o0U8xSKzVIG1t7weJU1g3gVlw807t3NzYxo+Q7eJV3nq0s33b/zqIz3JM/7uQxWkKXGlMlAuZUdOcYus2jLTy++RBunj8GWKZ5TnNr1ZQX9jeyl4PFSJVe3slFXCVatOT+3VO1rq5+T0X+QSQpp+RT/VfkXbgW4yfgD9IPOB3IoNbFBo8KZW/lg1RPERPyAPWjZIEPFR9zPjfA0pH0go9kjEle9nIjKi5DYoJlAY/0flaEpCRGSo/iEsbkNobdumTk00TUh/ubZgle0kIGx1xIWVKLffI4DehIuCOVAEajZ9pYLqRZFYhKvVdGtie2olJA7rkX0DkR8pXjHdAf/1Um2D9OztcIE3BA1mjIyu2b4uFKzhlLabyJVyE+DxmObpmx9SSd6B4NkXiYu2RkWT8ukrs7YTTgaeIRZv9DFe97oWGpts1dwc=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: db40d2a8-9ada-4c89-3bfc-08dbfdab13bc
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Dec 2023 20:19:01.6544
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: NJOHfgoPXAQBncOkcS2FzDkP8k0j+w5vdmbU0DZBFXtqTCW3xGo8w+eK3I1FdRVTGtjFN8SgVDWDXAtLy+gdVQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR10MB6162
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-15_10,2023-12-14_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 malwarescore=0 spamscore=0
 suspectscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312150142
X-Proofpoint-ORIG-GUID: I0QydNyofb7LbSzJ2VPCQci-8-VzdcGR
X-Proofpoint-GUID: I0QydNyofb7LbSzJ2VPCQci-8-VzdcGR


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
>