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>,
        Neil Brown <neilb@suse.de>
Subject: Re: [PATCH v3] nfsd: rework hashtable handling in
 nfsd_do_file_acquire
Thread-Topic: [PATCH v3] nfsd: rework hashtable handling in
 nfsd_do_file_acquire
Thread-Index: AQHY1xwiPOG/BlB/KUiRs5xLTvyF4638pTOA
Date:   Mon, 3 Oct 2022 13:11:56 +0000
Message-ID: <F4DF35B2-CE11-4BD9-8442-97852F57CE2E@oracle.com>
References: <20221003113436.24161-1-jlayton@kernel.org>
In-Reply-To: <20221003113436.24161-1-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?CcdVZpi5SQEIiNA2dRFR/esopsXRP2KYMQ30BVsyO7tHY083k4RUDQr6EXkS?=
 =?us-ascii?Q?RjAVikWK8zp7yPS0SenGywZ2o6LHyJb0WJPNhCguKR8HKKctT7dHkvXBNg8H?=
 =?us-ascii?Q?l0LfUdhWiOd/g2fjJJHByqEWPp1Du8ujrsQeJMWml4NPgM29A8illmkxap4v?=
 =?us-ascii?Q?vL9vCjdBlVfGxGgGkMtBebYifStOArrk5TVVggWHU+xwT6xbRi6sVMzurlz6?=
 =?us-ascii?Q?8PMrgJmah4Vr9njvNbYfE66/ug8zHPcuoe6e+FOSCkAohfqFQ/6AuLgd5WNp?=
 =?us-ascii?Q?zWhBdUQatPWfroyeHsbl59apqr/I2N6mTx8nS+gwy4T+XyxkwrYce25HfOvh?=
 =?us-ascii?Q?lg8xKx5Swmd5jPivdyPP0FRqIkF07d7BrZmW10gdOa597Egys9K/2ri9W4od?=
 =?us-ascii?Q?JCIYruo7PWKca9aswlV+3PETP09AaCgQwiIE9ESaWHMvyLoZyPWqEsANqpjX?=
 =?us-ascii?Q?kRTmUpm629+vpCyx4Nwv0a1o4pin0QqM2e3+LYZAYj+vvq6nIxbEdzYdiNKh?=
 =?us-ascii?Q?YBdQSVzsyPdLy+j/1bRDqDRVURAU4FAC3+BvRW7QGp0ZZAJ8tSLRLF7gyNHB?=
 =?us-ascii?Q?UBbKr1RPs2z56mvggkNftHkkY+X9A406pysx4bjzRBu4AQmMSfPxnXEt9kDZ?=
 =?us-ascii?Q?rlNYi7D8v01ytaTLXlNPm3LPbjjQtdkHFkSIDxHBsFJ6FEu8YVFnc2yc6w7P?=
 =?us-ascii?Q?4SWThGWAGxs+unS8blpNEvflZ5DeiqxS4FfullP9QwskyrJC/8jOR5bSXSyD?=
 =?us-ascii?Q?2CzMndbekd9h7AYd4D7zYhabN/00VHf399TmSXc5Yj7EYVvgl4Kq584x/2fo?=
 =?us-ascii?Q?2Ip+C4uHIUb3kCUNKe4iePoC4gV1H5sKHWSjX34rwOMpVpPn8rdsHtYHhVlx?=
 =?us-ascii?Q?IP6rAIgnGHZnuMU4QgzOgtW2/Rvodd6NmHogmQy/Ps52nsKSRZ3tGCtmecxy?=
 =?us-ascii?Q?crUzYmJ8hy/WCW9gL5teqyOR73tNLJcgfZ2nQlCSZLhvXL+OGvG7jLYkwt5g?=
 =?us-ascii?Q?Qb27N8UBsNqQDtVea/Yo5VfKcpVYodTFHEdnVCNiy3e/uu0CPpp3s+k/SZe4?=
 =?us-ascii?Q?509zZp3fyB9A9O/vnpPo9XwCLCQ3dXkQWhCIsBMeiFM4a5/W0SHR/x4YXqVw?=
 =?us-ascii?Q?nUI9r5c+c6h+lohu5cXF+5/GpMVVAEP48S1yF1HqOxd+wIWBVYS/tRRpy4/1?=
 =?us-ascii?Q?CrjfVke2JkXL7j16DHKSIU/QcIkm+FACj9I19GWy1uGeHpBJ3XSUx1BQyojC?=
 =?us-ascii?Q?dBsuGhAtDIZ/T+iKRssoiNm/KGi4NbRSYYUlTL7/EzioO+wdWA8rHPGTdD8b?=
 =?us-ascii?Q?03HT9xjnbdR1X8vnIVr06AwLq7vLUm/GmUlSqONlcSiA09ERXpWimpqI6g37?=
 =?us-ascii?Q?wcwqXV0P/WNu9UUpp0P4i26eo02dYOvB1XSJTN+UlmsxiYywX2u9PUWduzHp?=
 =?us-ascii?Q?5igAogxxrAzFm6Lm9xAKwbf3Jk+cjIBjYJN9A1NcDwpUogImIWd6DcuLzDkR?=
 =?us-ascii?Q?t8JtQrbWmVkRSRXcA89Su/LxqeSZoF2ey9jDf6dQWpk8sMEi4OZJPv5xl/pP?=
 =?us-ascii?Q?UkP2aXrQ1c4EUKlI/NWiKSv8xAEj2+aZwsWQurZzFfoUWnVbAfQO5p/HWR8y?=
 =?us-ascii?Q?Fg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <455588D41A0C794CB9F5527B4980F4C8@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: cc46e1c8-70ee-46a1-2a14-08daa540d923
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Oct 2022 13:11:56.5627
 (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: gVrW7ZIwaZAFJ9146qISVDvslf19VPoQFMK8ZEY4Fv5a3EQZQBk3M1buqJueRiN6pIr0NHlP7b/nj6dsonH0ng==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4696
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-10-03_02,2022-09-29_03,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 adultscore=0 spamscore=0
 suspectscore=0 bulkscore=0 mlxscore=0 mlxlogscore=999 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2209130000
 definitions=main-2210030081
X-Proofpoint-GUID: BJ1kbEGxqn_YhVg_MAfgrdyjcQhn11Jr
X-Proofpoint-ORIG-GUID: BJ1kbEGxqn_YhVg_MAfgrdyjcQhn11Jr
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 3, 2022, at 7:34 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> nfsd_file is RCU-freed, so we need to hold the rcu_read_lock long enough
> to get a reference after finding it in the hash. Take the
> rcu_read_lock() and call rhashtable_lookup directly.
>=20
> Switch to using rhashtable_lookup_insert_key as well, and use the usual
> retry mechanism if we hit an -EEXIST. Eliminiate the insert_err goto
> target as well.

The insert_err goto is there to remove a very rare case from
the hot path. I'd kinda like to keep that feature of this code.

The rest of the patch looks good.


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 46 ++++++++++++++++++++-------------------------
> 1 file changed, 20 insertions(+), 26 deletions(-)
>=20
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index be152e3e3a80..63955f3353ed 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -1043,9 +1043,10 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struc=
t svc_fh *fhp,
> 		.need	=3D may_flags & NFSD_FILE_MAY_MASK,
> 		.net	=3D SVC_NET(rqstp),
> 	};
> -	struct nfsd_file *nf, *new;
> +	struct nfsd_file *nf;
> 	bool retry =3D true;
> 	__be32 status;
> +	int ret;
>=20
> 	status =3D fh_verify(rqstp, fhp, S_IFREG,
> 				may_flags|NFSD_MAY_OWNER_OVERRIDE);
> @@ -1055,35 +1056,35 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, stru=
ct svc_fh *fhp,
> 	key.cred =3D get_current_cred();
>=20
> retry:
> -	/* Avoid allocation if the item is already in cache */
> -	nf =3D rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> -				    nfsd_file_rhash_params);
> +	rcu_read_lock();
> +	nf =3D rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> +			       nfsd_file_rhash_params);
> 	if (nf)
> 		nf =3D nfsd_file_get(nf);
> +	rcu_read_unlock();
> 	if (nf)
> 		goto wait_for_construction;
>=20
> -	new =3D nfsd_file_alloc(&key, may_flags);
> -	if (!new) {
> +	nf =3D nfsd_file_alloc(&key, may_flags);
> +	if (!nf) {
> 		status =3D nfserr_jukebox;
> 		goto out_status;
> 	}
>=20
> -	nf =3D rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
> -					      &key, &new->nf_rhash,
> -					      nfsd_file_rhash_params);
> -	if (!nf) {
> -		nf =3D new;
> -		goto open_file;
> -	}
> -	if (IS_ERR(nf))
> -		goto insert_err;
> -	nf =3D nfsd_file_get(nf);
> -	if (nf =3D=3D NULL) {
> -		nf =3D new;
> +	ret =3D rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
> +					   &key, &nf->nf_rhash,
> +					   nfsd_file_rhash_params);
> +	if (ret =3D=3D 0)
> 		goto open_file;
> +
> +	nfsd_file_slab_free(&nf->nf_rcu);
> +	if (retry && ret =3D=3D EEXIST) {
> +		retry =3D false;
> +		goto retry;
> 	}
> -	nfsd_file_slab_free(&new->nf_rcu);
> +	trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret);
> +	status =3D nfserr_jukebox;
> +	goto out_status;
>=20
> wait_for_construction:
> 	wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> @@ -1143,13 +1144,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struc=
t svc_fh *fhp,
> 	smp_mb__after_atomic();
> 	wake_up_bit(&nf->nf_flags, NFSD_FILE_PENDING);
> 	goto out;
> -
> -insert_err:
> -	nfsd_file_slab_free(&new->nf_rcu);
> -	trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, PTR_ERR(nf));
> -	nf =3D NULL;
> -	status =3D nfserr_jukebox;
> -	goto out_status;
> }
>=20
> /**
> --=20
> 2.37.3
>=20

--
Chuck Lever