Received-SPF: pass (google.com: domain of linux-nfs+bounces-658-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: <aef15e6d-20c2-461d-816b-9b8bc07a9387@oracle.com>
Date: Fri, 15 Dec 2023 13:55:20 -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: Chuck Lever <chuck.lever@oracle.com>
Cc: jlayton@kernel.org, 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>
 <ZXyvCnEuV9L18JSS@tissot.1015granger.net>
 <195ba461-0908-4690-85a9-a9d12ffbad90@oracle.com>
 <ZXzIGmhDZp7v87aZ@tissot.1015granger.net>
From: dai.ngo@oracle.com
In-Reply-To: <ZXzIGmhDZp7v87aZ@tissot.1015granger.net>
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?VjNnZG45WnYrQXc2M0JuUm5LY3R1Vy9qWjFYK05TUWQwWnJZWjc0c0RJV1dt?=
 =?utf-8?B?dllXb25ISk5XcVoxWE5sNmNyTXpGRnV5clZwL0JaM0dEN3U4SEV1TE5wRXQ3?=
 =?utf-8?B?d1Q0cVgvcE9TMG5FMDkwYlRRUTVhS2dYZTJCVHpwQW4wL1hpTU0wR0NsWHky?=
 =?utf-8?B?dEZFU0hVeGY4M3hMWExKTGtXQzNPTUFtclNIMEJqdmlhSHJtVHdlUXJLRDZO?=
 =?utf-8?B?ZVNoNzJNUFBVdk9rVkFPNys3eHkyek5xL0NGMjBNNU9xM1pZZCttMFRMMUxx?=
 =?utf-8?B?cVNZZkZ6WTRXSDJNdERWQWdleEpCWE1WNUYvdGxNL243cGV2NEY3UzhRSzJm?=
 =?utf-8?B?TzQ5bWdLNDRURWN0VmxRSlNPdFJSSG0yb3lQYjVEbnNCRStXdmhTdEF0Mzht?=
 =?utf-8?B?cDRYMkJLNmRoR3RYMnZSbU5oZHZ5c2tudnRVOGlDaWhHWnZwc3BIQ3FpNmt6?=
 =?utf-8?B?ODc2YzNpczh6d3I5OTRneVFNaDJ0cmtkeGdSNnZaWFRzZnEvK2wyRm5DOWtr?=
 =?utf-8?B?MHllcmtHaUFVdHE5b0tpbFZPSUQ0VVE5R0NCbThPRjE0Lzd6ZHo1T09MR2VX?=
 =?utf-8?B?VVg0S3NZcXJHS0Y2Yjl5Zy8yZGl3c0Y5RmpRcHhpa2Q3c0FPTWR0NzNWbHN0?=
 =?utf-8?B?anNHMXVKTzU2TnFCeFpaaXBOSHdEWE9vZ3ZIZmJMajBVS0s4KytxNFFPbm9K?=
 =?utf-8?B?ZWxUVVYwRUFSWXlHN2FtTTJwNzBHNGd5K2ptWE13K3hVYzlVTSt2VWVsTzF2?=
 =?utf-8?B?OE1DVCs1NG5FNUNBYzljeXhabmJLMkVpcnM2K2owVDlKRklhdnNHVkFRZ1ND?=
 =?utf-8?B?NUtnMEF2aEVvbG9Td1UvVVJOYnRFYnJwZEh1NTI5dWZUUm50UVFKVlpSdHFI?=
 =?utf-8?B?VHBvdW90REFGTkhUUlpaclc4L0tCODlFRjREZEJUU1JVREpiUDlSNmVKSzNS?=
 =?utf-8?B?TkFXeTZicUFZZXRZYitRSGxRblYzZTU1MktVekRoYzlSTnVxZVhXUE5uemFP?=
 =?utf-8?B?UklTbDVlRmFDNG03T0tkZVkrdXZtZ3IvQWQyRHN3K01sQlRtZDdEOEd6MTlv?=
 =?utf-8?B?M2ZJQlRoc2dEbklhbnZPY01wTk45UzROVk9lbWlFWjk2UEtDcmtaZkNkM1c1?=
 =?utf-8?B?TW1vYkF0eko4Q3JqMXVCUEt4Q1BGSW0rWTMvaGh6YlJidkRlMWthcXFja3Yx?=
 =?utf-8?B?NFlaR2VBRnd1ZVB0dGpYZkN2eHdxU1J3L0hEeENKWng0Vm1aZGN5TjRBL2ZB?=
 =?utf-8?B?WTZNbExRdllxbGJpS2dDN24zbzJTcDcvRUx1eDVrWlNMR2xzNHBqMzQvQ1g3?=
 =?utf-8?B?VXNCWlRPVWpEd1dhQWV1UXF5SUJRdDVBOXFoOXZ4ck9jTmJaWkhaVU1WZzAw?=
 =?utf-8?B?eU9JYVlHakdCamFDRXdmN3U0cTlPTWgzOURmNTVFTjFnUlF4OTYycW5kWVpz?=
 =?utf-8?B?SHQ2VmVZaEpZMTE2MmlxNVFLZHdHenNOdFJYaSswbHNvNzhGbTROQURWdWRR?=
 =?utf-8?B?TkQwK3dNcGdScS81UmNWTTZFaXBRV1YxZXBqMUFIcWNoaDlzUE1DZU10UW45?=
 =?utf-8?B?ekNIVEZmMFNFUHRCZEt1U3pVWEx6eTMzS2JWbjNwN0V1WDkwdEo5SXppZEVZ?=
 =?utf-8?B?TlVrUFI3TEpVNm16U1A4WmZaQlkxcWFLN2hlNzlhZHNhaVNtbEVsSHpHZGF2?=
 =?utf-8?B?U1M1NjFUVCs1Y2NSZ2VrTlAzMTZSZTVGTWxkUW4zSGlkckdsU1VTcFFoZGNM?=
 =?utf-8?B?QkpFV1J1NDlkSHJ2b2hDL1YyanlONCtUV0lQSVRieDhVWEJINVdyYjg1Mncw?=
 =?utf-8?B?aGJCcnIvZDhQVFA3cXVxQXZPV2E1K25icTNRR1ZOeUpjNzJRTm0rNVJxd0lU?=
 =?utf-8?B?U01ncTJubGhRWEN6WFRrRG1mY3M3aVdlbWxQbjQwZXVnbFFvVytsZStDcGg1?=
 =?utf-8?B?YjRNZE80eUd3R2Y5VG9FNytTdFV3VkF4MkdGQnVDQ0VKUzBPNFM0UzFQdXlQ?=
 =?utf-8?B?dGkyL2lqN3M4NG05cDU0ZFovbzFsQVQ3dEhNTVU0WTRIRUsrU3B3dDZkZzgr?=
 =?utf-8?B?MDhUd1FuL2NYbWlpeDJ6MUpka1lEVStYb3o4SjRBSTAxZm5kR0hXeWRRMzBL?=
 =?utf-8?Q?ZbxD2E+US+OBTiVZRrWZ9nu0a?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	wVzZua34mssQIHztQ9dxB3OUPM4LgJHzmm/CZ6wp/3sIkCsiVqw5Cq1u8+gLeX1IHHhiB+pLsQ8M2DEjH7jPE/1Tn6+4d+k4dOD7SfwJqZqNmcJT5ZRakkKnf37xrszGkyyfUis8YTqXjjockeFp0qjvvR3zjTtjz4c0mI49J9rGQGkD3E9zdUVleihOEAGtTI+VqVxluytSR86KfsiBmlMbun+LvwhwbF+eC8JXSCULOfCSS4wkcB0LgjIYCotf4S/GHj0A/teHkpqZiSCVcMKGhsI0euPLVAx9GSCSFzs3qsZtxwNMayJrXVh4fCXHvb72YezICq0dsh4D9yekBSa1/XArDkmwt1pYbxny6PWUB3g6bP9HRHdK3rHp4Bj0My/HMYjEHP5jeCj9rFFEU+F2jxPlEctGgE56vS0xTTpozcHCxxADyHybl35fktmyKxvcAgpFZ1WVxykBoZBA+K0N9LuZPAtxhXOKSVuimjGzjdG4wLKe1pbRinHboARYUBBMrXX++WhCCfdmkpPUgfpzz1Wrm5CE8ScMRqVEeiYULztOLEf16V0C1IKhXLn+JMjTzabeB+Jo28IA8nWVDRep3c7dbCDxPtmD5Ots5Lc=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1b377ab6-1ae9-40df-1fc0-08dbfdb88a61
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Dec 2023 21:55:24.2625
 (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: O0Ti5kBlgKE4HKCeel4GSXc8MXKNUQMdymWwUC1BCtD8IpStNYB79RGg8H8AtskcvwCACE8zIlaqa/0hPytFBg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB7323
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 mlxlogscore=999 bulkscore=0 spamscore=0
 mlxscore=0 adultscore=0 phishscore=0 malwarescore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312150153
X-Proofpoint-ORIG-GUID: bEOnVJHOmiHEkNkTcjJ8aIr9vpKzWE9p
X-Proofpoint-GUID: bEOnVJHOmiHEkNkTcjJ8aIr9vpKzWE9p

Sorry Chuck, I didn't see this before sending v2.

On 12/15/23 1:41 PM, Chuck Lever wrote:
> On Fri, Dec 15, 2023 at 12:40:07PM -0800, dai.ngo@oracle.com wrote:
>> On 12/15/23 11:54 AM, Chuck Lever wrote:
>>> On Fri, Dec 15, 2023 at 11:15:03AM -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);
>>> I'm still thinking the timeout here should be the same (or slightly
>>> longer than) the RPC retransmit timeout, rather than adding a new
>>> NFSD_CB_GETATTR_TIMEOUT macro.
>> The NFSD_CB_GETATTR_TIMEOUT is used only when we can not submit a
>> work item to the workqueue so RPC is not involved here.
> In the "RPC was sent successfully" case, there is an implicit
> assumption here that wait_on_bit_timeout() won't time out before the
> actual RPC CB_GETATTR timeout.
>
> You've chosen timeout values that happen to work, but there's
> nothing in this patch that ties the two timeout values together or
> in any other way documents this implicit assumption.

The timeout value was chosen to be greater then RPC callback receive
timeout. I can add this to the commit message.

>
>
>> We need to
>> time out here to prevent the client (that causes the conflict) to
>> hang waiting for the reply of the GETATTR and to prevent the server
>> reboot to hang due to a pending NFS request.
> Perhaps a better approach would be to not rely on a timeout, but
> instead have nfs4_cb_getattr() wake up the bit wait before
> returning, when it can't queue the work. That way, wait_on_bit()
> will return immediately in that case.

We can detect the condition where the work item can't be queue.
But I think we still need to use wait_on_bit_timeout since there
is no guarantee that the work will be executed even if it was
queued.

-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 */
>>>> +
>>>>    /*
>>>>     * 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
>>>> -- 
>>>> 2.39.3
>>>>