Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <71672c07-5e53-31e6-14b1-e067fd56df57@oracle.com>
Date:   Tue, 10 Jan 2023 09:33:14 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH 1/1] NFSD: fix WARN_ON_ONCE in __queue_delayed_work
Content-Language: en-US
To:     Jeff Layton <jlayton@kernel.org>, chuck.lever@oracle.com
Cc:     efault@gmx.de, linux-nfs@vger.kernel.org
References: <1673333310-24837-1-git-send-email-dai.ngo@oracle.com>
 <57dc06d57b4b643b4bf04daf28acca202c9f7a85.camel@kernel.org>
From:   dai.ngo@oracle.com
In-Reply-To: <57dc06d57b4b643b4bf04daf28acca202c9f7a85.camel@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MW5PU052THZDVzlGelkwYTRXN0dSUGNXZGMyR0x1NmZtVTBvbVh3T1NsWnFG?=
 =?utf-8?B?TzdkbkNzbTgxenpRUXR3QVJmTU80Tzd1bUg0STd2Unh5dTNzbUN4SEd3U283?=
 =?utf-8?B?NWdXUWh1Wm11TEtFRFVveS9yZGpuMXFCUUpQSlFKQ09iUWh4UGw1UUdRSE9E?=
 =?utf-8?B?VTdCNGtjb2tzOW1GUFdHS1F4SE5JbTdrTHJQZ21wUVN1RU5oaEpmaDBoVkU0?=
 =?utf-8?B?MUpmdUx4NVRrZDJEc2NOMjd1MTNlS1dCd2dNVHh3ME9Bdis3ZVdxd2k2MG9E?=
 =?utf-8?B?OWtLaWtmUllUUFBEaTlqSW0zanZEcE1EcnFHN0NtMm40bCtjSjV6UlNQL2tm?=
 =?utf-8?B?U3BGM1pLZFVUMmJIVFNjR0h4c0crYlFDQjViMlBXN29QT0hDK0tsR1pUbzV6?=
 =?utf-8?B?cmZkbE1FbWZ3Vi9HOE1FQkdwV0dkVmVLK2VxWFBzTFR5UEt4blRDV29aMlZa?=
 =?utf-8?B?alBMNkZXak0wQXpaUlBzaWNUMUxaQ0VVdEt3YzZLblJLNDZKUlpyTW0wMU9E?=
 =?utf-8?B?QlpjWW1wSkpyUXZlNk02V3FoOWVXbzNtUEpEZnFWdnlYMENHV0dPQ1BtdkpW?=
 =?utf-8?B?eFYyMXRNTTB2SFpicEJFY3FJZXprVmRpVEJmL29uellWWVlEUW8rYmlQVUFw?=
 =?utf-8?B?ZWxtUkRlNnI1RzA4T3dGVm1TbkRqcVVpTVA1MXU3a2R3a1M4TUx4eVE0b2x4?=
 =?utf-8?B?YWdZOE5taUpxOU1tc2szRi9pVzVoVTltUlpERldlN2NTL3BYbkc2Q0VaU0hI?=
 =?utf-8?B?TVQ3dE9OZkhteTNOeWZEOVBsTi9wNDh5OE9MdGRiMk54dzRnSFFLWkdTdTA0?=
 =?utf-8?B?TVFITFAyYTJLOUpwSm5KVnZBMXFON2lUR2dqQjRGdWhEMXA2c1pWQWNLWjdz?=
 =?utf-8?B?MUNZMUhuTnVkT3R2Z3IxYS9kNFFGN2xRRk5oaW9iclhmWXNnSkJ0NklOYm5u?=
 =?utf-8?B?ZEtEVkgvaU1oRVJiS1grUTUwOE5kQlpUVDdFNVhZb245akZKTlplVFQyaGF2?=
 =?utf-8?B?bGtnZytyK0wwM0I2TnBMZThqVUdlR2paZmI2RnNtSnhLdWVwaEoxd3MydDdO?=
 =?utf-8?B?d1RidmtLT1c3TjJTMkY5WHdHSWZjcWRDU2lTYW4yZTE2WGFDcWl5NVJxeFRx?=
 =?utf-8?B?SXhPWGtNQ29SeG5WYkxGNktxS1JQRVFpZEVTaWt0UHA3VlJpd2hVRVg3REs1?=
 =?utf-8?B?SmRJUXd2L2NtNEk4Q05BQ3huc0lFbVBoOUtiQWJtaEtzY1VUT0FoQ01velNo?=
 =?utf-8?B?eTArNS9lajJST0hmS29ScDA4NUNnWGVyTGo3VXpqTnJHR3h6R2RhSDhTRUc2?=
 =?utf-8?B?UzVqQjdvK1h2VVl0WnlvaFVwQWxTd21NOXdoblh2bS9Fc3J6cExWTTBMUHl4?=
 =?utf-8?B?TnMrVEh2N3JGTHdDcnFVNE9sejRZM0NNdEJ1RWpCVEJsS0lER2xyRmVXcHZX?=
 =?utf-8?B?MmN6RFArU2RHQkZNbHJaZjBtR1VGSkRsUzNLbmJOcE1IcktlL1pTL0FaK0p1?=
 =?utf-8?B?UXhtdXpNcE1MRTNnN2RuSVFqK2RzV21GQ0IzOWloNjdSaDlaRVFjb2VyY1VB?=
 =?utf-8?B?R3JTTTFJMGFhWDYzVGJRblFlV1lZcE1aRlJzbzBQV2Y4N1pjZnRYdUYwY29P?=
 =?utf-8?B?dk1xd0dyTnhydGd1ZmJCVVZVN05TYXhLS0o0R0ZIYk5oUERaWDlOLzNnaHpm?=
 =?utf-8?B?QkxuUzFpTVd2L1ozc3R3U0VwbXlSL1kvb0grV0pwUXE2VmFQbzIremJRYWcv?=
 =?utf-8?B?Y2h5WElVcmRDN01kZW41VGVDYkE5c21nSmpZeHVVVExsdjR1UHJ2dGRsT0FQ?=
 =?utf-8?B?Y3pmRjdKQTZaRExVMDU0aisyY2x2c0x2ZXNSNlB0cEQ2U2x2NGpzWWtidjRU?=
 =?utf-8?B?cThUNFdlUHE1TGVjRHMyR0lia0pLT3RUM2tRUVVJdzZUR2V6emh1MFpRSUoy?=
 =?utf-8?B?akNWNUtuSlJFc2JWUGlxVWFSY0J3YkxiT1ExUms2bGlnUHdaQ2tsWTFYYmRG?=
 =?utf-8?B?THdDUzkrSGNDcW9EQ3huSUlyYmZDSHNaSlJqSWhqVG5LdWhjWVhDdVY2N0hO?=
 =?utf-8?B?emJCTERmWk04TkFvbERFZ0FCU0JGaS9BVTNxb28rTzdDWnBzOUkzNWhKTEUx?=
 =?utf-8?Q?+MCMGMnb9JXkVcn1OclduMJYm?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 0O/1FxgWy+HIZ8l6C+ZAub4cUbZfa75Bjyi523Uf/F2DdyOXas1FrN3AwU3PVcNQjrPwr6KwpwFCyS+g00bBrDEQ+zT+nR9JfvFMnlWTqIutdvR0dgAnniCrEVLcyq/vks2Tl/Dlnei7glTdjVsFP9NLylUvOa5kCcE8By46mOmNeuPHerIcrrQ/Yy8NNOAWa1XsC39FLhStOkTbMl92B6DLJ7h8+6ow3TZwxBLjlVAtJ+E3KvTUMFsF/wW8N9Jw5yZ/OXGuJBTiAovlATxRO8oCwjdIEQmjmgRezdHEg5puyszM9j7M0PZ7j57zKSczg9/9YmOpA6BH8kGl/ZzTjJ7uhiLHftVHUlVmRv0xp8PA1u3tHzwepv1IqV866tB6TWUKpFnDR4QGfai7Ual67qCDZsWAqzPoA8ANaC2nvdF76WeyvlIrEfnHYXJURMgk//7Zy8UiISIK0yY+UDtC+T43OrzPcHlkUBpeG+frZjTlseUEqGIVmWLiLqmoQ2n2agcCt8pxCLSXfnWIhS8EY0lEvMxFN+O0oO2d/83G058PwCwwUNLpg4Ju2B7k88aUAZZhUSZPmJzOdRfddiEnK9ysOEjYoy3IaO/S5MZIvr84Qf0vxNpd03vcse7U0MaAWimej1fg2Okbum+xZqWSlvOLyP/P52DJci0OlikutnvvsxQng302G2LlkAPykVheWBXTPb/0iBZ0o7m5EZXIZgDpTTv5n/htFQr4iUSpo/XVUxIiWCaOCUxcITYvkOOdLaF8crN/ytf9JTGD3yJ0nzLH1rckF9ACWP8EVcd1jch6fQHzCXoFr6gx0/MXCgXS
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2de0cb42-ed52-4d52-0efa-08daf330c274
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2023 17:33:17.4118
 (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: YC9Mw7fP8xlAwF0va5qLmNqZN9sPUpcWM67skjIdTTayrUkaAwVQm0rzodG4P83+NmEevnJpQD+eN7aWf+kzWQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR10MB6622
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2023-01-10_07,2023-01-10_03,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxscore=0 malwarescore=0
 adultscore=0 mlxlogscore=999 spamscore=0 phishscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2301100112
X-Proofpoint-ORIG-GUID: Znr8-dzaplZT7X7LVKwKHBB7YkeKlncP
X-Proofpoint-GUID: Znr8-dzaplZT7X7LVKwKHBB7YkeKlncP
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


On 1/10/23 2:30 AM, Jeff Layton wrote:
> On Mon, 2023-01-09 at 22:48 -0800, Dai Ngo wrote:
>> Currently nfsd4_state_shrinker_worker can be schduled multiple times
>> from nfsd4_state_shrinker_count when memory is low. This causes
>> the WARN_ON_ONCE in __queue_delayed_work to trigger.
>>
>> This patch allows only one instance of nfsd4_state_shrinker_worker
>> at a time using the nfsd_shrinker_active flag, protected by the
>> client_lock.
>>
>> Replace mod_delayed_work with queue_delayed_work since we
>> don't expect to modify the delay of any pending work.
>>
>> Fixes: 44df6f439a17 ("NFSD: add delegation reaper to react to low memory condition")
>> Reported-by: Mike Galbraith <efault@gmx.de>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/netns.h     |  1 +
>>   fs/nfsd/nfs4state.c | 16 ++++++++++++++--
>>   2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>> index 8c854ba3285b..801d70926442 100644
>> --- a/fs/nfsd/netns.h
>> +++ b/fs/nfsd/netns.h
>> @@ -196,6 +196,7 @@ struct nfsd_net {
>>   	atomic_t		nfsd_courtesy_clients;
>>   	struct shrinker		nfsd_client_shrinker;
>>   	struct delayed_work	nfsd_shrinker_work;
>> +	bool			nfsd_shrinker_active;
>>   };
>>   
>>   /* Simple check to find out if a given net was properly initialized */
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index ee56c9466304..e00551af6a11 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4407,11 +4407,20 @@ nfsd4_state_shrinker_count(struct shrinker *shrink, struct shrink_control *sc)
>>   	struct nfsd_net *nn = container_of(shrink,
>>   			struct nfsd_net, nfsd_client_shrinker);
>>   
>> +	spin_lock(&nn->client_lock);
>> +	if (nn->nfsd_shrinker_active) {
>> +		spin_unlock(&nn->client_lock);
>> +		return 0;
>> +	}
> Is this extra machinery really necessary? The bool and spinlock don't
> seem to be needed. Typically there is no issue with calling
> queued_delayed_work when the work is already queued. It just returns
> false in that case without doing anything.

When there are multiple calls to mod_delayed_work/queue_delayed_work
we hit the WARN_ON_ONCE's in __queue_delayed_work and __queue_work if
the work is queued but not execute yet.

This problem was reported by Mike. I initially tried with only the
bool but that was not enough that was why the spinlock was added.
Mike verified that the patch fixed the problem.

-Dai

>
>>   	count = atomic_read(&nn->nfsd_courtesy_clients);
>>   	if (!count)
>>   		count = atomic_long_read(&num_delegations);
>> -	if (count)
>> -		mod_delayed_work(laundry_wq, &nn->nfsd_shrinker_work, 0);
>> +	if (count) {
>> +		nn->nfsd_shrinker_active = true;
>> +		spin_unlock(&nn->client_lock);
>> +		queue_delayed_work(laundry_wq, &nn->nfsd_shrinker_work, 0);
>> +	} else
>> +		spin_unlock(&nn->client_lock);
>>   	return (unsigned long)count;
>>   }
>>   
>> @@ -6239,6 +6248,9 @@ nfsd4_state_shrinker_worker(struct work_struct *work)
>>   
>>   	courtesy_client_reaper(nn);
>>   	deleg_reaper(nn);
>> +	spin_lock(&nn->client_lock);
>> +	nn->nfsd_shrinker_active = 0;
>> +	spin_unlock(&nn->client_lock);
>>   }
>>   
>>   static inline __be32 nfs4_check_fh(struct svc_fh *fhp, struct nfs4_stid *stp)