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:     Neil Brown <neilb@suse.de>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v4 3/7] NFSD: Add an NFSD_FILE_GC flag to enable nfsd_file
 garbage collection
Thread-Topic: [PATCH v4 3/7] NFSD: Add an NFSD_FILE_GC flag to enable
 nfsd_file garbage collection
Thread-Index: AQHY4ywL0+TmiIKEZUCtCfoSHjd9zq4c28YAgADxXACAAAjagA==
Date:   Mon, 24 Oct 2022 17:29:27 +0000
Message-ID: <07483E5C-D990-4D14-95C9-CE9CDF782CC5@oracle.com>
References: <166612295223.1291.11761205673682408148.stgit@manet.1015granger.net>
 <166612311828.1291.6808456808715954510.stgit@manet.1015granger.net>
 <166657883468.12462.7206160925496863213@noble.neil.brown.name>
 <3d86628c75009a4feb3d20804e6f190dee8b83a3.camel@kernel.org>
In-Reply-To: <3d86628c75009a4feb3d20804e6f190dee8b83a3.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?nk6JxzcUIvxdipCqVC44DxeO9Cmx5EGlHRoY10h9eDFB+enFc6rzdTEde6f4?=
 =?us-ascii?Q?MQLcotSqBbSK2VOT6KVPIGLvQjzWXHxfXBmB8HbiHilLTHXCklZXo8fc8J4Q?=
 =?us-ascii?Q?eVowo3rnLjxkGcPVqVxtMjzKAQxCnN510e+yFNRFHPtR5rfi2b/VF0k5Lkz1?=
 =?us-ascii?Q?5c7VbAvw3kZups/Lwz9lQCQZciEZ35PA1Q2r8tLSVxz9cVVwTkRW8buwLYap?=
 =?us-ascii?Q?Y02FwEP2LtxZ7LJclTEIW0j3Q7hOL5u3kTakg+jisTyDJZdSRduvk9SfiXJa?=
 =?us-ascii?Q?nCWNhRLvgHuqRu2M5445tWcuMN+YhagRXCzYbfVQ90Wat1phgusuXRKoutgq?=
 =?us-ascii?Q?l77sM8FB/1Mf16ipoZrLGc8URaUHgyWiA37+eqiwKcTDxthq2taifTotjwxd?=
 =?us-ascii?Q?P5S26F5U1RoOFVR+BaTA0rVzFSUgl9TJ1e/Cq3ByvnRixNAjKs+bgNgZMOI8?=
 =?us-ascii?Q?lBICWMIvhLuF4DR53CjFDir6voZnErD42uOJ2vpybWK4K6vP7Uw0rNZ3bZDD?=
 =?us-ascii?Q?IoFAtf2T1KPU8sxCiJCGRUO77KSE+mEmxvh0zNWMWCHt64jMVDceGvMPNiph?=
 =?us-ascii?Q?iLBkAA5Ibl6gSAoJjvGszURsG5gmfpGi/gksDTpAQ1/rt3VTvb0rHmd4LiNj?=
 =?us-ascii?Q?oHiw1lEM3sCi3xWOMsfluGnU5/YuGYUbWWmlHGaFf8gorYHuJltlTzZ6cEPM?=
 =?us-ascii?Q?h/0ODvktPcisdicffE4+ludi0iQPB90osh9HKK0W+JZXo68mWlEFNbobmLgf?=
 =?us-ascii?Q?PlghvFCmfgH2puadHABcESp8PFZFLjJmhwrM1E6gnQza/3LsKz375C368vEh?=
 =?us-ascii?Q?mC+gEiJmZ17hRyl3zhYVRDuDHkagYWCKAgSLf2yl4IK92r6KsIJcf4254HwE?=
 =?us-ascii?Q?tmCJAzZI+DxlszqyH8008+HOrI3HADdXUc1WUlJdLRjc78+zSNaIJkgTunsd?=
 =?us-ascii?Q?lteDgU+TJyKHjmxatTR2eAAvaiux70YV3pskR2I9i3IOOGBu7h/8s2oFRCYZ?=
 =?us-ascii?Q?vD13ZSfStemtSCThMiuUcPqm+wk722LwPVmuQhCJRDyAIJgV/6wFyiDEiQaq?=
 =?us-ascii?Q?1jGkXuGweV+TsBkSAr71J1anjo+3Z9FkSSMSZ+axoUraeag2O4Z++WDaNLuI?=
 =?us-ascii?Q?/0GCcRHboQBv8YHwLAoy9SycZj0Ijamhw/22fchyrzoa5XX+pPIxc3Ya0e0I?=
 =?us-ascii?Q?lOPqqonKfUZQlfz0etzmi40V90w4AkVGk0RuhQkZAg/F5kZ9qwKB5hqwsmor?=
 =?us-ascii?Q?ZTEYsldKXODtU9XJE2m/FtpNkFnWf8CjZSintz9b74EiMDPiQuV9mv57jv0n?=
 =?us-ascii?Q?q0W8YID9Xox3n/akhLXs6sjqpJZChuYBzv3V92KAkB/o4sgVuU523P9fBDX4?=
 =?us-ascii?Q?aKY3DWrYqNKrG22swpLBkAq0WnXf8jHmtx+gUV3CQyQ618xSiKus4meZcjeI?=
 =?us-ascii?Q?MnXtAXMKBXyciwgfjWJjn/wYIxl6kbd3sIbvnAv5Edvo2VIcFkqJBKqIBNU7?=
 =?us-ascii?Q?NDssTgzGdqiodG2uVTFhYV2M0xXHQZ/PSece2uPKqsT5cHIBEhbkTKGFGjAM?=
 =?us-ascii?Q?nTfQydKVcFz9R/vMRZYvIP3agDy8jbCHcDYYQK9tgBUKC0+IWilzhSS813e3?=
 =?us-ascii?Q?Dg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <14731EF4E840124B97AAE8E7FF0F154B@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: aac639ae-6d51-4258-cfdb-08dab5e54d69
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Oct 2022 17:29:27.6796
 (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: /LQg9w2v+I4fvZTY5KnqO8twQX4gCHHMg1paKsho5KGCP3wEV8Ukz8AsjfFG+sEyqGDGqbKYJ2/QPK7VYNsqwg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB5724
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-24_05,2022-10-21_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0
 mlxlogscore=999 mlxscore=0 phishscore=0 malwarescore=0 spamscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2210240106
X-Proofpoint-ORIG-GUID: KCUHsQY8jDdfUrYEi1-L0Ldy4YKLB6Qq
X-Proofpoint-GUID: KCUHsQY8jDdfUrYEi1-L0Ldy4YKLB6Qq
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 24, 2022, at 12:57 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Mon, 2022-10-24 at 13:33 +1100, NeilBrown wrote:
>> On Wed, 19 Oct 2022, Chuck Lever wrote:
>>> NFSv4 operations manage the lifetime of nfsd_file items they use by
>>> means of NFSv4 OPEN and CLOSE. Hence there's no need for them to be
>>> garbage collected.
>>>=20
>>> Introduce a mechanism to enable garbage collection for nfsd_file
>>> items used only by NFSv2/3 callers.
>>>=20
>>> Note that the change in nfsd_file_put() ensures that both CLOSE and
>>> DELEGRETURN will actually close out and free an nfsd_file on last
>>> reference of a non-garbage-collected file.
>>>=20
>>> Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=3D394
>>> Suggested-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> Tested-by: Jeff Layton <jlayton@kernel.org>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>> fs/nfsd/filecache.c |   61 ++++++++++++++++++++++++++++++++++++++++++++=
+------
>>> fs/nfsd/filecache.h |    3 +++
>>> fs/nfsd/nfs3proc.c  |    4 ++-
>>> fs/nfsd/trace.h     |    3 ++-
>>> fs/nfsd/vfs.c       |    4 ++-
>>> 5 files changed, 63 insertions(+), 12 deletions(-)
>>>=20
>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>> index b7aa523c2010..87fce5c95726 100644
>>> --- a/fs/nfsd/filecache.c
>>> +++ b/fs/nfsd/filecache.c
>>> @@ -63,6 +63,7 @@ struct nfsd_file_lookup_key {
>>> 	struct net			*net;
>>> 	const struct cred		*cred;
>>> 	unsigned char			need;
>>> +	unsigned char			gc:1;
>>> 	enum nfsd_file_lookup_type	type;
>>> };
>>>=20
>>> @@ -162,6 +163,8 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_co=
mpare_arg *arg,
>>> 			return 1;
>>> 		if (!nfsd_match_cred(nf->nf_cred, key->cred))
>>> 			return 1;
>>> +		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) !=3D key->gc)
>>> +			return 1;
>>> 		if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) =3D=3D 0)
>>> 			return 1;
>>> 		break;
>>> @@ -297,6 +300,8 @@ nfsd_file_alloc(struct nfsd_file_lookup_key *key, u=
nsigned int may)
>>> 		nf->nf_flags =3D 0;
>>> 		__set_bit(NFSD_FILE_HASHED, &nf->nf_flags);
>>> 		__set_bit(NFSD_FILE_PENDING, &nf->nf_flags);
>>> +		if (key->gc)
>>> +			__set_bit(NFSD_FILE_GC, &nf->nf_flags);
>>> 		nf->nf_inode =3D key->inode;
>>> 		/* nf_ref is pre-incremented for hash table */
>>> 		refcount_set(&nf->nf_ref, 2);
>>> @@ -428,16 +433,27 @@ nfsd_file_put_noref(struct nfsd_file *nf)
>>> 	}
>>> }
>>>=20
>>> +static void
>>> +nfsd_file_unhash_and_put(struct nfsd_file *nf)
>>> +{
>>> +	if (nfsd_file_unhash(nf))
>>> +		nfsd_file_put_noref(nf);
>>> +}
>>> +
>>> void
>>> nfsd_file_put(struct nfsd_file *nf)
>>> {
>>> 	might_sleep();
>>>=20
>>> -	nfsd_file_lru_add(nf);
>>> +	if (test_bit(NFSD_FILE_GC, &nf->nf_flags) =3D=3D 1)
>>=20
>> Clearly this is a style choice on which sensible people might disagree,
>> but I much prefer to leave out the "=3D=3D 1" That is what most callers =
in
>> fs/nfsd/ do - only exceptions are here in filecache.c.
>> Even "!=3D 0" would be better than "=3D=3D 1".
>> I think test_bit() is declared as a bool, but it is hard to be certain.
>>=20
>>> +		nfsd_file_lru_add(nf);
>>> +	else if (refcount_read(&nf->nf_ref) =3D=3D 2)
>>> +		nfsd_file_unhash_and_put(nf);
>>=20
>> Tests on the value of a refcount are almost always racy.
>=20
> Agreed, and there's a clear race above, now that I look more closely. If
> nf_ref is 3 and two puts are racing then neither of them will call
> nfsd_file_unhash_and_put. We really should be letting the outcome of the
> decrement drive things (like you say below).
>=20
>> I suspect there is an implication that as NFSD_FILE_GC is not set, this
>> *must* be hashed which implies there is guaranteed to be a refcount from
>> the hashtable.  So this is really a test to see if the pre-biased
>> refcount is one.  The safe way to test if a refcount is 1 is dec_and_tes=
t.
>>=20
>> i.e. linkage from the hash table should not count as a reference (in the
>> not-GC case).  Lookup in the hash table should fail if the found entry
>> cannot achieve an inc_not_zero.  When dec_and_test says the refcount is
>> zero, we remove from the hash table (certain that no new references will
>> be taken).
>>=20
>=20
> This does seem a more sensible approach. That would go a long way toward
> simplifying nfsd_file_put.

So I cut-and-pasted the approach you used in the patch you sent a few
weeks ago. I don't object to replacing that... but I don't see exactly
where you guys are going with this.


>>> +
>>> 	if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) =3D=3D 0) {
>>> 		nfsd_file_flush(nf);
>>> 		nfsd_file_put_noref(nf);
>>=20
>> This seems weird.  If the file was unhashed above (because nf_ref was
>> 2), it would now not be flushed.  Why don't we want it to be flushed in
>> that case?
>>=20
>>> -	} else if (nf->nf_file) {
>>> +	} else if (nf->nf_file && test_bit(NFSD_FILE_GC, &nf->nf_flags) =3D=
=3D 1) {
>>> 		nfsd_file_put_noref(nf);
>>> 		nfsd_file_schedule_laundrette();
>>> 	} else
>>> @@ -1017,12 +1033,14 @@ nfsd_file_is_cached(struct inode *inode)
>>>=20
>>> static __be32
>>> nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>> -		     unsigned int may_flags, struct nfsd_file **pnf, bool open)
>>> +		     unsigned int may_flags, struct nfsd_file **pnf,
>>> +		     bool open, int want_gc)
>>=20
>> I too would prefer "bool" for all intstance of gc and want_gc.
>>=20
>> NeilBrown
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever