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:     Jeff Layton <jlayton@kernel.org>
CC:     Neil Brown <neilb@suse.de>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v5 4/5] nfsd: close race between unhashing and LRU
 addition
Thread-Topic: [PATCH v5 4/5] nfsd: close race between unhashing and LRU
 addition
Thread-Index: AQHY7gDL+y7FIybaF067dT9j2NqLWK4qeNsA
Date:   Tue, 1 Nov 2022 19:45:02 +0000
Message-ID: <0A6BF2E4-5A7D-49BA-8292-FBD8E8693F54@oracle.com>
References: <20221101144647.136696-1-jlayton@kernel.org>
 <20221101144647.136696-5-jlayton@kernel.org>
In-Reply-To: <20221101144647.136696-5-jlayton@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?GPfYlqbuHCAmreNotmzeMO3QG0yuvkGihiymp+IQg1RavpCCLWGqU0BsrSAO?=
 =?us-ascii?Q?/i2slutmgnPxCVk9oGsjwZO8w+Z01VzZGL8FKBJHKP9aX9N6klQcZnsS1ivQ?=
 =?us-ascii?Q?6XfCREueQoFcbe03/oaHMNy49E2FIILErj8mnpPaUilsxUGQZCWvvHvDLAph?=
 =?us-ascii?Q?l83AAWloZW/rQOYCtT6rMB6C8LZj3epLJCtH4fXdMpDV5TK6YZmf5KlJfkE8?=
 =?us-ascii?Q?Pu63lEQQTETIpvPsQDqAhj4m/ogof9lkmWjzv5cmlCW+Ct9DuW3aNduCk+4f?=
 =?us-ascii?Q?jBJK8dsPOJRjBrqOL6lQ7BvhTIeUr5E575JxfCysh4wbYoLyGlPn40dh2v/5?=
 =?us-ascii?Q?Snf3Wl5Aks3fFWaL9l1KIVFfHLyvkWqjLntrzGGB8ULRedbtVpLcxxkuNXBI?=
 =?us-ascii?Q?eOOTtL7fMPL47Iy3Xr2iF9l9XYyE7yx/x+7sVzPh72iBrYBjAvypxO7CUM78?=
 =?us-ascii?Q?Pdrri5oqJ7HOLSnrLdHBylBiEGtyeB6+CgIcNmewKJWklipvUVg1PPMJy5tS?=
 =?us-ascii?Q?FdokyytFNEQMlA0IHOJIfvK+VkU3geCnuHb/ujPuP99DNBIY5vxEiTnwfnF4?=
 =?us-ascii?Q?bujxNYin8uqlZvJZrqN9Y/0yc621vtuhc2OMKcxHcKj0WzScQBM8GghuliT5?=
 =?us-ascii?Q?oVKQukcyj3WQCGoKd9XOXq5EHdSY+pj83ymLME96ivA19Rqc5AwoG0NjTT2h?=
 =?us-ascii?Q?NhwuLN+hoMqAQ9jEtFTIkIryVlRJjW4y+yLDTq753seAnsrh3YpTWs2NjB5q?=
 =?us-ascii?Q?lqHjMxKpB0fEiMLf93sAMWnwA0jy5AfS4AFkcy9JpjoQBJ32nf16PAm+zvtW?=
 =?us-ascii?Q?xav6qJ3fBpryUZ4hwoDyjsPjDD2/Dr/jLsR8N9SXjG3HEzDCPaQSzfRTOaLv?=
 =?us-ascii?Q?lMF3Pf+963Wlw1nznZCesJ3gH4MEcuCA22RLB+TZ9+esDAAdyi/Mph7d87zF?=
 =?us-ascii?Q?CUh6m95iNxZb00k8eaosXwZZOgluKuBbzdKPADkDIOTPJ5Yc232TDAZ4+4CP?=
 =?us-ascii?Q?7BQfIF0eqDJzxFPYL62nxESCCd9U/kHILycXmXhDPRNtbQADqPQxxJ+fouWx?=
 =?us-ascii?Q?TTYPFk8IMFIT80k95f/zvxW3gJp9RDD0wAsD6BukKibcZeCAzlFnuG4pVEGd?=
 =?us-ascii?Q?i96/5OFaCYQU9ZWxRLoV84k5oxkwDzCADqvVT9s+NP3jfZ4/ArKaeCXjWKq/?=
 =?us-ascii?Q?Gc6DbVl7uy5DqQGajnNj/aYaekDob7spyj7NU1Vc17UkErp+KYBz+KaMVuNi?=
 =?us-ascii?Q?IAR0/k+2zhTYydL5yq7tBUE8cHoJVGqFrnl7sO0/c3y88+BOWlpE31VKGb7h?=
 =?us-ascii?Q?xYG/9z/ghsNp75Qf2BtgV3blCg3KuLqA1ETKTrgDF7YF2whow2jZHLQ5sgOF?=
 =?us-ascii?Q?0dfw5mkGiEtXcBUAYUCl/tSsOnEqyOzpXBOp61sfj4jO2XRm/VJC0btNiZPb?=
 =?us-ascii?Q?+kY+F2pLKmPgHxfxfgAOJEtwe3n0o5NJj6lVvhk3/E0CGg/+PA8aHh0HWRUb?=
 =?us-ascii?Q?x7lnViyZbX3R07It2aMTeQPTpDlUJZ8870Rmm6Rtl7fAMn/cl2GyDRhmhOOm?=
 =?us-ascii?Q?V+M9DLL1jTw9Cva757U4U5fzI44kNbRbHoj8du+ZorT9f2DcIsuMLtwi0JvA?=
 =?us-ascii?Q?uA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <9DCAE7E328FCAD418D900701E6953FB8@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
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: 656c55d2-6f1b-4a0f-6728-08dabc419190
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Nov 2022 19:45:02.7246
 (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: eMWh7qHqur5OLD0M5dJSfGPUIoJ6SfWGutGuH5CI1olwE0JxGFG2e1/hQ1ncEmWqrf+lu8FquefsQQspH3oIHQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4430
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-11-01_10,2022-11-01_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0 phishscore=0
 mlxlogscore=999 suspectscore=0 spamscore=0 malwarescore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211010141
X-Proofpoint-GUID: 6P6YRLD0z1SbvaZb0lO90AV7GtPhyd6F
X-Proofpoint-ORIG-GUID: 6P6YRLD0z1SbvaZb0lO90AV7GtPhyd6F
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 Nov 1, 2022, at 10:46 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> The list_lru_add and list_lru_del functions use list_empty checks to see
> whether the object is already on the LRU. That's fine in most cases, but
> we occasionally repurpose nf_lru after unhashing. It's possible for an
> LRU removal to remove it from a different list altogether if we lose a
> race.
>=20
> Add a new NFSD_FILE_LRU flag, which indicates that the object actually
> resides on the LRU and not some other list. Use that when adding and
> removing it from the LRU instead of just relying on list_empty checks.
>=20
> Add an extra HASHED check after adding the entry to the LRU. If it's now
> clear, just remove it from the LRU again and put the reference if that
> remove is successful.
>=20
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 50 ++++++++++++++++++++++++++++++---------------
> fs/nfsd/filecache.h |  1 +
> 2 files changed, 35 insertions(+), 16 deletions(-)
>=20
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index e67297ad12bf..bcea201d79c3 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -409,18 +409,22 @@ nfsd_file_check_writeback(struct nfsd_file *nf)
> static bool nfsd_file_lru_add(struct nfsd_file *nf)
> {
> 	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> -	if (list_lru_add(&nfsd_file_lru, &nf->nf_lru)) {
> -		trace_nfsd_file_lru_add(nf);
> -		return true;
> +	if (!test_and_set_bit(NFSD_FILE_LRU, &nf->nf_flags)) {
> +		if (list_lru_add(&nfsd_file_lru, &nf->nf_lru)) {
> +			trace_nfsd_file_lru_add(nf);
> +			return true;
> +		}
> 	}
> 	return false;
> }
>=20
> static bool nfsd_file_lru_remove(struct nfsd_file *nf)
> {
> -	if (list_lru_del(&nfsd_file_lru, &nf->nf_lru)) {
> -		trace_nfsd_file_lru_del(nf);
> -		return true;
> +	if (test_and_clear_bit(NFSD_FILE_LRU, &nf->nf_flags)) {
> +		if (list_lru_del(&nfsd_file_lru, &nf->nf_lru)) {
> +			trace_nfsd_file_lru_del(nf);
> +			return true;
> +		}
> 	}
> 	return false;
> }
> @@ -476,21 +480,31 @@ nfsd_file_put(struct nfsd_file *nf)
> 	might_sleep();
> 	trace_nfsd_file_put(nf);
>=20
> -	/*
> -	 * The HASHED check is racy. We may end up with the occasional
> -	 * unhashed entry on the LRU, but they should get cleaned up
> -	 * like any other.
> -	 */
> 	if (test_bit(NFSD_FILE_GC, &nf->nf_flags) &&
> 	    test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> 		/*
> -		 * If this is the last reference (nf_ref =3D=3D 1), then transfer
> -		 * it to the LRU. If the add to the LRU fails, just put it as
> -		 * usual.
> +		 * If this is the last reference (nf_ref =3D=3D 1), then try to
> +		 * transfer it to the LRU.
> 		 */
> -		if (refcount_dec_not_one(&nf->nf_ref) || nfsd_file_lru_add(nf)) {
> -			nfsd_file_schedule_laundrette();
> +		if (refcount_dec_not_one(&nf->nf_ref))
> 			return;
> +
> +		/* Try to add it to the LRU.  If that fails, decrement. */
> +		if (nfsd_file_lru_add(nf)) {
> +			/* If it's still hashed, we're done */
> +			if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> +				nfsd_file_schedule_laundrette();
> +				return;
> +			}
> +
> +			/*
> +			 * We're racing with unhashing, so try to remove it from
> +			 * the LRU. If removal fails, then someone else already
> +			 * has our reference and we're done. If it succeeds,
> +			 * fall through to decrement.
> +			 */
> +			if (!nfsd_file_lru_remove(nf))
> +				return;
> 		}
> 	}
> 	if (refcount_dec_and_test(&nf->nf_ref))
> @@ -594,6 +608,10 @@ nfsd_file_lru_cb(struct list_head *item, struct list=
_lru_one *lru,
> 		return LRU_ROTATE;
> 	}
>=20
> +	/* Make sure we're not racing with another removal. */
> +	if (!test_and_clear_bit(NFSD_FILE_LRU, &nf->nf_flags))
> +		return LRU_SKIP;

The other return statements in nfsd_file_lru_cb have tracepoints so
that garbage collection activity can be observed. Can you add a
tracepoint for this new return as well?


> +
> 	/*
> 	 * Put the reference held on behalf of the LRU. If it wasn't the last
> 	 * one, then just remove it from the LRU and ignore it.
> diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
> index b7efb2c3ddb1..e52ab7d5a44c 100644
> --- a/fs/nfsd/filecache.h
> +++ b/fs/nfsd/filecache.h
> @@ -39,6 +39,7 @@ struct nfsd_file {
> #define NFSD_FILE_PENDING	(1)
> #define NFSD_FILE_REFERENCED	(2)
> #define NFSD_FILE_GC		(3)
> +#define NFSD_FILE_LRU		(4)	/* file is on LRU */
> 	unsigned long		nf_flags;
> 	struct inode		*nf_inode;	/* don't deref */
> 	refcount_t		nf_ref;
> --=20
> 2.38.1
>=20

--
Chuck Lever