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>
Subject: Re: [PATCH 3/3] nfsd: fix nfsd_file_unhash_and_dispose
Thread-Topic: [PATCH 3/3] nfsd: fix nfsd_file_unhash_and_dispose
Thread-Index: AQHY1QEStfw/wemJJEuDuuhGYIHZOa34W9WA
Date:   Fri, 30 Sep 2022 19:29:14 +0000
Message-ID: <71D7277C-4E90-476F-A381-BD13E264BA63@oracle.com>
References: <20220930191550.172087-1-jlayton@kernel.org>
 <20220930191550.172087-4-jlayton@kernel.org>
In-Reply-To: <20220930191550.172087-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?G9wdp+O5PZuTe99JJrd/vVbj+OhTnzaAnbzZ9ccECxNzRPX/cdOb0PHh15ve?=
 =?us-ascii?Q?zJBXUKpHsIFtIhudhx/OefPQY/GClb9p0oGUDvwaJK6RFiyytunX8liH479E?=
 =?us-ascii?Q?Bi/GtQoucIk0fhZkQbP1DW3XtLwAXUU0xrD5NorOz6xqLJpsTy1T1fwgy952?=
 =?us-ascii?Q?qVT2pGAmj6NqJjewZJzFIaabraIUB+C2cZiJMA+EtMH2B9teFTEARU+pXL2d?=
 =?us-ascii?Q?OLq5Isb5aO5ASa+iBNoZUuAVZ49n7Ps9isbWpZq1YNJV5zsMqPhERJuadcYE?=
 =?us-ascii?Q?BEJ2zyFivUf1LGArewtFKvwiMuAoGtZrZBE5By7aNVZ39DeZKJq92oKEcyi8?=
 =?us-ascii?Q?QMGiwjhwWeZQsnQZ+YdG7gNA3VmnhVCGIhhNqZc66HduMLszsuvmhopwoiEK?=
 =?us-ascii?Q?afO9R64HeAgmq//qQVupmmowZAfFSVSI3VuJ5bdzKmjSDR8IubI2UFkG9ckv?=
 =?us-ascii?Q?IAsrvuVI2yWruKDUNxDCJdn/YgdbJqoKGYGgjLKMR5NxowhfLH57hMydU94F?=
 =?us-ascii?Q?SLBjpgz+r4jn3czRyqESXWMcITWgl0Vp3Z8u7SM7Q0EJy0owx6FD4mnRMvw7?=
 =?us-ascii?Q?skT425qsmLNBQYQhsZbchxgLf94OrGciU+76Xt6Q9kZf2QL+oEvMlGM9jNWR?=
 =?us-ascii?Q?CZ0mGrycZgBzvoyvJB+waIc4QQYwv0yXT26DsOoU6+TACSp14C+lQDlareu3?=
 =?us-ascii?Q?mrXYBsRcmGkfBIk2W6eU1gkgED7/7U5xJ72ZyFwmkU8q2jaZZnB7QpvZ9RMQ?=
 =?us-ascii?Q?daLGhmiPds+1rUB4dWGNv4xOn332av4oj1sJGDv2B9hVGtvm9bZWGVDjMryi?=
 =?us-ascii?Q?lfdBOA+sllQfPyKhwufXr/ej1y5YoPcoUcaGzT1RjKsNeEtQDelL6Aw9zruX?=
 =?us-ascii?Q?enDH/vg5r4GK1xnjWcTGSJ41Ca+8elbY9Hxz8/aQyf0rSdwbxtJsZTF+q40K?=
 =?us-ascii?Q?//fbycuxdCoyWA4Bh0RNIXEuqKg+M5lR9O89dSA7pV66suzcHF7T3cza6CuV?=
 =?us-ascii?Q?jhx9QNHW9sEM/rpQhjx5qWrDA84/gvhqz5ugMvMCJZ5C0LQxAG8O0jrvtwBl?=
 =?us-ascii?Q?5S3bA0mqseIyTldFFJv0SugXenb6DgsN2IKKDo5HNJz7YFw0nJiQRBl7OZ78?=
 =?us-ascii?Q?twKZA2EU30VxsF8ROOugdpV3q9R4fxg670ht+uDoMkjUSwaub7YgTA35FnAV?=
 =?us-ascii?Q?xAuSXwlwbHvL17tHZpJ5p0SLWb/v/gdhcvuC9Ssy+Pv8usLfwC9wGR8TEc/i?=
 =?us-ascii?Q?DOJwY6p0/9rt6XYegLtvljSujP5KqevlNTjzYaw2F755PLT8laiQWNkKgEsM?=
 =?us-ascii?Q?c1thByxuYvLz1bhWIdIuM14UIPtFMV2Ik4mPIq9PpXc11EBO3/K5jEJWfndx?=
 =?us-ascii?Q?Abwx35fidErscNgC0wCH7KxAIckie3MUZFRdfVVfUN5f+LBy4ddh55QlgLjT?=
 =?us-ascii?Q?UCyH3oMr0fIh2E2/bsdqY8hYMR7SBRss7ALhAPvhRc7VICUBbZ1iuGpkR0B6?=
 =?us-ascii?Q?IbKIwml5Ciai69RuElXl08z18DAxq800X1QAlzYMHSg0xVehr/XYIR1Lm7xm?=
 =?us-ascii?Q?T5qh6Kabv2fhL/Pjo5LQAID4y4npAdCFT1pK2XvZQA+5OXyfv2NxCx3DZyXE?=
 =?us-ascii?Q?XQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <70A0228BE95D1743823421E60E22B0D2@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: 316103a2-7f5f-4b1b-e384-08daa31a0ee6
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Sep 2022 19:29:14.0680
 (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: /PnhQF5iyf4xV/qiVN3DGRawimMnPb3lit1yQyAdR9QGLZqdWY16KUegOo3l7U0cAAMpw5MWWfTRBTRT+d9xLA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4462
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-30_04,2022-09-29_03,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 bulkscore=0 mlxscore=0
 suspectscore=0 malwarescore=0 adultscore=0 spamscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2209130000
 definitions=main-2209300122
X-Proofpoint-GUID: -gQr0W7s181vALA_y5W0_c6QuBhof3Ec
X-Proofpoint-ORIG-GUID: -gQr0W7s181vALA_y5W0_c6QuBhof3Ec
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 Sep 30, 2022, at 3:15 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> This function is called two reasons:
>=20
> We're either shutting down and purging the filecache, or we've gotten a
> notification about a file delete, so we want to go ahead and unhash it
> so that it'll get cleaned up when we close.
>=20
> We're either walking the hashtable or doing a lookup in it and we
> don't take a reference in either case. What we want to do in both cases
> is to try and unhash the object and put it on the dispose list if that
> was successful. If it's no longer hashed, then we don't want to touch
> it, with the assumption being that something else is already cleaning
> up the sentinel reference.
>=20
> Instead of trying to selectively decrement the refcount in this
> function, just unhash it, and if that was successful, move it to the
> dispose list. Then, the disposal routine will just clean that up as
> usual.
>=20
> Also, just make this a void function, drop the WARN_ON_ONCE, and the
> comments about deadlocking since the nature of the purported deadlock
> is no longer clear.
>=20
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 32 ++++++--------------------------
> 1 file changed, 6 insertions(+), 26 deletions(-)
>=20
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 58f4d9267f4a..16bd71a3894e 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -408,19 +408,14 @@ nfsd_file_unhash(struct nfsd_file *nf)
> /*
>  * Return true if the file was unhashed.
>  */

If you're changing the function to return void, the above
comment is now stale.

> -static bool
> +static void
> nfsd_file_unhash_and_dispose(struct nfsd_file *nf, struct list_head *disp=
ose)
> {
> 	trace_nfsd_file_unhash_and_dispose(nf);
> -	if (!nfsd_file_unhash(nf))
> -		return false;
> -	/* keep final reference for nfsd_file_lru_dispose */

This comment has been stale since nfsd_file_lru_dispose() was
renamed or removed. The only trouble I have is there isn't a
comment left that explains why we're not decrementing the hash
table reference here. ("don't have to" is enough to say about
it, but there should be something).


> -	if (refcount_dec_not_one(&nf->nf_ref))
> -		return true;
> -
> -	nfsd_file_lru_remove(nf);
> -	list_add(&nf->nf_lru, dispose);
> -	return true;
> +	if (nfsd_file_unhash(nf)) {
> +		nfsd_file_lru_remove(nf);
> +		list_add(&nf->nf_lru, dispose);
> +	}
> }
>=20
> static void
> @@ -564,8 +559,6 @@ nfsd_file_dispose_list_delayed(struct list_head *disp=
ose)
>  * @lock: LRU list lock (unused)
>  * @arg: dispose list
>  *
> - * Note this can deadlock with nfsd_file_cache_purge.
> - *
>  * Return values:
>  *   %LRU_REMOVED: @item was removed from the LRU
>  *   %LRU_ROTATE: @item is to be moved to the LRU tail
> @@ -750,8 +743,6 @@ nfsd_file_close_inode(struct inode *inode)
>  *
>  * Walk the LRU list and close any entries that have not been used since
>  * the last scan.
> - *
> - * Note this can deadlock with nfsd_file_cache_purge.
>  */
> static void
> nfsd_file_delayed_close(struct work_struct *work)
> @@ -893,16 +884,12 @@ nfsd_file_cache_init(void)
> 	goto out;
> }
>=20
> -/*
> - * Note this can deadlock with nfsd_file_lru_cb.
> - */
> static void
> __nfsd_file_cache_purge(struct net *net)
> {
> 	struct rhashtable_iter iter;
> 	struct nfsd_file *nf;
> 	LIST_HEAD(dispose);
> -	bool del;
>=20
> 	rhashtable_walk_enter(&nfsd_file_rhash_tbl, &iter);
> 	do {
> @@ -912,14 +899,7 @@ __nfsd_file_cache_purge(struct net *net)
> 		while (!IS_ERR_OR_NULL(nf)) {
> 			if (net && nf->nf_net !=3D net)
> 				continue;
> -			del =3D nfsd_file_unhash_and_dispose(nf, &dispose);
> -
> -			/*
> -			 * Deadlock detected! Something marked this entry as
> -			 * unhased, but hasn't removed it from the hash list.
> -			 */
> -			WARN_ON_ONCE(!del);
> -
> +			nfsd_file_unhash_and_dispose(nf, &dispose);
> 			nf =3D rhashtable_walk_next(&iter);
> 		}
>=20
> --=20
> 2.37.3
>=20

--
Chuck Lever