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:     Dave Chinner <david@fromorbit.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>, "tgraf@suug.ch" <tgraf@suug.ch>,
        Jeff Layton <jlayton@redhat.com>
Subject: Re: [PATCH RFC 29/30] NFSD: Convert the filecache to use rhashtable
Thread-Topic: [PATCH RFC 29/30] NFSD: Convert the filecache to use rhashtable
Thread-Index: AQHYhkLHlIdZc73yTECQuJnTH2oZX61cJo8AgAAFjwA=
Date:   Thu, 23 Jun 2022 00:58:17 +0000
Message-ID: <1BB6647C-799E-463F-BF63-55B48450FF29@oracle.com>
References: <165590626293.75778.9843437418112335153.stgit@manet.1015granger.net>
 <165590735674.75778.2489188434203366753.stgit@manet.1015granger.net>
 <20220623003822.GF1098723@dread.disaster.area>
In-Reply-To: <20220623003822.GF1098723@dread.disaster.area>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?iQYNrdlcb7FzKkcY6BV3Ms7USBU0aUA2kKeAO7+8i0JDxyIsiQ+wYr1JyfoZ?=
 =?us-ascii?Q?0jrFOqM0zBGLUIFtPuZZSuXDLU7QOUlJgZrf+Q0cmSJzNnSAIvrndUtRMT7U?=
 =?us-ascii?Q?yF+Cue/5MOPmwQn83m3Qo8gtTCyTOkDkmeaoCLa2FQN5YshSP6+d18ClPwOM?=
 =?us-ascii?Q?1/NQC/iyn/k4PcLGZyixSW9njBhLCxr2P6o8kxYxPgeAsi0sbT40+LV2mEb4?=
 =?us-ascii?Q?Gs8sLbnyi6f6PxE2VJv/zwiSi/4Yj1tOgJyHLxhB02+H2ph8FRdAb7xj8Dds?=
 =?us-ascii?Q?k2amGDN4pu2vIxNqnBOyj6y/OfA+Jjl6dxeBkIoCCf0SOP0veIPiiWng5JUl?=
 =?us-ascii?Q?imoswfubLOjdmTfsBIQH/G4eoIkOP6hZOQ6ZBRvh2Zl0vVRO8hjIGo8k/GSw?=
 =?us-ascii?Q?9ML3bLnGrnSOQI55zPLlZpAgxDBxxpM3h7SxMcGs6s2kiVBnD0tdjU0KDrEp?=
 =?us-ascii?Q?V+0y4kowV3OsqR56FzYpljB9MG1tar2lu0EZLzTB1TN7+Wde+JGkl69ePvAs?=
 =?us-ascii?Q?5H6SM+g5xMMOF14Qnoldb46eCy2VE0YofZd4MLzEntodGE9vGC3c+25uetxd?=
 =?us-ascii?Q?JtQVLvNqOwuJCevdD4livVau/FGuZW7857Z3hlsAmZnhroWYlCBl80d7/p5q?=
 =?us-ascii?Q?xEkaNmN/wS+E7Ziu2mampqtvHKdmjIXFMu8dCb/uVjdHdIgDGAQC8O9essWS?=
 =?us-ascii?Q?8ZslsWFO06o+7K0YZq73KM7pzGfl5zv8GvuYstHqqUeP2zPfY7aKQ4UAMpao?=
 =?us-ascii?Q?5FEmXhD7h9b7OwIAc5Xj5VN7figUXOsjqypOOLwhy2PMSRpKJsUKpBIk6373?=
 =?us-ascii?Q?AlXsNRKGXLsMCtOxa03FLxJ3rCLK1aU2GF2RycZeuywXBE9rJa/VnSdth8Rd?=
 =?us-ascii?Q?sR6W2fadei+f+3aqaUMoi/+3w8jT1iI69QtpUzr4SJS2JARxxc0kVpMdNac0?=
 =?us-ascii?Q?kfMlH1nHPHSLABJBElqRsuBUxamcAIPhgjbgLXtGSdKZSAj1CxflTKNjl+wp?=
 =?us-ascii?Q?kw7okDnBY/UY1WrGfzcSl0aqZYDUDSxZndrExne0QinMAinPHoGS//Mkmrzz?=
 =?us-ascii?Q?ICx6gQn57u+Oe+n1u0adD7KhkcwmV4A1ndh6rVTrT5zDZLg4C1sKCF+QAgjh?=
 =?us-ascii?Q?/9BpmAVeCjb8b+fRXUbNx1J+qiwDHdX5lIWmnCWzq3XUlFtzT/1LLzefocsa?=
 =?us-ascii?Q?MrlFHaAyNibCedAXP/n/Qzv11sBewjoNXaWJKNuOnrciEfqBJOeghFoDDGL+?=
 =?us-ascii?Q?ds3TAJdsO+2r6teFefgu532YxPAa64e8MVo230OfDXToirajgFcdX6x2Srbl?=
 =?us-ascii?Q?fd9LhWbK2p77wfTvZeDsduaKsc5xfczEXzZozqOTZ/piHzTqgVCCSRFJNXu+?=
 =?us-ascii?Q?pYYVixfcBSRElDYa56t98AC9c3NOmTe/H2e4PEA9MIfs5N/Wj8HdbGXnjiX8?=
 =?us-ascii?Q?wlwNmYUFzw1A7dlhAmy/2H4frVEvDVQgpgn1/xM+/+3DqbKhYHZNfSIZsc/C?=
 =?us-ascii?Q?s/08H7MPTSCb8ipJQfZG2/DWuqRAdbVznkcBzyBNDHSnfvaINQtzOMNzTGcN?=
 =?us-ascii?Q?2TqGyyAv1jbnmCuIyz6B2mDEKqsd56f6zsSyuT/QXnmANnlJ4fWF6lyCjyxY?=
 =?us-ascii?Q?vauguJshOmKAk4eMWuOyTeePZ3dC8oyE+1upgZkVfAXQ9CP7ipTIEeStQZLb?=
 =?us-ascii?Q?JfkFBHpVSJcYZFTIhCL4IlUZRV1gS4eUHO0qVpd+5f40Q26+KJ1oJkWpJSEM?=
 =?us-ascii?Q?r8Jp51NUs4LawrTqxXpJg7GbkfrN6yM=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <543EA3397FD3534C87080B4220FF0D60@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: cd62388a-8b71-4a17-578a-08da54b3755c
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Jun 2022 00:58:17.1055
 (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: t7MzZ9jA6zZotvwrE2WwylzDyobOTSQkVD6xvF8bcHjwDNxUNRkTDJ5mDQOwka52fq+uT7AInVclgdxuUUsP0A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR10MB4118
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-06-22_09:2022-06-22,2022-06-22 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxlogscore=999
 malwarescore=0 adultscore=0 phishscore=0 bulkscore=0 mlxscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2204290000 definitions=main-2206230001
X-Proofpoint-GUID: CPP5OYq3pv6d9vxG_GFKmO-nXk3NoNZf
X-Proofpoint-ORIG-GUID: CPP5OYq3pv6d9vxG_GFKmO-nXk3NoNZf
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 22, 2022, at 8:38 PM, Dave Chinner <david@fromorbit.com> wrote:
>=20
> On Wed, Jun 22, 2022 at 10:15:56AM -0400, Chuck Lever wrote:
>> Enable the filecache hash table to start small, then grow with the
>> workload. Smaller server deployments benefit because there should
>> be lower memory utilization. Larger server deployments should see
>> improved scaling with the number of open files.
>>=20
>> I know this is a big and messy patch, but there's no good way to
>> rip out and replace a data structure like this.
>>=20
>> Suggested-by: Jeff Layton <jlayton@kernel.org>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>=20
> Pretty sure I mentioned converting to rhashtable as well when we
> were talking about the pointer-chasing overhead of list and tree
> based indexing of large caches.  :)

Jeff's suggestion was right in the source code :-) but fair enough.
The idea was also discussed when the filecache code was changed to
use kzvalloc recently.

I appreciate your review and your advice!


>> +
>> +/*
>> + * Atomically insert a new nfsd_file item into nfsd_file_rhash_tbl.
>> + *
>> + * Return values:
>> + *   %NULL: @new was inserted successfully
>> + *   %A valid pointer: @new was not inserted, a matching item is return=
ed
>> + *   %ERR_PTR: an unexpected error occurred during insertion
>> + */
>> +static struct nfsd_file *nfsd_file_insert(struct nfsd_file *new)
>> +{
>> +	struct nfsd_file_lookup_key key =3D {
>> +		.type	=3D NFSD_FILE_KEY_FULL,
>> +		.inode	=3D new->nf_inode,
>> +		.need	=3D new->nf_flags,
>> +		.net	=3D new->nf_net,
>> +		.cred	=3D current_cred(),
>> +	};
>> +	struct nfsd_file *nf;
>> +
>> +	nf =3D rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
>> +					      &key, &new->nf_rhash,
>> +					      nfsd_file_rhash_params);
>> +	if (!nf)
>> +		return nf;
>=20
> The insert can return an error (e.g. -ENOMEM) so need to check
> IS_ERR(nf) here as well.

That is likely the cause of the BUG that Wang just reported, as
that will send a ERR_PTR to nfsd_file_get(), which blows up when
it tries to defererence it.

I'll resend the series first thing tomorrow morning after some
more clean up and testing.


--
Chuck Lever