Attachment 283231 Details for Bug 417961 – patch 14 -- Fix potential data corruption when writing out cached dirty pages

[patch] patch 14 -- Fix potential data corruption when writing out cached dirty pages

0014-BZ-329431-CIFS-Fix-potential-data-corruption-when.patch (text/plain), 9.13 KB, created by Jeff Layton on 2007-12-10 20:24:03 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeff Layton

Created: 2007-12-10 20:24:03 UTC

Size: 9.13 KB

patch

obsolete

>From 8f754805c02e3ad5ce4585a18295920971cebfc3 Mon Sep 17 00:00:00 2001
>From: Jeff Layton <jlayton@redhat.com>
>Date: Mon, 10 Dec 2007 13:17:16 -0500
>Subject: [RHEL5.2 PATCH 14/16] BZ#329431: [CIFS] Fix potential data corruption when writing out cached dirty pages
>
>Fix RedHat bug 329431
>
>The idea here is separate "conscious" from "unconscious" flushes.
>Conscious flushes are those due to a fsync() or close(). Unconscious
>ones are flushes that occur as a side effect of some other operation or
>due to memory pressure.
>
>Currently, when an error occurs during an unconscious flush (ENOSPC or
>EIO), we toss out the page and don't preserve that error to report to
>the user when a conscious flush occurs. If after the unconscious flush,
>there are no more dirty pages for the inode, the conscious flush will
>simply return success even though there were previous errors when writing
>out pages. This can lead to data corruption.
>
>The easiest way to reproduce this is to mount up a CIFS share that's
>very close to being full or where the user is very close to quota. mv
>a file to the share that's slightly larger than the quota allows. The
>writes will all succeed (since they go to pagecache). The mv will do a
>setattr to set the new file's attributes. This calls
>filemap_write_and_wait,
>which will return an error since all of the pages can't be written out.
>Then later, when the flush and release ops occur, there are no more
>dirty pages in pagecache for the file and those operations return 0. mv
>then assumes that the file was written out correctly and deletes the
>original.
>
>CIFS already has a write_behind_rc variable where it stores the results
>from earlier flushes, but that value is only reported in cifs_close.
>Since the VFS ignores the return value from the release operation, this
>isn't helpful. We should be reporting this error during the flush
>operation.
>
>This patch does the following:
>
>1) changes cifs_fsync to use filemap_write_and_wait and cifs_flush and also
>sync to check its return code. If it returns successful, they then check
>the value of write_behind_rc to see if an earlier flush had reported any
>errors. If so, they return that error and clear write_behind_rc.
>
>2) sets write_behind_rc in a few other places where pages are written
>out as a side effect of other operations and the code waits on them.
>
>3) changes cifs_setattr to only call filemap_write_and_wait for
>ATTR_SIZE changes.
>
>4) makes cifs_writepages accurately distinguish between EIO and ENOSPC
>errors when writing out pages.
>
>Some simple testing indicates that the patch works as expected and that
>it fixes the reproduceable known problem.
>
>Acked-by: Dave Kleikamp <shaggy@austin.rr.com>
>Signed-off-by: Jeff Layton <jlayton@redhat.com>
>Signed-off-by: Steve French <sfrench@us.ibm.com>
>---
> fs/cifs/cifsfs.c |    7 +++++--
> fs/cifs/file.c   |   36 ++++++++++++++++++------------------
> fs/cifs/inode.c  |   31 ++++++++++++++++++++-----------
> 3 files changed, 43 insertions(+), 31 deletions(-)
>
>diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>index ed8e77e..4c1c4f3 100644
>--- a/fs/cifs/cifsfs.c
>+++ b/fs/cifs/cifsfs.c
>@@ -301,6 +301,7 @@ cifs_alloc_inode(struct super_block *sb)
> 	cifs_inode->cifsAttrs = 0x20;	/* default */
> 	atomic_set(&cifs_inode->inUse, 0);
> 	cifs_inode->time = 0;
>+	cifs_inode->write_behind_rc = 0;
> 	/* Until the file is open and we have gotten oplock
> 	info back from the server, can not assume caching of
> 	file data or metadata */
>@@ -1004,7 +1005,7 @@ static int cifs_oplock_thread(void *dummyarg)
> 	struct cifsTconInfo *pTcon;
> 	struct inode *inode;
> 	__u16  netfid;
>-	int rc;
>+	int rc, waitrc = 0;
> 
> #if LINUX_VERSION_CODE > KERNEL_VERSION(2, 6, 22)
> 	set_freezable();
>@@ -1040,9 +1041,11 @@ static int cifs_oplock_thread(void *dummyarg)
> 					   filemap_fdatawrite(inode->i_mapping);
> 					if (CIFS_I(inode)->clientCanCacheRead
> 									 == 0) {
>-						filemap_fdatawait(inode->i_mapping);
>+						waitrc = filemap_fdatawait(inode->i_mapping);
> 						invalidate_remote_inode(inode);
> 					}
>+					if (rc == 0)
>+						rc = waitrc;
> 				} else
> 					rc = 0;
> 				/* mutex_unlock(&inode->i_mutex);*/
>diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>index 4b97e94..8648450 100644
>--- a/fs/cifs/file.c
>+++ b/fs/cifs/file.c
>@@ -160,14 +160,9 @@ static inline int cifs_open_inode_helper(struct inode *inode, struct file *file,
> #endif
> 		/* BB no need to lock inode until after invalidate
> 		   since namei code should already have it locked? */
>-#if LINUX_VERSION_CODE > KERNEL_VERSION(2, 6, 19)
>-			filemap_write_and_wait(file->f_path.dentry->d_inode->i_mapping);		   
>-#elif LINUX_VERSION_CODE > KERNEL_VERSION(2, 6, 15)
>-			filemap_write_and_wait(file->f_dentry->d_inode->i_mapping);
>-#else
>-			filemap_fdatawrite(file->f_dentry->d_inode->i_mapping);
>-			filemap_fdatawait(file->f_dentry->d_inode->i_mapping);
>-#endif
>+			rc = filemap_write_and_wait(file->f_dentry->d_inode->i_mapping);
>+			if (rc != 0)
>+				CIFS_I(file->f_dentry->d_inode)->write_behind_rc = rc;
> 		}
> 		cFYI(1, ("invalidating remote inode since open detected it "
> 			 "changed"));
>@@ -517,12 +512,9 @@ reopen_error_exit:
> 		pCifsInode = CIFS_I(inode);
> 		if (pCifsInode) {
> 			if (can_flush) {
>-#if LINUX_VERSION_CODE > KERNEL_VERSION(2, 6, 15)
>-				filemap_write_and_wait(inode->i_mapping);
>-#else
>-				filemap_fdatawrite(inode->i_mapping);
>-				filemap_fdatawait(inode->i_mapping);
>-#endif
>+				rc = filemap_write_and_wait(inode->i_mapping);
>+				if (rc != 0)
>+					CIFS_I(inode)->write_behind_rc = rc;
> 			/* temporarily disable caching while we
> 			   go to server to get inode info */
> 				pCifsInode->clientCanCacheAll = FALSE;
>@@ -1533,7 +1525,10 @@ retry:
> 						  rc, bytes_written));
> 					/* BB what if continued retry is
> 					   requested via mount flags? */
>-					set_bit(AS_EIO, &mapping->flags);
>+					if (rc == -ENOSPC)
>+						set_bit(AS_ENOSPC, &mapping->flags);
>+					else
>+						set_bit(AS_EIO, &mapping->flags);
> 				} else {
> 					cifs_stats_bytes_written(cifs_sb->tcon,
> 								 bytes_written);
>@@ -1678,9 +1673,11 @@ int cifs_fsync(struct file *file, struct dentry *dentry, int datasync)
> 	cFYI(1, ("Sync file - name: %s datasync: 0x%x",
> 		dentry->d_name.name, datasync));
> 
>-	rc = filemap_fdatawrite(inode->i_mapping);
>-	if (rc == 0)
>+	rc = filemap_write_and_wait(inode->i_mapping);
>+	if (rc == 0) {
>+		rc = CIFS_I(inode)->write_behind_rc;
> 		CIFS_I(inode)->write_behind_rc = 0;
>+	}
> 	FreeXid(xid);
> 	return rc;
> }
>@@ -1740,8 +1737,11 @@ int cifs_flush(struct file *file)
> 	   filemapfdatawrite appears easier for the time being */
> 
> 	rc = filemap_fdatawrite(inode->i_mapping);
>-	if (!rc) /* reset wb rc if we were able to write out dirty pages */
>+	/* reset wb rc if we were able to write out dirty pages */
>+	if (!rc) {
>+		rc = CIFS_I(inode)->write_behind_rc;
> 		CIFS_I(inode)->write_behind_rc = 0;
>+	}
> 
> 	cFYI(1, ("Flush inode %p file %p rc %d", inode, file, rc));
> 
>diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>index 0dc603d..1b85561 100644
>--- a/fs/cifs/inode.c
>+++ b/fs/cifs/inode.c
>@@ -1228,7 +1228,7 @@ cifs_rename_exit:
> int cifs_revalidate(struct dentry *direntry)
> {
> 	int xid;
>-	int rc = 0;
>+	int rc = 0, wbrc = 0;
> 	char *full_path;
> 	struct cifs_sb_info *cifs_sb;
> 	struct cifsInodeInfo *cifsInode;
>@@ -1332,7 +1332,9 @@ int cifs_revalidate(struct dentry *direntry)
> 	if (direntry->d_inode->i_mapping) {
> 		/* do we need to lock inode until after invalidate completes
> 		   below? */
>-		filemap_fdatawrite(direntry->d_inode->i_mapping);
>+		wbrc = filemap_fdatawrite(direntry->d_inode->i_mapping);
>+		if (wbrc)
>+			CIFS_I(direntry->d_inode)->write_behind_rc = wbrc;
> 	}
> 	if (invalidate_inode) {
> 	/* shrink_dcache not necessary now that cifs dentry ops
>@@ -1341,7 +1343,9 @@ int cifs_revalidate(struct dentry *direntry)
> 			shrink_dcache_parent(direntry); */
> 		if (S_ISREG(direntry->d_inode->i_mode)) {
> 			if (direntry->d_inode->i_mapping)
>-				filemap_fdatawait(direntry->d_inode->i_mapping);
>+				wbrc = filemap_fdatawait(direntry->d_inode->i_mapping);
>+				if (wbrc)
>+					CIFS_I(direntry->d_inode)->write_behind_rc = wbrc;
> 			/* may eventually have to do this for open files too */
> 			if (list_empty(&(cifsInode->openFileList))) {
> 				/* changed on server - flush read ahead pages */
>@@ -1498,15 +1502,20 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
> 
> 	/* BB check if we need to refresh inode from server now ? BB */
> 
>-	/* need to flush data before changing file size on server */
>-#if LINUX_VERSION_CODE > KERNEL_VERSION(2, 6, 15)
>-	filemap_write_and_wait(direntry->d_inode->i_mapping);
>-#else
>-	filemap_fdatawrite(direntry->d_inode->i_mapping);
>-	filemap_fdatawait(direntry->d_inode->i_mapping);
>-#endif
>-
> 	if (attrs->ia_valid & ATTR_SIZE) {
>+		/*
>+		   Flush data before changing file size on server. If the
>+		   flush returns error, store it to report later and continue.
>+		   BB: This should be smarter. Why bother flushing pages that
>+		   will be truncated anyway? Also, should we error out here if
>+		   the flush returns error?
>+		 */
>+		rc = filemap_write_and_wait(direntry->d_inode->i_mapping);
>+		if (rc != 0) {
>+			CIFS_I(direntry->d_inode)->write_behind_rc = rc;
>+			rc = 0;
>+		}
>+
> 		/* To avoid spurious oplock breaks from server, in the case of
> 		   inodes that we already have open, avoid doing path based
> 		   setting of file size if we can do it by handle.
>-- 
>1.5.3.3
>

Actions: View | Diff

Attachments on bug 417961: 283091 | 283101 | 283111 | 283121 | 283131 | 283141 | 283151 | 283161 | 283171 | 283181 | 283191 | 283201 | 283211 | 283221 | 283231 | 283241 | 283251