> On Jun 22, 2022, at 8:58 PM, Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
>
>> On Jun 22, 2022, at 8:38 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>>
>> On Wed, Jun 22, 2022 at 10:15:56AM -0400, Chuck Lever wrote:
>
>>> +
>>> +/*
>>> + * 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.
Yep, that was it. I've fixed it, but some other doubts have surfaced
in the meantime.
Removing the .max_size cap also helps, and in the long run, I now
feel that cap should be left off. But I would like to be certain that
nfsd_file_acquire's logic works when hard errors occur, so I left the cap
in place for now. I found that the "failed to open newly created file!"
warning fires when insertion fails. I need to work on addressing that
case silently.
Also I just found Neil's nice rhashtable explainer:
https://lwn.net/Articles/751374/
Where he writes that:
> Sometimes you might want a hash table to potentially contain multiple objects for any given key. In that case you can use "rhltables" — rhashtables with lists of objects.
I believe that is the case for the filecache. The hash value is
computed based on the inode pointer, and therefore there can be more
than one nfsd_file object for a particular inode (depending on who
is opening and for what access). So I think filecache needs to use
rhltable, not rhashtable. Any thoughts from rhashtable experts?
--
Chuck Lever
