| Message ID | bcb2d38fe9c9bb15aeb9baa811aeb9a8697ea141.1560348835.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFS: Don't skip lookup when holding a delegation | expand |
On Wed, Jun 12, 2019 at 10:45:13AM -0400, Benjamin Coddington wrote: > If we skip lookup revalidation while holding a delegation, we might miss > that the file has changed directories on the server. The delegation should prevent the file disappearing from this directory, so if I've been following the discussion, the bug was due to overlooking the case where the change happened before we got the delegation. Given that history it seems worth calling out that case specifically? Maybe a comment along the lines of: /* * Note that the file can't move while we hold a * delegation. But this dentry could have been cached * before we got a delegation. So it's only safe to * skip revalidation when the parent directory is * unchanged: */ But maybe there's a pithier way to say that. --b. > The directory's > change attribute should still be checked against the dentry's d_time to > perform a complete revalidation. > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfs/dir.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index a71d0b42d160..10cc684dc082 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -1269,12 +1269,13 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, > goto out_bad; > } > > - if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) > - return nfs_lookup_revalidate_delegated(dir, dentry, inode); > - > /* Force a full look up iff the parent directory has changed */ > if (!(flags & (LOOKUP_EXCL | LOOKUP_REVAL)) && > nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) { > + > + if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) > + return nfs_lookup_revalidate_delegated(dir, dentry, inode); > + > error = nfs_lookup_verify_inode(inode, flags); > if (error) { > if (error == -ESTALE) > @@ -1707,9 +1708,6 @@ 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)) > - return nfs_lookup_revalidate_delegated(dir, dentry, inode); > - > /* NFS only supports OPEN on regular files */ > if (!S_ISREG(inode->i_mode)) > goto full_reval; > -- > 2.20.1
On Thu, Jun 13, 2019 at 11:00 AM J. Bruce Fields <bfields@fieldses.org> wrote: > > On Wed, Jun 12, 2019 at 10:45:13AM -0400, Benjamin Coddington wrote: > > If we skip lookup revalidation while holding a delegation, we might miss > > that the file has changed directories on the server. > > The delegation should prevent the file disappearing from this directory, > so if I've been following the discussion, the bug was due to overlooking > the case where the change happened before we got the delegation. Given > that history it seems worth calling out that case specifically? > > Maybe a comment along the lines of: > > /* > * Note that the file can't move while we hold a > * delegation. But this dentry could have been cached > * before we got a delegation. So it's only safe to > * skip revalidation when the parent directory is > * unchanged: > */ > > But maybe there's a pithier way to say that. What is preventing the file from disappearing from the directory while holding the delegation: is it the server's responsibility to recall the delegation when it gets a move or is it client's responsibility not to rely on the cached attributes? According to this patch it's client's responsibility, in the case, I find the working " file can't move" confusing as they imply to me that client can assume file isn't moved (ie, server will prevent it from happening). > > --b. > > > The directory's > > change attribute should still be checked against the dentry's d_time to > > perform a complete revalidation. > > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > > --- > > fs/nfs/dir.c | 10 ++++------ > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > > index a71d0b42d160..10cc684dc082 100644 > > --- a/fs/nfs/dir.c > > +++ b/fs/nfs/dir.c > > @@ -1269,12 +1269,13 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, > > goto out_bad; > > } > > > > - if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) > > - return nfs_lookup_revalidate_delegated(dir, dentry, inode); > > - > > /* Force a full look up iff the parent directory has changed */ > > if (!(flags & (LOOKUP_EXCL | LOOKUP_REVAL)) && > > nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) { > > + > > + if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) > > + return nfs_lookup_revalidate_delegated(dir, dentry, inode); > > + > > error = nfs_lookup_verify_inode(inode, flags); > > if (error) { > > if (error == -ESTALE) > > @@ -1707,9 +1708,6 @@ 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)) > > - return nfs_lookup_revalidate_delegated(dir, dentry, inode); > > - > > /* NFS only supports OPEN on regular files */ > > if (!S_ISREG(inode->i_mode)) > > goto full_reval; > > -- > > 2.20.1
On 13 Jun 2019, at 12:02, Olga Kornievskaia wrote: > On Thu, Jun 13, 2019 at 11:00 AM J. Bruce Fields > <bfields@fieldses.org> wrote: >> >> On Wed, Jun 12, 2019 at 10:45:13AM -0400, Benjamin Coddington wrote: >>> If we skip lookup revalidation while holding a delegation, we might >>> miss >>> that the file has changed directories on the server. >> >> The delegation should prevent the file disappearing from this >> directory, >> so if I've been following the discussion, the bug was due to >> overlooking >> the case where the change happened before we got the delegation. >> Given >> that history it seems worth calling out that case specifically? >> >> Maybe a comment along the lines of: >> >> /* >> * Note that the file can't move while we hold a >> * delegation. But this dentry could have been >> cached >> * before we got a delegation. So it's only safe to >> * skip revalidation when the parent directory is >> * unchanged: >> */ >> >> But maybe there's a pithier way to say that. I wish I had pith. I cannot improve on this comment.. I'm OK with or without it. > What is preventing the file from disappearing from the directory while > holding the delegation: is it the server's responsibility to recall > the delegation when it gets a move or is it client's responsibility > not to rely on the cached attributes? The server should recall the delegation if the file moves, since moving it means that additional calls to OPEN that file at that location should fail. The client shouldn't continue to independently handle those OPENs (unless by filehandle.. but how can the server know how the client intends to handle further delegated OPENs?) > According to this patch it's client's responsibility, in the case, I > find the working " file can't move" confusing as they imply to me that > client can assume file isn't moved (ie, server will prevent it from > happening). I think that it looks like its the client's responsibility only because the client lacks an easy way to determine what order delegations were acquired with respect to directory modifications. We could try to track all of that, but the structures and memory used would be hideous. Ben
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index a71d0b42d160..10cc684dc082 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1269,12 +1269,13 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, goto out_bad; } - if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) - return nfs_lookup_revalidate_delegated(dir, dentry, inode); - /* Force a full look up iff the parent directory has changed */ if (!(flags & (LOOKUP_EXCL | LOOKUP_REVAL)) && nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) { + + if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) + return nfs_lookup_revalidate_delegated(dir, dentry, inode); + error = nfs_lookup_verify_inode(inode, flags); if (error) { if (error == -ESTALE) @@ -1707,9 +1708,6 @@ 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)) - return nfs_lookup_revalidate_delegated(dir, dentry, inode); - /* NFS only supports OPEN on regular files */ if (!S_ISREG(inode->i_mode)) goto full_reval;
If we skip lookup revalidation while holding a delegation, we might miss that the file has changed directories on the server. The directory's change attribute should still be checked against the dentry's d_time to perform a complete revalidation. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/dir.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)