On Mon, 2022-07-18 at 13:21 +1000, NeilBrown wrote: > 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. 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 <jlayton@xxxxxxxxxx>
