2014-06-20 12:45:03

by Scott Mayhew

Subject: [PATCH] nfs: Fix handling of change_attr updates in nfs_update_inode()

When we detect that the change attribute has changed on the server, it's
only necessary to invalidate our caches if we're not holding a write
delegation. Otherwise, the change attribute is changing as the result
of our modifications and we can just silently update our copy.

Signed-off-by: Scott Mayhew <[email protected]>
---
fs/nfs/inode.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index c496f8a..186562d 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1563,15 +1563,17 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
/* More cache consistency checks */
if (fattr->valid & NFS_ATTR_FATTR_CHANGE) {
if (inode->i_version != fattr->change_attr) {
- dprintk("NFS: change_attr change on server for file %s/%ld\n",
- inode->i_sb->s_id, inode->i_ino);
- invalid |= NFS_INO_INVALID_ATTR
- | NFS_INO_INVALID_DATA
- | NFS_INO_INVALID_ACCESS
- | NFS_INO_INVALID_ACL
- | NFS_INO_REVAL_PAGECACHE;
- if (S_ISDIR(inode->i_mode))
- nfs_force_lookup_revalidate(inode);
+ if (!NFS_PROTO(inode)->have_delegation(inode, FMODE_WRITE)) {
+ dprintk("NFS: change_attr change on server for file %s/%ld\n",
+ inode->i_sb->s_id, inode->i_ino);
+ invalid |= NFS_INO_INVALID_ATTR
+ | NFS_INO_INVALID_DATA
+ | NFS_INO_INVALID_ACCESS
+ | NFS_INO_INVALID_ACL
+ | NFS_INO_REVAL_PAGECACHE;
+ if (S_ISDIR(inode->i_mode))
+ nfs_force_lookup_revalidate(inode);
+ }
inode->i_version = fattr->change_attr;
}
} else if (server->caps & NFS_CAP_CHANGE_ATTR)
--
1.9.3

2014-06-20 15:31:14

by Trond Myklebust

Subject: Re: [PATCH] nfs: Fix handling of change_attr updates in nfs_update_inode()

On Fri, Jun 20, 2014 at 8:45 AM, Scott Mayhew <[email protected]> wrote:
> When we detect that the change attribute has changed on the server, it's
> only necessary to invalidate our caches if we're not holding a write
> delegation. Otherwise, the change attribute is changing as the result
> of our modifications and we can just silently update our copy.
>
> Signed-off-by: Scott Mayhew <[email protected]>
> ---
> fs/nfs/inode.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index c496f8a..186562d 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -1563,15 +1563,17 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
> /* More cache consistency checks */
> if (fattr->valid & NFS_ATTR_FATTR_CHANGE) {
> if (inode->i_version != fattr->change_attr) {
> - dprintk("NFS: change_attr change on server for file %s/%ld\n",
> - inode->i_sb->s_id, inode->i_ino);
> - invalid |= NFS_INO_INVALID_ATTR
> - | NFS_INO_INVALID_DATA
> - | NFS_INO_INVALID_ACCESS
> - | NFS_INO_INVALID_ACL
> - | NFS_INO_REVAL_PAGECACHE;
> - if (S_ISDIR(inode->i_mode))
> - nfs_force_lookup_revalidate(inode);
> + if (!NFS_PROTO(inode)->have_delegation(inode, FMODE_WRITE)) {
> + dprintk("NFS: change_attr change on server for file %s/%ld\n",
> + inode->i_sb->s_id, inode->i_ino);
> + invalid |= NFS_INO_INVALID_ATTR
> + | NFS_INO_INVALID_DATA
> + | NFS_INO_INVALID_ACCESS
> + | NFS_INO_INVALID_ACL
> + | NFS_INO_REVAL_PAGECACHE;
> + if (S_ISDIR(inode->i_mode))
> + nfs_force_lookup_revalidate(inode);
> + }
> inode->i_version = fattr->change_attr;
> }
> } else if (server->caps & NFS_CAP_CHANGE_ATTR)
> --
> 1.9.3
>

Hi Scott,

This circumvents the INO_REVAL_FORCED flag, which is set in those
situations where the changes to the file are not a priori known to be
covered by the delegation.

In either case, we already have the check for ->have_delegation()
before deciding whether or not to apply the 'invalid' flags to
nfsi->cache_validity at the end of the function.

Cheers
Trond



--
Trond Myklebust

Linux NFS client maintainer, PrimaryData

[email protected]