Re: 2.4: NFS client race causes data loss when appending

[Trond Myklebust]

>>>>> " " == Trond Myklebust <[e-mail]> writes:

     >  What we really want is to prevent nfs_refresh_inode() from
     > overwriting newer attribute information with older
     > information. How therefore about something like the appended
     > patch, that uses the ctime field to determine which attribute
     > information is obsolete?  I'm afraid it's not going to work too
     > well for Linux servers because of the shitty 1 second
     > resolution we have on (a|m|c)time, but it will help against
     > most non-Linux servers.

Hah... I of course managed to get the sign wrong on the last
patch. The following should work better, including with Linux servers.

Cheers,
  Trond

--- linux-2.4.17-pre4/fs/nfs/inode.c.orig	Thu Dec  6 02:27:46 2001
+++ linux-2.4.17-pre4/fs/nfs/inode.c	Thu Dec  6 21:45:29 2001
@@ -655,20 +655,8 @@
 			inode->i_op = &nfs_symlink_inode_operations;
 		else
 			init_special_inode(inode, inode->i_mode, fattr->rdev);
-		/*
-		 * Preset the size and mtime, as there's no need
-		 * to invalidate the caches.
-		 */ 
-		inode->i_size  = nfs_size_to_loff_t(fattr->size);
-		inode->i_mtime = nfs_time_to_secs(fattr->mtime);
-		inode->i_atime = nfs_time_to_secs(fattr->atime);
-		inode->i_ctime = nfs_time_to_secs(fattr->ctime);
-		NFS_CACHE_CTIME(inode) = fattr->ctime;
-		NFS_CACHE_MTIME(inode) = fattr->mtime;
-		NFS_CACHE_ISIZE(inode) = fattr->size;
-		NFS_ATTRTIMEO(inode) = NFS_MINATTRTIMEO(inode);
-		NFS_ATTRTIMEO_UPDATE(inode) = jiffies;
 		memcpy(&inode->u.nfs_i.fh, fh, sizeof(inode->u.nfs_i.fh));
+		NFS_CACHEINV(inode);
 	}
 	nfs_refresh_inode(inode, fattr);
 }
@@ -966,6 +954,37 @@
 }
 
 /*
+ * nfs_fattr_obsolete - Test if attribute data is newer than cached data
+ * @inode: inode
+ * @fattr: attributes to test
+ *
+ * Avoid stuffing the attribute cache with obsolete information.
+ * We always accept updates if the attribute cache timed out, or if
+ * fattr->ctime is newer than our cached value.
+ * If fattr->ctime matches the cached value, we still accept the update
+ * if there also is a reasonable match for the Weak Cache Consistency
+ * data. This is in order to cope with NFS servers with crap time
+ * resolution...
+ */
+static inline
+int nfs_fattr_obsolete(struct inode *inode, struct nfs_fattr *fattr)
+{
+	s64 cdif;
+
+	if (time_after(jiffies, NFS_READTIME(inode)+NFS_ATTRTIMEO(inode)))
+		goto out_valid;
+	if ((cdif = (s64)fattr->ctime - (s64)NFS_CACHE_CTIME(inode)) > 0)
+		goto out_valid;
+	/* Ugh... */
+	if (cdif == 0 && (fattr->valid & NFS_ATTR_WCC)
+	    && (s64)fattr->pre_ctime - (s64)NFS_CACHE_CTIME(inode) >= 0)
+		goto out_valid;
+	return -1;
+ out_valid:
+	return 0;
+}
+
+/*
  * Many nfs protocol calls return the new file attributes after
  * an operation.  Here we update the inode to reflect the state
  * of the server's inode.
@@ -1003,6 +1022,10 @@
 	if ((inode->i_mode & S_IFMT) != (fattr->mode & S_IFMT))
 		goto out_changed;
 
+	/* Avoid races */
+	if (nfs_fattr_obsolete(inode, fattr))
+		return 0;
+
  	new_mtime = fattr->mtime;
 	new_size = fattr->size;
  	new_isize = nfs_size_to_loff_t(fattr->size);