On Oct 13, 2022, at 6:14 PM, NeilBrown [off-list ref] wrote:

On Fri, 14 Oct 2022, Chuck Lever III wrote:

quoted

quoted

On Oct 12, 2022, at 5:18 PM, NeilBrown [off-list ref] wrote:

On Thu, 13 Oct 2022, Chuck Lever III wrote:

quoted

I think I stopped at the non-list variant of rhashtable because
using rhl was more complex, and the non-list variant seemed to
work fine. There's no architectural reason either file_hashtbl
or the filecache must use the non-list variant.

In any event, it's worth taking the trouble now to change the
nfs4_file implementation proposed here as you suggest.

If you like you could leave it as-is for now and I can provide a patch
to convert to rhl-tables later (won't be until late October).
There is one thing I would need to understand though: why are the
nfsd_files per-filehandle instead of per-inode?  There is probably a
good reason, but I cannot think of one.

I'm not clear on your offer: do you mean converting the nfsd_file
cache from rhashtable to rhl, or converting the proposed nfs4_file
rework? I had planned to do the latter myself and post a refresh.

Either/both.  Of course if you do the refresh, then I'll just review it.

Yep, I plan to repost, as part of addressing (your) review comments.

quoted

The nfsd_file_acquire API is the only place that seems to want a
filehandle, and it's just to lookup the underlying inode. Perhaps
I don't understand your question?

Sorry, I meant nfs4_files, not nfsd_file: find_file() and find_or_add_file().
Why is there one nfs4_file per filehandle

I can't answer that (yet), but I suspect there is some semantic
association between the [current] filehandle and a particular
state ID that makes this a sensible arrangement.

I see that there can be several nfsd_file per inode - in different
network namespaces, or with different credentials or different access
modes.

My impression is that is by design. Each namespace and unique
credential needs a distinct nfsd_file. Each nfsd_file acts like
an open file descriptor in user space.

That really needs to be fixed.

I'm not sure I agree, but maybe "that" and "fixed" are doing some
heavy lifting. Jeff might be able to add some insight on design
purpose, and we can write a patch that adds that as a documenting
comment somewhere.

--
Chuck Lever