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:     Wang Yugui <wangyugui@e16-tech.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [RPC] nfsd: NFSv4 close a file completely
Thread-Topic: [RPC] nfsd: NFSv4 close a file completely
Thread-Index: AQHYfi1IwwfFdSiA302AZfT/YMJLJq1MFpUA
Date:   Sun, 12 Jun 2022 18:23:11 +0000
Message-ID: <0CBF71FB-7754-4992-BE16-A3CFD404DECC@oracle.com>
References: <20220612072253.66354-1-wangyugui@e16-tech.com>
In-Reply-To: <20220612072253.66354-1-wangyugui@e16-tech.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?kkxOTXAjaqEx/rRLpFuiHGg1sIBwbBXtsX4nNeFoXjJmxweEHK3rbfML3h9z?=
 =?us-ascii?Q?xZGgK66BtvalDtiuvGsNFquqR3C+Wqbx1oX62LBssXk3J9nlxad0+K0Amof0?=
 =?us-ascii?Q?GLdB2xpuNT0N0gX/rTuIl+sJjj1gMK+/v5hOrwrrWepRKwtaluI18mHOA3cG?=
 =?us-ascii?Q?OyHaUdT4/kms4d/dWxLptrHFvtAaQfXLL0tijcMabXcxyeqhT7asbCd/+1Nh?=
 =?us-ascii?Q?z3O4DI0yIdXdNMuQEfn4/4lzSl7V1q57w7rqQeB+VzjD0U5wxrLCCg/ZZB3c?=
 =?us-ascii?Q?1Cg9BLcKGBnPrq2kAeUv+5AFrYE0wtlRPnfi8GajIURlwzRW6MlAd6vfw4j3?=
 =?us-ascii?Q?NhYl33qUw4AQt+ye2+wrNcrxzit5lN9iIJKj2CVj81JgVdIWLdPH6pluHD1S?=
 =?us-ascii?Q?DPbGYwVqKGS4WXfrbZ2Bl5cT1R2tNg2kmNFerXbKnkGjT6NbRUxno5+tTaGd?=
 =?us-ascii?Q?WRZrHxS+M3hLEuxO/sD4k1b+uycbrg6XNW8Xu6hGjss6tdvXSy0snjW2dpiy?=
 =?us-ascii?Q?OLKsJ6kDeCOjrE7ZHkVYDnuBp4ShNuR9pjqmsCeqBbUblbveVVCDzL/ki5CH?=
 =?us-ascii?Q?4dYMHhXzYVPv7H4mm/Er7KQjRAlEjN3nkp3YpwSEsX0OKKYGciGIwS5N9j5N?=
 =?us-ascii?Q?j3amxIQV4qC3EybjUomNPkxwFIP6vuGzWxt1c2li+mLvgiB/nlsspzocFpJr?=
 =?us-ascii?Q?+De5IjYFJwMco1oKpCw9cfqlYN1s2Jo8m5LUX+4EzGNTtJp1O/gAlpmsUdIP?=
 =?us-ascii?Q?foN1nJWpLhx9mvgyilJVLjrm+sIRb1hNbgJHPF34Xi7JQivXyuIJ0npFiPGX?=
 =?us-ascii?Q?Me9vS71UH4wa7dpYDmK2e+o3heBu6PR94P1jp6b3mbsbMlz0cmv4FUd9vWk9?=
 =?us-ascii?Q?n+4skhkOb/4Cj4iq9ySXusMLfcAvM5GRxW0l/ftkoN13BM5GFjve0r5+q9OY?=
 =?us-ascii?Q?p9xnwPEodiOd4bnOqBAd6z2WC0SPQn6PbviDDe8pIywYyBuvVfkMHH2VfzKf?=
 =?us-ascii?Q?jxn0RtTl0LcVOoE2mtaHZZDUCHM3bQVICB7gPqvbUCgKDEkFfg5GmzNy/16I?=
 =?us-ascii?Q?vmN/IZi48wputtHmI/AwqKjddF0R6Rhdo1muoGqfq8Z5WOH8Mxu0TVcG3F5o?=
 =?us-ascii?Q?e3yFde7AI44S+9NQinF7DDE1dGgapsVpjChGi6HXh2kKZ7sh4h9pfvrXddWj?=
 =?us-ascii?Q?qWuiJafmtVYBVVBc66bLaDckapHMV3TGgfueqeey7qwlOqUy/mMHDz/JECxK?=
 =?us-ascii?Q?tITvZsqJfL2TuTJDQ1zkZGBdUpK79HOibaJVp0/TMklOEdV9o1V8Dqblf6z9?=
 =?us-ascii?Q?EWOklGzKe5KPdatIdE3b7853QgDN7ec8STmk7h62zf2DTQNNpQyJnfm0IU3w?=
 =?us-ascii?Q?+v2Yn+UXvy+v7JlcOs1Uf98Q8eG9OZShqAJngWpVbtDmogEZUe8+DXLALOVL?=
 =?us-ascii?Q?R0jH1EYHUeeK5fFOZnZKkB/O9W0G1CaXpNrVxyecIa1jUNO74tXfcbpubJSn?=
 =?us-ascii?Q?dT4YLdshg1c2NnzxMYHiq20Fr8CwI18RtAKD3YBNXob7kjUDnjXOorczkQL1?=
 =?us-ascii?Q?1PpFihgogFazUUCRL9EzwLCUr5rRScUUY8//IAbdwVmAC748fwy9z7eX+99H?=
 =?us-ascii?Q?LtvLO3rgoz7r9UPbZwNi793rgBO25hP2nn1iZfsddLczNIUWNMWuKfc6vHM2?=
 =?us-ascii?Q?eM7gwqwxJp+hRDQse9TvFtDdmcIRlGF25jcFeCgZL/vdzKqkUvM/kXw7lM6p?=
 =?us-ascii?Q?bUPw3x8z3GTe4uk1MUcn472ORfKAklI=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <F5496FB4FA3E374BA7168AAAC38F6289@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: DS7PR10MB5134.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7cb71aef-a25b-40fe-4f0b-08da4ca09b4e
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Jun 2022 18:23:11.0112
 (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: FJplf+ak/5QhfBbuq+mSdOz+lWnpfUmwNbOKvDsoecGn/rktc33SQO+/T9Oxv1Mww+Mh5gZd8sxas7SqiS2DCw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB2857
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.874
 definitions=2022-06-12_08:2022-06-09,2022-06-12 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 suspectscore=0
 spamscore=0 mlxlogscore=999 phishscore=0 adultscore=0 bulkscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2204290000 definitions=main-2206120088
X-Proofpoint-GUID: jp5MenCaxxDCY-LECVzj503i1sKWrNK-
X-Proofpoint-ORIG-GUID: jp5MenCaxxDCY-LECVzj503i1sKWrNK-
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,T_SCC_BODY_TEXT_LINE
        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 Jun 12, 2022, at 3:22 AM, Wang Yugui <wangyugui@e16-tech.com> wrote:
>=20
> NFSv4 need to close a file completely (no lingering open) when it does
> a CLOSE or DELEGRETURN.
>=20
> When multiple NFSv4/OPEN from different clients, we need to check the
> reference count. The flowing reference-count-check change the behavior
> of NFSv3 nfsd_rename()/nfsd_unlink() too.
>=20
> Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=3D387
> Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>
> ---
> TO-CHECK:
> 1) NFSv3 nfsd_rename()/nfsd_unlink() feature change is OK?
> 2) Can we do better performance than nfsd_file_close_inode_sync()?
> 3) nfsd_file_close_inode_sync()->nfsd_file_close_inode() in nfsd4_delegre=
turn()
> 	=3D> 'Text file busy' about 4s
> 4) reference-count-check : refcount_read(&nf->nf_ref) <=3D 1 or =3D=3D0?
> 	nfsd_file_alloc()	refcount_set(&nf->nf_ref, 1);
>=20
> fs/nfsd/filecache.c | 2 +-
> fs/nfsd/nfs4state.c | 4 ++++
> 2 files changed, 5 insertions(+), 1 deletion(-)

I suppose I owe you (and Frank) a progress report on #386. I've fixed
the LRU algorithm and added some observability features to measure
how the fix impacts the cache's efficiency for NFSv3 workloads.

These new features show that the hit rate and average age of cache
items goes down after the fix is applied. I'm trying to understand
if I've done something wrong or if the fix is supposed to do that.

To handle the case of hundreds of thousands of open files more
efficiently, I'd like to convert the filecache to use rhashtable.


> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 9cb2d590c036..8890a8fa7402 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -512,7 +512,7 @@ __nfsd_file_close_inode(struct inode *inode, unsigned=
 int hashval,
>=20
> 	spin_lock(&nfsd_file_hashtbl[hashval].nfb_lock);
> 	hlist_for_each_entry_safe(nf, tmp, &nfsd_file_hashtbl[hashval].nfb_head,=
 nf_node) {
> -		if (inode =3D=3D nf->nf_inode)
> +		if (inode =3D=3D nf->nf_inode && refcount_read(&nf->nf_ref) <=3D 1)
> 			nfsd_file_unhash_and_release_locked(nf, dispose);
> 	}
> 	spin_unlock(&nfsd_file_hashtbl[hashval].nfb_lock);
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 9409a0dc1b76..be4b480f5914 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6673,6 +6673,8 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_co=
mpound_state *cstate,
>=20
> 	/* put reference from nfs4_preprocess_seqid_op */
> 	nfs4_put_stid(&stp->st_stid);
> +
> +	nfsd_file_close_inode_sync(d_inode(cstate->current_fh.fh_dentry));

IIUC this closes /all/ nfsd_file objects that refer to the same inode.
CLOSE is supposed to release only the state held by a particular user
on a particular client. This is like closing a file descriptor; you
release the resources associated with that file descriptor, and other
users of the inode are not supposed to be affected. Thus using an
inode-based API in nfsd4_close/delegreturn seems like the wrong
approach.


> out:
> 	return status;
> }
> @@ -6702,6 +6704,8 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nf=
sd4_compound_state *cstate,
> 	destroy_delegation(dp);
> put_stateid:
> 	nfs4_put_stid(&dp->dl_stid);
> +
> +	nfsd_file_close_inode_sync(d_inode(cstate->current_fh.fh_dentry));
> out:
> 	return status;
> }
> --=20
> 2.36.1
>=20

--
Chuck Lever