Re: [PATCH 2/3] fs: hide another detail of delegation logic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux