[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230919101438.rjmxut6l2d6ufwnj@quack3>
Date: Tue, 19 Sep 2023 12:14:38 +0200
From: Jan Kara <jack@...e.cz>
To: Zhihao Cheng <chengzhihao1@...wei.com>
Cc: tytso@....edu, jack@...e.com, linux-ext4@...r.kernel.org,
linux-kernel@...r.kernel.org, yi.zhang@...wei.com
Subject: Re: [PATCH v3] jbd2: Fix potential data lost in recovering journal
raced with synchronizing fs bdev
On Tue 19-09-23 09:25:25, Zhihao Cheng wrote:
> JBD2 makes sure journal data is fallen on fs device by sync_blockdev(),
> however, other process could intercept the EIO information from bdev's
> mapping, which leads journal recovering successful even EIO occurs during
> data written back to fs device.
>
> We found this problem in our product, iscsi + multipath is chosen for block
> device of ext4. Unstable network may trigger kpartx to rescan partitions in
> device mapper layer. Detailed process is shown as following:
>
> mount kpartx irq
> jbd2_journal_recover
> do_one_pass
> memcpy(nbh->b_data, obh->b_data) // copy data to fs dev from journal
> mark_buffer_dirty // mark bh dirty
> vfs_read
> generic_file_read_iter // dio
> filemap_write_and_wait_range
> __filemap_fdatawrite_range
> do_writepages
> block_write_full_folio
> submit_bh_wbc
> >> EIO occurs in disk <<
> end_buffer_async_write
> mark_buffer_write_io_error
> mapping_set_error
> set_bit(AS_EIO, &mapping->flags) // set!
> filemap_check_errors
> test_and_clear_bit(AS_EIO, &mapping->flags) // clear!
> err2 = sync_blockdev
> filemap_write_and_wait
> filemap_check_errors
> test_and_clear_bit(AS_EIO, &mapping->flags) // false
> err2 = 0
>
> Filesystem is mounted successfully even data from journal is failed written
> into disk, and ext4/ocfs2 could become corrupted.
>
> Fix it by comparing the wb_err state in fs block device before recovering
> and after recovering.
>
> Fetch a reproducer in [Link].
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217888
> Cc: stable@...r.kernel.org
> Signed-off-by: Zhihao Cheng <chengzhihao1@...wei.com>
> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
> ---
> v1->v3: Initialize wb_err. Untialized wb_err could be same with
> mapping->wb_err(eg. EIO without ERRSEQ_SEEN). When EIO
> occurs again, mapping->wb_err won't be changed, and wb_err
> is still same with mapping->wb_err.
Yeah, good catch. The patch still looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@...e.cz>
Honza
> fs/jbd2/recovery.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index c269a7d29a46..5b771a3d8d9a 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -289,6 +289,8 @@ int jbd2_journal_recover(journal_t *journal)
> journal_superblock_t * sb;
>
> struct recovery_info info;
> + errseq_t wb_err;
> + struct address_space *mapping;
>
> memset(&info, 0, sizeof(info));
> sb = journal->j_superblock;
> @@ -306,6 +308,9 @@ int jbd2_journal_recover(journal_t *journal)
> return 0;
> }
>
> + wb_err = 0;
> + mapping = journal->j_fs_dev->bd_inode->i_mapping;
> + errseq_check_and_advance(&mapping->wb_err, &wb_err);
> err = do_one_pass(journal, &info, PASS_SCAN);
> if (!err)
> err = do_one_pass(journal, &info, PASS_REVOKE);
> @@ -327,6 +332,9 @@ int jbd2_journal_recover(journal_t *journal)
>
> jbd2_journal_clear_revoke(journal);
> err2 = sync_blockdev(journal->j_fs_dev);
> + if (!err)
> + err = err2;
> + err2 = errseq_check_and_advance(&mapping->wb_err, &wb_err);
> if (!err)
> err = err2;
> /* Make sure all replayed data is on permanent storage */
> --
> 2.39.2
>
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists