On Mon, Sep 04, 2017 at 02:52:43PM +1000, NeilBrown wrote: > On Fri, Sep 01 2017, J. Bruce Fields wrote: > > >> > >> nfsd would need to find that delegation, prevent further delegations > >> being handed out, and check that there aren't already conflicting > >> delegations. If there are conflicts, recall them. Once there are no > >> conflicting delegations, make the vfs_ request. > > > > The way that we currently serialize setting, unsetting, and breaking > > delegations is by locks on the delegated inodes which aren't taken till > > deeper in the vfs code. > > Do we? > I can see nfs4_set_delegation adding a new delegation for a new client > without entering the vfs at all if there is already a lease held. By "delegations", I meant locks of type FL_DELEG. But even then I was wrong, apologies. There is an inode_trylock in generic_add_lease that will prevent any new delegations from being given while the inode's locked. > If there isn't a lease already, vfs_setlease() is called, which doesn't > its own internal locking of course. Much the same applies to unsetting > delegations. > Breaking delegations involves nfsd_break_deleg_cb() which has a comment > that it is called with i_lock held.... that seems to be used to > be sure that it is safe to a reference to the delegation state id. > Is that the only dependency on the vfs locking, or did I miss something? > > > > > I guess you're suggesting adding a second mechanism to prevent > > delegations being given out on the inode. We could add an atomic > > counter taken by each nfsd breaker while it's in progress. Hrm. > > Something like that. > We would also need to be able to look up an nfs4_file by inode (why > *are* they hashed by file handle??) Grepping the logs.... That was ca9432178378 "nfsd: Use the filehandle to look up the struct nfs4_file instead of inode" which doesn't give a full justification. Later commits suggest it might be about keeping nfsv4 state in many-to-one filehandle->inode cases (spec requirement, I believe) and preventing the nfs4_file from pinning the inode (not seeing immediately why that was an issue). Anyway, I can't think of a reason why hashing the filehandle's a problem. > and add some wait queue somewhere > so the breaker could wait for a delegation to be returned. In the nfsd case we're just returning to the client immediately, so that's not really necessary, though maybe it could be useful. > My big-picture point is that any complexity created by NFSD's choice to > merge delegations to multiple clients into a single vfs-level delegation > should be handled by NFSD, and not imposed on the VFS. > It certainly makes sense for the VFS to understand that certain > operations are being performed by an fl_owner_t, and to allow > delegations to that owner to remain. It doesn't make as much sense for > the VFS to understand that there is a finer granularity of ownership > than the one that it already supports. Fair enough, I'll think about that. --b.
