Re: [PATCH v2] NFS: Revalidate once when holding a delegation

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

 



On Tue, 2020-01-28 at 16:20 -0500, Trond Myklebust wrote:
> On Tue, 2020-01-28 at 15:21 -0500, Trond Myklebust wrote:
> > On Tue, 2020-01-28 at 10:56 -0500, Trond Myklebust wrote:
> > > On Tue, 2020-01-28 at 10:26 -0500, Benjamin Coddington wrote:
> > > > If we skip lookup revalidataion while holding a delegation, we
> > > > might
> > > > miss
> > > > that the file has changed directories on the server.  This can
> > > > happen
> > > > if
> > > > the file is moved in between the client caching a dentry and
> > > > obtaining a
> > > > delegation.  That can be reproduced on a single system with
> > > > this
> > > > bash:
> > > > 
> > > > mkdir -p /exports/dir{1,2}
> > > > exportfs -o rw localhost:/exports
> > > > mount -t nfs -ov4.1,noac localhost:/exports /mnt/localhost
> > > > 
> > > > touch /exports/dir1/fubar
> > > > 
> > > > cat /mnt/localhost/dir1/fubar
> > > > mv /mnt/localhost/dir{1,2}/fubar
> > > > 
> > > > mv /exports/dir{2,1}/fubar
> > > > 
> > > > cat /mnt/localhost/dir1/fubar
> > > > mv /mnt/localhost/dir{1,2}/fubar
> > > > 
> > > > In this example, the final `mv` will stat source and
> > > > destination
> > > > and
> > > > find
> > > > they are the same dentry.
> > > > 
> > > > To fix this without giving up the increased lookup performance
> > > > that
> > > > holding
> > > > a delegation provides, let's revalidate the dentry only once
> > > > after
> > > > obtaining a delegation by placing a magic value in the dentry's
> > > > verifier.
> > > > 
> > > > Suggested-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> > > > ---
> > > >  fs/nfs/dir.c | 22 ++++++++++++++++++++--
> > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > > index e180033e35cf..e9d07dcd6d6f 100644
> > > > --- a/fs/nfs/dir.c
> > > > +++ b/fs/nfs/dir.c
> > > > @@ -949,6 +949,7 @@ static int nfs_fsync_dir(struct file *filp,
> > > > loff_t start, loff_t end,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +#define NFS_DELEGATION_VERF 0xfeeddeaf
> > > >  /**
> > > >   * nfs_force_lookup_revalidate - Mark the directory as having
> > > > changed
> > > >   * @dir: pointer to directory inode
> > > > @@ -962,6 +963,8 @@ static int nfs_fsync_dir(struct file *filp,
> > > > loff_t start, loff_t end,
> > > >  void nfs_force_lookup_revalidate(struct inode *dir)
> > > >  {
> > > >  	NFS_I(dir)->cache_change_attribute++;
> > > > +	if (NFS_I(dir)->cache_change_attribute ==
> > > > NFS_DELEGATION_VERF)
> > > > +		NFS_I(dir)->cache_change_attribute++;
> > > 
> > > Actually, I think a slight modification to this can might be
> > > beneficial. If we change to the following:
> > > 
> > > 	if (unlikely(NFS_I(dir)->cache_change_attribute ==
> > > NFS_DELEGATION_VERF - 1))
> > > 		NFS_I(dir)->cache_change_attribute =
> > > NFS_DELEGATION_VERF + 1;
> > >  	else
> > > 		NFS_I(dir)->cache_change_attribute++;
> > > 
> > 
> > ...actually, it would be nice to clean that up too using a
> > declaration
> > of 'struct nfs_inode *nfsi = NFS_I(dir));'
> > 
> > > then that should ensure those readers of cache_change_attribute
> > > that
> > > do
> > > not use locking will also never see the value NFS_DELEGATION_VERF
> > > (assuming that gcc doesn't optimise the above to something
> > > weird).
> > > 
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(nfs_force_lookup_revalidate);
> > > >  
> > > > @@ -1133,6 +1136,7 @@ nfs_lookup_revalidate_dentry(struct inode
> > > > *dir,
> > > > struct dentry *dentry,
> > > >  	struct nfs_fh *fhandle;
> > > >  	struct nfs_fattr *fattr;
> > > >  	struct nfs4_label *label;
> > > > +	unsigned long verifier;
> > > >  	int ret;
> > > >  
> > > >  	ret = -ENOMEM;
> > > > @@ -1154,6 +1158,11 @@ nfs_lookup_revalidate_dentry(struct
> > > > inode
> > > > *dir, struct dentry *dentry,
> > > >  	if (nfs_refresh_inode(inode, fattr) < 0)
> > > >  		goto out;
> > > >  
> > > > +	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
> > > > +		verifier = NFS_DELEGATION_VERF;
> > > > +	else
> > > > +		verifier = nfs_save_change_attribute(dir);
> > > > +
> > > >  	nfs_setsecurity(inode, fattr, label);
> > > >  	nfs_set_verifier(dentry,
> > > > nfs_save_change_attribute(dir));
> > 
> > Oops! When reviewing, I missed this. Shouldn't the above be changed
> > to
> > nfs_set_verifier(dentry, verifier) ?
> 
> ...and on a similar vein: nfs_lookup_revalidate_delegated() needs to
> change so as to not reset the verifier...
> 
> Sorry for not catching that one either.

Not my day...

nfs_prime_dcache() will clobber the verifier too in the nfs_same_file()
case. That one also needs to set NFS_DELEGATION_VERF if there is a
delegation.

Perhaps add a helper function for that +
nfs_lookup_revalidate_dentry()?

> 
> > > >  
> > > > @@ -1167,6 +1176,15 @@ nfs_lookup_revalidate_dentry(struct
> > > > inode
> > > > *dir, struct dentry *dentry,
> > > >  	return nfs_lookup_revalidate_done(dir, dentry, inode,
> > > > ret);
> > > >  }
> > > >  
> > > > +static int nfs_delegation_matches_dentry(struct inode *dir,
> > > > +			struct dentry *dentry, struct inode
> > > > *inode)
> > > > +{
> > > > +	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)
> > > > &&
> > > > +		dentry->d_time == NFS_DELEGATION_VERF)
> > > > +		return 1;
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  /*
> > > >   * This is called every time the dcache has a lookup hit,
> > > >   * and we should check whether we can really trust that
> > > > @@ -1197,7 +1215,7 @@ nfs_do_lookup_revalidate(struct inode
> > > > *dir,
> > > > struct dentry *dentry,
> > > >  		goto out_bad;
> > > >  	}
> > > >  
> > > > -	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
> > > > +	if (nfs_delegation_matches_dentry(dir, dentry, inode))
> > > >  		return nfs_lookup_revalidate_delegated(dir,
> > > > dentry,
> > > > inode);
> > > >  
> > > >  	/* Force a full look up iff the parent directory has
> > > > changed */
> > > > @@ -1635,7 +1653,7 @@ nfs4_do_lookup_revalidate(struct inode
> > > > *dir,
> > > > struct dentry *dentry,
> > > >  	if (inode == NULL)
> > > >  		goto full_reval;
> > > >  
> > > > -	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
> > > > +	if (nfs_delegation_matches_dentry(dir, dentry, inode))
> > > >  		return nfs_lookup_revalidate_delegated(dir,
> > > > dentry,
> > > > inode);
> > > >  
> > > >  	/* NFS only supports OPEN on regular files */
-- 
Trond Myklebust
CTO, Hammerspace Inc
4300 El Camino Real, Suite 105
Los Altos, CA 94022
www.hammer.space





[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux