Re: [ext3] Changes to block device after an ext3 mount point has been remounted readonly

[Dmitry Monakhov <dmonakhov@openvz.org>]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

Jan Kara <jack@suse.cz> writes:

>> The fact is that I've been able to reproduce the problem on LVM block
>> devices, and sd* block devices so it's definitely not a loop device
>> specific problem.
>> 
>> By the way, I tried several other things other than "echo s
>> >/proc/sysrq_trigger" I tried multiple sync followed with a one minute
>> "sleep",
>> 
>> "echo 3 >/proc/sys/vm/drop_caches" seems to lower the chances of "hash
>> changes" but doesn't stops them.
>   Strange. When I use sync(1) in your script and use /dev/sda5 instead of a
> /dev/loop0, I cannot reproduce the problem (was running the script for
> something like an hour).
Theoretically some pages may exist after rw=>ro remount
because of generic race between write/sync, And they will be written
in by writepage if page already has buffers. This not happen in ext4
because. Each time it try to perform writepages it try to start_journal
and this result in EROFS.
The race bug will be closed some day but new one may appear again.

Let's be honest and change ext3 writepage like follows:
- check ROFS flag inside write page
- dump writepage's errors.


[-- Attachment #2: 0001-ext3-add-sanity-checks-to-writeback.patch --]
[-- Type: text/plain, Size: 3645 bytes --]

>From a7cadf8017626cd80fcd8ea5a0e4deff4f63e02e Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmonakhov@openvz.org>
Date: Wed, 24 Feb 2010 18:17:58 +0300
Subject: [PATCH] ext3: add sanity checks to writeback

There is theoretical possibility to perform writepage on
RO superblock. Add explicit check for what case.

In fact writepage may fail by a number of reasons.
This is really rare case but sill may result in data loss.
At least we have to dump a error message.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext3/inode.c |   40 +++++++++++++++++++++++++++++++++++-----
 1 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index 455e6e6..cf0e3aa 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -1536,6 +1536,11 @@ static int ext3_ordered_writepage(struct page *page,
 	if (ext3_journal_current_handle())
 		goto out_fail;
 
+	if (inode->i_sb->s_flags & MS_RDONLY) {
+		err = -EROFS;
+		goto out_fail;
+	}
+
 	if (!page_has_buffers(page)) {
 		create_empty_buffers(page, inode->i_sb->s_blocksize,
 				(1 << BH_Dirty)|(1 << BH_Uptodate));
@@ -1546,7 +1551,8 @@ static int ext3_ordered_writepage(struct page *page,
 				       NULL, buffer_unmapped)) {
 			/* Provide NULL get_block() to catch bugs if buffers
 			 * weren't really mapped */
-			return block_write_full_page(page, NULL, wbc);
+			ret = block_write_full_page(page, NULL, wbc);
+			goto out;
 		}
 	}
 	handle = ext3_journal_start(inode, ext3_writepage_trans_blocks(inode));
@@ -1584,12 +1590,17 @@ static int ext3_ordered_writepage(struct page *page,
 	err = ext3_journal_stop(handle);
 	if (!ret)
 		ret = err;
+out:
+	if (ret)
+		ext3_msg(inode->i_sb, KERN_CRIT, "%s: failed "
+			"%ld pages, ino %lu; err %d\n", __func__,
+			wbc->nr_to_write, inode->i_ino, ret);
 	return ret;
 
 out_fail:
 	redirty_page_for_writepage(wbc, page);
 	unlock_page(page);
-	return ret;
+	goto out;
 }
 
 static int ext3_writeback_writepage(struct page *page,
@@ -1603,12 +1614,18 @@ static int ext3_writeback_writepage(struct page *page,
 	if (ext3_journal_current_handle())
 		goto out_fail;
 
+	if (inode->i_sb->s_flags & MS_RDONLY) {
+		err = -EROFS;
+		goto out_fail;
+	}
+
 	if (page_has_buffers(page)) {
 		if (!walk_page_buffers(NULL, page_buffers(page), 0,
 				      PAGE_CACHE_SIZE, NULL, buffer_unmapped)) {
 			/* Provide NULL get_block() to catch bugs if buffers
 			 * weren't really mapped */
-			return block_write_full_page(page, NULL, wbc);
+			ret = block_write_full_page(page, NULL, wbc);
+			goto out;
 		}
 	}
 
@@ -1626,12 +1643,17 @@ static int ext3_writeback_writepage(struct page *page,
 	err = ext3_journal_stop(handle);
 	if (!ret)
 		ret = err;
+out:
+	if (ret)
+		ext3_msg(inode->i_sb, KERN_CRIT, "%s: failed "
+			"%ld pages, ino %lu; err %d\n", __func__,
+			wbc->nr_to_write, inode->i_ino, ret);
 	return ret;
 
 out_fail:
 	redirty_page_for_writepage(wbc, page);
 	unlock_page(page);
-	return ret;
+	goto out;
 }
 
 static int ext3_journalled_writepage(struct page *page,
@@ -1645,6 +1667,11 @@ static int ext3_journalled_writepage(struct page *page,
 	if (ext3_journal_current_handle())
 		goto no_write;
 
+	if (inode->i_sb->s_flags & MS_RDONLY) {
+		err = -EROFS;
+		goto no_write;
+	}
+
 	handle = ext3_journal_start(inode, ext3_writepage_trans_blocks(inode));
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);
@@ -1684,8 +1711,11 @@ static int ext3_journalled_writepage(struct page *page,
 	if (!ret)
 		ret = err;
 out:
+	if (ret)
+		ext3_msg(inode->i_sb, KERN_CRIT, "%s: failed "
+			"%ld pages, ino %lu; err %d\n", __func__,
+			wbc->nr_to_write, inode->i_ino, ret);
 	return ret;