> 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.
>> 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
