On Mon, 2022-07-18 at 13:21 +1000, NeilBrown wrote:

On Fri, 15 Jul 2022, Jeff Layton wrote:

quoted

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

quoted

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.

But the only reason we hold i_rwsem at this point is to prevent renames
in the "opened existing file" case.  The "created new file" case holds
it as well just be be consistent with the first case.

If we "vet" the dentry, then we don't need the lock any more.  We can
then simplify nfsd_lookup_dentry() to always assume the dir is not
locked - so the "locked" arg can go, and nfsd_lookup() can lose the
"lock" arg and always return with the directory unlocked.

I'm tempted to add your patch to the front of my series.  The
inconsistency in locking can be fix by unlocking the directory before we
get even close to handing out a delegation - so the delegation never
sees a locked directory.

Hmm, ok. I suppose we don't necessarily have to care whether the thing
is locked before calling into nfsd_lookup_dentry. I'll take another stab
at fixing this in the kernel w/o your series. That'll make Chuck happy
too.

But right now I have a cold and don't trust myself to think clearly
enough to create code worth posting.  Hopefully I'll be thinking more
clearly later in the week.

While I'm here ...  is "vet" a good word?  The meaning is appropriate,
but I wonder if it would cause our friends for whom English isn't their
first language to stumble.  There are about 5 uses in the kernel at
present.

Would validate or verify be better?  Even though they are longer..

Good point. I'm all for helping out non-native English speakers. I'll
plan to change it to something less esoteric.
-- 
Jeff Layton [off-list ref]