[PATCH 0/5] jbd2: Add errseq to detect writeback

[PATCH 0/5] jbd2: Add errseq to detect writeback

[Zhihao Cheng]

According to discussions in [1], this patchset adds errseq in journal to
enable JDB2 detecting meatadata writeback error of fs dev. Then, orginal
checking mechanism could be removed.

[1] https://lore.kernel.org/all/20230908124317.2955345-1-chengzhihao1@huawei.com/T/

Zhihao Cheng (5):
  jbd2: Add errseq to detect client fs's bdev writeback error
  jbd2: Replace journal state flag by checking errseq
  jbd2: Remove unused 'JBD2_CHECKPOINT_IO_ERROR' and 'j_atomic_flags'
  jbd2: Abort journal when detecting metadata writeback error of fs dev
  ext4: Move ext4_check_bdev_write_error() into nojournal mode

 fs/ext4/ext4_jbd2.c   |  5 ++---
 fs/jbd2/checkpoint.c  | 11 -----------
 fs/jbd2/journal.c     | 11 ++++++-----
 fs/jbd2/recovery.c    |  7 +------
 fs/jbd2/transaction.c | 14 ++++++++++++++
 include/linux/jbd2.h  | 37 ++++++++++++++++++++++++++-----------
 6 files changed, 49 insertions(+), 36 deletions(-)

-- 
2.39.2

[PATCH 1/5] jbd2: Add errseq to detect client fs's bdev writeback error

[Zhihao Cheng]

Add errseq in journal, so that JBD2 can detect whether metadata is
successfully fallen on fs bdev. This patch adds detection in recovery
process to replace original solution(using local variable wb_err).

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Suggested-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/journal.c    |  1 +
 fs/jbd2/recovery.c   |  7 +------
 include/linux/jbd2.h | 26 ++++++++++++++++++++++++++
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 30dec2bd2ecc..a655d9a88f79 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1535,6 +1535,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	journal->j_fs_dev = fs_dev;
 	journal->j_blk_offset = start;
 	journal->j_total_len = len;
+	jbd2_init_fs_dev_write_error(journal);
 
 	err = journal_load_superblock(journal);
 	if (err)
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 01f744cb97a4..1f7664984d6e 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -289,8 +289,6 @@ 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;
@@ -308,9 +306,6 @@ 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);
@@ -334,7 +329,7 @@ int jbd2_journal_recover(journal_t *journal)
 	err2 = sync_blockdev(journal->j_fs_dev);
 	if (!err)
 		err = err2;
-	err2 = errseq_check_and_advance(&mapping->wb_err, &wb_err);
+	err2 = jbd2_check_fs_dev_write_error(journal);
 	if (!err)
 		err = err2;
 	/* Make sure all replayed data is on permanent storage */
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 52772c826c86..15798f88ade4 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -998,6 +998,13 @@ struct journal_s
 	 */
 	struct block_device	*j_fs_dev;
 
+	/**
+	 * @j_fs_dev_wb_err:
+	 *
+	 * Records the errseq of the client fs's backing block device.
+	 */
+	errseq_t		j_fs_dev_wb_err;
+
 	/**
 	 * @j_total_len: Total maximum capacity of the journal region on disk.
 	 */
@@ -1695,6 +1702,25 @@ static inline void jbd2_journal_abort_handle(handle_t *handle)
 	handle->h_aborted = 1;
 }
 
+static inline void jbd2_init_fs_dev_write_error(journal_t *journal)
+{
+	struct address_space *mapping = journal->j_fs_dev->bd_inode->i_mapping;
+
+	/*
+	 * Save the original wb_err value of client fs's bdev mapping which
+	 * could be used to detect the client fs's metadata async write error.
+	 */
+	errseq_check_and_advance(&mapping->wb_err, &journal->j_fs_dev_wb_err);
+}
+
+static inline int jbd2_check_fs_dev_write_error(journal_t *journal)
+{
+	struct address_space *mapping = journal->j_fs_dev->bd_inode->i_mapping;
+
+	return errseq_check(&mapping->wb_err,
+			    READ_ONCE(journal->j_fs_dev_wb_err));
+}
+
 #endif /* __KERNEL__   */
 
 /* Comparison functions for transaction IDs: perform comparisons using
-- 
2.39.2

[PATCH 2/5] jbd2: Replace journal state flag by checking errseq

[Zhihao Cheng]

Now JBD2 detects metadata writeback error of fs dev according to errseq.
Replace journal state flag by checking errseq.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Suggested-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/journal.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index a655d9a88f79..b60d19505f8a 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1850,7 +1850,7 @@ int jbd2_journal_update_sb_log_tail(journal_t *journal, tid_t tail_tid,
 
 	if (is_journal_aborted(journal))
 		return -EIO;
-	if (test_bit(JBD2_CHECKPOINT_IO_ERROR, &journal->j_atomic_flags)) {
+	if (jbd2_check_fs_dev_write_error(journal)) {
 		jbd2_journal_abort(journal, -EIO);
 		return -EIO;
 	}
@@ -2148,12 +2148,12 @@ int jbd2_journal_destroy(journal_t *journal)
 
 	/*
 	 * OK, all checkpoint transactions have been checked, now check the
-	 * write out io error flag and abort the journal if some buffer failed
-	 * to write back to the original location, otherwise the filesystem
-	 * may become inconsistent.
+	 * writeback errseq of fs dev and abort the journal if some buffer
+	 * failed to write back to the original location, otherwise the
+	 * filesystem may become inconsistent.
 	 */
 	if (!is_journal_aborted(journal) &&
-	    test_bit(JBD2_CHECKPOINT_IO_ERROR, &journal->j_atomic_flags))
+	    jbd2_check_fs_dev_write_error(journal))
 		jbd2_journal_abort(journal, -EIO);
 
 	if (journal->j_sb_buffer) {
-- 
2.39.2

[PATCH 3/5] jbd2: Remove unused 'JBD2_CHECKPOINT_IO_ERROR' and 'j_atomic_flags'

[Zhihao Cheng]

Since 'JBD2_CHECKPOINT_IO_ERROR' and j_atomic_flags' are not useful
anymore after fs dev's errseq is imported into jbd2, just remove them.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/jbd2/checkpoint.c | 11 -----------
 include/linux/jbd2.h | 11 -----------
 2 files changed, 22 deletions(-)

diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
index 118699fff2f9..1c97e64c4784 100644
--- a/fs/jbd2/checkpoint.c
+++ b/fs/jbd2/checkpoint.c
@@ -556,7 +556,6 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
 	struct transaction_chp_stats_s *stats;
 	transaction_t *transaction;
 	journal_t *journal;
-	struct buffer_head *bh = jh2bh(jh);
 
 	JBUFFER_TRACE(jh, "entry");
 
@@ -569,16 +568,6 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
 
 	JBUFFER_TRACE(jh, "removing from transaction");
 
-	/*
-	 * If we have failed to write the buffer out to disk, the filesystem
-	 * may become inconsistent. We cannot abort the journal here since
-	 * we hold j_list_lock and we have to be careful about races with
-	 * jbd2_journal_destroy(). So mark the writeback IO error in the
-	 * journal here and we abort the journal later from a better context.
-	 */
-	if (buffer_write_io_error(bh))
-		set_bit(JBD2_CHECKPOINT_IO_ERROR, &journal->j_atomic_flags);
-
 	__buffer_unlink(jh);
 	jh->b_cp_transaction = NULL;
 	percpu_counter_dec(&journal->j_checkpoint_jh_count);
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 15798f88ade4..bdde776b90d9 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -755,11 +755,6 @@ struct journal_s
 	 */
 	unsigned long		j_flags;
 
-	/**
-	 * @j_atomic_flags: Atomic journaling state flags.
-	 */
-	unsigned long		j_atomic_flags;
-
 	/**
 	 * @j_errno:
 	 *
@@ -1403,12 +1398,6 @@ JBD2_FEATURE_INCOMPAT_FUNCS(fast_commit,	FAST_COMMIT)
 #define JBD2_JOURNAL_FLUSH_VALID	(JBD2_JOURNAL_FLUSH_DISCARD | \
 					JBD2_JOURNAL_FLUSH_ZEROOUT)
 
-/*
- * Journal atomic flag definitions
- */
-#define JBD2_CHECKPOINT_IO_ERROR	0x001	/* Detect io error while writing
-						 * buffer back to disk */
-
 /*
  * Function declarations for the journaling transaction and buffer
  * management
-- 
2.39.2

[PATCH 4/5] jbd2: Abort journal when detecting metadata writeback error of fs dev

[Zhihao Cheng]

This is a replacement solution of commit bc71726c725767 ("ext4: abort
the filesystem if failed to async write metadata buffer"), JBD2 can
detects metadata writeback error of fs dev by 'j_fs_dev_wb_err'.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/jbd2/transaction.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 5f08b5fd105a..cb0b8d6fc0c6 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -1231,11 +1231,25 @@ static bool jbd2_write_access_granted(handle_t *handle, struct buffer_head *bh,
 int jbd2_journal_get_write_access(handle_t *handle, struct buffer_head *bh)
 {
 	struct journal_head *jh;
+	journal_t *journal;
 	int rc;
 
 	if (is_handle_aborted(handle))
 		return -EROFS;
 
+	journal = handle->h_transaction->t_journal;
+	if (jbd2_check_fs_dev_write_error(journal)) {
+		/*
+		 * If the fs dev has writeback errors, it may have failed
+		 * to async write out metadata buffers in the background.
+		 * In this case, we could read old data from disk and write
+		 * it out again, which may lead to on-disk filesystem
+		 * inconsistency. Aborting journal can avoid it happen.
+		 */
+		jbd2_journal_abort(journal, -EIO);
+		return -EIO;
+	}
+
 	if (jbd2_write_access_granted(handle, bh, false))
 		return 0;
 
-- 
2.39.2

[PATCH 5/5] ext4: Move ext4_check_bdev_write_error() into nojournal mode

[Zhihao Cheng]

Since JBD2 takes care of all metadata writeback errors of fs dev,
ext4_check_bdev_write_error() is useful only in nojournal mode.
Move it into '!ext4_handle_valid(handle)' branch.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Suggested-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4_jbd2.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index d1a2e6624401..5d8055161acd 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -235,8 +235,6 @@ int __ext4_journal_get_write_access(const char *where, unsigned int line,
 
 	might_sleep();
 
-	ext4_check_bdev_write_error(sb);
-
 	if (ext4_handle_valid(handle)) {
 		err = jbd2_journal_get_write_access(handle, bh);
 		if (err) {
@@ -244,7 +242,8 @@ int __ext4_journal_get_write_access(const char *where, unsigned int line,
 						  handle, err);
 			return err;
 		}
-	}
+	} else
+		ext4_check_bdev_write_error(sb);
 	if (trigger_type == EXT4_JTR_NONE || !ext4_has_metadata_csum(sb))
 		return 0;
 	BUG_ON(trigger_type >= EXT4_JOURNAL_TRIGGER_COUNT);
-- 
2.39.2

Re: [PATCH 0/5] jbd2: Add errseq to detect writeback

[Zhang Yi]

For series.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

On 2023/11/3 22:52, Zhihao Cheng wrote:
> According to discussions in [1], this patchset adds errseq in journal to
> enable JDB2 detecting meatadata writeback error of fs dev. Then, orginal
> checking mechanism could be removed.
> 
> [1] https://lore.kernel.org/all/20230908124317.2955345-1-chengzhihao1@huawei.com/T/
> 
> Zhihao Cheng (5):
>   jbd2: Add errseq to detect client fs's bdev writeback error
>   jbd2: Replace journal state flag by checking errseq
>   jbd2: Remove unused 'JBD2_CHECKPOINT_IO_ERROR' and 'j_atomic_flags'
>   jbd2: Abort journal when detecting metadata writeback error of fs dev
>   ext4: Move ext4_check_bdev_write_error() into nojournal mode
> 
>  fs/ext4/ext4_jbd2.c   |  5 ++---
>  fs/jbd2/checkpoint.c  | 11 -----------
>  fs/jbd2/journal.c     | 11 ++++++-----
>  fs/jbd2/recovery.c    |  7 +------
>  fs/jbd2/transaction.c | 14 ++++++++++++++
>  include/linux/jbd2.h  | 37 ++++++++++++++++++++++++++-----------
>  6 files changed, 49 insertions(+), 36 deletions(-)
> 

Re: [PATCH 1/5] jbd2: Add errseq to detect client fs's bdev writeback error

[Jan Kara]

On Fri 03-11-23 22:52:46, Zhihao Cheng wrote:
> Add errseq in journal, so that JBD2 can detect whether metadata is
> successfully fallen on fs bdev. This patch adds detection in recovery
               ^^^^^^^^^ written to

> process to replace original solution(using local variable wb_err).
> 
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> Suggested-by: Jan Kara <jack@suse.cz>

Otherwise the patch looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 30dec2bd2ecc..a655d9a88f79 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1535,6 +1535,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	journal->j_fs_dev = fs_dev;
>  	journal->j_blk_offset = start;
>  	journal->j_total_len = len;
> +	jbd2_init_fs_dev_write_error(journal);
>  
>  	err = journal_load_superblock(journal);
>  	if (err)
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 01f744cb97a4..1f7664984d6e 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -289,8 +289,6 @@ 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;
> @@ -308,9 +306,6 @@ 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);
> @@ -334,7 +329,7 @@ int jbd2_journal_recover(journal_t *journal)
>  	err2 = sync_blockdev(journal->j_fs_dev);
>  	if (!err)
>  		err = err2;
> -	err2 = errseq_check_and_advance(&mapping->wb_err, &wb_err);
> +	err2 = jbd2_check_fs_dev_write_error(journal);
>  	if (!err)
>  		err = err2;
>  	/* Make sure all replayed data is on permanent storage */
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 52772c826c86..15798f88ade4 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -998,6 +998,13 @@ struct journal_s
>  	 */
>  	struct block_device	*j_fs_dev;
>  
> +	/**
> +	 * @j_fs_dev_wb_err:
> +	 *
> +	 * Records the errseq of the client fs's backing block device.
> +	 */
> +	errseq_t		j_fs_dev_wb_err;
> +
>  	/**
>  	 * @j_total_len: Total maximum capacity of the journal region on disk.
>  	 */
> @@ -1695,6 +1702,25 @@ static inline void jbd2_journal_abort_handle(handle_t *handle)
>  	handle->h_aborted = 1;
>  }
>  
> +static inline void jbd2_init_fs_dev_write_error(journal_t *journal)
> +{
> +	struct address_space *mapping = journal->j_fs_dev->bd_inode->i_mapping;
> +
> +	/*
> +	 * Save the original wb_err value of client fs's bdev mapping which
> +	 * could be used to detect the client fs's metadata async write error.
> +	 */
> +	errseq_check_and_advance(&mapping->wb_err, &journal->j_fs_dev_wb_err);
> +}
> +
> +static inline int jbd2_check_fs_dev_write_error(journal_t *journal)
> +{
> +	struct address_space *mapping = journal->j_fs_dev->bd_inode->i_mapping;
> +
> +	return errseq_check(&mapping->wb_err,
> +			    READ_ONCE(journal->j_fs_dev_wb_err));
> +}
> +
>  #endif /* __KERNEL__   */
>  
>  /* Comparison functions for transaction IDs: perform comparisons using
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/5] jbd2: Replace journal state flag by checking errseq

[Jan Kara]

On Fri 03-11-23 22:52:47, Zhihao Cheng wrote:
> Now JBD2 detects metadata writeback error of fs dev according to errseq.
> Replace journal state flag by checking errseq.
> 
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> Suggested-by: Jan Kara <jack@suse.cz>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/jbd2/journal.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index a655d9a88f79..b60d19505f8a 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1850,7 +1850,7 @@ int jbd2_journal_update_sb_log_tail(journal_t *journal, tid_t tail_tid,
>  
>  	if (is_journal_aborted(journal))
>  		return -EIO;
> -	if (test_bit(JBD2_CHECKPOINT_IO_ERROR, &journal->j_atomic_flags)) {
> +	if (jbd2_check_fs_dev_write_error(journal)) {
>  		jbd2_journal_abort(journal, -EIO);
>  		return -EIO;
>  	}
> @@ -2148,12 +2148,12 @@ int jbd2_journal_destroy(journal_t *journal)
>  
>  	/*
>  	 * OK, all checkpoint transactions have been checked, now check the
> -	 * write out io error flag and abort the journal if some buffer failed
> -	 * to write back to the original location, otherwise the filesystem
> -	 * may become inconsistent.
> +	 * writeback errseq of fs dev and abort the journal if some buffer
> +	 * failed to write back to the original location, otherwise the
> +	 * filesystem may become inconsistent.
>  	 */
>  	if (!is_journal_aborted(journal) &&
> -	    test_bit(JBD2_CHECKPOINT_IO_ERROR, &journal->j_atomic_flags))
> +	    jbd2_check_fs_dev_write_error(journal))
>  		jbd2_journal_abort(journal, -EIO);
>  
>  	if (journal->j_sb_buffer) {
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/5] jbd2: Remove unused 'JBD2_CHECKPOINT_IO_ERROR' and 'j_atomic_flags'

[Jan Kara]

On Fri 03-11-23 22:52:48, Zhihao Cheng wrote:
> Since 'JBD2_CHECKPOINT_IO_ERROR' and j_atomic_flags' are not useful
> anymore after fs dev's errseq is imported into jbd2, just remove them.
> 
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>

Nice! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/jbd2/checkpoint.c | 11 -----------
>  include/linux/jbd2.h | 11 -----------
>  2 files changed, 22 deletions(-)
> 
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 118699fff2f9..1c97e64c4784 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -556,7 +556,6 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
>  	struct transaction_chp_stats_s *stats;
>  	transaction_t *transaction;
>  	journal_t *journal;
> -	struct buffer_head *bh = jh2bh(jh);
>  
>  	JBUFFER_TRACE(jh, "entry");
>  
> @@ -569,16 +568,6 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
>  
>  	JBUFFER_TRACE(jh, "removing from transaction");
>  
> -	/*
> -	 * If we have failed to write the buffer out to disk, the filesystem
> -	 * may become inconsistent. We cannot abort the journal here since
> -	 * we hold j_list_lock and we have to be careful about races with
> -	 * jbd2_journal_destroy(). So mark the writeback IO error in the
> -	 * journal here and we abort the journal later from a better context.
> -	 */
> -	if (buffer_write_io_error(bh))
> -		set_bit(JBD2_CHECKPOINT_IO_ERROR, &journal->j_atomic_flags);
> -
>  	__buffer_unlink(jh);
>  	jh->b_cp_transaction = NULL;
>  	percpu_counter_dec(&journal->j_checkpoint_jh_count);
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 15798f88ade4..bdde776b90d9 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -755,11 +755,6 @@ struct journal_s
>  	 */
>  	unsigned long		j_flags;
>  
> -	/**
> -	 * @j_atomic_flags: Atomic journaling state flags.
> -	 */
> -	unsigned long		j_atomic_flags;
> -
>  	/**
>  	 * @j_errno:
>  	 *
> @@ -1403,12 +1398,6 @@ JBD2_FEATURE_INCOMPAT_FUNCS(fast_commit,	FAST_COMMIT)
>  #define JBD2_JOURNAL_FLUSH_VALID	(JBD2_JOURNAL_FLUSH_DISCARD | \
>  					JBD2_JOURNAL_FLUSH_ZEROOUT)
>  
> -/*
> - * Journal atomic flag definitions
> - */
> -#define JBD2_CHECKPOINT_IO_ERROR	0x001	/* Detect io error while writing
> -						 * buffer back to disk */
> -
>  /*
>   * Function declarations for the journaling transaction and buffer
>   * management
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/5] jbd2: Abort journal when detecting metadata writeback error of fs dev

[Jan Kara]

On Fri 03-11-23 22:52:49, Zhihao Cheng wrote:
> This is a replacement solution of commit bc71726c725767 ("ext4: abort
> the filesystem if failed to async write metadata buffer"), JBD2 can
> detects metadata writeback error of fs dev by 'j_fs_dev_wb_err'.
  ^^^ detect

> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>

Otherwise looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/jbd2/transaction.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 5f08b5fd105a..cb0b8d6fc0c6 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -1231,11 +1231,25 @@ static bool jbd2_write_access_granted(handle_t *handle, struct buffer_head *bh,
>  int jbd2_journal_get_write_access(handle_t *handle, struct buffer_head *bh)
>  {
>  	struct journal_head *jh;
> +	journal_t *journal;
>  	int rc;
>  
>  	if (is_handle_aborted(handle))
>  		return -EROFS;
>  
> +	journal = handle->h_transaction->t_journal;
> +	if (jbd2_check_fs_dev_write_error(journal)) {
> +		/*
> +		 * If the fs dev has writeback errors, it may have failed
> +		 * to async write out metadata buffers in the background.
> +		 * In this case, we could read old data from disk and write
> +		 * it out again, which may lead to on-disk filesystem
> +		 * inconsistency. Aborting journal can avoid it happen.
> +		 */
> +		jbd2_journal_abort(journal, -EIO);
> +		return -EIO;
> +	}
> +
>  	if (jbd2_write_access_granted(handle, bh, false))
>  		return 0;
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 5/5] ext4: Move ext4_check_bdev_write_error() into nojournal mode

[Jan Kara]

On Fri 03-11-23 22:52:50, Zhihao Cheng wrote:
> Since JBD2 takes care of all metadata writeback errors of fs dev,
> ext4_check_bdev_write_error() is useful only in nojournal mode.
> Move it into '!ext4_handle_valid(handle)' branch.
> 
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> Suggested-by: Jan Kara <jack@suse.cz>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> ---
>  fs/ext4/ext4_jbd2.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> index d1a2e6624401..5d8055161acd 100644
> --- a/fs/ext4/ext4_jbd2.c
> +++ b/fs/ext4/ext4_jbd2.c
> @@ -235,8 +235,6 @@ int __ext4_journal_get_write_access(const char *where, unsigned int line,
>  
>  	might_sleep();
>  
> -	ext4_check_bdev_write_error(sb);
> -
>  	if (ext4_handle_valid(handle)) {
>  		err = jbd2_journal_get_write_access(handle, bh);
>  		if (err) {
> @@ -244,7 +242,8 @@ int __ext4_journal_get_write_access(const char *where, unsigned int line,
>  						  handle, err);
>  			return err;
>  		}
> -	}
> +	} else
> +		ext4_check_bdev_write_error(sb);
>  	if (trigger_type == EXT4_JTR_NONE || !ext4_has_metadata_csum(sb))
>  		return 0;
>  	BUG_ON(trigger_type >= EXT4_JOURNAL_TRIGGER_COUNT);
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/5] jbd2: Add errseq to detect writeback

[Jan Kara]

On Fri 03-11-23 22:52:45, Zhihao Cheng wrote:
> According to discussions in [1], this patchset adds errseq in journal to
> enable JDB2 detecting meatadata writeback error of fs dev. Then, orginal
> checking mechanism could be removed.
> 
> [1] https://lore.kernel.org/all/20230908124317.2955345-1-chengzhihao1@huawei.com/T/

Thanks for the series! I'm sorry for the very delayed review. There has
been a lot of work, conference and other stuff happening lately... Anyway
the series looks good, I had just some language corrections.

								Honza

> 
> Zhihao Cheng (5):
>   jbd2: Add errseq to detect client fs's bdev writeback error
>   jbd2: Replace journal state flag by checking errseq
>   jbd2: Remove unused 'JBD2_CHECKPOINT_IO_ERROR' and 'j_atomic_flags'
>   jbd2: Abort journal when detecting metadata writeback error of fs dev
>   ext4: Move ext4_check_bdev_write_error() into nojournal mode
> 
>  fs/ext4/ext4_jbd2.c   |  5 ++---
>  fs/jbd2/checkpoint.c  | 11 -----------
>  fs/jbd2/journal.c     | 11 ++++++-----
>  fs/jbd2/recovery.c    |  7 +------
>  fs/jbd2/transaction.c | 14 ++++++++++++++
>  include/linux/jbd2.h  | 37 ++++++++++++++++++++++++++-----------
>  6 files changed, 49 insertions(+), 36 deletions(-)
> 
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR