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:     Ruben Vestergaard <rubenv@drcmr.dk>,
        Torkil Svensgaard <torkil@drcmr.dk>,
        Shachar Kagan <skagan@nvidia.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] nfsd: don't free files unconditionally in
 __nfsd_file_cache_purge
Thread-Topic: [PATCH] nfsd: don't free files unconditionally in
 __nfsd_file_cache_purge
Thread-Index: AQHZLQi3y7Mea6cMiE2cfMbyen4b566nv26A
Date:   Fri, 20 Jan 2023 20:21:18 +0000
Message-ID: <C04F7C2C-848A-47C5-9C4E-9B9E5E82BAFF@oracle.com>
References: <20230120195215.42243-1-jlayton@kernel.org>
In-Reply-To: <20230120195215.42243-1-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?lDAcbRO+mgo1+/c7vSdguIjqTj5s4N/kwpU+cKW5q5ergfyKuw9lb0R7pWBd?=
 =?us-ascii?Q?iXicv7SQHb4ZqQht5Np2SRqAuceiR/u4Rv3A8TK9cEJXtkEWXPvOLneEF/p9?=
 =?us-ascii?Q?R16xzZ1qKS8ZxyA7I68hA7IEdqnZBb7ijvFM/cDfQnyL7g9lGSlm6tr1tz0k?=
 =?us-ascii?Q?6YPr5B1jcjVE5KtTk/wtnefB6pHPudG6Hu5iatIe0ucPu9k0VIw+3JFc5yOt?=
 =?us-ascii?Q?t5WFBLpKFNG5zxsyG6bQnlUhEKq2rQgnDvhfR1bF66htyY1mr/kyDEY3BgPR?=
 =?us-ascii?Q?P3Ja1mbkzgctDwNpJ+GH3zUr7a8hzPiO3akwQgIKBQtX28yJeMAyNv/Rq6c5?=
 =?us-ascii?Q?9R22zhQrk3+DqhEWjsnck8lDnWRsTBTOPosOS96twmHa0EEk6BfenhCnrrQR?=
 =?us-ascii?Q?r09cM8T1x3w7qZbAO6doO11PbF6+XDHCHNxELweysUWbRnK8sQv/QrWBBCFM?=
 =?us-ascii?Q?FeE0sc9SMLSuKYE/HITKR5KYCKiQA7zbMgqIxn6nToL4lnbza/hVHka30v+/?=
 =?us-ascii?Q?V5EDo7dgIEt2zhwVNiPjl37RFG/p40oqk7Y+DuBBX/dik7FiUTesZxsD0kuI?=
 =?us-ascii?Q?K5OuK+WDfX65mtxLlhSZZ4M9tYvWWIh9x9g//JMtcT/qBM13WktXeBzmUlCP?=
 =?us-ascii?Q?WvzWwCVkxfTlgXtzmwy+dYgv0se+3388DIgRT3NFAdJoObznMT5wKiAwh3F3?=
 =?us-ascii?Q?dPotcVs0auVPKTYQne1oRiQXctgP3sk27LcE9YdB9FMhPQG70LlgV7LRLfq/?=
 =?us-ascii?Q?rHNsbqf3N26swEzy3kwbDctvoDPK4fdHOuh+1swDX3kAnllMpz3sLWZEfsNc?=
 =?us-ascii?Q?V3IHb/wX6v9Pewxhio3/NQ+k9EnQzkY+UxFeadywT+Wj1wPw90Xggs0gmZZx?=
 =?us-ascii?Q?b2od5tRZu4L1VkNH1UdhYsLq50TmHOmjzJhY//FOFHNmjdrL8vAf9h34fMmw?=
 =?us-ascii?Q?EplkWiHqrGLnCD7qvz0moEsFC/HRHx8bLpbgD9EzZQ9Tauim1rfZTx7Ts5vz?=
 =?us-ascii?Q?1MkV8gnUdawMI7DUVLaKsCy8GQjBf2yL/06mSKvzdJhz8t7M4sX3FGF9V1S0?=
 =?us-ascii?Q?7LcpUMLIm2htmKnAWRDswt+zlYO02Xd3yWG95oeF/bfbIW5BnIDGaxYsZJHM?=
 =?us-ascii?Q?NLhyHgMtmruLTd512kHhftFkfTtDadvRWWseRfQ4zsukQF5lvu5HzaigJqmX?=
 =?us-ascii?Q?9Rjqs9QprQoQijg8m4W0P5ZJhxvGrAGiGtkt3ynNQiD4yeQ3zmaVFTL7u8CR?=
 =?us-ascii?Q?4GAvnOajVAPdAnlcjy9PJThREeZXmli4okzgqxkGIPcjoD9UBGocgiF45eXj?=
 =?us-ascii?Q?iyq427K6s2NelRuzU79upWeQNgZBHHu2f148zcmB1wpk9mq05imoqJHVNdnr?=
 =?us-ascii?Q?K6/1Ek15Ln/OFht2dreGzDpBomEXrlGM5aUkDjx3Jed3CuCzAFI4ZpgXq+R2?=
 =?us-ascii?Q?mhyrGKqCOdBjtAI1ElSPesDx2G3z90iMKxg3tMQb3gwASXU0jDDOEEJ3Rnn4?=
 =?us-ascii?Q?d1vRNuxKDzLTRVjnc29GrOOKJbZTrDS4Jkm2yzJ6bbKr+0g0kC+d+N0psIgy?=
 =?us-ascii?Q?BmD8Iv+AFu45msNPVsedrHS0Oxb2GEOtzPVyYFR7VzwOC/Vht8sV5jvkLpFb?=
 =?us-ascii?Q?xg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <8FF5F8FC603A1241BD02A0B016FBA93E@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: H+ZtCl0CLaBH+Lep7ivWuNNjQrtmD2rHqbptY7//LGxAUz6V/Rb9S9x+U+bxrNUu2nZIPFD1QP1fkK3oBo6enR+Jljf5G6omW7iucvLE4U/9CkIEcBz3xzAQzf3SFN/Yhga9vzJYA6YLeDn62x+g37aMbIbv26a5GSJreT2NahJrocPQk9sikZfTwBbogL4XsJ1bho1oj9Ck7DnqykSLnTlJXTAOYdcxGdNF9rFytED2GmNRWN2uRczDq1uqaL6RZmoNZC9zPi4hwc1APYRHDl3V7gBGpceTzk9IfJcaNuADX+EWT6jH8JfMsHQ5b6c7+u4WKdmhHdxJoNz+8G4UwKrb00+NW8aEEORjymZYnY/dxDrILBY0Qqgj/lgyK/z/wJ5XwMTUCGrA4CB6TpglHIKhdMdLA88efCxkjLJ9peEV68iGecnbBEyiTHnJ6WpZOdwDwOEUjGNfbS7bySdH+kMiCAJKq4a/a3WnRN50T8cec8tPOOrKa+pgD/PKApjJJi9GVws68bveK24OQGXDlkPS3kyADPS1tMQbSaYJnL/j7UcSMLHNve9Bsv9t+W5RylGcQgSuIxaQ32hFJDVdOYd4lXxCvYguDUvMzIY94H9oLOJZfvshoascuVMNBNtfrB/3B4/Q4z0JbKsCgrjBq5jbfrZB2n8o3HHCDGDu/TWgv3rjBzpzQGPLT55S6bVztuboexAhdxqwz1R12PRcx7zKwaX9n0VrXfO7o4W6XkWBpaCzUzhvcyr0j9ViywuJZH0gJtjHCzej9coQjX0L40EdAWoLnQLqJZBF1V8we48Dz+6R/g+HOQaeu8ucmHtFzF3v6e/LN024vobWZ1ExBw==
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: 68bf456c-f5fd-43a4-4b99-08dafb23e3b8
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jan 2023 20:21:18.8811
 (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: xZ4d63cbncsEEaiXClAbQqlkd0OcHRqViAKTEOzz5F1pkP7Kzs0SWkA02/yZM+5dj8PXfiGz6BCP+qyvmRvZFg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB5819
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-01-20_10,2023-01-20_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0
 phishscore=0 adultscore=0 mlxscore=0 mlxlogscore=999 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2301200193
X-Proofpoint-ORIG-GUID: t7OVUMigBQg3yrcGw35yz6sa7ZhTUfxT
X-Proofpoint-GUID: t7OVUMigBQg3yrcGw35yz6sa7ZhTUfxT
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 20, 2023, at 2:52 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> nfsd_file_cache_purge is called when the server is shutting down, in
> which case, tearing things down is generally fine, but it also gets
> called when the exports cache is flushed.

Yeah... cache flush is probably the case we've been missing.


> Instead of walking the cache and freeing everything unconditionally,
> handle it the same as when we have a notification of conflicting access.
>=20
> Fixes: ac3a2585f018 ("nfsd: rework refcounting in filecache")
> Reported-by: Ruben Vestergaard <rubenv@drcmr.dk>
> Reported-by: Torkil Svensgaard <torkil@drcmr.dk>
> Reported-by: Shachar Kagan <skagan@nvidia.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 61 +++++++++++++++++++++++++++------------------
> 1 file changed, 37 insertions(+), 24 deletions(-)
>=20
> v2: use the same method to purge entries from the cache as we do when
>    there is a notification of conflicting access.
>=20
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 58ac93e7e680..397ae212b98d 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -661,6 +661,40 @@ static struct shrinker	nfsd_file_shrinker =3D {
> 	.seeks =3D 1,
> };
>=20
> +/**
> + * maybe_queue_nfsd_file - try to unhash and queue a nfsd_file to be fre=
ed
> + * @nf: nfsd_file to attempt to queue
> + * @dispose: private list to queue successfully-put objects
> + *
> + * Unhash an nfsd_file, try to get a reference to it, and then put that
> + * reference. If it's the last reference, queue it to the dispose list.
> + *
> + * The caller MUST hold the rcu_read_lock() !

__nfsd_file_cache_purge() isn't holding rcu_read_lock(), it's
holding the nfsd_mutex. Is this comment incorrect, or is it just
too specific? Or did I miss something obvious?


> + */
> +static void
> +maybe_queue_nfsd_file(struct nfsd_file *nf, struct list_head *dispose)

I prefer the name nfsd_file_try_to_queue() or nfsd_file_try_to_dispose().
nfsd_file_ should be the prefix where possible. Unless you're
redriving, I can fix that.


> +{
> +	int decrement =3D 1;
> +
> +	/* If we raced with someone else unhashing, ignore it */
> +	if (!nfsd_file_unhash(nf))
> +		return;
> +
> +	/* If we can't get a reference, ignore it */
> +	if (!nfsd_file_get(nf))
> +		return;
> +
> +	/* Extra decrement if we remove from the LRU */
> +	if (nfsd_file_lru_remove(nf))
> +		++decrement;
> +
> +	/* If refcount goes to 0, then put on the dispose list */
> +	if (refcount_sub_and_test(decrement, &nf->nf_ref)) {
> +		list_add(&nf->nf_lru, dispose);
> +		trace_nfsd_file_closing(nf);
> +	}
> +}
> +
> /**
>  * nfsd_file_queue_for_close: try to close out any open nfsd_files for an=
 inode
>  * @inode:   inode on which to close out nfsd_files
> @@ -688,30 +722,12 @@ nfsd_file_queue_for_close(struct inode *inode, stru=
ct list_head *dispose)
>=20
> 	rcu_read_lock();
> 	do {
> -		int decrement =3D 1;
> -
> 		nf =3D rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> 				       nfsd_file_rhash_params);
> 		if (!nf)
> 			break;
>=20
> -		/* If we raced with someone else unhashing, ignore it */
> -		if (!nfsd_file_unhash(nf))
> -			continue;
> -
> -		/* If we can't get a reference, ignore it */
> -		if (!nfsd_file_get(nf))
> -			continue;
> -
> -		/* Extra decrement if we remove from the LRU */
> -		if (nfsd_file_lru_remove(nf))
> -			++decrement;
> -
> -		/* If refcount goes to 0, then put on the dispose list */
> -		if (refcount_sub_and_test(decrement, &nf->nf_ref)) {
> -			list_add(&nf->nf_lru, dispose);
> -			trace_nfsd_file_closing(nf);
> -		}
> +		maybe_queue_nfsd_file(nf, dispose);
> 	} while (1);
> 	rcu_read_unlock();
> }
> @@ -928,11 +944,8 @@ __nfsd_file_cache_purge(struct net *net)
>=20
> 		nf =3D rhashtable_walk_next(&iter);
> 		while (!IS_ERR_OR_NULL(nf)) {
> -			if (!net || nf->nf_net =3D=3D net) {
> -				nfsd_file_unhash(nf);
> -				nfsd_file_lru_remove(nf);
> -				list_add(&nf->nf_lru, &dispose);
> -			}
> +			if (!net || nf->nf_net =3D=3D net)
> +				maybe_queue_nfsd_file(nf, &dispose);
> 			nf =3D rhashtable_walk_next(&iter);
> 		}
>=20
> --=20
> 2.39.0
>=20

--
Chuck Lever