Re: [PATCH] NFS: Fix attribute cache revalidation

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

 

On 30.12.2015 01:02, Trond Myklebust wrote:

If a NFSv4 client uses the cache_consistency_bitmask in order to
request only information about the change attribute, timestamps and
size, then it has not revalidated all attributes, and hence the
attribute timeout timestamp should not be updated.

Reported-by: Donald Buczek <buczek@xxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
---
Hi Donald,

Can you please apply this on top of the other 2 patches and see if it
fixes your access() testcase?

Thanks!


  fs/nfs/inode.c | 51 +++++++++++++++++++++++++++++++++++++--------------
  1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index c7e8b87da5b2..7431feeb16f1 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1641,6 +1641,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
  	unsigned long invalid = 0;
  	unsigned long now = jiffies;
  	unsigned long save_cache_validity;
+	bool cache_revalidated = true;
dfprintk(VFS, "NFS: %s(%s/%lu fh_crc=0x%08x ct=%d info=0x%x)\n", 
  			__func__, inode->i_sb->s_id, inode->i_ino,
@@ -1702,22 +1703,28 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
  				nfs_force_lookup_revalidate(inode);
  			inode->i_version = fattr->change_attr;
  		}
-	} else
+	} else {
  		nfsi->cache_validity |= save_cache_validity;
+		cache_revalidated = false;
+	}
if (fattr->valid & NFS_ATTR_FATTR_MTIME) { 
  		memcpy(&inode->i_mtime, &fattr->mtime, sizeof(inode->i_mtime));
-	} else if (server->caps & NFS_CAP_MTIME)
+	} else if (server->caps & NFS_CAP_MTIME) {
  		nfsi->cache_validity |= save_cache_validity &
  				(NFS_INO_INVALID_ATTR
  				| NFS_INO_REVAL_FORCED);
+		cache_revalidated = false;
+	}
if (fattr->valid & NFS_ATTR_FATTR_CTIME) { 
  		memcpy(&inode->i_ctime, &fattr->ctime, sizeof(inode->i_ctime));
-	} else if (server->caps & NFS_CAP_CTIME)
+	} else if (server->caps & NFS_CAP_CTIME) {
  		nfsi->cache_validity |= save_cache_validity &
  				(NFS_INO_INVALID_ATTR
  				| NFS_INO_REVAL_FORCED);
+		cache_revalidated = false;
+	}
/* Check if our cached file size is stale */ 
  	if (fattr->valid & NFS_ATTR_FATTR_SIZE) {
@@ -1737,19 +1744,23 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
  					(long long)cur_isize,
  					(long long)new_isize);
  		}
-	} else
+	} else {
  		nfsi->cache_validity |= save_cache_validity &
  				(NFS_INO_INVALID_ATTR
  				| NFS_INO_REVAL_PAGECACHE
  				| NFS_INO_REVAL_FORCED);
+		cache_revalidated = false;
+	}
if (fattr->valid & NFS_ATTR_FATTR_ATIME) 
  		memcpy(&inode->i_atime, &fattr->atime, sizeof(inode->i_atime));
-	else if (server->caps & NFS_CAP_ATIME)
+	else if (server->caps & NFS_CAP_ATIME) {
  		nfsi->cache_validity |= save_cache_validity &
  				(NFS_INO_INVALID_ATIME
  				| NFS_INO_REVAL_FORCED);
+		cache_revalidated = false;
+	}
if (fattr->valid & NFS_ATTR_FATTR_MODE) { 
  		if ((inode->i_mode & S_IALLUGO) != (fattr->mode & S_IALLUGO)) {
@@ -1758,36 +1769,42 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
  			inode->i_mode = newmode;
  			invalid |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL;
  		}
-	} else if (server->caps & NFS_CAP_MODE)
+	} else if (server->caps & NFS_CAP_MODE) {
  		nfsi->cache_validity |= save_cache_validity &
  				(NFS_INO_INVALID_ATTR
  				| NFS_INO_INVALID_ACCESS
  				| NFS_INO_INVALID_ACL
  				| NFS_INO_REVAL_FORCED);
+		cache_revalidated = false;
+	}
if (fattr->valid & NFS_ATTR_FATTR_OWNER) { 
  		if (!uid_eq(inode->i_uid, fattr->uid)) {
  			invalid |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL;
  			inode->i_uid = fattr->uid;
  		}
-	} else if (server->caps & NFS_CAP_OWNER)
+	} else if (server->caps & NFS_CAP_OWNER) {
  		nfsi->cache_validity |= save_cache_validity &
  				(NFS_INO_INVALID_ATTR
  				| NFS_INO_INVALID_ACCESS
  				| NFS_INO_INVALID_ACL
  				| NFS_INO_REVAL_FORCED);
+		cache_revalidated = false;
+	}
if (fattr->valid & NFS_ATTR_FATTR_GROUP) { 
  		if (!gid_eq(inode->i_gid, fattr->gid)) {
  			invalid |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL;
  			inode->i_gid = fattr->gid;
  		}
-	} else if (server->caps & NFS_CAP_OWNER_GROUP)
+	} else if (server->caps & NFS_CAP_OWNER_GROUP) {
  		nfsi->cache_validity |= save_cache_validity &
  				(NFS_INO_INVALID_ATTR
  				| NFS_INO_INVALID_ACCESS
  				| NFS_INO_INVALID_ACL
  				| NFS_INO_REVAL_FORCED);
+		cache_revalidated = false;
+	}
if (fattr->valid & NFS_ATTR_FATTR_NLINK) { 
  		if (inode->i_nlink != fattr->nlink) {
@@ -1796,19 +1813,22 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
  				invalid |= NFS_INO_INVALID_DATA;
  			set_nlink(inode, fattr->nlink);
  		}
-	} else if (server->caps & NFS_CAP_NLINK)
+	} else if (server->caps & NFS_CAP_NLINK) {
  		nfsi->cache_validity |= save_cache_validity &
  				(NFS_INO_INVALID_ATTR
  				| NFS_INO_REVAL_FORCED);
+		cache_revalidated = false;
+	}
if (fattr->valid & NFS_ATTR_FATTR_SPACE_USED) { 
  		/*
  		 * report the blocks in 512byte units
  		 */
  		inode->i_blocks = nfs_calc_block_size(fattr->du.nfs3.used);
- 	}
-	if (fattr->valid & NFS_ATTR_FATTR_BLOCKS_USED)
+ 	} else if (fattr->valid & NFS_ATTR_FATTR_BLOCKS_USED)
  		inode->i_blocks = fattr->du.nfs2.blocks;
+	else
+		cache_revalidated = false;
/* Update attrtimeo value if we're out of the unstable period */ 
  	if (invalid & NFS_INO_INVALID_ATTR) {
@@ -1818,8 +1838,11 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
  		/* Set barrier to be more recent than all outstanding updates */
  		nfsi->attr_gencount = nfs_inc_attr_generation_counter();
  	} else {
-		if (!time_in_range_open(now, nfsi->attrtimeo_timestamp, nfsi->attrtimeo_timestamp + nfsi->attrtimeo)) {
-			if ((nfsi->attrtimeo <<= 1) > NFS_MAXATTRTIMEO(inode))
+		if (cache_revalidated &&
+		    !time_in_range_open(now, nfsi->attrtimeo_timestamp,
+				nfsi->attrtimeo_timestamp + nfsi->attrtimeo)) {
+			nfsi->attrtimeo <<= 1;
+			if (nfsi->attrtimeo > NFS_MAXATTRTIMEO(inode))
  				nfsi->attrtimeo = NFS_MAXATTRTIMEO(inode);
  			nfsi->attrtimeo_timestamp = now;
  		}
@@ -1829,7 +1852,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
  	}
/* Don't declare attrcache up to date if there were no attrs! */ 
-	if (fattr->valid != 0)
+	if (cache_revalidated)
  		invalid &= ~NFS_INO_INVALID_ATTR;
/* Don't invalidate the data if we were to blame */ 
Hi, Trond,

your three patches
[PATCH] NFSv4: Don't perform cached access checks before we've OPENed the file 
[PATCH] NFS: Ensure we revalidate attributes before using execute_ok()
[PATCH] NFS: Fix attribute cache revalidation
applied to your github master fix the user-visible problems (exec and access case). I currently don't have time to analyze the code or do more testing than running my test script, though. I hope I can apply these on our cluster nodes during the holiday and we'd have it on production systems in January.
Btw: There is a little whitespace error in the last patch (space before tab at the "} else if (fattr->valid & NFS_ATTR_FATTR_BLOCKS_USED)" line). 

Thank you very much & Happy New Year

  Donald

--
Donald Buczek
buczek@xxxxxxxxxxxxx
Tel: +49 30 8413 1433

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[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