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;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Dai Ngo <dai.ngo@oracle.com>
CC:     Jeff Layton <jlayton@kernel.org>, Mike Galbraith <efault@gmx.de>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 1/1] NFSD: fix WARN_ON_ONCE in __queue_delayed_work
Thread-Topic: [PATCH 1/1] NFSD: fix WARN_ON_ONCE in __queue_delayed_work
Thread-Index: AQHZJL+WL3xe9S8ATEuPlYmGP9jNkK6Xc6oAgAB2EgCAAAxGgA==
Date:   Tue, 10 Jan 2023 18:17:10 +0000
Message-ID: <8C3345FB-6EDF-411A-B942-5AFA03A89BA2@oracle.com>
References: <1673333310-24837-1-git-send-email-dai.ngo@oracle.com>
 <57dc06d57b4b643b4bf04daf28acca202c9f7a85.camel@kernel.org>
 <71672c07-5e53-31e6-14b1-e067fd56df57@oracle.com>
In-Reply-To: <71672c07-5e53-31e6-14b1-e067fd56df57@oracle.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?12fDvp6fLo5gIqLrR2/ODdCHUcu6F9ODMiO5KHvCR9HvggfcNyAeHiwawZsV?=
 =?us-ascii?Q?fWV99fdPoSbHGEUdK5+FsoyCffFCefZ65NoT36x76u2g3P4Z5yYbQ7CQzgfi?=
 =?us-ascii?Q?PsqkqPe4Wo/QaJu4ziXx16dfwQQWsnUdwPLV5Aw/XDVGnOqfmVfciiYQJmRN?=
 =?us-ascii?Q?oMIIVvEGEbdZgLp+qcdaqwFFK7cfKfo+LitCArLDepb5rzS3T/4iHo5XVyt6?=
 =?us-ascii?Q?weNyzg36QN7G03tTOUnNfBUgaAa3/eyWCVYf3pGkE1XxTLkRCOJLGLWD4Onp?=
 =?us-ascii?Q?bIlaJOmCf9sLyXEWmFflyp0OGEHUxz4tPeN6GTIiRKvUuQX1s9qcwMHs9M87?=
 =?us-ascii?Q?uVi6Nc0bc/Whk9Ycxp/yW4Hus8/EPKuAAPEQjNbTcBR59/Ne41zOzNBEkcEV?=
 =?us-ascii?Q?P2MCTo4Kw1sBnyLJb+QU9+kGxpnrGdFxGo+xt2EZgfwr531XOEQdQ1gEpwqb?=
 =?us-ascii?Q?ih8xJTJnGGi2zQRypct/QMoU5XqYBQ1srv3kAv2z/tzdJKNbbj80nc6N2uKG?=
 =?us-ascii?Q?SIkJFNUcyrhUwWK0niogJlRie8ATdBUorNbscAkpI1AUYDttGxUz+cKKGQWd?=
 =?us-ascii?Q?rIcp7QylwHfpKe6ziGF+E7oG6pVh44CZHFVnv3X5zm/hYY5iDgRuVqiYREGs?=
 =?us-ascii?Q?bzHPxEp0SaFhphrFRdIgAb3Mx37gXd4ToQATIFAaHKPVo9YIpI7mTuz3RYbR?=
 =?us-ascii?Q?mEUO6EsPxU0GqflPAhvs/xOEAdEj1AIdWA3UtPWVKlblPm1Z1He3BPlsYPRu?=
 =?us-ascii?Q?Z/jeCy2JXArfPCQcarzsboF2zmeSyA7feLBDAD/zLiXrDhXmpT1qBbD54Xp+?=
 =?us-ascii?Q?C+YRnf0grMDeFPmT8ZbPs0MDbMruCKLeqFYokew5rioUP7RDEfJdXG3kVi9e?=
 =?us-ascii?Q?h4sMpIY6X16nakI638cHrB4vC18VasH+9ot0u39kMIfjBEMpgG0510jV0uwY?=
 =?us-ascii?Q?V8ZlhDSxhkci4FZDcNApGxaVqxgD3cqcFNP77WnpeKFBzcEwR9CM1SO90UTj?=
 =?us-ascii?Q?rgev3pxbdgYMndGBivAFysjvt4jl38QJhMdC+bnRNbSu7eJoWsdJuaVBmrS/?=
 =?us-ascii?Q?cU1RtLcgVGJIafODSvgQeIrVF/fdxhsnaIk/qi+VxbljdFx2I4ZirCX8NmY+?=
 =?us-ascii?Q?/E7D7IUvvtYCxU8F7E9FKo8zzEZVK7Qa7S+GZOJ96GuCcKbrQ2UQVBJ9bqzQ?=
 =?us-ascii?Q?iDKWASizg05pJyBauHgfLg4Nb1ERNAjJXl99RJYbc6xhNqmF3+cEVBzKXiOO?=
 =?us-ascii?Q?kCmEYPO0+zsVhZ9mb7jtoasnkiX5/OwJGyonsYUS8K16eDQXqBSur9YnNNjQ?=
 =?us-ascii?Q?QKmBOEjs6Z3YnFlMamaLpeS58j/IT66WhZ1CdpDYewpNuQKoiSN+u4Kcsbu8?=
 =?us-ascii?Q?e53bELYm75MlClMl4N+K5L0c77JbZ5DZaILLBuzm8o/egoG6mpByBUvXkMhC?=
 =?us-ascii?Q?FSm49nGO1For+Ms8JFcrknhZ4MplcwjGqPy/a8MVbWvcIpd0k2+HgxBcZf/w?=
 =?us-ascii?Q?X+onJui7KUmOSGLJEsr1YPaaiPKXTw9NsRCPJaxUY713m0CsNMgBNOhb3pzM?=
 =?us-ascii?Q?WvflZ8gq9hdvPb28md3mVeNfGVMx1C4yUkykhC6AidkVTt9HuVT4Aq/q50Ai?=
 =?us-ascii?Q?TA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <6823F7D870166340AC4611D978ACA31A@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: FBCTznHFIid36nP94LO0gbACcv92+xDhAJY7RAMnCJy62QxGnHG1OyjuSCRxuECCqiM8p57GbxbdQrDi5gRz6HSw2azNJq8hC31CqkWvsxHsSg+CtWzz3wuuZMy7cHkW09F1BKsXg+zGXbKEeXFq1Yv7PbXJfHNai37xrNoDy+kx/W2Mc9wjLTMaKpQ8otAYuSd7pCu3ETj4R+14E0UtSNE6HXln5beQO8uv5M9QBK6rjuVWJePpaUZRUHiw8HtK3aQwampbzzxfe4iGy2TbSiycBHWX6ahh3W3tIw32LuLiLwQhaYxtwjVtg5WBCDstRX5i+vXJBqcbxQKeBsu4v+p12Di6YBdR4h+LHAqbRi5T09d5MUuXnUvaCnucVpfHgshG/6wdPLL7+i1RvJLSJdSonUsHjQpjCDbCQBtON9RNxNKWCctaNPjr1wJqizKBP9qldsy2NDakfrzIEH9VTPlpwkzyT0cFNWgI8NHu7uEEeIGY1XyeqfFcLQSlTq2n99GbQTQ54PxscdRsyk06V2Rhw5IuwkCM6n6RaYjLkLLhlZS7Ud6Gr1r/aXrGp9jv8NVF2OUnChadFoQeWkyW/SytEHSTNNTLf08zpS2sn9Qwqv8WV4ov1cOV387I3fWMZrs25wqrJjJ6LB7xG3RQWBzu4PK0oqiVsHKhBwYHKn/mXiDGpuJ9/cUqzSxs2638BYpEsTxCi2JOMe8VvEklYL4FpGan3+BopdUc58CjGIEHuxuMNQWgeM/B79mZI4shMc48S+zHVNx7u5W6uOZ+dSUhG1uFU7vTFkEbmnmvRPiOY0x/j+DmQdX3egaXiw7s
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 391f2b63-30b7-416f-43fe-08daf336e3e5
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Jan 2023 18:17:10.3270
 (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: 7OGjgeUfh/5QFShq36kH206gKdXmf/IwX4m05v+chm08z9YGemCVb9ehkQKi25VtcF4iCsicBOw6FOOgYDqdlQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB7296
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 mlxlogscore=999 bulkscore=0
 suspectscore=0 spamscore=0 mlxscore=0 adultscore=0 malwarescore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2301100117
X-Proofpoint-GUID: BCDpMPklvzCijWBSskY8Eq5qj_NnHoLL
X-Proofpoint-ORIG-GUID: BCDpMPklvzCijWBSskY8Eq5qj_NnHoLL
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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 Jan 10, 2023, at 12:33 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>=20
>=20
> 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.
>>>=20
>>> 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.
>>>=20
>>> Replace mod_delayed_work with queue_delayed_work since we
>>> don't expect to modify the delay of any pending work.
>>>=20
>>> Fixes: 44df6f439a17 ("NFSD: add delegation reaper to react to low memor=
y 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(-)
>>>=20
>>> 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 *shr=
ink, struct shrink_control *sc)
>>>  	struct nfsd_net *nn =3D 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.
>=20
> 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?


> 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.
>=20
> -Dai
>=20
>>=20
>>>  	count =3D atomic_read(&nn->nfsd_courtesy_clients);
>>>  	if (!count)
>>>  		count =3D atomic_long_read(&num_delegations);
>>> -	if (count)
>>> -		mod_delayed_work(laundry_wq, &nn->nfsd_shrinker_work, 0);
>>> +	if (count) {
>>> +		nn->nfsd_shrinker_active =3D 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 =3D 0;
>>> +	spin_unlock(&nn->client_lock);
>>>  }
>>>    static inline __be32 nfs4_check_fh(struct svc_fh *fhp, struct nfs4_s=
tid *stp)

--
Chuck Lever