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: <42876697-ba42-c38f-219d-f760b94e5fed@oracle.com>
Date:   Tue, 10 Jan 2023 11:17:17 -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 III <chuck.lever@oracle.com>
Cc:     Mike Galbraith <efault@gmx.de>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
References: <1673333310-24837-1-git-send-email-dai.ngo@oracle.com>
 <57dc06d57b4b643b4bf04daf28acca202c9f7a85.camel@kernel.org>
 <71672c07-5e53-31e6-14b1-e067fd56df57@oracle.com>
 <8C3345FB-6EDF-411A-B942-5AFA03A89BA2@oracle.com>
 <5e34288720627d2a09ae53986780b2d293a54eea.camel@kernel.org>
From:   dai.ngo@oracle.com
In-Reply-To: <5e34288720627d2a09ae53986780b2d293a54eea.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?bHl0ekhNeFMwM3VldTROOFBjSVdrNmpnRzVGbGRodHdYZm4yQ0x1dGlIUXdS?=
 =?utf-8?B?ZmZLRTBybTd2eWtaazlEZWprQ0p1L2hkeG51NXpvVXpWVll0U0dGaW5YaWsy?=
 =?utf-8?B?dlJIaFNTSy9EL2ttSlNwVHBxci9qNStIMkxkMXg4bFdHUmhFNml5ZnVVdTlu?=
 =?utf-8?B?NEI5VDU1S1Bnd0ZEQk9XTW15amo5ZU1UTXcvWGtSK0V0S0F4ZzdmK0w0Mkpn?=
 =?utf-8?B?ZWY3ZUZjZ3YzTVQrc3Y5elFEM2FaTG5YbzJjM0hMY1A4OG1vamV3SlVpa1RG?=
 =?utf-8?B?TkgwTHZqejJ2VWVsZ1Q3ck5ScSttcUt6RG85Q3NINkVlTFRIbmF2OEY5YzhE?=
 =?utf-8?B?cVFBZ3NXb1VacncyR0ZQVVk4R05rTGFLWkszRjd6NHR2VE9GbVFqa0dlS0dv?=
 =?utf-8?B?elgvV3NvVVpCS0NkZW5scU1zclpld3NRWmhCWkZKODhocDVybCtvTnkvUDZ6?=
 =?utf-8?B?dmlYM0dkbVJOL0VsMmFLeGl1a09DWUY4dHNYS3lTSzR6cEhYNjZkdTlxL0VB?=
 =?utf-8?B?anRNZWxXcU9VZ0pSVUR0UXUzKzZhaU1EQUZLZTY0cDh1dkpqZHZiT3Bja0ZL?=
 =?utf-8?B?b25leEFKL2hpRlZOSExxSnBKRDZ0Y1BpYmVJejVuSmhhUUxSbGFUaFJPbWMz?=
 =?utf-8?B?VC8yV0FaMWpLSEhxSE5ES1E4Ti81V0FsL0l4UGdDenJoWVI2M3ZDdzQzcjMx?=
 =?utf-8?B?MmJQbFlsemZ5QjFSWmxsQ2RmSnRzdTVYa2ZCNnpOd0JLUjFCWW91eXhGdk9W?=
 =?utf-8?B?b1NBcC8xU3VXaFB0ZHFLVXpteEpzMThGMUgxSndmYy96eldPYko5azZZa2lC?=
 =?utf-8?B?enBZSi9sdHRVVDBPOVZZS2VrVjkrT1JPRmk1ekF5a2JVa3B3dlpFZlZaclNU?=
 =?utf-8?B?eUQ0Q2JPWVE1bDFmZThXMjVTeXRFcGNxS081eWRkK3AzVVlHV0pLWDErK0Vq?=
 =?utf-8?B?aVRJK2tVOW9EMFdDTytFWngxZk8vZUFBaitBZ2p6eHpTVllMeWY2clVPSmZr?=
 =?utf-8?B?Qy9UZGZsd01HRnQrWWxZUTArVFVDenlKTEg5UGkxVHJDK1JjZDNGWjJzelF3?=
 =?utf-8?B?TGp1dHdEVmxHN094ZGxOcGNKYjJ1Q2xRdU1XUkZSMHVzUXlsLzJtdGExVzZk?=
 =?utf-8?B?OFlENXJaeFk0Y3Z5L0hsNTc2cjA5MWRjSVpWN24ycGN5alFjbTNaWXVlZEFz?=
 =?utf-8?B?Zkc1OVM0TnVjWTdZd1pUNzNPc0l0bWIxMzBINlB1cEdsa3lQbXlhUG55UFJP?=
 =?utf-8?B?NitzeXBUKzQvM1Rud3B5U3JkTmlUL0lUNlVQMmc4SGp0SlRSbEQrdDBCUEpn?=
 =?utf-8?B?RmUwVjhIbjV3TzB3S3JLcWh6UmFaZ0dObHIwbVV0aUdLelRkOU93MTFCK1p1?=
 =?utf-8?B?M1lJTVMwZkkvY01zM3BwdXBpb09tMjVEQ1MvMUZkbmJldVhyUGZYVmdlVDZu?=
 =?utf-8?B?Q1k0T3dHNTdqK0FpVTl3UUJXSFJ2Y0VuZGRsNDV1MmpvaStGV3NsT1ptcXFR?=
 =?utf-8?B?ekdoaUYvdTRIc2w4eVJpaFRJOEFmNzlhay9nQXZPRzVDVmNtcFBUeTRCcTF6?=
 =?utf-8?B?ZVlxUDBmUngvNU14amh5VHl1SDBCWDk0N3psVStGWkZUWDRJMFJXSlJqV1FX?=
 =?utf-8?B?UW1uVm5Hc0R2bzNOcHliYzl1b1BsM0tsYkNyVTliUmJTMnFtY3hjczF5TkVT?=
 =?utf-8?B?SmF3WW9SUHZNZE4vb2pIZS9VOFkybEtGTHhVMlM0a2VuOXptYXhxaWNvOWFr?=
 =?utf-8?B?WlhvTGhPVm4rdG9VbTJJb2E4eDI3TnFiWWNOVnA0WGlvSUluRk9vcG80dlJm?=
 =?utf-8?B?bGdmV0h0cnZFR3Z4ZldBWHZGazZSOWh6bEJpTUtSVURveVpyNWkrbFMrRysv?=
 =?utf-8?B?MDAwMVFoek5tYkVtTWNTZnpmRnJBcWZZUGRjQlpneFZEVDJEeERmTnRZckRL?=
 =?utf-8?B?cVJxRzU0NFk4S0hqOWNja3duaWdCdmk1NFpJZFc3bEN1Z0J6c0pjMVhOUDlU?=
 =?utf-8?B?YnBNSXF4WEVhODBXdkFhT2YyWXU0azY2Y1VYdytkZzJqRUIwbzc4K2kxZ1ly?=
 =?utf-8?B?MHZ3YXc2eWVqcHNwOHJhT24xdTRLdE5DNzViSEdwT0M0Z0RvekhDTUZsVnBZ?=
 =?utf-8?Q?Uc+pkzYHi8/2bnTHlSNeSmwsM?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: mdWkTN4qFAXl1pagMSOBDBt7WmFwDf7ULN+oUjn+9royyS94r1RDNGTC/KdAqiHdI8mHz9vvTU8eBF/psjE4yPTIUeES81zC2bVN6ogChWjFP+ncqH0hNlrshP6ozT2fap4EiKoD+TBwEtokk2xBHt8rA5PmV7UTarHINhu5BBxlZ1wUoDAm4hPI9iSb15dS74EUFQ+T7gX3PJmhaS1VI/ZUQ7Zidr/I9vzEc/3LhdmwG2fiFH2O6/vU9dKII6Rl5MrV/5Ks+0v1mlhHi2ncuOlRwwJ+3O63NIBCfNGxa7S+XIp/FyB74ULSNo+nmO/pQ70tcbxXNknAByNNY8sjMk2Egv2xZDGUbj2CsiVSp3PftiBXzeEkQDSeGhRjzqrQT/szeV2mT1uYzDAFAuA5pWV0UXGZLiju4JTjWJX8ujaWCDLqiI660UvXz4z19iYicEdzCy6Gp/4Wc+GQYisjll94g3mCITiXHSeMr2f9YagfwwnbaKdXQdXC0bsPTbiiOq/eO7RwIhaN9ryLbWndo7jjOe3edS3SGEYjLHfMMVm61kyInQV2spwTyQC65WTMjN3vImxJ+zLSC7CgdptK+zgwvI/0oSBshaGN/UeRxe3IAUbKAdX5EjWDczIxBeyhnCVjg0ayEAhjB6UseNotj7Ltjjl3Py7A2ofEtryBDMlQZ2hgRMUtgJB7kAUtn4P1c/LvCH6ohiEToMX8h/nntCmOh5BJoIENnqL+0iFfS75q85TE77gjNlKs/gfNiCe9LWo1mh+1LnL17kNAiNgVmVcoUXSQdV22e55fBtr5E48ThXyGnyMQUfJLMxZh+/vP
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1407ffcb-4b49-430f-5906-08daf33f4c0a
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2023 19:17:21.2768
 (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: cim8xqqcVi3Zma962TN6/55+9pEtF3c2AhAXW/0iLK2NezciP0zKiJBWg8FkAlNt4oxfv0w3dslqzD/7c2r4xw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4574
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_08,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-2301100126
X-Proofpoint-ORIG-GUID: 90wQatnKE-h_WvpHxrILd4cVpKoybQPy
X-Proofpoint-GUID: 90wQatnKE-h_WvpHxrILd4cVpKoybQPy
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 10:34 AM, Jeff Layton wrote:
> On Tue, 2023-01-10 at 18:17 +0000, Chuck Lever III wrote:
>>> On Jan 10, 2023, at 12:33 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>>
>>>
>>> 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.
>> The delay argument of zero is interesting. If it's set to a value
>> greater than zero, do you still see a problem?
>>
> It should be safe to call it with a delay of 0. If it's always queued
> with a delay of 0 though (and it seems to be), you could save a little
> space by using a struct work_struct instead.

Can I defer this optimization for now? I need some time to look into it.

>
> Also, I'm not sure if this is related, but where do you cancel the
> nfsd_shrinker_work before tearing down the struct nfsd_net? I'm not sure
> that would explains the problem Mike reported, but I think that needs to
> be addressed.

Yes, good catch. I will add the cancelling in v2 patch.

Thanks,
-Dai

>
>>> 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)
>> --
>> Chuck Lever
>>
>>
>>