Re: [PATCH] jbd2: don't abort if flushing file data failed

[Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>]

Theodore Tso wrote:

> On Tue, Jul 29, 2008 at 02:53:36PM +0900, Hidehiro Kawai wrote:
> 
>>>[PATCH] ext4: don't read inode block if the buffer has a write error
>>>[PATCH] jbd2: don't abort if flushing file data failed
>>
>>Thank you for porting my ext3/jbd patches to ext4/jbd2.
>>I would appreciate if you port the remained two patch set (haven't
>>been posted yet), too.  Although I'll try to port them by myself. 
> 
> If they are the ones which I think you are referring to, I don't
> believe they apply to ext4 since we are now using the
> generic_writepages routines to flush data pages to disk during a
> journal commit.  I've checked those code paths, and I believe they do
> set AS_EIO correctly; I would appreciate it though if you could double
> check to make sure they are do everything that needs to be done to
> handle write errors correctly.

I tested this patch on 2.6.27-rc1, but it didn't work properly.
Then, I noticed two mistakes.

(1) journal_submit_data_buffers() just submits I/Os, doesn't wait
    their completion.  We have to check file data write errors
    after calling journal_finish_inode_data_buffers()
(2) AS_EIO is cleared by wait_on_page_writeback_range(), so we
    have to set it again

I attached the revised patch below.  journal_submit_data_buffers()
can return error, but it wouldn't be write I/O error AFAIK, so I
didn't touched.

Best regards,
-- 
Hidehiro Kawai
Hitachi, Systems Development Laboratory
Linux Technology Center


Subcject: [PATCH] jbd2: don't abort if flushing file data failed

In ordered mode, the current jbd2 aborts the journal if a file data buffer
has an error.  But this behavior is unintended, and we found that it has
been adopted accidentally.

This patch undoes it and just calls printk() instead of aborting the
journal.  Unlike a similar patch for ext3/jbd, file data buffers are
written via generic_writepages().  But we also need to set AS_EIO
into their mappings because wait_on_page_writeback_range() clears
AS_EIO before a user process sees it.

Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
---
 fs/jbd2/commit.c |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

Index: linux-2.6.27-rc1/fs/jbd2/commit.c
===================================================================
--- linux-2.6.27-rc1.orig/fs/jbd2/commit.c
+++ linux-2.6.27-rc1/fs/jbd2/commit.c
@@ -262,8 +262,18 @@ static int journal_finish_inode_data_buf
 		jinode->i_flags |= JI_COMMIT_RUNNING;
 		spin_unlock(&journal->j_list_lock);
 		err = filemap_fdatawait(jinode->i_vfs_inode->i_mapping);
-		if (!ret)
-			ret = err;
+		if (err) {
+			/*
+			 * Because AS_EIO is cleared by
+			 * wait_on_page_writeback_range(), set it again so
+			 * that user process can get -EIO from fsync().
+			 */
+			set_bit(AS_EIO,
+				&jinode->i_vfs_inode->i_mapping->flags);
+
+			if (!ret)
+				ret = err;
+		}
 		spin_lock(&journal->j_list_lock);
 		jinode->i_flags &= ~JI_COMMIT_RUNNING;
 		wake_up_bit(&jinode->i_flags, __JI_COMMIT_RUNNING);
@@ -670,8 +680,14 @@ start_journal_io:
 	 * commit block, which happens below in such setting.
 	 */
 	err = journal_finish_inode_data_buffers(journal, commit_transaction);
-	if (err)
-		jbd2_journal_abort(journal, err);
+	if (err) {
+		char b[BDEVNAME_SIZE];
+
+		printk(KERN_WARNING
+			"JBD2: Detected IO errors while flushing file data "
+			"on %s\n", bdevname(journal->j_fs_dev, b));
+		err = 0;
+	}
 
 	/* Lo and behold: we have just managed to send a transaction to
            the log.  Before we can commit it, wait for the IO so far to