HGFS: Partial fix for corruption when using different file handles to the same file

File is corrupted while our customer using two threads to read or write a
file via Linux HGFS client. This is because the read handle interferes with
the writes by causing a revalidation of the inode's file attributes. These
were mishandled wrt to flushing out the cached pages. If the new attributes
were different for modification time or file size then the pages in the cache
were invalidated. This causes pages of valid data to be thrown away and the
writes lost. Therefore resulting in a file with gaps of blocks of zero bytes
where writes were not sent to the HGFS server.

This is fixed by replicating what NFS does in this regard, which is only invalidate
the cache if the HGFS server returned file size only differs from the cached
inode value and only then if the new size is greater.

Cleaned up the write begin and end which was initially causing problems due
to very buggy code. Have now based this on simplicity from fs/libfs.c and
the simple_write_begin/simple_write_end which shows what the minimal settings
should do handling writes to pages and partial page writes.
These can be viewed under you favorite linux source cross-reference website.

Signed-off-by: Dmitry Torokhov <dtor@vmware.com>

diff --git a/open-vm-tools/modules/linux/vmhgfs/fsutil.c b/open-vm-tools/modules/linux/vmhgfs/fsutil.c
index f298b8d7fd102e59b2b2019143cac3c2911e9c0d..2b1bcff515da4c3ed564e290ca573c706d23cf04 100644 (file)
--- a/open-vm-tools/modules/linux/vmhgfs/fsutil.c
+++ b/open-vm-tools/modules/linux/vmhgfs/fsutil.c
@@ -702,15 +702,16 @@ HgfsChangeFileAttributes(struct inode *inode,          // IN/OUT: Inode
       loff_t oldSize = compat_i_size_read(inode);
       inode->i_blocks = HgfsCalcBlockSize(attr->size);
       if (oldSize != attr->size) {
+         if (oldSize < attr->size) {
+            needInvalidate = TRUE;
+         }
          LOG(4, (KERN_DEBUG "VMware hgfs: HgfsChangeFileAttributes: new file "
                  "size: %"FMT64"u, old file size: %Lu\n", attr->size, oldSize));
-         needInvalidate = TRUE;
+         compat_i_size_write(inode, attr->size);
       }
-      compat_i_size_write(inode, attr->size);
    } else {
       LOG(4, (KERN_DEBUG "VMware hgfs: HgfsChangeFileAttributes: did not "
               "get file size\n"));
-      needInvalidate = TRUE;
    }
 
    if (attr->mask & HGFS_ATTR_VALID_ACCESS_TIME) {
@@ -730,11 +731,9 @@ HgfsChangeFileAttributes(struct inode *inode,          // IN/OUT: Inode
          LOG(4, (KERN_DEBUG "VMware hgfs: HgfsChangeFileAttributes: new mod "
                  "time: %ld:%lu, old mod time: %ld:%lu\n",
                  HGFS_PRINT_TIME(newTime), HGFS_PRINT_TIME(inode->i_mtime)));
-         needInvalidate = TRUE;
       }
       HGFS_SET_TIME(inode->i_mtime, attr->writeTime);
    } else {
-      needInvalidate = TRUE;
       LOG(4, (KERN_DEBUG "VMware hgfs: HgfsChangeFileAttributes: did not "
               "get mod time\n"));
       HGFS_SET_TIME(inode->i_mtime, HGFS_GET_CURRENT_TIME());

diff --git a/open-vm-tools/modules/linux/vmhgfs/page.c b/open-vm-tools/modules/linux/vmhgfs/page.c
index 5b68232697f7137ab0198f79191a1508ea91a2d9..387bc02ff566cc747b4aba3d5fb810ac66f8728c 100644 (file)
--- a/open-vm-tools/modules/linux/vmhgfs/page.c
+++ b/open-vm-tools/modules/linux/vmhgfs/page.c
@@ -64,9 +64,10 @@ static int HgfsDoWritepage(HgfsHandle handle,
                            struct page *page,
                            unsigned pageFrom,
                            unsigned pageTo);
-static void HgfsDoWriteBegin(struct page *page,
-                             unsigned pageFrom,
-                             unsigned pageTo);
+static int HgfsDoWriteBegin(struct file *file,
+                            struct page *page,
+                            unsigned pageFrom,
+                            unsigned pageTo);
 static int HgfsDoWriteEnd(struct file *file,
                           struct page *page,
                           unsigned pageFrom,
@@ -875,7 +876,7 @@ HgfsWritepage(struct page *page,             // IN: Page to write from
  *      Initialize the page if the file is to be appended.
  *
  * Results:
- *      None.
+ *    Zero on success, always.
  *
  * Side effects:
  *      None.
@@ -883,36 +884,38 @@ HgfsWritepage(struct page *page,             // IN: Page to write from
  *-----------------------------------------------------------------------------
  */
 
-static void
-HgfsDoWriteBegin(struct page *page,         // IN: Page to be written
+static int
+HgfsDoWriteBegin(struct file *file,         // IN: File to be written
+                 struct page *page,         // IN: Page to be written
                  unsigned pageFrom,         // IN: Starting page offset
                  unsigned pageTo)           // IN: Ending page offset
 {
-   loff_t offset;
-   loff_t currentFileSize;
-
    ASSERT(page);
 
-   offset = (loff_t)page->index << PAGE_CACHE_SHIFT;
-   currentFileSize = compat_i_size_read(page->mapping->host);
-
-   LOG(6, (KERN_DEBUG "VMware hgfs: %s: file size %Lu off %Lu: %u to %u\n", __func__,
-           currentFileSize, offset, pageFrom, pageTo));
-   /*
-    * If we are doing a partial write into a new page (beyond end of
-    * file), then intialize it. This allows other writes to this page
-    * to accumulate before we need to write it to the server.
-    */
-   if ((offset >= currentFileSize) ||
-       ((pageFrom == 0) && (offset + pageTo) >= currentFileSize)) {
-      void *kaddr = compat_kmap_atomic(page);
-
-      if (pageTo < PAGE_CACHE_SIZE) {
+   LOG(6, (KERN_DEBUG "VMware hgfs: %s: off %Lu: %u to %u\n", __func__,
+           (loff_t)page->index << PAGE_CACHE_SHIFT, pageFrom, pageTo));
+
+   if (!PageUptodate(page)) {
+      /*
+       * If we are doing a partial write into a new page (beyond end of
+       * file), then intialize it. This allows other writes to this page
+       * to accumulate before we need to write it to the server.
+       */
+      if (pageTo - pageFrom != PAGE_CACHE_SIZE) {
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 25)
+         zero_user_segments(page, 0, pageFrom, pageTo, PAGE_CACHE_SIZE);
+#else
+         void *kaddr = compat_kmap_atomic(page);
+         memset(kaddr, 0, pageFrom);
          memset(kaddr + pageTo, 0, PAGE_CACHE_SIZE - pageTo);
+         flush_dcache_page(page);
+         compat_kunmap_atomic(kaddr);
+#endif
       }
-      compat_kunmap_atomic(kaddr);
-      flush_dcache_page(page);
    }
+
+   LOG(6, (KERN_DEBUG "VMware hgfs: %s: returns 0\n", __func__));
+   return 0;
 }
 
 
@@ -927,7 +930,7 @@ HgfsDoWriteBegin(struct page *page,         // IN: Page to be written
  *      receiving the write.
  *
  * Results:
- *      Always zero.
+ *      On success zero, always.
  *
  * Side effects:
  *      None.
@@ -936,14 +939,12 @@ HgfsDoWriteBegin(struct page *page,         // IN: Page to be written
  */
 
 static int
-HgfsPrepareWrite(struct file *file,  // IN: Ignored
+HgfsPrepareWrite(struct file *file,  // IN: File to be written
                  struct page *page,  // IN: Page to prepare
                  unsigned pageFrom,  // IN: Beginning page offset
                  unsigned pageTo)    // IN: Ending page offset
 {
-   HgfsDoWriteBegin(page, pageFrom, pageTo);
-
-   return 0;
+   return HgfsDoWriteBegin(file, page, pageFrom, pageTo);
 }
 
 #else
@@ -982,6 +983,7 @@ HgfsWriteBegin(struct file *file,             // IN: File to be written
    unsigned pageFrom = pos & (PAGE_CACHE_SIZE - 1);
    unsigned pageTo = pageFrom + len;
    struct page *page;
+   int result;
 
    LOG(6, (KERN_WARNING "VMware hgfs: %s: (%s/%s(%ld), %u@%lld)\n",
            __func__, file->f_dentry->d_parent->d_name.name,
@@ -990,12 +992,22 @@ HgfsWriteBegin(struct file *file,             // IN: File to be written
 
    page = compat_grab_cache_page_write_begin(mapping, index, flags);
    if (page == NULL) {
-      return -ENOMEM;
+      result = -ENOMEM;
+      goto exit;
    }
    *pagePtr = page;
 
-   HgfsDoWriteBegin(page, pageFrom, pageTo);
-   return 0;
+   LOG(6, (KERN_DEBUG "VMware hgfs: %s: file size %Lu @ %Lu page %u to %u\n", __func__,
+         (loff_t)compat_i_size_read(page->mapping->host),
+         (loff_t)page->index << PAGE_CACHE_SHIFT,
+         pageFrom, pageTo));
+
+   result = HgfsDoWriteBegin(file, page, pageFrom, pageTo);
+   ASSERT(result == 0);
+
+exit:
+   LOG(6, (KERN_DEBUG "VMware hgfs: %s: return %d\n", __func__, result));
+   return result;
 }
 #endif
 
@@ -1027,59 +1039,35 @@ HgfsDoWriteEnd(struct file *file, // IN: File we're writing to
                loff_t writeTo,    // IN: File position to write to
                unsigned copied)   // IN: Number of bytes copied to the page
 {
-   HgfsHandle handle;
-   struct inode *inode;
    loff_t currentFileSize;
-   loff_t offset;
+   struct inode *inode;
 
    ASSERT(file);
    ASSERT(page);
    inode = page->mapping->host;
    currentFileSize = compat_i_size_read(inode);
-   offset = (loff_t)page->index << PAGE_CACHE_SHIFT;
 
    LOG(6, (KERN_WARNING "VMware hgfs: %s: (%s/%s(%ld), from %u to %u@%lld => %u)\n",
            __func__, file->f_dentry->d_parent->d_name.name,
            file->f_dentry->d_name.name,
            page->mapping->host->i_ino, pageFrom, pageTo, (long long) writeTo, copied));
 
-   if (writeTo > currentFileSize) {
-      compat_i_size_write(inode, writeTo);
-   }
-
-   /* We wrote a complete page, so it is up to date. */
-   if (copied == PAGE_CACHE_SIZE) {
-      SetPageUptodate(page);
-   }
-
    /*
-    * Check if this is a partial write to a new page, which was
-    * initialized in HgfsDoWriteBegin.
+    * Zero any uninitialised parts of the page, and then mark the page
+    * as up to date if it turns out that we're extending the file.
     */
-   if ((offset >= currentFileSize) ||
-       ((pageFrom == 0) && (writeTo >= currentFileSize))) {
+   if (!PageUptodate(page)) {
       SetPageUptodate(page);
    }
 
-   /*
-    * If the page is uptodate, then just mark it dirty and let
-    * the page cache write it when it wants to.
-    */
-   if (PageUptodate(page)) {
-      set_page_dirty(page);
-      return 0;
+   if (writeTo > currentFileSize) {
+      compat_i_size_write(inode, writeTo);
    }
 
-   /*
-    * We've recieved a partial write to page that is not uptodate, so
-    * do the write now while the page is still locked.  Another
-    * alternative would be to read the page in HgfsDoWriteBegin, which
-    * would make it uptodate (ie a complete cached page).
-    */
-   handle = FILE_GET_FI_P(file)->handle;
-   LOG(6, (KERN_WARNING "VMware hgfs: %s: writing to handle %u\n", __func__,
-           handle));
-   return HgfsDoWritepage(handle, page, pageFrom, pageTo);
+   set_page_dirty(page);
+
+   LOG(6, (KERN_WARNING "VMware hgfs: %s: return 0\n", __func__));
+   return 0;
 }
 
 
@@ -1161,7 +1149,7 @@ HgfsWriteEnd(struct file *file,              // IN: File to write
              void *clientData)               // IN: From write_begin, unused.
 {
    unsigned pageFrom = pos & (PAGE_CACHE_SIZE - 1);
-   unsigned pageTo = pageFrom + copied;
+   unsigned pageTo = pageFrom + len;
    loff_t writeTo = pos + copied;
    int ret;
 
@@ -1175,6 +1163,10 @@ HgfsWriteEnd(struct file *file,              // IN: File to write
            file->f_dentry->d_name.name,
            mapping->host->i_ino, len, (long long) pos, copied));
 
+   if (copied < len) {
+      zero_user_segment(page, pageFrom + copied, pageFrom + len);
+   }
+
    ret = HgfsDoWriteEnd(file, page, pageFrom, pageTo, writeTo, copied);
    if (ret == 0) {
       ret = copied;