> On Jul 11, 2024, at 2:20 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Thu, 2024-07-11 at 18:16 +0000, Chuck Lever III wrote:
>>
>>
>>> On Jul 11, 2024, at 1:53 PM, Jeff Layton <jlayton@xxxxxxxxxx>
>>> wrote:
>>>
>>> On Thu, 2024-07-11 at 13:05 -0400, Youzhong Yang wrote:
>>>> Shouldn't we have fh_put(fhp) before 'retry'?
>>>>
>>>
>>> A subtle question, actually...
>>>
>>> It probably wouldn't hurt to do that, but I don't think it's
>>> required.
>>>
>>> The main reason we call fh_put is to force a second call to
>>> nfsd_set_fh_dentry. IOW, we want to redo the lookup by filehandle
>>> and
>>> find the inode.
>>>
>>> In the EEXIST case, presumably we have found the inode but we raced
>>> with another task in setting an nfsd_file for it in the hash.
>>> That's
>>> different from the case where the thing was unhashed or we got an
>>> EOPENSTALE. So, I think we probably don't require refinding the
>>> inode
>>> in that case.
>>>
>>> More pointedly, I'm not sure this particular case is actually
>>> possible.
>>> The entries are hashed on the inode pointer value, and we're
>>> searching
>>> and inserting into the hash under the i_lock.
>>>
>>> Chuck, thoughts?
>>
>> Is the question whether we want to dput() the dentry that
>> is attached to the fhp ? fh_verify's API contract says:
>>
>> 310 * Regardless of success or failure of fh_verify(), fh_put()
>> should be
>> 311 * called on @fhp when the caller is finished with the
>> filehandle.
>>
>> It looks like none of nfsd_file_acquire's callers do an
>> fh_put() in their error flows.
>>
>> But maybe I've misunderstood the issue.
>>
>
> Note that this API is weird and doesn't conform to typical get/put
> semantics.
>
> The fhp is instantiated before nfsd_file_do_acquire is called, and all
> of the callers that I can see do eventually call fh_put on it. fh_put
> is idempotent, so there should be no harm in calling it multiple times.
>
> My question here though was more about this EEXIST case. Should we even
> bother checking for that? I don't see how it's possible.
If memory serves, at one point nfsd_file_acquire() used
rhtable_insert_yada(), which returns -EEXIST for certain
table overflow cases. Possibly with the list version of
rhtable, consumers can't get -EEXIST at all.
>>>> On Wed, Jul 10, 2024 at 9:06 AM Jeff Layton <jlayton@xxxxxxxxxx>
>>>> wrote:
>>>>>
>>>>> At this point, we have a new nf that we couldn't properly
>>>>> insert
>>>>> into
>>>>> the hashtable. Just free it before retrying, since it was never
>>>>> hashed.
>>>>>
>>>>> Fixes: c6593366c0bf ("nfsd: don't kill nfsd_files because of
>>>>> lease
>>>>> break error")
>>>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>>>> ---
>>>>> fs/nfsd/filecache.c | 4 +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>>>> index f84913691b78..4fb5e8546831 100644
>>>>> --- a/fs/nfsd/filecache.c
>>>>> +++ b/fs/nfsd/filecache.c
>>>>> @@ -1038,8 +1038,10 @@ nfsd_file_do_acquire(struct svc_rqst
>>>>> *rqstp,
>>>>> struct svc_fh *fhp,
>>>>> if (likely(ret == 0))
>>>>> goto open_file;
>>>>>
>>>>> - if (ret == -EEXIST)
>>>>> + if (ret == -EEXIST) {
>>>>> + nfsd_file_free(nf);
>>>>> goto retry;
>>>>> + }
>>>>> trace_nfsd_file_insert_err(rqstp, inode, may_flags,
>>>>> ret);
>>>>> status = nfserr_jukebox;
>>>>> goto construction_err;
>>>>>
>>>>> --
>>>>> 2.45.2
>>>>>
>>>
>>> --
>>> Jeff Layton <jlayton@xxxxxxxxxx>
>>
>>
>> --
>> Chuck Lever
>>
>>
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
--
Chuck Lever
