[v2,3/5] ext4: detect metadata async write error when getting journal's write access
diff mbox series

Message ID 20200617115947.836221-4-yi.zhang@huawei.com
State New
Headers show
Series
  • ext4: fix inconsistency since reading old metadata from disk
Related show

Commit Message

zhangyi (F) June 17, 2020, 11:59 a.m. UTC
Although we have already introduce s_bdev_wb_err_work to detect and
handle async write metadata buffer error as soon as possible, there is
still a potential race that could lead to filesystem inconsistency,
which is the buffer may reading and re-writing out to journal before
s_bdev_wb_err_work run. So this patch detect bdev mapping->wb_err when
getting journal's write access and also mark the filesystem error if
something bad happened.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/ext4/ext4.h      |  4 ++++
 fs/ext4/ext4_jbd2.c | 34 +++++++++++++++++++++++++++++-----
 fs/ext4/page-io.c   | 12 +++++++++++-
 fs/ext4/super.c     | 17 +++++++++++++++++
 4 files changed, 61 insertions(+), 6 deletions(-)

Comments

Jan Kara June 17, 2020, 12:41 p.m. UTC | #1
On Wed 17-06-20 19:59:45, zhangyi (F) wrote:
> Although we have already introduce s_bdev_wb_err_work to detect and
> handle async write metadata buffer error as soon as possible, there is
> still a potential race that could lead to filesystem inconsistency,
> which is the buffer may reading and re-writing out to journal before
> s_bdev_wb_err_work run. So this patch detect bdev mapping->wb_err when
> getting journal's write access and also mark the filesystem error if
> something bad happened.
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>

So instead of all this, cannot we just do:

	if (work_pending(sbi->s_bdev_wb_err_work))
		flush_work(sbi->s_bdev_wb_err_work);

? And so we are sure the filesystem is aborted if the abort was pending?

								Honza
zhangyi (F) June 17, 2020, 1:44 p.m. UTC | #2
On 2020/6/17 20:41, Jan Kara wrote:
> On Wed 17-06-20 19:59:45, zhangyi (F) wrote:
>> Although we have already introduce s_bdev_wb_err_work to detect and
>> handle async write metadata buffer error as soon as possible, there is
>> still a potential race that could lead to filesystem inconsistency,
>> which is the buffer may reading and re-writing out to journal before
>> s_bdev_wb_err_work run. So this patch detect bdev mapping->wb_err when
>> getting journal's write access and also mark the filesystem error if
>> something bad happened.
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> 
> So instead of all this, cannot we just do:
> 
> 	if (work_pending(sbi->s_bdev_wb_err_work))
> 		flush_work(sbi->s_bdev_wb_err_work);
> 
> ? And so we are sure the filesystem is aborted if the abort was pending?
> 

Thanks for this suggestion. Yeah, we could do this, it depends on the second
patch, if we check and flush the pending work here, we could not use the
end_buffer_async_write() in ext4_end_buffer_async_write(), we need to open
coding ext4_end_buffer_async_write() and queue the error work before the
buffer is unlocked, or else the race is still there. Do you agree ?

Thanks,
Yi.
zhangyi (F) June 18, 2020, 3:53 a.m. UTC | #3
On 2020/6/17 21:44, zhangyi (F) wrote:
> On 2020/6/17 20:41, Jan Kara wrote:
>> On Wed 17-06-20 19:59:45, zhangyi (F) wrote:
>>> Although we have already introduce s_bdev_wb_err_work to detect and
>>> handle async write metadata buffer error as soon as possible, there is
>>> still a potential race that could lead to filesystem inconsistency,
>>> which is the buffer may reading and re-writing out to journal before
>>> s_bdev_wb_err_work run. So this patch detect bdev mapping->wb_err when
>>> getting journal's write access and also mark the filesystem error if
>>> something bad happened.
>>>
>>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>>
>> So instead of all this, cannot we just do:
>>
>> 	if (work_pending(sbi->s_bdev_wb_err_work))
>> 		flush_work(sbi->s_bdev_wb_err_work);
>>
>> ? And so we are sure the filesystem is aborted if the abort was pending?
>>
> 
> Thanks for this suggestion. Yeah, we could do this, it depends on the second
> patch, if we check and flush the pending work here, we could not use the
> end_buffer_async_write() in ext4_end_buffer_async_write(), we need to open
> coding ext4_end_buffer_async_write() and queue the error work before the
> buffer is unlocked, or else the race is still there. Do you agree ?
> 

Add one point, add work_pending check here may not safe. We need to make sure
the filesystem is aborted, so we need to wait the error handle work is finished,
but the work's pending bit is cleared before it start running. I think may
better to just invoke flush_work() here.

BTW, I also notice another race condition that may lead to inconsistency. In
bdev_try_to_free_page(), if we free a write error buffer before the worker
is finished, the jbd2 checkpoint procedure will miss this error and wrongly
think it has already been written to disk successfully, and finally it will
destroy the log and lead to inconsistency (the same to no-journal mode).
So I think the ninth patch in my v1 patch set is still needed. What do you
think?

Thanks,
Yi.

Patch
diff mbox series

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 2f22476f41d2..82ae41a828dd 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1519,6 +1519,10 @@  struct ext4_sb_info {
 	struct workqueue_struct *s_bdev_wb_err_wq;
 	struct work_struct s_bdev_wb_err_work;
 
+	/* Record the errseq of the backing block device */
+	errseq_t s_bdev_wb_err;
+	spinlock_t s_bdev_wb_lock;
+
 	/* timer for periodic error stats printing */
 	struct timer_list s_err_report;
 
diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index 0c76cdd44d90..66620324f019 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -198,16 +198,40 @@  static void ext4_journal_abort_handle(const char *caller, unsigned int line,
 int __ext4_journal_get_write_access(const char *where, unsigned int line,
 				    handle_t *handle, struct buffer_head *bh)
 {
+	struct super_block *sb = bh->b_bdev->bd_super;
 	int err = 0;
 
 	might_sleep();
 
-	if (ext4_handle_valid(handle)) {
-		err = jbd2_journal_get_write_access(handle, bh);
-		if (err)
-			ext4_journal_abort_handle(where, line, __func__, bh,
-						  handle, err);
+	/*
+	 * If the block device has write error flag, it may have failed to
+	 * async write out metadata buffers in the background but the error
+	 * handle worker hasn't been executed yet. In this case, we could
+	 * read old data from disk and write it out again, which may lead
+	 * to on-disk filesystem inconsistency.
+	 */
+	if (sb) {
+		struct address_space *mapping = sb->s_bdev->bd_inode->i_mapping;
+		struct ext4_sb_info *sbi = EXT4_SB(sb);
+
+		if (errseq_check(&mapping->wb_err, READ_ONCE(sbi->s_bdev_wb_err))) {
+			spin_lock(&sbi->s_bdev_wb_lock);
+			err = errseq_check_and_advance(&mapping->wb_err,
+						       &sbi->s_bdev_wb_err);
+			spin_unlock(&sbi->s_bdev_wb_lock);
+			if (err) {
+				ext4_error_err(sb, err,
+					       "Error while async write back metadata");
+				goto out;
+			}
+		}
 	}
+
+	if (ext4_handle_valid(handle))
+		err = jbd2_journal_get_write_access(handle, bh);
+out:
+	if (err)
+		ext4_journal_abort_handle(where, line, __func__, bh, handle, err);
 	return err;
 }
 
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 50aa8e26e38c..b2c3da4be2aa 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -569,13 +569,23 @@  void ext4_end_buffer_async_write_error(struct work_struct *work)
 {
 	struct ext4_sb_info *sbi = container_of(work,
 				struct ext4_sb_info, s_bdev_wb_err_work);
+	struct address_space *mapping = sbi->s_sb->s_bdev->bd_inode->i_mapping;
+	int err;
 
 	/*
 	 * If we failed to async write back metadata buffer, there is a risk
 	 * of filesystem inconsistency since we may read old metadata from the
 	 * disk successfully and write them out again.
 	 */
-	ext4_error_err(sbi->s_sb, -EIO, "Error while async write back metadata buffer");
+	if (errseq_check(&mapping->wb_err, READ_ONCE(sbi->s_bdev_wb_err))) {
+		spin_lock(&sbi->s_bdev_wb_lock);
+		err = errseq_check_and_advance(&mapping->wb_err,
+					       &sbi->s_bdev_wb_err);
+		spin_unlock(&sbi->s_bdev_wb_lock);
+		if (err)
+			ext4_error_err(sbi->s_sb, -EIO,
+				       "Error while async write back metadata buffer");
+	}
 }
 
 static void ext4_end_buffer_async_write(struct buffer_head *bh, int uptodate)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index f04b161a64a0..3e867ff452cd 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4715,6 +4715,15 @@  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	}
 #endif  /* CONFIG_QUOTA */
 
+	/*
+	 * Save the original bdev mapping's wb_err value which could be
+	 * used to detect the metadata async write error.
+	 */
+	spin_lock_init(&sbi->s_bdev_wb_lock);
+	if (!sb_rdonly(sb))
+		errseq_check_and_advance(&sb->s_bdev->bd_inode->i_mapping->wb_err,
+					 &sbi->s_bdev_wb_err);
+	sb->s_bdev->bd_super = sb;
 	EXT4_SB(sb)->s_mount_state |= EXT4_ORPHAN_FS;
 	ext4_orphan_cleanup(sb, es);
 	EXT4_SB(sb)->s_mount_state &= ~EXT4_ORPHAN_FS;
@@ -5580,6 +5589,14 @@  static int ext4_remount(struct super_block *sb, int *flags, char *data)
 				goto restore_opts;
 			}
 
+			/*
+			 * Update the original bdev mapping's wb_err value
+			 * which could be used to detect the metadata async
+			 * write error.
+			 */
+			errseq_check_and_advance(&sb->s_bdev->bd_inode->i_mapping->wb_err,
+						 &sbi->s_bdev_wb_err);
+
 			/*
 			 * Mounting a RDONLY partition read-write, so reread
 			 * and store the current valid flag.  (It may have