[PATCH] NFS: Fix the file size revalidation

 Instead of looking at whether or not the file is open for writes before
 we accept to update the length using the server value, we should rather
 be looking at whether or not we are currently caching any writes.

 Failure to do so means in particular that we're not updating the file
 length correctly after obtaining a POSIX or BSD lock.

 Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 68df803f27caaa5639c63dd2e707216c540f43d9..d6a30c844de303b60b1ac1302cd44ac413bb6023 100644 (file)
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -517,7 +517,7 @@ retry:
        result = tot_bytes;
 
 out:
-       nfs_end_data_update_defer(inode);
+       nfs_end_data_update(inode);
        nfs_writedata_free(wdata);
        return result;
 

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 6300e05e94635ca9a103f3121b6fcb7631f7a5d3..b2d16758ced86a6d5f608a3932927aea12000b81 100644 (file)
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1147,27 +1147,6 @@ void nfs_end_data_update(struct inode *inode)
        atomic_dec(&nfsi->data_updates);
 }
 
-/**
- * nfs_end_data_update_defer
- * @inode - pointer to inode
- * Declare end of the operations that will update file data
- * This will defer marking the inode as needing revalidation
- * unless there are no other pending updates.
- */
-void nfs_end_data_update_defer(struct inode *inode)
-{
-       struct nfs_inode *nfsi = NFS_I(inode);
-
-       if (atomic_dec_and_test(&nfsi->data_updates)) {
-               /* Mark the attribute cache for revalidation */
-               nfsi->flags |= NFS_INO_INVALID_ATTR;
-               /* Directories and symlinks: invalidate page cache too */
-               if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode))
-                       nfsi->flags |= NFS_INO_INVALID_DATA;
-               nfsi->cache_change_attribute ++;
-       }
-}
-
 /**
  * nfs_refresh_inode - verify consistency of the inode attribute cache
  * @inode - pointer to inode
@@ -1222,8 +1201,8 @@ int nfs_refresh_inode(struct inode *inode, struct nfs_fattr *fattr)
                if (!timespec_equal(&inode->i_mtime, &fattr->mtime)
                                || cur_size != new_isize)
                        nfsi->flags |= NFS_INO_INVALID_ATTR;
-       } else if (S_ISREG(inode->i_mode) && new_isize > cur_size)
-                       nfsi->flags |= NFS_INO_INVALID_ATTR;
+       } else if (new_isize != cur_size && nfsi->npages == 0)
+               nfsi->flags |= NFS_INO_INVALID_ATTR;
 
        /* Have any file permissions changed? */
        if ((inode->i_mode & S_IALLUGO) != (fattr->mode & S_IALLUGO)
@@ -1257,10 +1236,8 @@ int nfs_refresh_inode(struct inode *inode, struct nfs_fattr *fattr)
 static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr, unsigned long verifier)
 {
        struct nfs_inode *nfsi = NFS_I(inode);
-       __u64           new_size;
-       loff_t          new_isize;
+       loff_t cur_isize, new_isize;
        unsigned int    invalid = 0;
-       loff_t          cur_isize;
        int data_unstable;
 
        dfprintk(VFS, "NFS: %s(%s/%ld ct=%d info=0x%x)\n",
@@ -1293,49 +1270,39 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr, unsign
        /* Are we racing with known updates of the metadata on the server? */
        data_unstable = ! nfs_verify_change_attribute(inode, verifier);
 
-       /* Check if the file size agrees */
-       new_size = fattr->size;
+       /* Check if our cached file size is stale */
        new_isize = nfs_size_to_loff_t(fattr->size);
        cur_isize = i_size_read(inode);
-       if (cur_isize != new_size) {
-#ifdef NFS_DEBUG_VERBOSE
-               printk(KERN_DEBUG "NFS: isize change on %s/%ld\n", inode->i_sb->s_id, inode->i_ino);
-#endif
-               /*
-                * If we have pending writebacks, things can get
-                * messy.
-                */
-               if (S_ISREG(inode->i_mode) && data_unstable) {
-                       if (new_isize > cur_isize) {
+       if (new_isize != cur_isize) {
+               /* Do we perhaps have any outstanding writes? */
+               if (nfsi->npages == 0) {
+                       /* No, but did we race with nfs_end_data_update()? */
+                       if (verifier  ==  nfsi->cache_change_attribute) {
                                inode->i_size = new_isize;
-                               invalid |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA;
+                               invalid |= NFS_INO_INVALID_DATA;
                        }
-               } else {
+                       invalid |= NFS_INO_INVALID_ATTR;
+               } else if (new_isize > cur_isize) {
                        inode->i_size = new_isize;
                        invalid |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA;
                }
+               dprintk("NFS: isize change on server for file %s/%ld\n",
+                               inode->i_sb->s_id, inode->i_ino);
        }
 
-       /*
-        * Note: we don't check inode->i_mtime since pipes etc.
-        *       can change this value in VFS without requiring a
-        *       cache revalidation.
-        */
+       /* Check if the mtime agrees */
        if (!timespec_equal(&inode->i_mtime, &fattr->mtime)) {
                memcpy(&inode->i_mtime, &fattr->mtime, sizeof(inode->i_mtime));
-#ifdef NFS_DEBUG_VERBOSE
-               printk(KERN_DEBUG "NFS: mtime change on %s/%ld\n", inode->i_sb->s_id, inode->i_ino);
-#endif
+               dprintk("NFS: mtime change on server for file %s/%ld\n",
+                               inode->i_sb->s_id, inode->i_ino);
                if (!data_unstable)
                        invalid |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA;
        }
 
        if ((fattr->valid & NFS_ATTR_FATTR_V4)
            && nfsi->change_attr != fattr->change_attr) {
-#ifdef NFS_DEBUG_VERBOSE
-               printk(KERN_DEBUG "NFS: change_attr change on %s/%ld\n",
+               dprintk("NFS: change_attr change on server for file %s/%ld\n",
                       inode->i_sb->s_id, inode->i_ino);
-#endif
                nfsi->change_attr = fattr->change_attr;
                if (!data_unstable)
                        invalid |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL;

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 6f7a4af3bc46d9d7286b4c329005bd94b34331cc..c574d551f029f73ddc03afbf29fc769f4c10b945 100644 (file)
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -220,7 +220,7 @@ static int nfs_writepage_sync(struct nfs_open_context *ctx, struct inode *inode,
                ClearPageError(page);
 
 io_error:
-       nfs_end_data_update_defer(inode);
+       nfs_end_data_update(inode);
        nfs_writedata_free(wdata);
        return written ? written : result;
 }
@@ -401,7 +401,7 @@ static void nfs_inode_remove_request(struct nfs_page *req)
        nfsi->npages--;
        if (!nfsi->npages) {
                spin_unlock(&nfsi->req_lock);
-               nfs_end_data_update_defer(inode);
+               nfs_end_data_update(inode);
                iput(inode);
        } else
                spin_unlock(&nfsi->req_lock);

diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index c90313bfa4352e0b7ba8e1c895fdfa8f2dd0eccd..211266c56ce537d9df63c281f97e45121e3ffa6f 100644 (file)
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -294,7 +294,6 @@ extern void nfs_begin_attr_update(struct inode *);
 extern void nfs_end_attr_update(struct inode *);
 extern void nfs_begin_data_update(struct inode *);
 extern void nfs_end_data_update(struct inode *);
-extern void nfs_end_data_update_defer(struct inode *);
 extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, struct rpc_cred *cred);
 extern struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx);
 extern void put_nfs_open_context(struct nfs_open_context *ctx);