On Fri, 2022-07-15 at 09:59 +1000, NeilBrown wrote: > 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 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 <jlayton@xxxxxxxxxx>
