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:     Neil Brown <neilb@suse.de>, Jeff Layton <jlayton@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 2/3] nfsd: only keep unused entries on the LRU
Thread-Topic: [PATCH v2 2/3] nfsd: only keep unused entries on the LRU
Thread-Index: AQHY6k5kuvFkReNVOEunPzGZ9l7Pra4i0CqAgAAHXoCAAAI1gIAAD+cA
Date:   Thu, 27 Oct 2022 23:52:11 +0000
Message-ID: <AFE97DFF-1E60-4904-9AF3-092894428A0B@oracle.com>
References: <20221027215213.138304-1-jlayton@kernel.org>
 <20221027215213.138304-3-jlayton@kernel.org>
 <166690925944.13915.14734120966513564215@noble.neil.brown.name>
 <dd3936feb109857040ad79e7da47d7b7e5732a41.camel@kernel.org>
 <166691131566.13915.14122113998554165159@noble.neil.brown.name>
In-Reply-To: <166691131566.13915.14122113998554165159@noble.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?vEQzX70uwOCs463Y6hnPT2ASggKF18WB/ICRZr0nBNH/o3xOHzDq3PnipIBP?=
 =?us-ascii?Q?lABMNW33CKP6vuAuoKcxatF6sl2AT6oJv9gzpC5o3XnZF3YtOqr+fwwSH18L?=
 =?us-ascii?Q?PPlm5cHVh2IotDisDVhC2omLxw87FsyDlha40zZkJG4/2TN1O2wJcceNyHRV?=
 =?us-ascii?Q?DAcJGIhiX2nFpM47S3leE3jQswM55iDR1xTinuNrYOURHLWaOjKIM26obwmw?=
 =?us-ascii?Q?GGOEZ2ir65xdKnyjY1iTxZBYpZBRTJtkMZ6Q99P/640sjrZJDMHo2aKk2uFg?=
 =?us-ascii?Q?A28GldLsLsASAWouSOLCvn6gNoymOW/aakkWLBdxNTHomNMJ9RgZ3r4xXwD+?=
 =?us-ascii?Q?7kHkusQvu3gpQsVjqLS2SD57cRKpH8qQB1dWat5DLSlxnyNQ0W3IuH1CQIt8?=
 =?us-ascii?Q?pXrSS5NgPhea+dZCqhJCvwP2EiT8nZfk9YrOwy6VEux9Js5Z3anqEvK02w9X?=
 =?us-ascii?Q?dek/5Ks1p2Nw2LlQekDyqBYNBiZa61aCZ65L2K4sRy+TLudz2ieLl7eFreNU?=
 =?us-ascii?Q?7DaXJ60EePvDDqqaeDNi7UPE6JlH3sCeOm02aYoXOVo2MgXjPRv1yFZor4lu?=
 =?us-ascii?Q?K7BuPkvZLHAOCP8ky1+O5YnzRER/cyLw9PMrPn1t3GbYlN84ugbFVFco9pzh?=
 =?us-ascii?Q?3MKgtvA1AX8NyzWBsdXwJtsa5QUFIiLyjfnTjcKH9ps0haUZMvtg0rJF/7Tb?=
 =?us-ascii?Q?RFBFLavu5QX5aKduouQPFXm9cWpl+OTT87+ipL2tC+NC7mpEurXhnmcpQDAo?=
 =?us-ascii?Q?Wr2JLe0iSf+xMf055lijTPmcNJEmxe0Aecj4fgzd/skE4WumOWbyfhqTjOUc?=
 =?us-ascii?Q?HaAmfZQGk1c3mQgS61nWFamSUlR5tyBGDiJoUYwZraAOa6Vvwc8/R4xiaufO?=
 =?us-ascii?Q?4z5J9zn7G7Ao8e4iJIMxg5jcxqkMxsRTwrrbdydXBY63Kxbx/6kmmilYYzj1?=
 =?us-ascii?Q?t3lqM4FfpUZ0FSmBAP6Nd2S0A9/Kx7JIghPrcGQwZw3kftDcKHjUJCMcpV8H?=
 =?us-ascii?Q?qvjXhouFfn2WqAS73HIuE6ByIHjH7MLKB56ECYxEFsyZV+1hMyde1OI2fIcF?=
 =?us-ascii?Q?YDJNrgt1485clC+Flw4/xorZDD5q/yNKjdkH4Sehdg+9RVDNEUtP2JtxhjIp?=
 =?us-ascii?Q?TZ2LgPj6+FTRGDoAr7hl4xsy2eeQsIxceQSNYRTfmDQQOHweLKyBusNhCU7j?=
 =?us-ascii?Q?cxMOvQn9vJ/Yei3oEEv2dgOXJCweT7RnyoT1VPAMCUrFW7/wFlCYlxaobZsr?=
 =?us-ascii?Q?7gWyueFtzKowjY6K6HvW2ys0wHU4iCgESrXip0p93FRPRMZ3FAq6BrHGqz5y?=
 =?us-ascii?Q?QNSe4XAxp+RJnL8Jaojzlmhnkvjnrv1pWAtba1VwgW/3yEQpS00rr9RSH1Lv?=
 =?us-ascii?Q?sw+DpQ9s85HAG84adXeL/P4V/nf8bGnYYIByPo7AYOQJQDhXJK7MSFrRYV4L?=
 =?us-ascii?Q?Jh/6YaFkPLeo0cSidXbbKxqPHq47Mx1NH4VU/sDElBUG2Nnq9Mk6pAnITQoy?=
 =?us-ascii?Q?ytjFrYMWS8TTUZnIYNwzKd+h4Q8kRh4gj9LP+CGWOLUQwV7d/4Q1NMY39kCn?=
 =?us-ascii?Q?6iMhRzmST7KyLqUnXGH2AQk1ydrOyonXlk72flF4602gZkSThHpWfjuG77Bt?=
 =?us-ascii?Q?4w=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <14043F8BBF525F499EA799419295589D@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: 118b6bb6-6840-43a9-7ed7-08dab876441e
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Oct 2022 23:52:11.2504
 (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: QrG/2h3ftqIrQkiH84gcnZcHj79SRytwPO/lOnaH4uE/TfPRUpO2449Tr4rumH9FcSyVotFWHe3LalFpQB666A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB5947
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-27_07,2022-10-27_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxlogscore=999
 suspectscore=0 mlxscore=0 spamscore=0 phishscore=0 malwarescore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2210270135
X-Proofpoint-ORIG-GUID: YpZqVcGHUi-eHE1nXnjOSTtiKDOP4esE
X-Proofpoint-GUID: YpZqVcGHUi-eHE1nXnjOSTtiKDOP4esE
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 27, 2022, at 6:55 PM, NeilBrown <neilb@suse.de> wrote:
>=20
> On Fri, 28 Oct 2022, Jeff Layton wrote:
>> On Fri, 2022-10-28 at 09:20 +1100, NeilBrown wrote:
>>> On Fri, 28 Oct 2022, Jeff Layton wrote:
>>>> Currently, nfsd_files live on the LRU once they are added until they a=
re
>>>> unhashed. There's no need to keep ones that are actively in use there.
>>>=20
>>> Is that true?
>>> nfsd_file_do_acquire() calls nfsd_file_lru_remove()
>>> Isn't that enough to keep the file off the lru while it is active?
>>>=20
>>> Thanks,
>>> NeilBrown
>>>=20
>>=20
>> After patch #1, it doesn't call that anymore. That's probably a (minor)
>> regression then.
>=20
> Yes, I eventually found that - thanks.
>=20
>>=20
>> After patch #1, the LRU holds a reference. If you successfully remove it
>> from the LRU, you need to transfer or put that reference. Doing the LRU
>> handling in the get and put routines seems more natural, I think.
>=20
> Maybe.  But then you need a __get as well as a get.
> Though it might seem asymmetric, I would prefer removing from the lru in
> 'acquire' and adding to the lru in put.

That's exactly the design introduced by commit 4a0e73e635e3
("NFSD: Leave open files out of the filecache LRU"). I also
would like to keep that behavior -- that's what a real LRU
is for.


>> Maybe I just need to squash this patch into #1?
>=20
> Or do the "put" if lru_remove succeeds in the first patch.  Then revise
> it all in the second.
>=20
> Thanks,
> NeilBrown
>=20
>=20
>>=20
>>>=20
>>>>=20
>>>> Before incrementing the refcount, do a lockless check for nf_lru being
>>>> empty. If it's not then attempt to remove the entry from the LRU. If
>>>> that's successful, claim the LRU reference and return it. If the remov=
al
>>>> fails (or if the list_head was empty), then just increment the counter
>>>> as we normally would.
>>>>=20
>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>> ---
>>>> fs/nfsd/filecache.c | 23 ++++++++++++++++++++---
>>>> 1 file changed, 20 insertions(+), 3 deletions(-)
>>>>=20
>>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>>> index e63534f4b9f8..d2bbded805d4 100644
>>>> --- a/fs/nfsd/filecache.c
>>>> +++ b/fs/nfsd/filecache.c
>>>> @@ -420,14 +420,31 @@ nfsd_file_unhash(struct nfsd_file *nf)
>>>> 	return false;
>>>> }
>>>>=20
>>>> -struct nfsd_file *
>>>> -nfsd_file_get(struct nfsd_file *nf)
>>>> +static struct nfsd_file *
>>>> +__nfsd_file_get(struct nfsd_file *nf)
>>>> {
>>>> 	if (likely(refcount_inc_not_zero(&nf->nf_ref)))
>>>> 		return nf;
>>>> 	return NULL;
>>>> }
>>>>=20
>>>> +struct nfsd_file *
>>>> +nfsd_file_get(struct nfsd_file *nf)
>>>> +{
>>>> +	/*
>>>> +	 * Do a lockless list_empty check first, before attempting to
>>>> +	 * remove it, so we can avoid the spinlock when it's not on the
>>>> +	 * list.
>>>> +	 *
>>>> +	 * If we successfully remove it from the LRU, then we can just
>>>> +	 * claim the LRU reference and return it. Otherwise, we need to
>>>> +	 * bump the counter the old-fashioned way.
>>>> +	 */
>>>> +	if (!list_empty(&nf->nf_lru) && nfsd_file_lru_remove(nf))
>>>> +		return nf;
>>>> +	return __nfsd_file_get(nf);
>>>> +}
>>>> +
>>>> /**
>>>>  * nfsd_file_unhash_and_queue - unhash a file and queue it to the disp=
ose list
>>>>  * @nf: nfsd_file to be unhashed and queued
>>>> @@ -449,7 +466,7 @@ nfsd_file_unhash_and_queue(struct nfsd_file *nf, s=
truct list_head *dispose)
>>>> 		 * to take a reference. If that fails, just ignore
>>>> 		 * the file altogether.
>>>> 		 */
>>>> -		if (!nfsd_file_lru_remove(nf) && !nfsd_file_get(nf))
>>>> +		if (!nfsd_file_lru_remove(nf) && !__nfsd_file_get(nf))
>>>> 			return false;
>>>> 		list_add(&nf->nf_lru, dispose);
>>>> 		return true;
>>>> --=20
>>>> 2.37.3
>>>>=20
>>>>=20
>>=20
>> --=20
>> Jeff Layton <jlayton@kernel.org>
>>=20

--
Chuck Lever