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/wemJJEuDuuhGYIHZOa34W9WAgAAD1QCAAAs/gA==
Date:   Fri, 30 Sep 2022 20:23:12 +0000
Message-ID: <E06BAB8A-42DA-484E-96C7-EE7A9254476C@oracle.com>
References: <20220930191550.172087-1-jlayton@kernel.org>
 <20220930191550.172087-4-jlayton@kernel.org>
 <71D7277C-4E90-476F-A381-BD13E264BA63@oracle.com>
 <649ea8a13435734a54fc6755bd6599c2cacc3a53.camel@kernel.org>
In-Reply-To: <649ea8a13435734a54fc6755bd6599c2cacc3a53.camel@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?JNDqRxROsy+HYaEgGxjryKnL3zJ8n4Gq1Y7ginoI6Q+UzJoAqJznjhB9Y1Bb?=
 =?us-ascii?Q?Ht08eagh20GG8X3bFgHIUwU4pdnAn6mo4w5FyJdOIt3TdziWPwJS/z5tvk07?=
 =?us-ascii?Q?HwDZNyv67kQ5uMR5LDcNfZ/MfVAlz87rTHeNBjaJTW1rlPMJB6/aq9W5EKln?=
 =?us-ascii?Q?4qsFue8XBWaklgEMvdOD8IV9KAbq+GNWI3rG0EnaoxOTDm3wdd1QN6UbAXQK?=
 =?us-ascii?Q?+m4s+X2fEqsId6Ns6jyhU9wcNCiTThWp5htI/6HF3kAAvcCdp6ZsPGBCgHEA?=
 =?us-ascii?Q?kcsoBcMYSd3Gi1f9i7d8k2xJBCJRJm7yF7OJlM6qEnoE3CYf8CRLUovw31an?=
 =?us-ascii?Q?oQ3oz6fdvxjFWYz0Sy9gZfbLQvfgJ5YhRJ9WkE5h7PfnO4ik4BbTxMiDUIdD?=
 =?us-ascii?Q?FW5s+GW4seqlfhf9Y7zTOtcyQ/uAnfTktl+SwvqTaZtNGQ4ryhF5+Yq4u2Qw?=
 =?us-ascii?Q?SjVdJoneJd5LjVKI4LolMMqq47S04CAqoRnUh5IBNDL2czALJDAE63ZDNF7m?=
 =?us-ascii?Q?goVyHmFtWQS0017rURl47XRa7v9kvSiYJUugVJ42jw/t9L/dMdHaPmgC8DjL?=
 =?us-ascii?Q?u76Vl4h4rTX/gspqO5E8U9pacGwOlJwZ2wTqIPtBobnbqQQxEuxMLsv065fj?=
 =?us-ascii?Q?I3sCcwScZFMj4aE/kJqvuEXHyKclKkFZQdYNG8r2LszR0tCifeJkscCt+SaX?=
 =?us-ascii?Q?nWRgU8otWqcXFe+7D6LWPTIeyKZ3GixZ+cRXw1cl8WU4RvgSGrPP2BoWWYO+?=
 =?us-ascii?Q?wt1NqqnR8Jm/v8jpi4wVldtTtcRGVauE+NV8NR7+OLcDKSSVm53kDEYqwJLI?=
 =?us-ascii?Q?eeuT6/xjHGYFoeXAaYNKwHKpii0t1BYAPo4xH1lvce/sdAnlhSuWghk1yICA?=
 =?us-ascii?Q?apCtonyS/iHhDlQWRzXqWhKN4XscDN6TeE9mGOyjJFgmf52srRwG2UCZ9MFS?=
 =?us-ascii?Q?ZpIdv8b6rWXbsKJfiT4hI3fgMw6cCLw1piEQ9zdH1HwqqXyUusmt+Txg4EeB?=
 =?us-ascii?Q?gye9gcQW7xhT1gZv5NCGfbvgB+6tWUQKDaEUGu/cIyqCOGiyhsg/x7GulDxv?=
 =?us-ascii?Q?QeOtXss97sCPFGpsLlKeUIHW4I6WgsBLXGb/zNKImjsX6Qxa/WWXYOdSIoV/?=
 =?us-ascii?Q?i2GqGcpfNs8BkHWNy6zoG+Uq/rw49kRGfCRsq4dDkvPZa0rqC8Adx1NntvrG?=
 =?us-ascii?Q?rWJrT8JyQqurN7Kj5nt3YwuIo1KuhuMTMg/mjzZLsZQjL1tovDgUEtr7MqCl?=
 =?us-ascii?Q?hGcwh7jPDzqrICPR5OVwgGVxaJleihh3siQteRf7iHJvwH/h2PgnVNAjsF0k?=
 =?us-ascii?Q?6/dPRhBtDvA3SzOQqUe37vgD+6/PO0NIT7pOsbDYr5x2rqvWQ2dqO/nsHfOq?=
 =?us-ascii?Q?yw8kkp8mnQz70qAwNsdVL/zHi1LLUc60vDtyo8HOu/v+XiNPL2W+S6VJip60?=
 =?us-ascii?Q?pftjW8Z93Qz+UCinB6SQ9tKwfBmggqgNmx1FSGHpEeLFp04y7W4I8B69TIAy?=
 =?us-ascii?Q?CfVlInCt1mTksWdMGK9SqjMuZwR/NBoQOxNRf3jkJYcIefqnJBctk8q247AW?=
 =?us-ascii?Q?89kOMzVN7C0KI91qv6Zyk+3ba+Cy2eMLIDVByTomaJDYhGfoCgZO1W9GZGKW?=
 =?us-ascii?Q?YQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <8E0F4365F77DDC49B63AB352E4DD6020@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: af17db79-8481-4819-9e85-08daa321993d
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Sep 2022 20:23:12.5922
 (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: Ij8U5S/oriNexg4+/4EO6ecDCQIk9YfRmT9ZpLJ6mqQORHRgldIANA+AywGeR0nQ4hRZSIaQKK7z4U1d5OQmRg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5318
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 malwarescore=0
 suspectscore=0 adultscore=0 mlxscore=0 spamscore=0 bulkscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2209300128
X-Proofpoint-ORIG-GUID: rUNXnRX8GQyq858AW0mFFGBICzCZsvF6
X-Proofpoint-GUID: rUNXnRX8GQyq858AW0mFFGBICzCZsvF6
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:42 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Fri, 2022-09-30 at 19:29 +0000, Chuck Lever III wrote:
>>=20
>>> 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.
>>> */
>>=20
>> If you're changing the function to return void, the above
>> comment is now stale.
>>=20
>>> -static bool
>>> +static void
>>> nfsd_file_unhash_and_dispose(struct nfsd_file *nf, struct list_head *di=
spose)
>>> {
>>> 	trace_nfsd_file_unhash_and_dispose(nf);
>>> -	if (!nfsd_file_unhash(nf))
>>> -		return false;
>>> -	/* keep final reference for nfsd_file_lru_dispose */
>>=20
>> 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).
>>=20
>>=20
>=20
> How about this?
>=20
> +       if (nfsd_file_unhash(nf)) {
> +               /*
> +                * Unhashing was successful. Transfer it to the dispose l=
ist
> +                * so that we can put the sentinel reference for it later=
.
> +                */

Right idea, but I would say nothing more than "nfsd_file_dispose_list()
will put the sentinel reference later."


> +               nfsd_file_lru_remove(nf);
> +               list_add(&nf->nf_lru, dispose);
> +       }

I was staring at this earlier today and thinking it needed clean
up. This looks right to me.


> In this case, we're basically transferring the sentinel reference to the
> "dispose" list. Later, we'll call nfsd_file_dispose_list and drop it.
>=20
> Now that we don't have such onerous spinlocking in this code, we might
> be able to just put each reference as we go instead of deferring it to a
> list and putting them all at the end. That's probably best done later in
> a separate patch however.
>=20
>=20
>>> -	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 *di=
spose)
>>> * @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
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever