[PATCH v2 0/8] ext4: fix issues caused by data write-back failures

[PATCH v2 0/8] ext4: fix issues caused by data write-back failures

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

Changes since v1:
 * Patch 5: Make data_err=abort work for all Buffer IO, not just add
            support in dioread_nolock mode.
 * Add patch 3,4,6.
 * Collect RVB from Jan Kara and Zhang Yi.(Thanks for your review!)

v1: https://lore.kernel.org/r/20241220060757.1781418-1-libaokun@huaweicloud.com

Recently some of our customers remounted ext4 from
"dioread_nolock,data_err=abort" to "dioread_lock,data_err=abort" and the
ext4 filesystem became read-only.

Then I found that "data_err=abort" is not working in dioread_nolock mode,
when data writeback fails, the error is always recorded in inode mapping,
but no one will check it, not even when converting unwritten to written,
which could expose stale data. When remounted with dioread_lock, the error
recorded in the inode mapping was checked and the journal aborted, and the
file system became read-only later.

Patch 1: Clean up duplicate code and ensure that an warning is printed
         when data may be lost;
Patch 2: Fix an issue that could expose stale data when data writeback
         fails;
Patch 3: Reject data_err=abort in nojournal mode to ensure
         sbi->s_journal != NULL when DATA_ERR_ABORT set.
Patch 4: Add the ext4_check_nojournal_options() helper to
         reduce code duplication.
Patch 5: Make data_err=abort work for all Buffer IO, not just order mode.
Patch 6: Update the description of data_err=abort|ignore in the DOC.
Patch 7: Remove useless i_unwritten and related code.
Patch 8: Pack holes in ext4_inode_info to save memory.

Comments and questions are, as always, welcome.

Thanks,
Baokun

Baokun Li (8):
  ext4: replace opencoded ext4_end_io_end() in ext4_put_io_end()
  ext4: do not convert the unwritten extents if data writeback fails
  ext4: reject the 'data_err=abort' option in nojournal mode
  ext4: extract ext4_check_nojournal_options() from __ext4_fill_super()
  ext4: abort journal on data writeback failure if in data_err=abort mode
  ext4: update the descriptions of data_err=abort and data_err=ignore
  ext4: remove unused member 'i_unwritten' from 'ext4_inode_info'
  ext4: pack holes in ext4_inode_info

 Documentation/admin-guide/ext4.rst |  7 ++-
 fs/ext4/ext4.h                     | 34 ++++----------
 fs/ext4/inode.c                    |  2 +-
 fs/ext4/page-io.c                  | 75 ++++++++++++++++++++----------
 fs/ext4/super.c                    | 61 +++++++++++++-----------
 5 files changed, 98 insertions(+), 81 deletions(-)

-- 
2.39.2

[PATCH v2 1/8] ext4: replace opencoded ext4_end_io_end() in ext4_put_io_end()

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

This reduces duplicate code and ensures that a “potential data loss”
warning is available if the unwritten conversion fails.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/page-io.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 69b8a7221a2b..f53b018ea259 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -299,18 +299,13 @@ void ext4_put_io_end_defer(ext4_io_end_t *io_end)
 
 int ext4_put_io_end(ext4_io_end_t *io_end)
 {
-	int err = 0;
-
 	if (refcount_dec_and_test(&io_end->count)) {
-		if (io_end->flag & EXT4_IO_END_UNWRITTEN) {
-			err = ext4_convert_unwritten_io_end_vec(io_end->handle,
-								io_end);
-			io_end->handle = NULL;
-			ext4_clear_io_unwritten_flag(io_end);
-		}
+		if (io_end->flag & EXT4_IO_END_UNWRITTEN)
+			return ext4_end_io_end(io_end);
+
 		ext4_release_io_end(io_end);
 	}
-	return err;
+	return 0;
 }
 
 ext4_io_end_t *ext4_get_io_end(ext4_io_end_t *io_end)
-- 
2.39.2

[PATCH v2 2/8] ext4: do not convert the unwritten extents if data writeback fails

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

When dioread_nolock is turned on (the default), it will convert unwritten
extents to written at ext4_end_io_end(), even if the data writeback fails.

It leads to the possibility that stale data may be exposed when the
physical block corresponding to the file data is read-only (i.e., writes
return -EIO, but reads are normal).

Therefore a new ext4_io_end->flags EXT4_IO_END_FAILED is added, which
indicates that some bio write-back failed in the current ext4_io_end.
When this flag is set, the unwritten to written conversion is no longer
performed. Users can read the data normally until the caches are dropped,
after that, the failed extents can only be read to all 0.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/ext4.h    |  3 ++-
 fs/ext4/page-io.c | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 4e7de7eaa374..9da0e32af02a 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -278,7 +278,8 @@ struct ext4_system_blocks {
 /*
  * Flags for ext4_io_end->flags
  */
-#define	EXT4_IO_END_UNWRITTEN	0x0001
+#define EXT4_IO_END_UNWRITTEN	0x0001
+#define EXT4_IO_END_FAILED	0x0002
 
 struct ext4_io_end_vec {
 	struct list_head list;		/* list of io_end_vec */
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index f53b018ea259..6054ec27fb48 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -181,14 +181,25 @@ static int ext4_end_io_end(ext4_io_end_t *io_end)
 		   "list->prev 0x%p\n",
 		   io_end, inode->i_ino, io_end->list.next, io_end->list.prev);
 
-	io_end->handle = NULL;	/* Following call will use up the handle */
-	ret = ext4_convert_unwritten_io_end_vec(handle, io_end);
+	/*
+	 * Do not convert the unwritten extents if data writeback fails,
+	 * or stale data may be exposed.
+	 */
+	io_end->handle = NULL;  /* Following call will use up the handle */
+	if (unlikely(io_end->flag & EXT4_IO_END_FAILED)) {
+		ret = -EIO;
+		if (handle)
+			jbd2_journal_free_reserved(handle);
+	} else {
+		ret = ext4_convert_unwritten_io_end_vec(handle, io_end);
+	}
 	if (ret < 0 && !ext4_forced_shutdown(inode->i_sb)) {
 		ext4_msg(inode->i_sb, KERN_EMERG,
 			 "failed to convert unwritten extents to written "
 			 "extents -- potential data loss!  "
 			 "(inode %lu, error %d)", inode->i_ino, ret);
 	}
+
 	ext4_clear_io_unwritten_flag(io_end);
 	ext4_release_io_end(io_end);
 	return ret;
@@ -339,6 +350,7 @@ static void ext4_end_bio(struct bio *bio)
 			     bio->bi_status, inode->i_ino,
 			     (unsigned long long)
 			     bi_sector >> (inode->i_blkbits - 9));
+		io_end->flag |= EXT4_IO_END_FAILED;
 		mapping_set_error(inode->i_mapping,
 				blk_status_to_errno(bio->bi_status));
 	}
-- 
2.39.2

[PATCH v2 3/8] ext4: reject the 'data_err=abort' option in nojournal mode

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

data_err=abort aborts the journal on I/O errors. However, this option is
meaningless if journal is disabled, so it is rejected in nojournal mode
to reduce unnecessary checks. Also, this option is ignored upon remount.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
---
Or maybe we should make mount and remount consistent?
Rejecting it in both would make things a lot easier.

 fs/ext4/super.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index a50e5c31b937..34a7b6523f8b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2785,6 +2785,13 @@ static int ext4_check_opt_consistency(struct fs_context *fc,
 	}
 
 	if (is_remount) {
+		if (!sbi->s_journal &&
+		    ctx_test_mount_opt(ctx, EXT4_MOUNT_DATA_ERR_ABORT)) {
+			ext4_msg(NULL, KERN_WARNING,
+				 "Remounting fs w/o journal so ignoring data_err option");
+			ctx_clear_mount_opt(ctx, EXT4_MOUNT_DATA_ERR_ABORT);
+		}
+
 		if (ctx_test_mount_opt(ctx, EXT4_MOUNT_DAX_ALWAYS) &&
 		    (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)) {
 			ext4_msg(NULL, KERN_ERR, "can't mount with "
@@ -5428,6 +5435,11 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 				 "data=, fs mounted w/o journal");
 			goto failed_mount3a;
 		}
+		if (test_opt(sb, DATA_ERR_ABORT)) {
+			ext4_msg(sb, KERN_ERR,
+				 "can't mount with data_err=abort, fs mounted w/o journal");
+			goto failed_mount3a;
+		}
 		sbi->s_def_mount_opt &= ~EXT4_MOUNT_JOURNAL_CHECKSUM;
 		clear_opt(sb, JOURNAL_CHECKSUM);
 		clear_opt(sb, DATA_FLAGS);
-- 
2.39.2

[PATCH v2 4/8] ext4: extract ext4_check_nojournal_options() from __ext4_fill_super()

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

Extract the ext4_check_nojournal_options() helper function to reduce code
duplication. No functional changes.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/super.c | 50 ++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 34a7b6523f8b..78f2433f6ca0 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5020,6 +5020,24 @@ static int ext4_check_journal_data_mode(struct super_block *sb)
 	return 0;
 }
 
+static const char *ext4_check_nojournal_options(struct super_block *sb)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+
+	if (test_opt(sb, JOURNAL_ASYNC_COMMIT))
+		return "journal_async_commit";
+	if (test_opt2(sb, EXPLICIT_JOURNAL_CHECKSUM))
+		return "journal_checksum";
+	if (sbi->s_commit_interval != JBD2_DEFAULT_MAX_COMMIT_AGE*HZ)
+		return "commit=";
+	if (EXT4_MOUNT_DATA_FLAGS &
+	    (sbi->s_mount_opt ^ sbi->s_def_mount_opt))
+		return "data=";
+	if (test_opt(sb, DATA_ERR_ABORT))
+		return "data_err=abort";
+	return NULL;
+}
+
 static int ext4_load_super(struct super_block *sb, ext4_fsblk_t *lsb,
 			   int silent)
 {
@@ -5411,35 +5429,17 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 		       "suppressed and not mounted read-only");
 		goto failed_mount3a;
 	} else {
-		/* Nojournal mode, all journal mount options are illegal */
-		if (test_opt(sb, JOURNAL_ASYNC_COMMIT)) {
-			ext4_msg(sb, KERN_ERR, "can't mount with "
-				 "journal_async_commit, fs mounted w/o journal");
-			goto failed_mount3a;
-		}
+		const char *journal_option;
 
-		if (test_opt2(sb, EXPLICIT_JOURNAL_CHECKSUM)) {
-			ext4_msg(sb, KERN_ERR, "can't mount with "
-				 "journal_checksum, fs mounted w/o journal");
-			goto failed_mount3a;
-		}
-		if (sbi->s_commit_interval != JBD2_DEFAULT_MAX_COMMIT_AGE*HZ) {
-			ext4_msg(sb, KERN_ERR, "can't mount with "
-				 "commit=%lu, fs mounted w/o journal",
-				 sbi->s_commit_interval / HZ);
-			goto failed_mount3a;
-		}
-		if (EXT4_MOUNT_DATA_FLAGS &
-		    (sbi->s_mount_opt ^ sbi->s_def_mount_opt)) {
-			ext4_msg(sb, KERN_ERR, "can't mount with "
-				 "data=, fs mounted w/o journal");
-			goto failed_mount3a;
-		}
-		if (test_opt(sb, DATA_ERR_ABORT)) {
+		/* Nojournal mode, all journal mount options are illegal */
+		journal_option = ext4_check_nojournal_options(sb);
+		if (journal_option != NULL) {
 			ext4_msg(sb, KERN_ERR,
-				 "can't mount with data_err=abort, fs mounted w/o journal");
+				 "can't mount with %s, fs mounted w/o journal",
+				 journal_option);
 			goto failed_mount3a;
 		}
+
 		sbi->s_def_mount_opt &= ~EXT4_MOUNT_JOURNAL_CHECKSUM;
 		clear_opt(sb, JOURNAL_CHECKSUM);
 		clear_opt(sb, DATA_FLAGS);
-- 
2.39.2

[PATCH v2 5/8] ext4: abort journal on data writeback failure if in data_err=abort mode

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

The data_err=abort was initially introduced to address users' worries
about data corruption spreading unnoticed. With direct writes, we can
rely on return values to confirm successful writes to disk. But with
buffered writes, a successful return only means the data has been written
to memory. Users have no way of knowing if the data has actually written
it to disk unless they use fsync (which impacts performance and can
sometimes miss errors).

The current data_err=abort implementation relies on the ordered data list,
but past changes have inadvertently altered its behavior. For example, if
an extent is unwritten, we do not add the inode to the ordered data list.
Therefore, jbd2 will not wait for the data write-back of that inode to
complete and check for errors in the inode mapping. Moreover, the checks
performed by jbd2 can also miss errors.

Now, all buffered writes eventually call ext4_end_bio(), where I/O errors
are checked. Therefore, we can check for the data_err=abort mode at this
point and abort the journal in a kworker (due to the interrupt context).

Therefore, when data_err=abort is enabled, the journal is aborted in
ext4_end_io_end() when an I/O error is detected in ext4_end_bio() to make
users who are concerned about the contents of the file happy.

Suggested-by: Jan Kara <jack@suse.cz>
Link: https://patch.msgid.link/c7ab26f3-85ad-4b31-b132-0afb0e07bf79@huawei.com
Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/ext4.h    |  2 ++
 fs/ext4/page-io.c | 48 +++++++++++++++++++++++++++++++++--------------
 2 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 9da0e32af02a..e0b536df8872 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -281,6 +281,8 @@ struct ext4_system_blocks {
 #define EXT4_IO_END_UNWRITTEN	0x0001
 #define EXT4_IO_END_FAILED	0x0002
 
+#define EXT4_IO_END_NEED_COMPLETION (EXT4_IO_END_UNWRITTEN | EXT4_IO_END_FAILED)
+
 struct ext4_io_end_vec {
 	struct list_head list;		/* list of io_end_vec */
 	loff_t offset;			/* offset in the file */
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 6054ec27fb48..5dbd3ff188a3 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -164,7 +164,8 @@ static void ext4_release_io_end(ext4_io_end_t *io_end)
 }
 
 /*
- * Check a range of space and convert unwritten extents to written. Note that
+ * On successful IO, check a range of space and convert unwritten extents to
+ * written. On IO failure, check if journal abort is needed. Note that
  * we are protected from truncate touching same part of extent tree by the
  * fact that truncate code waits for all DIO to finish (thus exclusion from
  * direct IO is achieved) and also waits for PageWriteback bits. Thus we
@@ -175,6 +176,7 @@ static int ext4_end_io_end(ext4_io_end_t *io_end)
 {
 	struct inode *inode = io_end->inode;
 	handle_t *handle = io_end->handle;
+	struct super_block *sb = inode->i_sb;
 	int ret = 0;
 
 	ext4_debug("ext4_end_io_nolock: io_end 0x%p from inode %lu,list->next 0x%p,"
@@ -190,11 +192,15 @@ static int ext4_end_io_end(ext4_io_end_t *io_end)
 		ret = -EIO;
 		if (handle)
 			jbd2_journal_free_reserved(handle);
+
+		if (test_opt(sb, DATA_ERR_ABORT))
+			jbd2_journal_abort(EXT4_SB(sb)->s_journal, ret);
 	} else {
 		ret = ext4_convert_unwritten_io_end_vec(handle, io_end);
 	}
-	if (ret < 0 && !ext4_forced_shutdown(inode->i_sb)) {
-		ext4_msg(inode->i_sb, KERN_EMERG,
+	if (ret < 0 && !ext4_forced_shutdown(sb) &&
+	    io_end->flag & EXT4_IO_END_UNWRITTEN) {
+		ext4_msg(sb, KERN_EMERG,
 			 "failed to convert unwritten extents to written "
 			 "extents -- potential data loss!  "
 			 "(inode %lu, error %d)", inode->i_ino, ret);
@@ -228,6 +234,16 @@ static void dump_completed_IO(struct inode *inode, struct list_head *head)
 #endif
 }
 
+static bool ext4_io_end_need_completion(ext4_io_end_t *io_end)
+{
+	if (io_end->flag & EXT4_IO_END_UNWRITTEN)
+		return true;
+	if (test_opt(io_end->inode->i_sb, DATA_ERR_ABORT) &&
+	    io_end->flag & EXT4_IO_END_FAILED)
+		return true;
+	return false;
+}
+
 /* Add the io_end to per-inode completed end_io list. */
 static void ext4_add_complete_io(ext4_io_end_t *io_end)
 {
@@ -236,9 +252,11 @@ static void ext4_add_complete_io(ext4_io_end_t *io_end)
 	struct workqueue_struct *wq;
 	unsigned long flags;
 
-	/* Only reserved conversions from writeback should enter here */
-	WARN_ON(!(io_end->flag & EXT4_IO_END_UNWRITTEN));
-	WARN_ON(!io_end->handle && sbi->s_journal);
+	/* Only reserved conversions or pending IO errors will enter here. */
+	WARN_ON(!(io_end->flag & EXT4_IO_END_NEED_COMPLETION));
+	WARN_ON(io_end->flag & EXT4_IO_END_UNWRITTEN &&
+		!io_end->handle && sbi->s_journal);
+
 	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
 	wq = sbi->rsv_conversion_wq;
 	if (list_empty(&ei->i_rsv_conversion_list))
@@ -263,7 +281,7 @@ static int ext4_do_flush_completed_IO(struct inode *inode,
 
 	while (!list_empty(&unwritten)) {
 		io_end = list_entry(unwritten.next, ext4_io_end_t, list);
-		BUG_ON(!(io_end->flag & EXT4_IO_END_UNWRITTEN));
+		BUG_ON(!(io_end->flag & EXT4_IO_END_NEED_COMPLETION));
 		list_del_init(&io_end->list);
 
 		err = ext4_end_io_end(io_end);
@@ -274,7 +292,8 @@ static int ext4_do_flush_completed_IO(struct inode *inode,
 }
 
 /*
- * work on completed IO, to convert unwritten extents to extents
+ * Used to convert unwritten extents to written extents upon IO completion,
+ * or used to abort the journal upon IO errors.
  */
 void ext4_end_io_rsv_work(struct work_struct *work)
 {
@@ -299,19 +318,20 @@ ext4_io_end_t *ext4_init_io_end(struct inode *inode, gfp_t flags)
 void ext4_put_io_end_defer(ext4_io_end_t *io_end)
 {
 	if (refcount_dec_and_test(&io_end->count)) {
-		if (!(io_end->flag & EXT4_IO_END_UNWRITTEN) ||
-				list_empty(&io_end->list_vec)) {
-			ext4_release_io_end(io_end);
+		if (io_end->flag & EXT4_IO_END_FAILED ||
+		    (io_end->flag & EXT4_IO_END_UNWRITTEN &&
+		     !list_empty(&io_end->list_vec))) {
+			ext4_add_complete_io(io_end);
 			return;
 		}
-		ext4_add_complete_io(io_end);
+		ext4_release_io_end(io_end);
 	}
 }
 
 int ext4_put_io_end(ext4_io_end_t *io_end)
 {
 	if (refcount_dec_and_test(&io_end->count)) {
-		if (io_end->flag & EXT4_IO_END_UNWRITTEN)
+		if (ext4_io_end_need_completion(io_end))
 			return ext4_end_io_end(io_end);
 
 		ext4_release_io_end(io_end);
@@ -355,7 +375,7 @@ static void ext4_end_bio(struct bio *bio)
 				blk_status_to_errno(bio->bi_status));
 	}
 
-	if (io_end->flag & EXT4_IO_END_UNWRITTEN) {
+	if (ext4_io_end_need_completion(io_end)) {
 		/*
 		 * Link bio into list hanging from io_end. We have to do it
 		 * atomically as bio completions can be racing against each
-- 
2.39.2

[PATCH v2 6/8] ext4: update the descriptions of data_err=abort and data_err=ignore

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

We now print error messages in ext4_end_bio() when page writeback
encounters an error. If data_err=abort is set, the journal will also
be aborted in a kworker. This means that we now check all Buffer I/O
in all modes and decide whether to abort the journal based on the
data_err option. Therefore, we remove the ordered mode restriction
in the descriptions of data_err=abort and data_err=ignore.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
---
 Documentation/admin-guide/ext4.rst | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/ext4.rst b/Documentation/admin-guide/ext4.rst
index 2418b0c2d3df..b857eb6ca1b6 100644
--- a/Documentation/admin-guide/ext4.rst
+++ b/Documentation/admin-guide/ext4.rst
@@ -238,11 +238,10 @@ When mounting an ext4 filesystem, the following option are accepted:
         configured using tune2fs)
 
   data_err=ignore(*)
-        Just print an error message if an error occurs in a file data buffer in
-        ordered mode.
+        Just print an error message if an error occurs in a file data buffer.
+
   data_err=abort
-        Abort the journal if an error occurs in a file data buffer in ordered
-        mode.
+        Abort the journal if an error occurs in a file data buffer.
 
   grpid | bsdgroups
         New objects have the group ID of their parent.
-- 
2.39.2

[PATCH v2 7/8] ext4: remove unused member 'i_unwritten' from 'ext4_inode_info'

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

After commit 378f32bab371 ("ext4: introduce direct I/O write using iomap
infrastructure"), no one cares about the value of i_unwritten, so there
is no need to maintain this variable, remove it, and clean up the
associated logic.

Suggested-by: Zhang Yi <yi.zhang@huawei.com>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/ext4.h  | 22 +++-------------------
 fs/ext4/inode.c |  2 +-
 fs/ext4/super.c |  9 +--------
 3 files changed, 5 insertions(+), 28 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index e0b536df8872..6fd550fc0c5c 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1061,7 +1061,6 @@ struct ext4_inode_info {
 
 	/* Number of ongoing updates on this inode */
 	atomic_t  i_fc_updates;
-	atomic_t i_unwritten; /* Nr. of inflight conversions pending */
 
 	/* Fast commit wait queue for this inode */
 	wait_queue_head_t i_fc_wait;
@@ -3788,34 +3787,19 @@ static inline void set_bitmap_uptodate(struct buffer_head *bh)
 	set_bit(BH_BITMAP_UPTODATE, &(bh)->b_state);
 }
 
-/* For ioend & aio unwritten conversion wait queues */
-#define EXT4_WQ_HASH_SZ		37
-#define ext4_ioend_wq(v)   (&ext4__ioend_wq[((unsigned long)(v)) %\
-					    EXT4_WQ_HASH_SZ])
-extern wait_queue_head_t ext4__ioend_wq[EXT4_WQ_HASH_SZ];
-
 extern int ext4_resize_begin(struct super_block *sb);
 extern int ext4_resize_end(struct super_block *sb, bool update_backups);
 
-static inline void ext4_set_io_unwritten_flag(struct inode *inode,
-					      struct ext4_io_end *io_end)
+static inline void ext4_set_io_unwritten_flag(struct ext4_io_end *io_end)
 {
-	if (!(io_end->flag & EXT4_IO_END_UNWRITTEN)) {
+	if (!(io_end->flag & EXT4_IO_END_UNWRITTEN))
 		io_end->flag |= EXT4_IO_END_UNWRITTEN;
-		atomic_inc(&EXT4_I(inode)->i_unwritten);
-	}
 }
 
 static inline void ext4_clear_io_unwritten_flag(ext4_io_end_t *io_end)
 {
-	struct inode *inode = io_end->inode;
-
-	if (io_end->flag & EXT4_IO_END_UNWRITTEN) {
+	if (io_end->flag & EXT4_IO_END_UNWRITTEN)
 		io_end->flag &= ~EXT4_IO_END_UNWRITTEN;
-		/* Wake up anyone waiting on unwritten extent conversion */
-		if (atomic_dec_and_test(&EXT4_I(inode)->i_unwritten))
-			wake_up_all(ext4_ioend_wq(inode));
-	}
 }
 
 extern const struct iomap_ops ext4_iomap_ops;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 7c54ae5fcbd4..36b1f9fb690a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2225,7 +2225,7 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
 			mpd->io_submit.io_end->handle = handle->h_rsv_handle;
 			handle->h_rsv_handle = NULL;
 		}
-		ext4_set_io_unwritten_flag(inode, mpd->io_submit.io_end);
+		ext4_set_io_unwritten_flag(mpd->io_submit.io_end);
 	}
 
 	BUG_ON(map->m_len == 0);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 78f2433f6ca0..187cccb2870a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1426,7 +1426,6 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
 	spin_lock_init(&ei->i_completed_io_lock);
 	ei->i_sync_tid = 0;
 	ei->i_datasync_tid = 0;
-	atomic_set(&ei->i_unwritten, 0);
 	INIT_WORK(&ei->i_rsv_conversion_work, ext4_end_io_rsv_work);
 	ext4_fc_init_inode(&ei->vfs_inode);
 	mutex_init(&ei->i_fc_lock);
@@ -7393,12 +7392,9 @@ static struct file_system_type ext4_fs_type = {
 };
 MODULE_ALIAS_FS("ext4");
 
-/* Shared across all ext4 file systems */
-wait_queue_head_t ext4__ioend_wq[EXT4_WQ_HASH_SZ];
-
 static int __init ext4_init_fs(void)
 {
-	int i, err;
+	int err;
 
 	ratelimit_state_init(&ext4_mount_msg_ratelimit, 30 * HZ, 64);
 	ext4_li_info = NULL;
@@ -7406,9 +7402,6 @@ static int __init ext4_init_fs(void)
 	/* Build-time check for flags consistency */
 	ext4_check_flag_values();
 
-	for (i = 0; i < EXT4_WQ_HASH_SZ; i++)
-		init_waitqueue_head(&ext4__ioend_wq[i]);
-
 	err = ext4_init_es();
 	if (err)
 		return err;
-- 
2.39.2

[PATCH v2 8/8] ext4: pack holes in ext4_inode_info

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

When CONFIG_DEBUG_SPINLOCK is not enabled (general case), there are four
4 bytes holes and one 2 bytes hole in struct ext4_inode_info. Move the
members to pack the four 4 bytes holes.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/ext4.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 6fd550fc0c5c..c1693ba836ee 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1062,6 +1062,8 @@ struct ext4_inode_info {
 	/* Number of ongoing updates on this inode */
 	atomic_t  i_fc_updates;
 
+	spinlock_t i_raw_lock;	/* protects updates to the raw inode */
+
 	/* Fast commit wait queue for this inode */
 	wait_queue_head_t i_fc_wait;
 
@@ -1099,8 +1101,6 @@ struct ext4_inode_info {
 	struct inode vfs_inode;
 	struct jbd2_inode *jinode;
 
-	spinlock_t i_raw_lock;	/* protects updates to the raw inode */
-
 	/*
 	 * File creation time. Its function is same as that of
 	 * struct timespec64 i_{a,c,m}time in the generic inode.
@@ -1143,6 +1143,7 @@ struct ext4_inode_info {
 	/* quota space reservation, managed internally by quota code */
 	qsize_t i_reserved_quota;
 #endif
+	spinlock_t i_block_reservation_lock;
 
 	/* Lock protecting lists below */
 	spinlock_t i_completed_io_lock;
@@ -1153,8 +1154,6 @@ struct ext4_inode_info {
 	struct list_head i_rsv_conversion_list;
 	struct work_struct i_rsv_conversion_work;
 
-	spinlock_t i_block_reservation_lock;
-
 	/*
 	 * Transactions that contain inode's metadata needed to complete
 	 * fsync and fdatasync, respectively.
-- 
2.39.2

Re: [PATCH v2 4/8] ext4: extract ext4_check_nojournal_options() from __ext4_fill_super()

[Jan Kara]

On Tue 21-01-25 15:10:46, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> Extract the ext4_check_nojournal_options() helper function to reduce code
> duplication. No functional changes.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Just a small naming suggestion:

> +static const char *ext4_check_nojournal_options(struct super_block *sb)

I'd call this ext4_has_journal_option() to make it somewhat more obvious
what we are checking. Otherwise feel free to add:

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

								Honza


> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +
> +	if (test_opt(sb, JOURNAL_ASYNC_COMMIT))
> +		return "journal_async_commit";
> +	if (test_opt2(sb, EXPLICIT_JOURNAL_CHECKSUM))
> +		return "journal_checksum";
> +	if (sbi->s_commit_interval != JBD2_DEFAULT_MAX_COMMIT_AGE*HZ)
> +		return "commit=";
> +	if (EXT4_MOUNT_DATA_FLAGS &
> +	    (sbi->s_mount_opt ^ sbi->s_def_mount_opt))
> +		return "data=";
> +	if (test_opt(sb, DATA_ERR_ABORT))
> +		return "data_err=abort";
> +	return NULL;
> +}
> +
>  static int ext4_load_super(struct super_block *sb, ext4_fsblk_t *lsb,
>  			   int silent)
>  {
> @@ -5411,35 +5429,17 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		       "suppressed and not mounted read-only");
>  		goto failed_mount3a;
>  	} else {
> -		/* Nojournal mode, all journal mount options are illegal */
> -		if (test_opt(sb, JOURNAL_ASYNC_COMMIT)) {
> -			ext4_msg(sb, KERN_ERR, "can't mount with "
> -				 "journal_async_commit, fs mounted w/o journal");
> -			goto failed_mount3a;
> -		}
> +		const char *journal_option;
>  
> -		if (test_opt2(sb, EXPLICIT_JOURNAL_CHECKSUM)) {
> -			ext4_msg(sb, KERN_ERR, "can't mount with "
> -				 "journal_checksum, fs mounted w/o journal");
> -			goto failed_mount3a;
> -		}
> -		if (sbi->s_commit_interval != JBD2_DEFAULT_MAX_COMMIT_AGE*HZ) {
> -			ext4_msg(sb, KERN_ERR, "can't mount with "
> -				 "commit=%lu, fs mounted w/o journal",
> -				 sbi->s_commit_interval / HZ);
> -			goto failed_mount3a;
> -		}
> -		if (EXT4_MOUNT_DATA_FLAGS &
> -		    (sbi->s_mount_opt ^ sbi->s_def_mount_opt)) {
> -			ext4_msg(sb, KERN_ERR, "can't mount with "
> -				 "data=, fs mounted w/o journal");
> -			goto failed_mount3a;
> -		}
> -		if (test_opt(sb, DATA_ERR_ABORT)) {
> +		/* Nojournal mode, all journal mount options are illegal */
> +		journal_option = ext4_check_nojournal_options(sb);
> +		if (journal_option != NULL) {
>  			ext4_msg(sb, KERN_ERR,
> -				 "can't mount with data_err=abort, fs mounted w/o journal");
> +				 "can't mount with %s, fs mounted w/o journal",
> +				 journal_option);
>  			goto failed_mount3a;
>  		}
> +
>  		sbi->s_def_mount_opt &= ~EXT4_MOUNT_JOURNAL_CHECKSUM;
>  		clear_opt(sb, JOURNAL_CHECKSUM);
>  		clear_opt(sb, DATA_FLAGS);
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 5/8] ext4: abort journal on data writeback failure if in data_err=abort mode

[Jan Kara]

On Tue 21-01-25 15:10:47, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> The data_err=abort was initially introduced to address users' worries
> about data corruption spreading unnoticed. With direct writes, we can
> rely on return values to confirm successful writes to disk. But with
> buffered writes, a successful return only means the data has been written
> to memory. Users have no way of knowing if the data has actually written
> it to disk unless they use fsync (which impacts performance and can
> sometimes miss errors).
> 
> The current data_err=abort implementation relies on the ordered data list,
> but past changes have inadvertently altered its behavior. For example, if
> an extent is unwritten, we do not add the inode to the ordered data list.
> Therefore, jbd2 will not wait for the data write-back of that inode to
> complete and check for errors in the inode mapping. Moreover, the checks
> performed by jbd2 can also miss errors.
> 
> Now, all buffered writes eventually call ext4_end_bio(), where I/O errors
> are checked. Therefore, we can check for the data_err=abort mode at this
> point and abort the journal in a kworker (due to the interrupt context).
> 
> Therefore, when data_err=abort is enabled, the journal is aborted in
> ext4_end_io_end() when an I/O error is detected in ext4_end_bio() to make
> users who are concerned about the contents of the file happy.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Link: https://patch.msgid.link/c7ab26f3-85ad-4b31-b132-0afb0e07bf79@huawei.com
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

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

Just one naming suggestion below:

> +#define EXT4_IO_END_NEED_COMPLETION (EXT4_IO_END_UNWRITTEN | EXT4_IO_END_FAILED)

I'd call this EXT4_IO_END_DEFER_COMPLETION

> +static bool ext4_io_end_need_completion(ext4_io_end_t *io_end)

And this would then be ext4_io_end_defer_completion().

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 6/8] ext4: update the descriptions of data_err=abort and data_err=ignore

[Jan Kara]

On Tue 21-01-25 15:10:48, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> We now print error messages in ext4_end_bio() when page writeback
> encounters an error. If data_err=abort is set, the journal will also
> be aborted in a kworker. This means that we now check all Buffer I/O
> in all modes and decide whether to abort the journal based on the
> data_err option. Therefore, we remove the ordered mode restriction
> in the descriptions of data_err=abort and data_err=ignore.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  Documentation/admin-guide/ext4.rst | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/admin-guide/ext4.rst b/Documentation/admin-guide/ext4.rst
> index 2418b0c2d3df..b857eb6ca1b6 100644
> --- a/Documentation/admin-guide/ext4.rst
> +++ b/Documentation/admin-guide/ext4.rst
> @@ -238,11 +238,10 @@ When mounting an ext4 filesystem, the following option are accepted:
>          configured using tune2fs)
>  
>    data_err=ignore(*)
> -        Just print an error message if an error occurs in a file data buffer in
> -        ordered mode.
> +        Just print an error message if an error occurs in a file data buffer.
> +
>    data_err=abort
> -        Abort the journal if an error occurs in a file data buffer in ordered
> -        mode.
> +        Abort the journal if an error occurs in a file data buffer.
>  
>    grpid | bsdgroups
>          New objects have the group ID of their parent.
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 4/8] ext4: extract ext4_check_nojournal_options() from __ext4_fill_super()

[Baokun Li]

On 2025/1/21 19:04, Jan Kara wrote:
> On Tue 21-01-25 15:10:46, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> Extract the ext4_check_nojournal_options() helper function to reduce code
>> duplication. No functional changes.
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> Looks good. Just a small naming suggestion:
>
>> +static const char *ext4_check_nojournal_options(struct super_block *sb)
> I'd call this ext4_has_journal_option() to make it somewhat more obvious
> what we are checking. Otherwise feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> 								Honza
Awesome, this function name is much better and makes more sense.
I'll use it in the next version. Naming is really hard! 😅

Thanks a lot for the review!

Cheers,
Baokun
>
>> +{
>> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
>> +
>> +	if (test_opt(sb, JOURNAL_ASYNC_COMMIT))
>> +		return "journal_async_commit";
>> +	if (test_opt2(sb, EXPLICIT_JOURNAL_CHECKSUM))
>> +		return "journal_checksum";
>> +	if (sbi->s_commit_interval != JBD2_DEFAULT_MAX_COMMIT_AGE*HZ)
>> +		return "commit=";
>> +	if (EXT4_MOUNT_DATA_FLAGS &
>> +	    (sbi->s_mount_opt ^ sbi->s_def_mount_opt))
>> +		return "data=";
>> +	if (test_opt(sb, DATA_ERR_ABORT))
>> +		return "data_err=abort";
>> +	return NULL;
>> +}
>> +
>>   static int ext4_load_super(struct super_block *sb, ext4_fsblk_t *lsb,
>>   			   int silent)
>>   {
>> @@ -5411,35 +5429,17 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>>   		       "suppressed and not mounted read-only");
>>   		goto failed_mount3a;
>>   	} else {
>> -		/* Nojournal mode, all journal mount options are illegal */
>> -		if (test_opt(sb, JOURNAL_ASYNC_COMMIT)) {
>> -			ext4_msg(sb, KERN_ERR, "can't mount with "
>> -				 "journal_async_commit, fs mounted w/o journal");
>> -			goto failed_mount3a;
>> -		}
>> +		const char *journal_option;
>>   
>> -		if (test_opt2(sb, EXPLICIT_JOURNAL_CHECKSUM)) {
>> -			ext4_msg(sb, KERN_ERR, "can't mount with "
>> -				 "journal_checksum, fs mounted w/o journal");
>> -			goto failed_mount3a;
>> -		}
>> -		if (sbi->s_commit_interval != JBD2_DEFAULT_MAX_COMMIT_AGE*HZ) {
>> -			ext4_msg(sb, KERN_ERR, "can't mount with "
>> -				 "commit=%lu, fs mounted w/o journal",
>> -				 sbi->s_commit_interval / HZ);
>> -			goto failed_mount3a;
>> -		}
>> -		if (EXT4_MOUNT_DATA_FLAGS &
>> -		    (sbi->s_mount_opt ^ sbi->s_def_mount_opt)) {
>> -			ext4_msg(sb, KERN_ERR, "can't mount with "
>> -				 "data=, fs mounted w/o journal");
>> -			goto failed_mount3a;
>> -		}
>> -		if (test_opt(sb, DATA_ERR_ABORT)) {
>> +		/* Nojournal mode, all journal mount options are illegal */
>> +		journal_option = ext4_check_nojournal_options(sb);
>> +		if (journal_option != NULL) {
>>   			ext4_msg(sb, KERN_ERR,
>> -				 "can't mount with data_err=abort, fs mounted w/o journal");
>> +				 "can't mount with %s, fs mounted w/o journal",
>> +				 journal_option);
>>   			goto failed_mount3a;
>>   		}
>> +
>>   		sbi->s_def_mount_opt &= ~EXT4_MOUNT_JOURNAL_CHECKSUM;
>>   		clear_opt(sb, JOURNAL_CHECKSUM);
>>   		clear_opt(sb, DATA_FLAGS);
>> -- 
>> 2.39.2
>>

Re: [PATCH v2 3/8] ext4: reject the 'data_err=abort' option in nojournal mode

[Jan Kara]

On Tue 21-01-25 15:10:45, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> data_err=abort aborts the journal on I/O errors. However, this option is
> meaningless if journal is disabled, so it is rejected in nojournal mode
> to reduce unnecessary checks. Also, this option is ignored upon remount.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

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

> ---
> Or maybe we should make mount and remount consistent?
> Rejecting it in both would make things a lot easier.

The reason why it is ignored on remount is that we cannot modify
JBD2_ABORT_ON_SYNCDATA_ERR on remount. Now that we don't really depend on
JBD2_ABORT_ON_SYNCDATA_ERR, we could indeed make mount and remount
consistent and probably just drop JBD2_ABORT_ON_SYNCDATA_ERR altogether
(and only warn in jbd2 as it used to be long ago).

								Honza
> 
>  fs/ext4/super.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index a50e5c31b937..34a7b6523f8b 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2785,6 +2785,13 @@ static int ext4_check_opt_consistency(struct fs_context *fc,
>  	}
>  
>  	if (is_remount) {
> +		if (!sbi->s_journal &&
> +		    ctx_test_mount_opt(ctx, EXT4_MOUNT_DATA_ERR_ABORT)) {
> +			ext4_msg(NULL, KERN_WARNING,
> +				 "Remounting fs w/o journal so ignoring data_err option");
> +			ctx_clear_mount_opt(ctx, EXT4_MOUNT_DATA_ERR_ABORT);
> +		}
> +
>  		if (ctx_test_mount_opt(ctx, EXT4_MOUNT_DAX_ALWAYS) &&
>  		    (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)) {
>  			ext4_msg(NULL, KERN_ERR, "can't mount with "
> @@ -5428,6 +5435,11 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  				 "data=, fs mounted w/o journal");
>  			goto failed_mount3a;
>  		}
> +		if (test_opt(sb, DATA_ERR_ABORT)) {
> +			ext4_msg(sb, KERN_ERR,
> +				 "can't mount with data_err=abort, fs mounted w/o journal");
> +			goto failed_mount3a;
> +		}
>  		sbi->s_def_mount_opt &= ~EXT4_MOUNT_JOURNAL_CHECKSUM;
>  		clear_opt(sb, JOURNAL_CHECKSUM);
>  		clear_opt(sb, DATA_FLAGS);
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 5/8] ext4: abort journal on data writeback failure if in data_err=abort mode

[Baokun Li]

On 2025/1/21 19:13, Jan Kara wrote:
> On Tue 21-01-25 15:10:47, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> The data_err=abort was initially introduced to address users' worries
>> about data corruption spreading unnoticed. With direct writes, we can
>> rely on return values to confirm successful writes to disk. But with
>> buffered writes, a successful return only means the data has been written
>> to memory. Users have no way of knowing if the data has actually written
>> it to disk unless they use fsync (which impacts performance and can
>> sometimes miss errors).
>>
>> The current data_err=abort implementation relies on the ordered data list,
>> but past changes have inadvertently altered its behavior. For example, if
>> an extent is unwritten, we do not add the inode to the ordered data list.
>> Therefore, jbd2 will not wait for the data write-back of that inode to
>> complete and check for errors in the inode mapping. Moreover, the checks
>> performed by jbd2 can also miss errors.
>>
>> Now, all buffered writes eventually call ext4_end_bio(), where I/O errors
>> are checked. Therefore, we can check for the data_err=abort mode at this
>> point and abort the journal in a kworker (due to the interrupt context).
>>
>> Therefore, when data_err=abort is enabled, the journal is aborted in
>> ext4_end_io_end() when an I/O error is detected in ext4_end_bio() to make
>> users who are concerned about the contents of the file happy.
>>
>> Suggested-by: Jan Kara <jack@suse.cz>
>> Link: https://patch.msgid.link/c7ab26f3-85ad-4b31-b132-0afb0e07bf79@huawei.com
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> Looks good. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> Just one naming suggestion below:
Thank you for your review!
>
>> +#define EXT4_IO_END_NEED_COMPLETION (EXT4_IO_END_UNWRITTEN | EXT4_IO_END_FAILED)
> I'd call this EXT4_IO_END_DEFER_COMPLETION
>
>> +static bool ext4_io_end_need_completion(ext4_io_end_t *io_end)
> And this would then be ext4_io_end_defer_completion().
>
> 								Honza
Okay, I'll replace need_completion with defer_completion
in the next version.

Thanks,
Baokun

Re: [PATCH v2 3/8] ext4: reject the 'data_err=abort' option in nojournal mode

[Baokun Li]

Hi Honza,

On 2025/1/21 19:37, Jan Kara wrote:
> On Tue 21-01-25 15:10:45, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> data_err=abort aborts the journal on I/O errors. However, this option is
>> meaningless if journal is disabled, so it is rejected in nojournal mode
>> to reduce unnecessary checks. Also, this option is ignored upon remount.
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> Looks good. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
Thanks for the review!
>> ---
>> Or maybe we should make mount and remount consistent?
>> Rejecting it in both would make things a lot easier.
> The reason why it is ignored on remount is that we cannot modify
> JBD2_ABORT_ON_SYNCDATA_ERR on remount. Now that we don't really depend on
> JBD2_ABORT_ON_SYNCDATA_ERR, we could indeed make mount and remount
> consistent and probably just drop JBD2_ABORT_ON_SYNCDATA_ERR altogether
> (and only warn in jbd2 as it used to be long ago).
>
> 								Honza
Okay, I'll add a patch in the next version to clean up
JBD2_ABORT_ON_SYNCDATA_ERR.

Cheers,
Baokun
>>   fs/ext4/super.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index a50e5c31b937..34a7b6523f8b 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -2785,6 +2785,13 @@ static int ext4_check_opt_consistency(struct fs_context *fc,
>>   	}
>>   
>>   	if (is_remount) {
>> +		if (!sbi->s_journal &&
>> +		    ctx_test_mount_opt(ctx, EXT4_MOUNT_DATA_ERR_ABORT)) {
>> +			ext4_msg(NULL, KERN_WARNING,
>> +				 "Remounting fs w/o journal so ignoring data_err option");
>> +			ctx_clear_mount_opt(ctx, EXT4_MOUNT_DATA_ERR_ABORT);
>> +		}
>> +
>>   		if (ctx_test_mount_opt(ctx, EXT4_MOUNT_DAX_ALWAYS) &&
>>   		    (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)) {
>>   			ext4_msg(NULL, KERN_ERR, "can't mount with "
>> @@ -5428,6 +5435,11 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>>   				 "data=, fs mounted w/o journal");
>>   			goto failed_mount3a;
>>   		}
>> +		if (test_opt(sb, DATA_ERR_ABORT)) {
>> +			ext4_msg(sb, KERN_ERR,
>> +				 "can't mount with data_err=abort, fs mounted w/o journal");
>> +			goto failed_mount3a;
>> +		}
>>   		sbi->s_def_mount_opt &= ~EXT4_MOUNT_JOURNAL_CHECKSUM;
>>   		clear_opt(sb, JOURNAL_CHECKSUM);
>>   		clear_opt(sb, DATA_FLAGS);
>> -- 
>> 2.39.2
>>