On Fri, 15 Jul 2022, Jeff Layton wrote: > On Fri, 2022-07-15 at 09:59 +1000, NeilBrown wrote: > > > 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. 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.. Thanks, NeilBrown
