On Fri, 2022-07-15 at 09:59 +1000, NeilBrown wrote:

On Fri, 15 Jul 2022, Jeff Layton wrote:

quoted

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

Thanks, good to know. I wasn't sure how reliable that bool is. I guess
though that once you have a svc_fh, then you can more or less assume
that you have exclusive access to it for the life of the RPC being
processed.

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

Yeah, we probably don't need to lock the dir. That said, after your
patch series we already hold the i_rwsem on the parent at this point so
lookup_one_len is fine in this instance.

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.

Agreed, but that will take a bit more work. nfsd_lookup_dentry takes
several parameters that we don't currently have access to in
nfs4_set_delegation (e.g. the rqstp). Those will need to be plumbed
through several functions.

Thanks for working on this!

...and thank you for the locking cleanup! Getting rid of fh_lock/_unlock
is a really nice cleanup that makes it a lot more clear how this should
work.
-- 
Jeff Layton [off-list ref]