On Wed, Sep 06, 2017 at 07:35:47AM +1000, NeilBrown wrote: > On Tue, Sep 05 2017, J. Bruce Fields wrote: > > > On Mon, Sep 04, 2017 at 02:52:43PM +1000, NeilBrown wrote: > >> 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. > > Ah yes, so we do. I inverted the logic in my mind. That makes it easier. (Minor derail: it might be worth waiting briefly before returning NFS4ERR_DELAY. It would be easy enough to implement, the hard part would be testing whether it helped. I think the initial client retry time is 100ms (NFS4_POLL_RETRY_MIN), so it'd have to beat that frequently enough.) > Thanks. Below is a patch that does compile but is probably wrong is > various ways and definitely needs cleanliness work at least. I provide > it just to be more concrete about my thinking. Gah, I hate having to patch every notify_change caller. But maybe I should get over that, the resulting logic is simpler. Anyway, stripping away all those callers: Right, the advantage is that this makes checking for conflicts simple and obvious: > diff --git a/fs/locks.c b/fs/locks.c > index afefeb4ad6de..231d93bfbdc1 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -1408,6 +1408,8 @@ static bool leases_conflict(struct file_lock *lease, struct file_lock *breaker) > return false; > if ((breaker->fl_flags & FL_DELEG) && (lease->fl_flags & FL_LEASE)) > return false; > + if (breaker->fl_owner && breaker->fl_owner == lease->fl_owner) > + return false; > return locks_conflict(breaker, lease); > } notify_change, vfs_unlink, etc., all get a new argument: > + * @owner: allow delegation to this owner to remain And, right, we need a way to lookup nfs4_file by inode: > +static struct nfs4_file * > +find_deleg_file_by_inode(struct inode *ino) (ignoring how we do it for now). > /* > * Called from nfsd_lookup and encode_dirent. Check if we have crossed > * a mount point. > @@ -455,7 +458,8 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, > .ia_size = iap->ia_size, > }; > > - host_err = notify_change(dentry, &size_attr, NULL); > + host_err = nfsd_conflicting_leases(dentry, rqstp); > + host_err = host_err ?: notify_change(dentry, &size_attr, nfsd_deleg_owner, NULL); And then you recall nfsd delegations and delegations held by (hypothetical) non-nfsd users separately, OK (also ignoring how). There are no such users currently, so nfsd could just pass NULL. --b.
