On Fri, 15 Jul 2022, Jeff Layton wrote:

This is a respin of the patchset that I sent earlier today. I hit a
deadlock with that one because of the ambiguous locking.

This series is based on top of Neil's set entitled:

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

His patchset makes the locking in the nfsd4_open codepath much more
consistent, and this becomes a lot simpler to fix. Without that set
however, the state of the parent's i_rwsem is unclear after nfsd_lookup
is called, and I don't see a way to determine it reliably.

I haven't examined these patch very closely, but a few initial thoughts
are:

1/ Before my series, you can unambiguously tell if i_rwsem is held by
   checking fhp->fh_locked.  In fact, just call "fh_lock()", and you can
   then be sure the fh is locked, whether or not it was locked before
however...
2/ Do we really need to lock the parent?  If a rename or unlink happens
   after the lease was taken, the lease will be broken. So
    take lease.
    repeat lookup (locklessly)
    Check if lease has been broken
   Should provide all you need.

   You don't *need* to lock the directory to open an existing file and
   with my pending parallel-updates patch set, you only need a shared
   lock on the directory to create a file.  So I'd rather not be locking
   the directory at all to get a delegation

3/ When you vet the name you only do a lookup_one_len(), while
   nfsd_lookup_dentry() also calls nfsd_cross_mnt() as it is possible
   for a file to be mounted on.
   That means that if I did bind mount one file over another and export
   over NFSD, the file will never offer a delegation.
   This is a minor point, but I think it would be best to be as correct
   and consistent as possible.

Thanks for working on this!

NeilBrown

Jeff Layton (2):
  nfsd: drop fh argument from alloc_init_deleg
  nfsd: vet the opened dentry after setting a delegation

 fs/nfsd/nfs4state.c | 54 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 9 deletions(-)

-- 
2.36.1