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:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Neil Brown <neilb@suse.de>
Subject: Re: [PATCH v3 3/4] nfsd: close race between unhashing and LRU
 addition
Thread-Topic: [PATCH v3 3/4] nfsd: close race between unhashing and LRU
 addition
Thread-Index: AQHY6v8bPfwg35VNl0uAHle+vjwISq4kNwiA
Date:   Fri, 28 Oct 2022 19:50:21 +0000
Message-ID: <08778EE0-CBDC-467B-ACA6-9D8E6719E1BB@oracle.com>
References: <20221028185712.79863-1-jlayton@kernel.org>
 <20221028185712.79863-4-jlayton@kernel.org>
In-Reply-To: <20221028185712.79863-4-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?L38skDxZYrMAT/fo+5gqt9A8IjmCuwH4xmV3kCM69fY7xwFjNrCQ0fEcZwdB?=
 =?us-ascii?Q?wC/oWuso+g9xmjB8GYE+j1IDidE3bxizEOJ0rfaagRQCAVimcVxUkFVA+1CT?=
 =?us-ascii?Q?p51zXzWDkW1MEMO717uJv50o4+2UXE/IRc8LWIWRAWYqx4ZNwHH2hBppFy6o?=
 =?us-ascii?Q?LU1fsTMkPcRqAL04X+TJJwOXTqODx39s7HGT2l0/oGnzDs5yMXKpr9p9HZeo?=
 =?us-ascii?Q?ODBA7pgmsGepuc/sdRqU+Fv49aQdskrFyE6jF6xyDPZvuMO4EpFE366WMqK4?=
 =?us-ascii?Q?t0zaH3AtazbOhGxEkLcv4OW6Qs6Q7yvN5XXOsxFDA7e5jpRVUn0rE7x1kRM0?=
 =?us-ascii?Q?JZHIBCW7vccn1GJJ7zmiaDXCUBLb9HIuC645ijZkVNXrGS+D4SVodWV6RTo6?=
 =?us-ascii?Q?3DX5pp9PiI6Kk32J5eoU9kyiABXsncxk/cR6dzw+6lmeV2P/2wpjL5jc2tYF?=
 =?us-ascii?Q?IiMwM1QPp9DqAx295FqdyenCupgWmc1Hs7ZQNAyrf/9PjC/QsRGSdtDUgx1b?=
 =?us-ascii?Q?769cqyHMUn/ucSBKYvdlJcFCbFfDR/FxZeMdUoMtSOkP4coHlUOkBdTky3MH?=
 =?us-ascii?Q?zMYjDXOV9A55irygQDTx7KoWbv9bIfZPGyTeXNfc21Gk9ITrEW2Y/qRW+0WW?=
 =?us-ascii?Q?R++xQYwOyZ7H2+ojoZfp5sIAlD5acvnGhIrCUFYkX5QGA8It5ioUHoZK8AxT?=
 =?us-ascii?Q?sWrTrKdMT+t0c5mHXLfV6R48ybEIqnZwa60Og0COwU6JQ8P1Abab/tYeVEcD?=
 =?us-ascii?Q?I2lfp3ucWvWSKw2EAhbnHIM9dyZqIJg9c4+Pg/eNYb3uM4Qx9VuXyIG1WBEY?=
 =?us-ascii?Q?kqLm01OJeKG3xv4hh9bcZb6JaiLEwfh6RbBkZ+/qilCC9UXrk33cJ+Y8G2Jn?=
 =?us-ascii?Q?7JGFY8vujC8+CypzXBNr4Z3Wv9Kjxh1Pru6TAa+FUT47QdBJHH3avEK1VHwh?=
 =?us-ascii?Q?4FWkpxR+TO+mIJ6b62G++N/IjuDjjsJsMxCelZQSej1uNKyJMfUa/fMsjnz2?=
 =?us-ascii?Q?mytFSbevJ0i6dfGmVWbuDiFW/wwsPxmH7szekJ97dc134X6ekzdDkqCHTSY+?=
 =?us-ascii?Q?ofWB52zUGHSiMuL3rWls8AjRpr+c4FjCzrv+MQOpipA5AuP13Q50PVcWHGxr?=
 =?us-ascii?Q?Rs1EFxYfghOOZQ39+fx/AyLa0Y3RW2i4tt1Bb1C+1Ar0e6Gn8JUxzeJOWEfH?=
 =?us-ascii?Q?CRhiav1OlYCn4nkoiwLaipl8vIMv4GTfkJ4FE8XdH+1aipXOhowlaIi21WCj?=
 =?us-ascii?Q?ACStQG0JWuD4Em8K8Ta0hsCmG4gto+P+MT4/DPkPkXKEYb8JvgXbdP59ezf+?=
 =?us-ascii?Q?ky+b7f6QZkbfXEAy9R7q841gKUthDo8boQR8F3D2BOkOZdyD8TIIH0voWGOX?=
 =?us-ascii?Q?rDX1tEU6sOF1a9QxzPjoK5EPMlu8IAmnix4dLE5vS01LeHs9czeAMlrpqqck?=
 =?us-ascii?Q?acaSV6Oy6XzroF3Fg6N6xPHHujpJS7444fZVx79EogleuZ7WcmjL+IvJnfhX?=
 =?us-ascii?Q?uig5iOO2JK/U75n4tpOOp3iynkTIhegs1k5GTBbdY/fzwwPe72kJwZ2w9JxN?=
 =?us-ascii?Q?7sCQDoRYttieBYhtD9891pfBkiZ5wBZTnNCIklfGwpFCJLM07Xi6klxjfPne?=
 =?us-ascii?Q?GQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <7A4DBB6F1AEA6742B8803A3ECDBE9FCD@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: 5afc4b5c-b478-4b99-f257-08dab91da61f
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Oct 2022 19:50:21.8335
 (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: qqtRBdiy4MNuzW207VvzhpXFp4actQ8QtPmLUtmV03KfMuqHa8h8TiLXt+1XzD/SdoGCZDm21s6YhuEpLnKBbg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR10MB6086
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-10-28_10,2022-10-27_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0 malwarescore=0
 phishscore=0 mlxlogscore=999 mlxscore=0 spamscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2210280125
X-Proofpoint-GUID: z_zYiE2eqaKBLgC1nIfCkLpUAfAe2nmH
X-Proofpoint-ORIG-GUID: z_zYiE2eqaKBLgC1nIfCkLpUAfAe2nmH
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 Oct 28, 2022, at 2:57 PM, 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.

I've never seen that happen. lru field re-use is actually used in other
places in the kernel. Shouldn't we try to find and fix such races?

Wasn't the idea to reduce the complexity of nfsd_file_put ?


> 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 | 44 +++++++++++++++++++++++++++++---------------
> fs/nfsd/filecache.h |  1 +
> 2 files changed, 30 insertions(+), 15 deletions(-)
>=20
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index d928c5e38eeb..47cdc6129a7b 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -403,18 +403,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;
> }
> @@ -469,20 +473,30 @@ nfsd_file_put(struct nfsd_file *nf)
> {
> 	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))
> +		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))
> +				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))
> 		nfsd_file_free(nf);
> 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.37.3
>=20

--
Chuck Lever