Re: [PATCH 0/8] NFSD: clean up locking.

[Chuck Lever III <chuck.lever@oracle.com>]

> On Jul 6, 2022, at 12:18 AM, NeilBrown <neilb@suse.de> 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