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) ?
> >
> > @@ -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
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
