| Message ID | c65905b1e2fdaddc6271cbf510d7e30c8790de63.1579874894.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFS: Revalidate once when holding a delegation | expand |
On Fri, 2020-01-24 at 09:09 -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@hammerspace.com> > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfs/dir.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index e180033e35cf..81a5a28d0015 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -1073,6 +1073,7 @@ int nfs_neg_need_reval(struct inode *dir, > struct dentry *dentry, > return !nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU); > } > > +#define NFS_DELEGATION_VERF 0xfeeddeaf > static int > nfs_lookup_revalidate_done(struct inode *dir, struct dentry *dentry, > struct inode *inode, int error) > @@ -1133,6 +1134,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 +1156,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)); > > @@ -1167,6 +1174,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 +1213,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 +1651,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 */ This patch needs to fix nfs_force_lookup_revalidate() to avoid the value NFS_DELEGATION_VERF.
On 24 Jan 2020, at 14:24, Trond Myklebust wrote: > On Fri, 2020-01-24 at 09:09 -0500, Benjamin Coddington wrote: > > This patch needs to fix nfs_force_lookup_revalidate() to avoid the > value NFS_DELEGATION_VERF. I don't understand why. Doesn't nfs_force_lookup_revalidate() just bump the directory's cache_change_attribute, which is value we don't care about at all here. This patch just sets a magic value on the dentry's d_time after a revalidation occurs for that dentry while holding a delegation. It doesn't care at all about the directory's change_attr. What am I missing? Ben
On Fri, 2020-01-24 at 20:33 -0500, Benjamin Coddington wrote: > On 24 Jan 2020, at 14:24, Trond Myklebust wrote: > > > On Fri, 2020-01-24 at 09:09 -0500, Benjamin Coddington wrote: > > > > This patch needs to fix nfs_force_lookup_revalidate() to avoid the > > value NFS_DELEGATION_VERF. > > I don't understand why. Doesn't nfs_force_lookup_revalidate() just > bump the > directory's cache_change_attribute, which is value we don't care > about at > all here. This patch just sets a magic value on the dentry's d_time > after a > revalidation occurs for that dentry while holding a delegation. It > doesn't > care at all about the directory's change_attr. > > What am I missing? Those calls to nfs_set_verifier(dentry, nfs_save_change_attribute(dir)) are storing the parent directory cache_change_attribute() in the d_time field of the child dentry. That's the normal value for the child dentry verifiers of that directory when there is no delegation. So to avoid collisions, you need to ensure that dir- >cache_change_attribute never takes the value NFS_DELEGATION_VERF.
On 25 Jan 2020, at 14:36, Trond Myklebust wrote: > On Fri, 2020-01-24 at 20:33 -0500, Benjamin Coddington wrote: >> On 24 Jan 2020, at 14:24, Trond Myklebust wrote: >> >>> On Fri, 2020-01-24 at 09:09 -0500, Benjamin Coddington wrote: >>> >>> This patch needs to fix nfs_force_lookup_revalidate() to avoid the >>> value NFS_DELEGATION_VERF. >> >> I don't understand why. Doesn't nfs_force_lookup_revalidate() just >> bump the >> directory's cache_change_attribute, which is value we don't care >> about at >> all here. This patch just sets a magic value on the dentry's d_time >> after a >> revalidation occurs for that dentry while holding a delegation. It >> doesn't >> care at all about the directory's change_attr. >> >> What am I missing? > > Those calls to nfs_set_verifier(dentry, nfs_save_change_attribute(dir)) > are storing the parent directory cache_change_attribute() in the d_time > field of the child dentry. That's the normal value for the child dentry > verifiers of that directory when there is no delegation. > > So to avoid collisions, you need to ensure that dir- > cache_change_attribute never takes the value NFS_DELEGATION_VERF. Ah - of course. A _complete_ fix. Thanks for helping me with the obvious. Ok, after not feeling good about adding a comparison and jump into that path - maybe its not as big a deal as I'm thinking, but ugh it seems gross for just this single case - I wondered about doing this other ways. As alternatives, I tried to get d_fsdata freed up for this case (there's already two users of that, it was ugly), and then playing with only some of the bits in d_time for flags (every approach I tried is slower than just checking and skipping NFS_DELEGATION_VERF). I thought maybe we could grow another d_flag for this sort of thing, but I doubt that will fly. Oh well, I'll just send a V2 that skips the magic value. Thanks again, Ben
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index e180033e35cf..81a5a28d0015 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1073,6 +1073,7 @@ int nfs_neg_need_reval(struct inode *dir, struct dentry *dentry, return !nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU); } +#define NFS_DELEGATION_VERF 0xfeeddeaf static int nfs_lookup_revalidate_done(struct inode *dir, struct dentry *dentry, struct inode *inode, int error) @@ -1133,6 +1134,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 +1156,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)); @@ -1167,6 +1174,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 +1213,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 +1651,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 */
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@hammerspace.com> Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/dir.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)