> On Jul 6, 2022, at 12:18 AM, NeilBrown <neilb@xxxxxxx> wrote:
>
> This series prepares NFSD to be able to adjust to work with a proposed
> patch which allows updates to directories to happen in parallel.
> This patch set changes the way directories are locked, so the present
> series cleans up some locking in nfsd.
>
> Specifically we remove fh_lock() and fh_unlock().
> These functions are problematic for a few reasons.
> - they are deliberately idempotent - setting or clearing a flag
> so that a second call does nothing. This makes locking errors harder,
> but it results in code that looks wrong ... and maybe sometimes is a
> little bit wrong.
> Early patches clean up several places where this idempotent nature of
> the functions is depended on, and so makes the code clearer.
>
> - they transparently call fh_fill_pre/post_attrs(), including at times
> when this is not necessary. Having the calls only when necessary is
> marginally more efficient, and arguably makes the code clearer.
>
> nfsd_lookup() currently always locks the directory, though often no lock
> is needed. So a patch in this series reduces this locking.
>
> There is an awkward case that could still be further improved.
> NFSv4 open needs to ensure the file is not renamed (or unlinked etc)
> between the moment when the open succeeds, and a later moment when a
> "lease" is attached to support a delegation. The handling of this lock
> is currently untidy, particularly when creating a file.
> It would probably be better to take a lease immediately after
> opening the file, and then discarding if after deciding not to provide a
> delegation.
>
> I have run fstests and cthon tests on this, but I wouldn't be surprised
> if there is a corner case that I've missed.
Hi Neil, thanks for (re)posting.
Let me make a few general comments here before I send out specific
review nits.
I'm concerned mostly with how this series can be adequately tested.
The two particular areas I'm worried about:
- There are some changes to NFSv2 code, which is effectively
fallow. I think I can run v2 tests, once we decide what tests
should be run.
- More critically, ("NFSD: reduce locking in nfsd_lookup()") does
some pretty heavy lifting. How should this change be tested?
Secondarily, the series adds more bells and whistles to the generic
NFSD VFS APIs on behalf of NFSv4-specific requirements. In particular:
- ("NFSD: change nfsd_create() to unlock directory before returning.")
makes some big changes to nfsd_create(). But that helper itself
is pretty small. Seems like cleaner code would result if NFSv4
had its own version of nfsd_create() to deal with the post-change
cases.
- ("NFSD: reduce locking in nfsd_lookup()") has a similar issue:
nfsd_lookup() is being changed such that its semantics are
substantially different for NFSv4 than for others. This is
possibly an indication that nfsd_lookup() should also be
duplicated into the NFSv4-specific code and the generic VFS
version should be left alone.
I would prefer the code duplication approach in both these cases,
unless you can convince me that is a bad idea.
Finally, with regard to the awkward case you mention above. The
NFSv4 OPEN code is a hairy mess, mostly because the protocol is
a Swiss army knife and our implementation has had small fixes
plastered onto it for many years. I won't be disappointed if
you don't manage to address the rename/unlink/delegation race
you mention above this time around. Just don't make it worse ;-)
Meanwhile we should start accruing some thoughts and designs
about how this code path needs to work.
> NeilBrown
>
>
> ---
>
> NeilBrown (8):
> NFSD: drop rqstp arg to do_set_nfs4_acl()
> NFSD: change nfsd_create() to unlock directory before returning.
> NFSD: always drop directory lock in nfsd_unlink()
> NFSD: only call fh_unlock() once in nfsd_link()
> NFSD: reduce locking in nfsd_lookup()
> NFSD: use explicit lock/unlock for directory ops
> NFSD: use (un)lock_inode instead of fh_(un)lock for file operations
> NFSD: discard fh_locked flag and fh_lock/fh_unlock
>
>
> fs/nfsd/nfs2acl.c | 6 +-
> fs/nfsd/nfs3acl.c | 4 +-
> fs/nfsd/nfs3proc.c | 21 ++---
> fs/nfsd/nfs4acl.c | 19 ++---
> fs/nfsd/nfs4proc.c | 106 +++++++++++++++---------
> fs/nfsd/nfs4state.c | 8 +-
> fs/nfsd/nfsfh.c | 3 +-
> fs/nfsd/nfsfh.h | 56 +------------
> fs/nfsd/nfsproc.c | 14 ++--
> fs/nfsd/vfs.c | 193 ++++++++++++++++++++++++++------------------
> fs/nfsd/vfs.h | 19 +++--
> 11 files changed, 238 insertions(+), 211 deletions(-)
>
> --
> Signature
>
--
Chuck Lever
