netdev - Re: [PATCH RFC 29/30] NFSD: Convert the filecache to use rhashtable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1BB6647C-799E-463F-BF63-55B48450FF29@oracle.com>
Date:   Thu, 23 Jun 2022 00:58:17 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Dave Chinner <david@...morbit.com>
CC:     Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>, "tgraf@...g.ch" <tgraf@...g.ch>,
        Jeff Layton <jlayton@...hat.com>
Subject: Re: [PATCH RFC 29/30] NFSD: Convert the filecache to use rhashtable



> On Jun 22, 2022, at 8:38 PM, Dave Chinner <david@...morbit.com> wrote:
> 
> 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.
>> 
>> 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.
>> 
>> Suggested-by: Jeff Layton <jlayton@...nel.org>
>> Signed-off-by: Chuck Lever <chuck.lever@...cle.com>
> 
> 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 returned
>> + *   %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 = {
>> +		.type	= NFSD_FILE_KEY_FULL,
>> +		.inode	= new->nf_inode,
>> +		.need	= new->nf_flags,
>> +		.net	= new->nf_net,
>> +		.cred	= current_cred(),
>> +	};
>> +	struct nfsd_file *nf;
>> +
>> +	nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
>> +					      &key, &new->nf_rhash,
>> +					      nfsd_file_rhash_params);
>> +	if (!nf)
>> +		return nf;
> 
> 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