Attachment 237941 Details for Bug 329431 – proposed upstream patch 3

[patch] proposed upstream patch 3

0001-CIFS-if-there-are-errors-writing-out-dirty-pages.patch (text/plain), 6.97 KB, created by Jeff Layton on 2007-10-25 19:54:06 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeff Layton

Created: 2007-10-25 19:54:06 UTC

Size: 6.97 KB

patch

obsolete

>From 86641bb7fc96aeec54ab04abc0dc31efa6632f70 Mon Sep 17 00:00:00 2001
>From: Jeff Layton <jlayton@redhat.com>
>Date: Thu, 25 Oct 2007 15:27:25 -0400
>Subject: [PATCH] [CIFS] if there are errors writing out dirty pages, make sure they're eventually reported to userspace
>
>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 errors when writing out
>those pages. This can lead to data corruption.
>
>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 changes cifs_flush and cifs_fsync to use
>filemap_write_and_wait and to check its return code. If it returns
>successful, it then checks the value of write_behind_rc to see if an
>earlier flush had reported any errors. If so, it returns that error and
>clears it. It also sets write_behind_rc in a few other places where
>pages are written out as a side effect.
>
>Signed-off-by: Jeff Layton <jlayton@redhat.com>
>---
> fs/cifs/cifsfs.c |    6 ++++--
> fs/cifs/file.c   |   26 +++++++++++++++++++-------
> fs/cifs/inode.c  |   21 +++++++++++++++------
> 3 files changed, 38 insertions(+), 15 deletions(-)
>
>diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>index a6fbea5..36c61f9 100644
>--- a/fs/cifs/cifsfs.c
>+++ b/fs/cifs/cifsfs.c
>@@ -850,7 +850,7 @@ static int cifs_oplock_thread(void *dummyarg)
> 	struct cifsTconInfo *pTcon;
> 	struct inode *inode;
> 	__u16  netfid;
>-	int rc;
>+	int rc, waitrc = 0;
> 
> 	set_freezable();
> 	do {
>@@ -882,9 +882,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 1e7e4c0..ca91e84 100644
>--- a/fs/cifs/file.c
>+++ b/fs/cifs/file.c
>@@ -130,7 +130,9 @@ static inline int cifs_open_inode_helper(struct inode *inode, struct file *file,
> 		if (file->f_path.dentry->d_inode->i_mapping) {
> 		/* BB no need to lock inode until after invalidate
> 		   since namei code should already have it locked? */
>-			filemap_write_and_wait(file->f_path.dentry->d_inode->i_mapping);
>+			rc = filemap_write_and_wait(file->f_path.dentry->d_inode->i_mapping);
>+			if (rc != 0)
>+				CIFS_I(file->f_path.dentry->d_inode)->write_behind_rc = rc;
> 		}
> 		cFYI(1, ("invalidating remote inode since open detected it "
> 			 "changed"));
>@@ -425,7 +427,9 @@ reopen_error_exit:
> 		pCifsInode = CIFS_I(inode);
> 		if (pCifsInode) {
> 			if (can_flush) {
>-				filemap_write_and_wait(inode->i_mapping);
>+				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;
>@@ -1336,7 +1340,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);
>@@ -1468,9 +1475,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;
> }
>@@ -1521,9 +1530,12 @@ int cifs_flush(struct file *file, fl_owner_t id)
> 	   unlock inode for writing
> 	   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 */
>+	rc = filemap_write_and_wait(inode->i_mapping);
>+	/* 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 9a5c0c9..b60bf02 100644
>--- a/fs/cifs/inode.c
>+++ b/fs/cifs/inode.c
>@@ -1234,7 +1234,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;
>@@ -1334,7 +1334,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
>@@ -1343,7 +1345,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 */
>@@ -1486,10 +1490,15 @@ 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 */
>-	filemap_write_and_wait(direntry->d_inode->i_mapping);
>-
> 	if (attrs->ia_valid & ATTR_SIZE) {
>+		/* need to flush data before changing file size on server */
>+		rc = filemap_write_and_wait(direntry->d_inode->i_mapping);
>+		if (rc != 0) {
>+			CIFS_I(direntry->d_inode)->write_behind_rc = rc;
>+			/* BB: is this correct, do we want to error out here? */
>+			goto cifs_setattr_exit;
>+		}
>+
> 		/* 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.2.1
>

Actions: View | Diff

Attachments on bug 329431: 225561 | 225571 | 225581 | 225591 | 225601 | 225611 | 225861 | 226081 | 228691 | 237941 | 259751