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 2/4] nfsd: NFSD_FILE_KEY_INODE only needs to find GC'ed
 entries
Thread-Topic: [PATCH 2/4] nfsd: NFSD_FILE_KEY_INODE only needs to find GC'ed
 entries
Thread-Index: AQHZIP9icvcrgLzObkKRs2F8ILhq166P30YA
Date:   Thu, 5 Jan 2023 14:18:52 +0000
Message-ID: <22E267B9-BA8C-46BA-A76E-A7A72FA5D9B3@oracle.com>
References: <20230105121512.21484-1-jlayton@kernel.org>
 <20230105121512.21484-3-jlayton@kernel.org>
In-Reply-To: <20230105121512.21484-3-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?n5yA8s+jIp2hkKgSFAKYv6GlgMvGTsqPp9UK0fb8Z0KNXQLkurqQskdcrPq2?=
 =?us-ascii?Q?b2kR+EiaiavbptiWYDXZubuYrcpKz737IHaBRgRoXmHdbGAj4jKTpekKbUaP?=
 =?us-ascii?Q?zXG3uUdN/zTJ8O4RPJQsVKC+x8mMCYnb7MkIRRBm6CxD8dMt08lLoZWzQA9I?=
 =?us-ascii?Q?lp74voa4Cr7TSMLBrkXmM4KNexxTKWNoC33l2AzRbZjBnLzDy15vs07UQfib?=
 =?us-ascii?Q?iuPoyzJRMc3CUuErfYdyMVr2eqLswn6G4LvRs3JfE1YY+wlYYqJ2wLHeDzm+?=
 =?us-ascii?Q?ZTiHDR97pqxfNk2iEsYQKFFaOv5cJ85NjBbmG/URU2pWe5Z+gMeRPLFkHvGj?=
 =?us-ascii?Q?awyyf9+5DMadzSMv/OewgHwFGIsRwMLDeoRaHg3EXafcAT3I4pgVMjF4cpJ8?=
 =?us-ascii?Q?arMNiC71xu1mTqBj/OwbjlMRZ+Rquc6trtMnR3W2H/kiOpnNlJu3fYwmCp/v?=
 =?us-ascii?Q?O5Dy9HJiFCHreUAHyv3ZLlwnKb46G4HBKYHoST4QMAKTE/sQHxhFoGk1l/SG?=
 =?us-ascii?Q?mbavUoT67qLLR6XlmiqdnQIPrSskK+Ao7OMELTiEIrbbbDnQ1fSMXFr+FpoV?=
 =?us-ascii?Q?p24ClBV07OI4cfQHj1TjxWViNomtZ0om1p0XXahbdz2T+BlnAe1QvP+eqZFC?=
 =?us-ascii?Q?vxwCcqKIKGDPYyf1BAdfooU85vWyoMOljaFQvwxzVxxQTlhlHyT1oEyNMwWW?=
 =?us-ascii?Q?UYneGbf7raTn7PfxRYF6bOxcrVYoG0nbFkJnCe+U9swC+h1lB0aWMevSDVJU?=
 =?us-ascii?Q?OsCqxL5n1AmOxWS/6gyZyYe6xbFwPb+dG+iyfaS+QF5rTgPpcVRBnZseyCQU?=
 =?us-ascii?Q?gx/40lN51geGkWxjXB5yX3fho1FZi9iz/OSS/2MxcVJDLKwFLKmCoS87yqUk?=
 =?us-ascii?Q?zK8ODgu+fhwFCvAsrZ+LLDMds/4ncCBG4oNGjfzoCaeANX2fUW0SU1LSW7I9?=
 =?us-ascii?Q?V+gatfxnqBA/Rxm8DXoW4MOjuQWqzhpr2FjwGkDy4im6GAs7aSFXLhi2oie8?=
 =?us-ascii?Q?fS2K2v7NlZya48fIYbksNz2nXGnyJx5QAJjXV+eq8Z9dmGHwDQVv8NPdRA87?=
 =?us-ascii?Q?71apTfqoFNU5n3T2ZfZF6D9HKhRMid8HgOdanaH/sSaIamAF0OgbrQYb0bnq?=
 =?us-ascii?Q?iqH7KNmX27S6GPnWh/NN0KGjffic4XhPDmxyKHK23a2EDODjXGSD/uJBSBPj?=
 =?us-ascii?Q?db2lPy5ALv17Zhsowz/myTAOxnht0gkkA4CRndjMNl+RRbjJHXT3bOJcDlxp?=
 =?us-ascii?Q?IZ9bOJgg+HuKq2M+CbhcFzLENJ39vXKe8zU6t+zbq5MjNJwARsdjZiXmoWwG?=
 =?us-ascii?Q?+RL1SeMwFQhZ5usQ3gde1RTXQ0wNetf7zWw6oDcBylqaAa/Ds+ZcBgak3wpV?=
 =?us-ascii?Q?M6D7xwZzrhoMmeP30hYvmPfzWYc75ccwaKiyGsgDgAY68bLT0kp9Bw6IbRU0?=
 =?us-ascii?Q?8SqwU0kyE54Bcu3L0JpdyNrbajwY6UM6sJHT55Vhyatxdx5UypPEmgtRBJDV?=
 =?us-ascii?Q?R4UI15CBsdS2x917A03n2POoLbcp4VYf1gskKGWB1rKmXvrtkbMrhPiLY18u?=
 =?us-ascii?Q?2b4KL5nwRBcZnCmyrRo89VrEuEvXUbvSIeLcEGlXrLy9+UNk1bAW2J8gohxE?=
 =?us-ascii?Q?qg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <342CABB23312504EAE59453323B7B533@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: b2gawy8YONXTKrpRmJ4nK4QH0an4B0VR9y2UIScF59tXsNUMPyVi62mS/bROuuK2rsU9pI2g6I9WJ4K5gyKvfggk3kwxIp6uKpHq6Mb5tEdhdTlfH9KWI3xQymWOvbWMZliJRYmzuSx3SB8qUDSV1yN+h7bUmHN9t1xG4KEpg3ZNSX4Ha7yH/x64XwY52oom5Hjh1Mudx2QbZCDFzJ5XCRcvTTvFlTlaiillCS6qCfV0vN9sEw8k71bDQenvtCIsnonGuycOX5N4uAytEwE4AxLUip06I+Gufkl/lSjHfHI5pGBUzRpT4Ig6j6bD9OnVdqnuf4aadvrjRtwYb0/ptZvgiGhs/DiWNYwQPW1827rN7UAa/G55OPGIrvrukmbY+Juj4wKbMR9p/XXijP1pbBaFeR47pSi+RuMfbdRQcpylfTeQCwgb6o+MuhY8aWEGQEI/02AmW2hCfXEhEP1dKChz6WajhX2aUycl8guPXd5iOb8xqYVDKOGq+Wcvu+dfNNQ9ijJyzX9eix9SIuwDCR2xOLqaHxPu0yDG/sKEOP1qyddF7oViuiW+/fMmuM7LqaT8a574W3VizSZau+qKakHrpGtBdcmYiI2lVgR9B9FHTEIwVoCHKCOVAgw59QrqjqsHtAcM9FhCY4q2mpEMl6nyiCTEb7ng8ScWbUwvukZq2tPMdDL1Qu7+o/7jEXnTO68nfFjvZGsXLuw5ObsUNmUxGg1TR8j8t9R7PtDPlDHMYDadhAiedNLvdTyIxFxVCeVdkvIvfwjNkS0GAyHYxxk4D6e5NrIZlTO5plVPyno=
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: f4438c66-22a8-4b54-33eb-08daef27c5e2
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Jan 2023 14:18:52.8576
 (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: EC6oFH/XBS3Wuef3Ac6VAtaCv8FHCQrwOWz1dhk8nAZhNP2VW2DNVKpFrDptjyE02teeLYmI2nky2Fh6GzHtrA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN4PR10MB5541
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2023-01-05_05,2023-01-05_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 suspectscore=0
 mlxlogscore=999 spamscore=0 bulkscore=0 phishscore=0 mlxscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2301050113
X-Proofpoint-GUID: 0JpphyGahzC96Vt9cufCd1Cm1kNi-lA8
X-Proofpoint-ORIG-GUID: 0JpphyGahzC96Vt9cufCd1Cm1kNi-lA8
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

Hi Jeff-


> On Jan 5, 2023, at 7:15 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> Since v4 files are expected to be long-lived, there's little value in
> closing them out of the cache when there is conflicting access.

Seems like targeting long-lived nfsd_file items could actually
be a hazardous behavior. Are you sure it's safe to leave it in
stable kernels?


> Rename NFSD_FILE_KEY_INODE to NFSD_FILE_KEY_INODE_GC,

I'd prefer to keep the name as it is. It's for searching for
inodes, and adding the ".gc =3D true" to the search criteria is
enough to show what you are looking for.


> and change the
> comparator to also match the gc value in the key. Change both of the
> current users of that key to set the gc value in the key to "true".
>=20
> Also, test_bit returns bool, AFAICT, so I think we're ok to drop the
> !! from the condition later in the comparator.

And I'd prefer that you leave this clean up for another patch.


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>=20
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 9fff1fa09d08..a67b22579c6e 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -78,7 +78,7 @@ static struct rhashtable		nfsd_file_rhash_tbl
> 						____cacheline_aligned_in_smp;
>=20
> enum nfsd_file_lookup_type {
> -	NFSD_FILE_KEY_INODE,
> +	NFSD_FILE_KEY_INODE_GC,
> 	NFSD_FILE_KEY_FULL,
> };
>=20
> @@ -174,7 +174,9 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_comp=
are_arg *arg,
> 	const struct nfsd_file *nf =3D ptr;
>=20
> 	switch (key->type) {
> -	case NFSD_FILE_KEY_INODE:
> +	case NFSD_FILE_KEY_INODE_GC:
> +		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) !=3D key->gc)
> +			return 1;
> 		if (nf->nf_inode !=3D key->inode)
> 			return 1;
> 		break;
> @@ -187,7 +189,7 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_comp=
are_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)
> +		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;
> @@ -681,8 +683,9 @@ static void
> nfsd_file_queue_for_close(struct inode *inode, struct list_head *dispose)
> {
> 	struct nfsd_file_lookup_key key =3D {
> -		.type	=3D NFSD_FILE_KEY_INODE,
> +		.type	=3D NFSD_FILE_KEY_INODE_GC,
> 		.inode	=3D inode,
> +		.gc	=3D true,
> 	};
> 	struct nfsd_file *nf;
>=20
> @@ -1057,8 +1060,9 @@ bool
> nfsd_file_is_cached(struct inode *inode)
> {
> 	struct nfsd_file_lookup_key key =3D {
> -		.type	=3D NFSD_FILE_KEY_INODE,
> +		.type	=3D NFSD_FILE_KEY_INODE_GC,
> 		.inode	=3D inode,
> +		.gc	=3D true,
> 	};
> 	bool ret =3D false;
>=20
> --=20
> 2.39.0
>=20

--
Chuck Lever