diff options
author | VMware, Inc <> | 2013-09-17 20:35:50 -0700 |
---|---|---|
committer | Dmitry Torokhov <dmitry.torokhov@gmail.com> | 2013-09-22 22:26:42 -0700 |
commit | 2946894f1783f7a735d2e3d3770853b9b29f95ea (patch) | |
tree | f3c3e6ad576203b0e06a1ab4471d748fc5f6637e | |
parent | 1a3eb34dd0ad5aa3e51d586cf1fcf310c1f4350e (diff) |
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>
-rw-r--r-- | open-vm-tools/modules/linux/vmhgfs/fsutil.c | 9 | ||||
-rw-r--r-- | open-vm-tools/modules/linux/vmhgfs/page.c | 130 |
2 files changed, 65 insertions, 74 deletions
diff --git a/open-vm-tools/modules/linux/vmhgfs/fsutil.c b/open-vm-tools/modules/linux/vmhgfs/fsutil.c index f298b8d7..2b1bcff5 100644 --- 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 5b682326..387bc02f 100644 --- 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; |