[PATCH 0/7] ext4: correct behaviors under errors=remount-ro mode

[PATCH 0/7] ext4: correct behaviors under errors=remount-ro mode

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

After commit d3476f3dad4a ("ext4: don't set SB_RDONLY after filesystem
errors") in v6.12-rc1, the 'errors=remount-ro' mode no longer sets
SB_RDONLY on errors, which results in us seeing the filesystem is still
in rw state after errors.

What's worse is that after commit
  95257987a638 ("ext4: drop EXT4_MF_FS_ABORTED flag")
was merged in v6.6-rc1, the EXT4_FLAGS_SHUTDOWN bit is set in
ext4_handle_error(). This causes the file system to not be read-only
when an error is triggered in "errors=remount-ro" mode, because
EXT4_FLAGS_SHUTDOWN prevents both writing and reading.

This patch set fixes the above behavior change. See the link[1] for the
previous discussion:

Link: https://lore.kernel.org/all/22d652f6-cb3c-43f5-b2fe-0a4bb6516a04@huawei.com [1]

Comments and questions are, as always, welcome.

Thanks,
Baokun

Baokun Li (7):
  ext4: convert EXT4_FLAGS_* defines to enum
  ext4: add EXT4_FLAGS_EMERGENCY_RO bit
  ext4: add ext4_is_emergency() helper function
  ext4: add ext4_sb_rdonly() helper function
  ext4: correct behavior under errors=remount-ro mode
  ext4: show 'emergency_ro' when EXT4_FLAGS_EMERGENCY_RO is set
  ext4: show 'shutdown' hint when ext4 is forced to shutdown

 fs/ext4/ext4.h      | 28 ++++++++++++++++++++---
 fs/ext4/ext4_jbd2.c |  6 +++--
 fs/ext4/file.c      | 26 ++++++++++++++-------
 fs/ext4/fsync.c     | 12 ++++------
 fs/ext4/ialloc.c    |  5 +++--
 fs/ext4/inline.c    |  2 +-
 fs/ext4/inode.c     | 47 +++++++++++++++++++++-----------------
 fs/ext4/ioctl.c     |  2 +-
 fs/ext4/mballoc.c   |  4 ++--
 fs/ext4/mmp.c       |  2 +-
 fs/ext4/namei.c     | 20 ++++++++++-------
 fs/ext4/page-io.c   |  2 +-
 fs/ext4/super.c     | 55 ++++++++++++++++++++++-----------------------
 13 files changed, 126 insertions(+), 85 deletions(-)

-- 
2.39.2

[PATCH 1/7] ext4: convert EXT4_FLAGS_* defines to enum

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

Do away with the defines and use an enum as it's cleaner.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/ext4.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 4e7de7eaa374..612208527512 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2232,9 +2232,11 @@ extern int ext4_feature_set_ok(struct super_block *sb, int readonly);
 /*
  * Superblock flags
  */
-#define EXT4_FLAGS_RESIZING	0
-#define EXT4_FLAGS_SHUTDOWN	1
-#define EXT4_FLAGS_BDEV_IS_DAX	2
+enum {
+	EXT4_FLAGS_RESIZING,	/* Avoid superblock update and resize race */
+	EXT4_FLAGS_SHUTDOWN,	/* Prevent access to the file system */
+	EXT4_FLAGS_BDEV_IS_DAX	/* Current block device support DAX */
+};
 
 static inline int ext4_forced_shutdown(struct super_block *sb)
 {
-- 
2.39.2

[PATCH 2/7] ext4: add EXT4_FLAGS_EMERGENCY_RO bit

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

EXT4_FLAGS_EMERGENCY_RO Indicates that the current file system has become
read-only due to some error. Compared to SB_RDONLY, setting it does not
require a lock because we won't clear it, which avoids over-coupling with
vfs freeze. Also, add a helper function ext4_emergency_ro() to check if
the bit is set.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/ext4.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 612208527512..c5b775482897 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2235,7 +2235,8 @@ extern int ext4_feature_set_ok(struct super_block *sb, int readonly);
 enum {
 	EXT4_FLAGS_RESIZING,	/* Avoid superblock update and resize race */
 	EXT4_FLAGS_SHUTDOWN,	/* Prevent access to the file system */
-	EXT4_FLAGS_BDEV_IS_DAX	/* Current block device support DAX */
+	EXT4_FLAGS_BDEV_IS_DAX,	/* Current block device support DAX */
+	EXT4_FLAGS_EMERGENCY_RO	/* Emergency read-only due to fs errors */
 };
 
 static inline int ext4_forced_shutdown(struct super_block *sb)
@@ -2243,6 +2244,11 @@ static inline int ext4_forced_shutdown(struct super_block *sb)
 	return test_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
 }
 
+static inline int ext4_emergency_ro(struct super_block *sb)
+{
+	return test_bit(EXT4_FLAGS_EMERGENCY_RO, &EXT4_SB(sb)->s_ext4_flags);
+}
+
 /*
  * Default values for user and/or group using reserved blocks
  */
-- 
2.39.2

[PATCH 3/7] ext4: add ext4_is_emergency() helper function

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

Since both SHUTDOWN and EMERGENCY_RO are emergency states of the ext4 file
system, and they are checked in similar locations, we have added a helper
function, ext4_is_emergency(), to determine whether the current file system
is in one of these two emergency states.

Then, replace calls to ext4_forced_shutdown() with ext4_is_emergency() in
those functions that could potentially trigger write operations.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/ext4.h      |  9 +++++++++
 fs/ext4/ext4_jbd2.c |  6 ++++--
 fs/ext4/file.c      | 24 ++++++++++++++++-------
 fs/ext4/fsync.c     | 12 ++++--------
 fs/ext4/ialloc.c    |  5 +++--
 fs/ext4/inline.c    |  2 +-
 fs/ext4/inode.c     | 47 ++++++++++++++++++++++++++-------------------
 fs/ext4/mballoc.c   |  4 ++--
 fs/ext4/mmp.c       |  2 +-
 fs/ext4/namei.c     | 20 +++++++++++--------
 fs/ext4/page-io.c   |  2 +-
 fs/ext4/super.c     | 19 +++++++++---------
 12 files changed, 91 insertions(+), 61 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index c5b775482897..ca01b476e42b 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2249,6 +2249,15 @@ static inline int ext4_emergency_ro(struct super_block *sb)
 	return test_bit(EXT4_FLAGS_EMERGENCY_RO, &EXT4_SB(sb)->s_ext4_flags);
 }
 
+static inline int ext4_is_emergency(struct super_block *sb)
+{
+	if (unlikely(ext4_forced_shutdown(sb)))
+		return -EIO;
+	if (unlikely(ext4_emergency_ro(sb)))
+		return -EROFS;
+	return 0;
+}
+
 /*
  * Default values for user and/or group using reserved blocks
  */
diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index da4a82456383..2c4e976360f1 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -63,12 +63,14 @@ static void ext4_put_nojournal(handle_t *handle)
  */
 static int ext4_journal_check_start(struct super_block *sb)
 {
+	int ret;
 	journal_t *journal;
 
 	might_sleep();
 
-	if (unlikely(ext4_forced_shutdown(sb)))
-		return -EIO;
+	ret = ext4_is_emergency(sb);
+	if (unlikely(ret))
+		return ret;
 
 	if (WARN_ON_ONCE(sb_rdonly(sb)))
 		return -EROFS;
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index a5205149adba..6db052a87b9b 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -688,10 +688,12 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
 static ssize_t
 ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
+	int ret;
 	struct inode *inode = file_inode(iocb->ki_filp);
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
-		return -EIO;
+	ret = ext4_is_emergency(inode->i_sb);
+	if (unlikely(ret))
+		return ret;
 
 #ifdef CONFIG_FS_DAX
 	if (IS_DAX(inode))
@@ -700,7 +702,6 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 
 	if (iocb->ki_flags & IOCB_ATOMIC) {
 		size_t len = iov_iter_count(from);
-		int ret;
 
 		if (len < EXT4_SB(inode->i_sb)->s_awu_min ||
 		    len > EXT4_SB(inode->i_sb)->s_awu_max)
@@ -803,11 +804,16 @@ static const struct vm_operations_struct ext4_file_vm_ops = {
 
 static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
+	int ret;
 	struct inode *inode = file->f_mapping->host;
 	struct dax_device *dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
-		return -EIO;
+	if (file->f_mode & FMODE_WRITE)
+		ret = ext4_is_emergency(inode->i_sb);
+	else
+		ret = ext4_forced_shutdown(inode->i_sb) ? -EIO : 0;
+	if (unlikely(ret))
+		return ret;
 
 	/*
 	 * We don't support synchronous mappings for non-DAX files and
@@ -881,8 +887,12 @@ static int ext4_file_open(struct inode *inode, struct file *filp)
 {
 	int ret;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
-		return -EIO;
+	if (filp->f_mode & FMODE_WRITE)
+		ret = ext4_is_emergency(inode->i_sb);
+	else
+		ret = ext4_forced_shutdown(inode->i_sb) ? -EIO : 0;
+	if (unlikely(ret))
+		return ret;
 
 	ret = ext4_sample_last_mounted(inode->i_sb, filp->f_path.mnt);
 	if (ret)
diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index b40d3b29f7e5..ee9078a5d098 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -132,20 +132,16 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	bool needs_barrier = false;
 	struct inode *inode = file->f_mapping->host;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
-		return -EIO;
+	ret = ext4_is_emergency(inode->i_sb);
+	if (unlikely(ret))
+		return ret;
 
 	ASSERT(ext4_journal_current_handle() == NULL);
 
 	trace_ext4_sync_file_enter(file, datasync);
 
-	if (sb_rdonly(inode->i_sb)) {
-		/* Make sure that we read updated s_ext4_flags value */
-		smp_rmb();
-		if (ext4_forced_shutdown(inode->i_sb))
-			ret = -EROFS;
+	if (sb_rdonly(inode->i_sb))
 		goto out;
-	}
 
 	if (!EXT4_SB(inode->i_sb)->s_journal) {
 		ret = ext4_fsync_nojournal(file, start, end, datasync,
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 21d228073d79..4d0af20fa319 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -951,8 +951,9 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 	sb = dir->i_sb;
 	sbi = EXT4_SB(sb);
 
-	if (unlikely(ext4_forced_shutdown(sb)))
-		return ERR_PTR(-EIO);
+	ret2 = ext4_is_emergency(sb);
+	if (unlikely(ret2))
+		return ERR_PTR(ret2);
 
 	ngroups = ext4_get_groups_count(sb);
 	trace_ext4_request_inode(dir, mode);
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 3536ca7e4fcc..d44cc9b5589e 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -228,7 +228,7 @@ static void ext4_write_inline_data(struct inode *inode, struct ext4_iloc *iloc,
 	struct ext4_inode *raw_inode;
 	int cp_len = 0;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
+	if (unlikely(ext4_is_emergency(inode->i_sb)))
 		return;
 
 	BUG_ON(!EXT4_I(inode)->i_inline_off);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 7c54ae5fcbd4..3971e10874eb 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1149,8 +1149,9 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
 	pgoff_t index;
 	unsigned from, to;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
-		return -EIO;
+	ret = ext4_is_emergency(inode->i_sb);
+	if (unlikely(ret))
+		return ret;
 
 	trace_ext4_write_begin(inode, pos, len);
 	/*
@@ -2273,7 +2274,7 @@ static int mpage_map_and_submit_extent(handle_t *handle,
 		if (err < 0) {
 			struct super_block *sb = inode->i_sb;
 
-			if (ext4_forced_shutdown(sb))
+			if (ext4_is_emergency(sb))
 				goto invalidate_dirty_pages;
 			/*
 			 * Let the uper layers retry transient errors.
@@ -2599,10 +2600,9 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 	 * *never* be called, so if that ever happens, we would want
 	 * the stack trace.
 	 */
-	if (unlikely(ext4_forced_shutdown(mapping->host->i_sb))) {
-		ret = -EROFS;
+	ret = ext4_is_emergency(mapping->host->i_sb);
+	if (unlikely(ret))
 		goto out_writepages;
-	}
 
 	/*
 	 * If we have inline data and arrive here, it means that
@@ -2817,8 +2817,9 @@ static int ext4_writepages(struct address_space *mapping,
 	int ret;
 	int alloc_ctx;
 
-	if (unlikely(ext4_forced_shutdown(sb)))
-		return -EIO;
+	ret = ext4_is_emergency(sb);
+	if (unlikely(ret))
+		return ret;
 
 	alloc_ctx = ext4_writepages_down_read(sb);
 	ret = ext4_do_writepages(&mpd);
@@ -2858,8 +2859,9 @@ static int ext4_dax_writepages(struct address_space *mapping,
 	struct inode *inode = mapping->host;
 	int alloc_ctx;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
-		return -EIO;
+	ret = ext4_is_emergency(inode->i_sb);
+	if (unlikely(ret))
+		return ret;
 
 	alloc_ctx = ext4_writepages_down_read(inode->i_sb);
 	trace_ext4_writepages(inode, wbc);
@@ -2915,8 +2917,9 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
 	pgoff_t index;
 	struct inode *inode = mapping->host;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
-		return -EIO;
+	ret = ext4_is_emergency(inode->i_sb);
+	if (unlikely(ret))
+		return ret;
 
 	index = pos >> PAGE_SHIFT;
 
@@ -5228,8 +5231,9 @@ int ext4_write_inode(struct inode *inode, struct writeback_control *wbc)
 	if (WARN_ON_ONCE(current->flags & PF_MEMALLOC))
 		return 0;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
-		return -EIO;
+	err = ext4_is_emergency(inode->i_sb);
+	if (unlikely(err))
+		return err;
 
 	if (EXT4_SB(inode->i_sb)->s_journal) {
 		if (ext4_journal_current_handle()) {
@@ -5351,8 +5355,9 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 	const unsigned int ia_valid = attr->ia_valid;
 	bool inc_ivers = true;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
-		return -EIO;
+	error = ext4_is_emergency(inode->i_sb);
+	if (unlikely(error))
+		return error;
 
 	if (unlikely(IS_IMMUTABLE(inode)))
 		return -EPERM;
@@ -5796,9 +5801,10 @@ int ext4_mark_iloc_dirty(handle_t *handle,
 {
 	int err = 0;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb))) {
+	err = ext4_is_emergency(inode->i_sb);
+	if (unlikely(err)) {
 		put_bh(iloc->bh);
-		return -EIO;
+		return err;
 	}
 	ext4_fc_track_inode(handle, inode);
 
@@ -5822,8 +5828,9 @@ ext4_reserve_inode_write(handle_t *handle, struct inode *inode,
 {
 	int err;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
-		return -EIO;
+	err = ext4_is_emergency(inode->i_sb);
+	if (unlikely(err))
+		return err;
 
 	err = ext4_get_inode_loc(inode, iloc);
 	if (!err) {
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index b25a27c86696..7c783cb2a1dc 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5653,7 +5653,7 @@ static inline void ext4_mb_show_pa(struct super_block *sb)
 {
 	ext4_group_t i, ngroups;
 
-	if (ext4_forced_shutdown(sb))
+	if (ext4_is_emergency(sb))
 		return;
 
 	ngroups = ext4_get_groups_count(sb);
@@ -5687,7 +5687,7 @@ static void ext4_mb_show_ac(struct ext4_allocation_context *ac)
 {
 	struct super_block *sb = ac->ac_sb;
 
-	if (ext4_forced_shutdown(sb))
+	if (ext4_is_emergency(sb))
 		return;
 
 	mb_debug(sb, "Can't allocate:"
diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index d64c04ed061a..a3ae72ce3aa1 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -162,7 +162,7 @@ static int kmmpd(void *data)
 	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
 	       sizeof(mmp->mmp_nodename));
 
-	while (!kthread_should_stop() && !ext4_forced_shutdown(sb)) {
+	while (!kthread_should_stop() && !ext4_is_emergency(sb)) {
 		if (!ext4_has_feature_mmp(sb)) {
 			ext4_warning(sb, "kmmpd being stopped since MMP feature"
 				     " has been disabled.");
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 536d56d15072..72907dd96e6a 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3151,8 +3151,9 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry)
 	struct ext4_dir_entry_2 *de;
 	handle_t *handle = NULL;
 
-	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
-		return -EIO;
+	retval = ext4_is_emergency(dir->i_sb);
+	if (unlikely(retval))
+		return retval;
 
 	/* Initialize quotas before so that eventual writes go in
 	 * separate transaction */
@@ -3309,8 +3310,9 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
 {
 	int retval;
 
-	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
-		return -EIO;
+	retval = ext4_is_emergency(dir->i_sb);
+	if (unlikely(retval))
+		return retval;
 
 	trace_ext4_unlink_enter(dir, dentry);
 	/*
@@ -3376,8 +3378,9 @@ static int ext4_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	struct fscrypt_str disk_link;
 	int retries = 0;
 
-	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
-		return -EIO;
+	err = ext4_is_emergency(dir->i_sb);
+	if (unlikely(err))
+		return err;
 
 	err = fscrypt_prepare_symlink(dir, symname, len, dir->i_sb->s_blocksize,
 				      &disk_link);
@@ -4199,8 +4202,9 @@ static int ext4_rename2(struct mnt_idmap *idmap,
 {
 	int err;
 
-	if (unlikely(ext4_forced_shutdown(old_dir->i_sb)))
-		return -EIO;
+	err = ext4_is_emergency(old_dir->i_sb);
+	if (unlikely(err))
+		return err;
 
 	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE | RENAME_WHITEOUT))
 		return -EINVAL;
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 69b8a7221a2b..0e5e1de6b534 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -183,7 +183,7 @@ static int ext4_end_io_end(ext4_io_end_t *io_end)
 
 	io_end->handle = NULL;	/* Following call will use up the handle */
 	ret = ext4_convert_unwritten_io_end_vec(handle, io_end);
-	if (ret < 0 && !ext4_forced_shutdown(inode->i_sb)) {
+	if (ret < 0 && !ext4_is_emergency(inode->i_sb)) {
 		ext4_msg(inode->i_sb, KERN_EMERG,
 			 "failed to convert unwritten extents to written "
 			 "extents -- potential data loss!  "
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index a50e5c31b937..c12133628ee9 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -819,7 +819,7 @@ void __ext4_error(struct super_block *sb, const char *function,
 	struct va_format vaf;
 	va_list args;
 
-	if (unlikely(ext4_forced_shutdown(sb)))
+	if (unlikely(ext4_is_emergency(sb)))
 		return;
 
 	trace_ext4_error(sb, function, line);
@@ -844,7 +844,7 @@ void __ext4_error_inode(struct inode *inode, const char *function,
 	va_list args;
 	struct va_format vaf;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
+	if (unlikely(ext4_is_emergency(inode->i_sb)))
 		return;
 
 	trace_ext4_error(inode->i_sb, function, line);
@@ -879,7 +879,7 @@ void __ext4_error_file(struct file *file, const char *function,
 	struct inode *inode = file_inode(file);
 	char pathname[80], *path;
 
-	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
+	if (unlikely(ext4_is_emergency(inode->i_sb)))
 		return;
 
 	trace_ext4_error(inode->i_sb, function, line);
@@ -959,7 +959,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
 	char nbuf[16];
 	const char *errstr;
 
-	if (unlikely(ext4_forced_shutdown(sb)))
+	if (unlikely(ext4_is_emergency(sb)))
 		return;
 
 	/* Special case: if the error is EROFS, and we're not already
@@ -1053,7 +1053,7 @@ __acquires(bitlock)
 	struct va_format vaf;
 	va_list args;
 
-	if (unlikely(ext4_forced_shutdown(sb)))
+	if (unlikely(ext4_is_emergency(sb)))
 		return;
 
 	trace_ext4_error(sb, function, line);
@@ -6336,8 +6336,9 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
 	bool needs_barrier = false;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 
-	if (unlikely(ext4_forced_shutdown(sb)))
-		return -EIO;
+	ret = ext4_is_emergency(sb);
+	if (unlikely(ret))
+		return ret;
 
 	trace_ext4_sync_fs(sb, wait);
 	flush_workqueue(sbi->rsv_conversion_wq);
@@ -6419,7 +6420,7 @@ static int ext4_freeze(struct super_block *sb)
  */
 static int ext4_unfreeze(struct super_block *sb)
 {
-	if (ext4_forced_shutdown(sb))
+	if (ext4_is_emergency(sb))
 		return 0;
 
 	if (EXT4_SB(sb)->s_journal) {
@@ -6575,7 +6576,7 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
 	flush_work(&sbi->s_sb_upd_work);
 
 	if ((bool)(fc->sb_flags & SB_RDONLY) != sb_rdonly(sb)) {
-		if (ext4_forced_shutdown(sb)) {
+		if (ext4_is_emergency(sb)) {
 			err = -EROFS;
 			goto restore_opts;
 		}
-- 
2.39.2

[PATCH 4/7] ext4: add ext4_sb_rdonly() helper function

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

Because both SB_RDONLY and EXT4_FLAGS_EMERGENCY_RO indicate the file system
is read-only, the ext4_sb_rdonly() helper function is added. This function
returns true if either flag is set, signifying that the file system is
read-only.

Then replace some sb_rdonly() with ext4_sb_rdonly() to avoid unexpected
failures of some read-only operations or modification of the superblock
after setting EXT4_FLAGS_EMERGENCY_RO.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/ext4.h  |  5 +++++
 fs/ext4/file.c  |  2 +-
 fs/ext4/ioctl.c |  2 +-
 fs/ext4/super.c | 17 +++++++++--------
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index ca01b476e42b..610c18036dc8 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2258,6 +2258,11 @@ static inline int ext4_is_emergency(struct super_block *sb)
 	return 0;
 }
 
+static inline int ext4_sb_rdonly(struct super_block *sb)
+{
+	return sb_rdonly(sb) || ext4_emergency_ro(sb);
+}
+
 /*
  * Default values for user and/or group using reserved blocks
  */
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 6db052a87b9b..70b556c87b88 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -844,7 +844,7 @@ static int ext4_sample_last_mounted(struct super_block *sb,
 	if (likely(ext4_test_mount_flag(sb, EXT4_MF_MNTDIR_SAMPLED)))
 		return 0;
 
-	if (sb_rdonly(sb) || !sb_start_intwrite_trylock(sb))
+	if (ext4_sb_rdonly(sb) || !sb_start_intwrite_trylock(sb))
 		return 0;
 
 	ext4_set_mount_flag(sb, EXT4_MF_MNTDIR_SAMPLED);
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 7b9ce71c1c81..0807ee8cbcdc 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1705,7 +1705,7 @@ int ext4_update_overhead(struct super_block *sb, bool force)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 
-	if (sb_rdonly(sb))
+	if (ext4_sb_rdonly(sb))
 		return 0;
 	if (!force &&
 	    (sbi->s_overhead == 0 ||
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c12133628ee9..fc5d30123f22 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -473,7 +473,7 @@ static void ext4_maybe_update_superblock(struct super_block *sb)
 	__u64 lifetime_write_kbytes;
 	__u64 diff_size;
 
-	if (sb_rdonly(sb) || !(sb->s_flags & SB_ACTIVE) ||
+	if (ext4_sb_rdonly(sb) || !(sb->s_flags & SB_ACTIVE) ||
 	    !journal || (journal->j_flags & JBD2_UNMOUNT))
 		return;
 
@@ -707,7 +707,7 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
 	if (test_opt(sb, WARN_ON_ERROR))
 		WARN_ON_ONCE(1);
 
-	if (!continue_fs && !sb_rdonly(sb)) {
+	if (!continue_fs && !ext4_sb_rdonly(sb)) {
 		set_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
 		if (journal)
 			jbd2_journal_abort(journal, -EIO);
@@ -737,7 +737,7 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
 			sb->s_id);
 	}
 
-	if (sb_rdonly(sb) || continue_fs)
+	if (ext4_sb_rdonly(sb) || continue_fs)
 		return;
 
 	ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
@@ -765,7 +765,7 @@ static void update_super_work(struct work_struct *work)
 	 * We use directly jbd2 functions here to avoid recursing back into
 	 * ext4 error handling code during handling of previous errors.
 	 */
-	if (!sb_rdonly(sbi->s_sb) && journal) {
+	if (!ext4_sb_rdonly(sbi->s_sb) && journal) {
 		struct buffer_head *sbh = sbi->s_sbh;
 		bool call_notify_err = false;
 
@@ -1325,12 +1325,12 @@ static void ext4_put_super(struct super_block *sb)
 	ext4_mb_release(sb);
 	ext4_ext_release(sb);
 
-	if (!sb_rdonly(sb) && !aborted) {
+	if (!ext4_sb_rdonly(sb) && !aborted) {
 		ext4_clear_feature_journal_needs_recovery(sb);
 		ext4_clear_feature_orphan_present(sb);
 		es->s_state = cpu_to_le16(sbi->s_mount_state);
 	}
-	if (!sb_rdonly(sb))
+	if (!ext4_sb_rdonly(sb))
 		ext4_commit_super(sb);
 
 	ext4_group_desc_free(sbi);
@@ -3693,7 +3693,8 @@ static int ext4_run_li_request(struct ext4_li_request *elr)
 		if (group >= elr->lr_next_group) {
 			ret = 1;
 			if (elr->lr_first_not_zeroed != ngroups &&
-			    !sb_rdonly(sb) && test_opt(sb, INIT_INODE_TABLE)) {
+			    !ext4_sb_rdonly(sb) &&
+			    test_opt(sb, INIT_INODE_TABLE)) {
 				elr->lr_next_group = elr->lr_first_not_zeroed;
 				elr->lr_mode = EXT4_LI_MODE_ITABLE;
 				ret = 0;
@@ -3998,7 +3999,7 @@ int ext4_register_li_request(struct super_block *sb,
 		goto out;
 	}
 
-	if (sb_rdonly(sb) ||
+	if (ext4_sb_rdonly(sb) ||
 	    (test_opt(sb, NO_PREFETCH_BLOCK_BITMAPS) &&
 	     (first_not_zeroed == ngroups || !test_opt(sb, INIT_INODE_TABLE))))
 		goto out;
-- 
2.39.2

[PATCH 5/7] ext4: correct behavior under errors=remount-ro mode

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

And after commit 95257987a638 ("ext4: drop EXT4_MF_FS_ABORTED flag") in
v6.6-rc1, the EXT4_FLAGS_SHUTDOWN bit is set in ext4_handle_error() under
errors=remount-ro mode. This causes the read to fail even when the error
is triggered in errors=remount-ro mode.

To correct the behavior under errors=remount-ro, EXT4_FLAGS_SHUTDOWN is
replaced by the newly introduced EXT4_FLAGS_EMERGENCY_RO. This new flag
only prevents writes, matching the previous behavior with SB_RDONLY.

Fixes: 95257987a638 ("ext4: drop EXT4_MF_FS_ABORTED flag")
Closes: https://lore.kernel.org/all/22d652f6-cb3c-43f5-b2fe-0a4bb6516a04@huawei.com/
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/super.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index fc5d30123f22..8d9ac8770764 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -707,11 +707,8 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
 	if (test_opt(sb, WARN_ON_ERROR))
 		WARN_ON_ONCE(1);
 
-	if (!continue_fs && !ext4_sb_rdonly(sb)) {
-		set_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
-		if (journal)
-			jbd2_journal_abort(journal, -EIO);
-	}
+	if (!continue_fs && !ext4_sb_rdonly(sb) && journal)
+		jbd2_journal_abort(journal, -EIO);
 
 	if (!bdev_read_only(sb->s_bdev)) {
 		save_error_info(sb, error, ino, block, func, line);
@@ -741,13 +738,7 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
 		return;
 
 	ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
-	/*
-	 * EXT4_FLAGS_SHUTDOWN was set which stops all filesystem
-	 * modifications. We don't set SB_RDONLY because that requires
-	 * sb->s_umount semaphore and setting it without proper remount
-	 * procedure is confusing code such as freeze_super() leading to
-	 * deadlocks and other problems.
-	 */
+	set_bit(EXT4_FLAGS_EMERGENCY_RO, &EXT4_SB(sb)->s_ext4_flags);
 }
 
 static void update_super_work(struct work_struct *work)
-- 
2.39.2

[PATCH 6/7] ext4: show 'emergency_ro' when EXT4_FLAGS_EMERGENCY_RO is set

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

After commit d3476f3dad4a ("ext4: don't set SB_RDONLY after filesystem
errors") in v6.12-rc1, the 'errors=remount-ro' mode no longer sets
SB_RDONLY on errors, which results in us seeing the filesystem is still
in rw state after errors.

Therefore, after setting EXT4_FLAGS_EMERGENCY_RO, display the emergency_ro
option so that users can query whether the current file system has become
emergency read-only due to errors through commands such as 'mount' or
'cat /proc/fs/ext4/sdx/options'.

Fixes: d3476f3dad4a ("ext4: don't set SB_RDONLY after filesystem errors")
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/super.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8d9ac8770764..2377ebf0aff1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3029,6 +3029,9 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,
 	if (nodefs && !test_opt(sb, NO_PREFETCH_BLOCK_BITMAPS))
 		SEQ_OPTS_PUTS("prefetch_block_bitmaps");
 
+	if (ext4_emergency_ro(sb))
+		SEQ_OPTS_PUTS("emergency_ro");
+
 	ext4_show_quota_options(seq, sb);
 	return 0;
 }
-- 
2.39.2

[PATCH 7/7] ext4: show 'shutdown' hint when ext4 is forced to shutdown

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

Now, if dmesg is cleared, we have no way of knowing if the file system has
been shutdown. Moreover, ext4 allows directory reads even after the file
system has been shutdown, so when reading a file returns -EIO, we cannot
determine whether this is a hardware issue or if the file system has been
shutdown.

Therefore, when ext4 file system is shutdown, we're adding a 'shutdown'
hint to commands like mount so users can easily check the file system's
status.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/super.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 2377ebf0aff1..b15c36df934c 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3032,6 +3032,9 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,
 	if (ext4_emergency_ro(sb))
 		SEQ_OPTS_PUTS("emergency_ro");
 
+	if (ext4_forced_shutdown(sb))
+		SEQ_OPTS_PUTS("shutdown");
+
 	ext4_show_quota_options(seq, sb);
 	return 0;
 }
-- 
2.39.2

Re: [PATCH 1/7] ext4: convert EXT4_FLAGS_* defines to enum

[Jan Kara]

On Fri 17-01-25 16:23:09, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> Do away with the defines and use an enum as it's cleaner.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Yeah, why not. Feel free to add:

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

								Honza

> ---
>  fs/ext4/ext4.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 4e7de7eaa374..612208527512 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2232,9 +2232,11 @@ extern int ext4_feature_set_ok(struct super_block *sb, int readonly);
>  /*
>   * Superblock flags
>   */
> -#define EXT4_FLAGS_RESIZING	0
> -#define EXT4_FLAGS_SHUTDOWN	1
> -#define EXT4_FLAGS_BDEV_IS_DAX	2
> +enum {
> +	EXT4_FLAGS_RESIZING,	/* Avoid superblock update and resize race */
> +	EXT4_FLAGS_SHUTDOWN,	/* Prevent access to the file system */
> +	EXT4_FLAGS_BDEV_IS_DAX	/* Current block device support DAX */
			      ^^ we usually put comma here so that future
additions doesn't need to modify this line.

> +};
>  
>  static inline int ext4_forced_shutdown(struct super_block *sb)
>  {
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/7] ext4: add EXT4_FLAGS_EMERGENCY_RO bit

[Jan Kara]

On Fri 17-01-25 16:23:10, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> EXT4_FLAGS_EMERGENCY_RO Indicates that the current file system has become
> read-only due to some error. Compared to SB_RDONLY, setting it does not
> require a lock because we won't clear it, which avoids over-coupling with
> vfs freeze. Also, add a helper function ext4_emergency_ro() to check if
> the bit is set.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

The same comment about comma after the last enum member. Otherwise looks
good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/ext4.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 612208527512..c5b775482897 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2235,7 +2235,8 @@ extern int ext4_feature_set_ok(struct super_block *sb, int readonly);
>  enum {
>  	EXT4_FLAGS_RESIZING,	/* Avoid superblock update and resize race */
>  	EXT4_FLAGS_SHUTDOWN,	/* Prevent access to the file system */
> -	EXT4_FLAGS_BDEV_IS_DAX	/* Current block device support DAX */
> +	EXT4_FLAGS_BDEV_IS_DAX,	/* Current block device support DAX */
> +	EXT4_FLAGS_EMERGENCY_RO	/* Emergency read-only due to fs errors */
>  };
>  
>  static inline int ext4_forced_shutdown(struct super_block *sb)
> @@ -2243,6 +2244,11 @@ static inline int ext4_forced_shutdown(struct super_block *sb)
>  	return test_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
>  }
>  
> +static inline int ext4_emergency_ro(struct super_block *sb)
> +{
> +	return test_bit(EXT4_FLAGS_EMERGENCY_RO, &EXT4_SB(sb)->s_ext4_flags);
> +}
> +
>  /*
>   * Default values for user and/or group using reserved blocks
>   */
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/7] ext4: add ext4_is_emergency() helper function

[Jan Kara]

On Fri 17-01-25 16:23:11, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> Since both SHUTDOWN and EMERGENCY_RO are emergency states of the ext4 file
> system, and they are checked in similar locations, we have added a helper
> function, ext4_is_emergency(), to determine whether the current file system
> is in one of these two emergency states.
> 
> Then, replace calls to ext4_forced_shutdown() with ext4_is_emergency() in
> those functions that could potentially trigger write operations.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good, just one naming suggestion:

> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index c5b775482897..ca01b476e42b 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2249,6 +2249,15 @@ static inline int ext4_emergency_ro(struct super_block *sb)
>  	return test_bit(EXT4_FLAGS_EMERGENCY_RO, &EXT4_SB(sb)->s_ext4_flags);
>  }
>  
> +static inline int ext4_is_emergency(struct super_block *sb)
> +{
> +	if (unlikely(ext4_forced_shutdown(sb)))
> +		return -EIO;
> +	if (unlikely(ext4_emergency_ro(sb)))
> +		return -EROFS;
> +	return 0;
> +}

Since this actually returns error I'd call it ext4_emergency_state() or
something like that. Otherwise feel free to add:

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

								Honza


> +
>  /*
>   * Default values for user and/or group using reserved blocks
>   */
> diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> index da4a82456383..2c4e976360f1 100644
> --- a/fs/ext4/ext4_jbd2.c
> +++ b/fs/ext4/ext4_jbd2.c
> @@ -63,12 +63,14 @@ static void ext4_put_nojournal(handle_t *handle)
>   */
>  static int ext4_journal_check_start(struct super_block *sb)
>  {
> +	int ret;
>  	journal_t *journal;
>  
>  	might_sleep();
>  
> -	if (unlikely(ext4_forced_shutdown(sb)))
> -		return -EIO;
> +	ret = ext4_is_emergency(sb);
> +	if (unlikely(ret))
> +		return ret;
>  
>  	if (WARN_ON_ONCE(sb_rdonly(sb)))
>  		return -EROFS;
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index a5205149adba..6db052a87b9b 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -688,10 +688,12 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  static ssize_t
>  ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  {
> +	int ret;
>  	struct inode *inode = file_inode(iocb->ki_filp);
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	ret = ext4_is_emergency(inode->i_sb);
> +	if (unlikely(ret))
> +		return ret;
>  
>  #ifdef CONFIG_FS_DAX
>  	if (IS_DAX(inode))
> @@ -700,7 +702,6 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  
>  	if (iocb->ki_flags & IOCB_ATOMIC) {
>  		size_t len = iov_iter_count(from);
> -		int ret;
>  
>  		if (len < EXT4_SB(inode->i_sb)->s_awu_min ||
>  		    len > EXT4_SB(inode->i_sb)->s_awu_max)
> @@ -803,11 +804,16 @@ static const struct vm_operations_struct ext4_file_vm_ops = {
>  
>  static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
>  {
> +	int ret;
>  	struct inode *inode = file->f_mapping->host;
>  	struct dax_device *dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	if (file->f_mode & FMODE_WRITE)
> +		ret = ext4_is_emergency(inode->i_sb);
> +	else
> +		ret = ext4_forced_shutdown(inode->i_sb) ? -EIO : 0;
> +	if (unlikely(ret))
> +		return ret;
>  
>  	/*
>  	 * We don't support synchronous mappings for non-DAX files and
> @@ -881,8 +887,12 @@ static int ext4_file_open(struct inode *inode, struct file *filp)
>  {
>  	int ret;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	if (filp->f_mode & FMODE_WRITE)
> +		ret = ext4_is_emergency(inode->i_sb);
> +	else
> +		ret = ext4_forced_shutdown(inode->i_sb) ? -EIO : 0;
> +	if (unlikely(ret))
> +		return ret;
>  
>  	ret = ext4_sample_last_mounted(inode->i_sb, filp->f_path.mnt);
>  	if (ret)
> diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
> index b40d3b29f7e5..ee9078a5d098 100644
> --- a/fs/ext4/fsync.c
> +++ b/fs/ext4/fsync.c
> @@ -132,20 +132,16 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>  	bool needs_barrier = false;
>  	struct inode *inode = file->f_mapping->host;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	ret = ext4_is_emergency(inode->i_sb);
> +	if (unlikely(ret))
> +		return ret;
>  
>  	ASSERT(ext4_journal_current_handle() == NULL);
>  
>  	trace_ext4_sync_file_enter(file, datasync);
>  
> -	if (sb_rdonly(inode->i_sb)) {
> -		/* Make sure that we read updated s_ext4_flags value */
> -		smp_rmb();
> -		if (ext4_forced_shutdown(inode->i_sb))
> -			ret = -EROFS;
> +	if (sb_rdonly(inode->i_sb))
>  		goto out;
> -	}
>  
>  	if (!EXT4_SB(inode->i_sb)->s_journal) {
>  		ret = ext4_fsync_nojournal(file, start, end, datasync,
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index 21d228073d79..4d0af20fa319 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -951,8 +951,9 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
>  	sb = dir->i_sb;
>  	sbi = EXT4_SB(sb);
>  
> -	if (unlikely(ext4_forced_shutdown(sb)))
> -		return ERR_PTR(-EIO);
> +	ret2 = ext4_is_emergency(sb);
> +	if (unlikely(ret2))
> +		return ERR_PTR(ret2);
>  
>  	ngroups = ext4_get_groups_count(sb);
>  	trace_ext4_request_inode(dir, mode);
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 3536ca7e4fcc..d44cc9b5589e 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -228,7 +228,7 @@ static void ext4_write_inline_data(struct inode *inode, struct ext4_iloc *iloc,
>  	struct ext4_inode *raw_inode;
>  	int cp_len = 0;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> +	if (unlikely(ext4_is_emergency(inode->i_sb)))
>  		return;
>  
>  	BUG_ON(!EXT4_I(inode)->i_inline_off);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 7c54ae5fcbd4..3971e10874eb 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1149,8 +1149,9 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
>  	pgoff_t index;
>  	unsigned from, to;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	ret = ext4_is_emergency(inode->i_sb);
> +	if (unlikely(ret))
> +		return ret;
>  
>  	trace_ext4_write_begin(inode, pos, len);
>  	/*
> @@ -2273,7 +2274,7 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>  		if (err < 0) {
>  			struct super_block *sb = inode->i_sb;
>  
> -			if (ext4_forced_shutdown(sb))
> +			if (ext4_is_emergency(sb))
>  				goto invalidate_dirty_pages;
>  			/*
>  			 * Let the uper layers retry transient errors.
> @@ -2599,10 +2600,9 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
>  	 * *never* be called, so if that ever happens, we would want
>  	 * the stack trace.
>  	 */
> -	if (unlikely(ext4_forced_shutdown(mapping->host->i_sb))) {
> -		ret = -EROFS;
> +	ret = ext4_is_emergency(mapping->host->i_sb);
> +	if (unlikely(ret))
>  		goto out_writepages;
> -	}
>  
>  	/*
>  	 * If we have inline data and arrive here, it means that
> @@ -2817,8 +2817,9 @@ static int ext4_writepages(struct address_space *mapping,
>  	int ret;
>  	int alloc_ctx;
>  
> -	if (unlikely(ext4_forced_shutdown(sb)))
> -		return -EIO;
> +	ret = ext4_is_emergency(sb);
> +	if (unlikely(ret))
> +		return ret;
>  
>  	alloc_ctx = ext4_writepages_down_read(sb);
>  	ret = ext4_do_writepages(&mpd);
> @@ -2858,8 +2859,9 @@ static int ext4_dax_writepages(struct address_space *mapping,
>  	struct inode *inode = mapping->host;
>  	int alloc_ctx;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	ret = ext4_is_emergency(inode->i_sb);
> +	if (unlikely(ret))
> +		return ret;
>  
>  	alloc_ctx = ext4_writepages_down_read(inode->i_sb);
>  	trace_ext4_writepages(inode, wbc);
> @@ -2915,8 +2917,9 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
>  	pgoff_t index;
>  	struct inode *inode = mapping->host;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	ret = ext4_is_emergency(inode->i_sb);
> +	if (unlikely(ret))
> +		return ret;
>  
>  	index = pos >> PAGE_SHIFT;
>  
> @@ -5228,8 +5231,9 @@ int ext4_write_inode(struct inode *inode, struct writeback_control *wbc)
>  	if (WARN_ON_ONCE(current->flags & PF_MEMALLOC))
>  		return 0;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	err = ext4_is_emergency(inode->i_sb);
> +	if (unlikely(err))
> +		return err;
>  
>  	if (EXT4_SB(inode->i_sb)->s_journal) {
>  		if (ext4_journal_current_handle()) {
> @@ -5351,8 +5355,9 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>  	const unsigned int ia_valid = attr->ia_valid;
>  	bool inc_ivers = true;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	error = ext4_is_emergency(inode->i_sb);
> +	if (unlikely(error))
> +		return error;
>  
>  	if (unlikely(IS_IMMUTABLE(inode)))
>  		return -EPERM;
> @@ -5796,9 +5801,10 @@ int ext4_mark_iloc_dirty(handle_t *handle,
>  {
>  	int err = 0;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb))) {
> +	err = ext4_is_emergency(inode->i_sb);
> +	if (unlikely(err)) {
>  		put_bh(iloc->bh);
> -		return -EIO;
> +		return err;
>  	}
>  	ext4_fc_track_inode(handle, inode);
>  
> @@ -5822,8 +5828,9 @@ ext4_reserve_inode_write(handle_t *handle, struct inode *inode,
>  {
>  	int err;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	err = ext4_is_emergency(inode->i_sb);
> +	if (unlikely(err))
> +		return err;
>  
>  	err = ext4_get_inode_loc(inode, iloc);
>  	if (!err) {
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index b25a27c86696..7c783cb2a1dc 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -5653,7 +5653,7 @@ static inline void ext4_mb_show_pa(struct super_block *sb)
>  {
>  	ext4_group_t i, ngroups;
>  
> -	if (ext4_forced_shutdown(sb))
> +	if (ext4_is_emergency(sb))
>  		return;
>  
>  	ngroups = ext4_get_groups_count(sb);
> @@ -5687,7 +5687,7 @@ static void ext4_mb_show_ac(struct ext4_allocation_context *ac)
>  {
>  	struct super_block *sb = ac->ac_sb;
>  
> -	if (ext4_forced_shutdown(sb))
> +	if (ext4_is_emergency(sb))
>  		return;
>  
>  	mb_debug(sb, "Can't allocate:"
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index d64c04ed061a..a3ae72ce3aa1 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -162,7 +162,7 @@ static int kmmpd(void *data)
>  	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
>  	       sizeof(mmp->mmp_nodename));
>  
> -	while (!kthread_should_stop() && !ext4_forced_shutdown(sb)) {
> +	while (!kthread_should_stop() && !ext4_is_emergency(sb)) {
>  		if (!ext4_has_feature_mmp(sb)) {
>  			ext4_warning(sb, "kmmpd being stopped since MMP feature"
>  				     " has been disabled.");
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 536d56d15072..72907dd96e6a 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3151,8 +3151,9 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry)
>  	struct ext4_dir_entry_2 *de;
>  	handle_t *handle = NULL;
>  
> -	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
> -		return -EIO;
> +	retval = ext4_is_emergency(dir->i_sb);
> +	if (unlikely(retval))
> +		return retval;
>  
>  	/* Initialize quotas before so that eventual writes go in
>  	 * separate transaction */
> @@ -3309,8 +3310,9 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
>  {
>  	int retval;
>  
> -	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
> -		return -EIO;
> +	retval = ext4_is_emergency(dir->i_sb);
> +	if (unlikely(retval))
> +		return retval;
>  
>  	trace_ext4_unlink_enter(dir, dentry);
>  	/*
> @@ -3376,8 +3378,9 @@ static int ext4_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	struct fscrypt_str disk_link;
>  	int retries = 0;
>  
> -	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
> -		return -EIO;
> +	err = ext4_is_emergency(dir->i_sb);
> +	if (unlikely(err))
> +		return err;
>  
>  	err = fscrypt_prepare_symlink(dir, symname, len, dir->i_sb->s_blocksize,
>  				      &disk_link);
> @@ -4199,8 +4202,9 @@ static int ext4_rename2(struct mnt_idmap *idmap,
>  {
>  	int err;
>  
> -	if (unlikely(ext4_forced_shutdown(old_dir->i_sb)))
> -		return -EIO;
> +	err = ext4_is_emergency(old_dir->i_sb);
> +	if (unlikely(err))
> +		return err;
>  
>  	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE | RENAME_WHITEOUT))
>  		return -EINVAL;
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 69b8a7221a2b..0e5e1de6b534 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -183,7 +183,7 @@ static int ext4_end_io_end(ext4_io_end_t *io_end)
>  
>  	io_end->handle = NULL;	/* Following call will use up the handle */
>  	ret = ext4_convert_unwritten_io_end_vec(handle, io_end);
> -	if (ret < 0 && !ext4_forced_shutdown(inode->i_sb)) {
> +	if (ret < 0 && !ext4_is_emergency(inode->i_sb)) {
>  		ext4_msg(inode->i_sb, KERN_EMERG,
>  			 "failed to convert unwritten extents to written "
>  			 "extents -- potential data loss!  "
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index a50e5c31b937..c12133628ee9 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -819,7 +819,7 @@ void __ext4_error(struct super_block *sb, const char *function,
>  	struct va_format vaf;
>  	va_list args;
>  
> -	if (unlikely(ext4_forced_shutdown(sb)))
> +	if (unlikely(ext4_is_emergency(sb)))
>  		return;
>  
>  	trace_ext4_error(sb, function, line);
> @@ -844,7 +844,7 @@ void __ext4_error_inode(struct inode *inode, const char *function,
>  	va_list args;
>  	struct va_format vaf;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> +	if (unlikely(ext4_is_emergency(inode->i_sb)))
>  		return;
>  
>  	trace_ext4_error(inode->i_sb, function, line);
> @@ -879,7 +879,7 @@ void __ext4_error_file(struct file *file, const char *function,
>  	struct inode *inode = file_inode(file);
>  	char pathname[80], *path;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> +	if (unlikely(ext4_is_emergency(inode->i_sb)))
>  		return;
>  
>  	trace_ext4_error(inode->i_sb, function, line);
> @@ -959,7 +959,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
>  	char nbuf[16];
>  	const char *errstr;
>  
> -	if (unlikely(ext4_forced_shutdown(sb)))
> +	if (unlikely(ext4_is_emergency(sb)))
>  		return;
>  
>  	/* Special case: if the error is EROFS, and we're not already
> @@ -1053,7 +1053,7 @@ __acquires(bitlock)
>  	struct va_format vaf;
>  	va_list args;
>  
> -	if (unlikely(ext4_forced_shutdown(sb)))
> +	if (unlikely(ext4_is_emergency(sb)))
>  		return;
>  
>  	trace_ext4_error(sb, function, line);
> @@ -6336,8 +6336,9 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
>  	bool needs_barrier = false;
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  
> -	if (unlikely(ext4_forced_shutdown(sb)))
> -		return -EIO;
> +	ret = ext4_is_emergency(sb);
> +	if (unlikely(ret))
> +		return ret;
>  
>  	trace_ext4_sync_fs(sb, wait);
>  	flush_workqueue(sbi->rsv_conversion_wq);
> @@ -6419,7 +6420,7 @@ static int ext4_freeze(struct super_block *sb)
>   */
>  static int ext4_unfreeze(struct super_block *sb)
>  {
> -	if (ext4_forced_shutdown(sb))
> +	if (ext4_is_emergency(sb))
>  		return 0;
>  
>  	if (EXT4_SB(sb)->s_journal) {
> @@ -6575,7 +6576,7 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
>  	flush_work(&sbi->s_sb_upd_work);
>  
>  	if ((bool)(fc->sb_flags & SB_RDONLY) != sb_rdonly(sb)) {
> -		if (ext4_forced_shutdown(sb)) {
> +		if (ext4_is_emergency(sb)) {
>  			err = -EROFS;
>  			goto restore_opts;
>  		}
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/7] ext4: convert EXT4_FLAGS_* defines to enum

[Baokun Li]

On 2025/1/21 20:07, Jan Kara wrote:
> On Fri 17-01-25 16:23:09, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> Do away with the defines and use an enum as it's cleaner.
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Yeah, why not. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> 								Honza
Thank you for the review!
>> ---
>>   fs/ext4/ext4.h | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 4e7de7eaa374..612208527512 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -2232,9 +2232,11 @@ extern int ext4_feature_set_ok(struct super_block *sb, int readonly);
>>   /*
>>    * Superblock flags
>>    */
>> -#define EXT4_FLAGS_RESIZING	0
>> -#define EXT4_FLAGS_SHUTDOWN	1
>> -#define EXT4_FLAGS_BDEV_IS_DAX	2
>> +enum {
>> +	EXT4_FLAGS_RESIZING,	/* Avoid superblock update and resize race */
>> +	EXT4_FLAGS_SHUTDOWN,	/* Prevent access to the file system */
>> +	EXT4_FLAGS_BDEV_IS_DAX	/* Current block device support DAX */
> 			      ^^ we usually put comma here so that future
> additions doesn't need to modify this line.
Okay, I will add a comma here in the next version.

Thanks,
Baokun
>> +};
>>   
>>   static inline int ext4_forced_shutdown(struct super_block *sb)
>>   {
>> -- 
>> 2.39.2
>>

Re: [PATCH 2/7] ext4: add EXT4_FLAGS_EMERGENCY_RO bit

[Baokun Li]

On 2025/1/21 20:08, Jan Kara wrote:
> On Fri 17-01-25 16:23:10, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> EXT4_FLAGS_EMERGENCY_RO Indicates that the current file system has become
>> read-only due to some error. Compared to SB_RDONLY, setting it does not
>> require a lock because we won't clear it, which avoids over-coupling with
>> vfs freeze. Also, add a helper function ext4_emergency_ro() to check if
>> the bit is set.
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> The same comment about comma after the last enum member. Otherwise looks
> good. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> 								Honza
Okay, I'm going to add a comma here in the next version as well.

Thank you for your review!


Regards,
Baokun

>> ---
>>   fs/ext4/ext4.h | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 612208527512..c5b775482897 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -2235,7 +2235,8 @@ extern int ext4_feature_set_ok(struct super_block *sb, int readonly);
>>   enum {
>>   	EXT4_FLAGS_RESIZING,	/* Avoid superblock update and resize race */
>>   	EXT4_FLAGS_SHUTDOWN,	/* Prevent access to the file system */
>> -	EXT4_FLAGS_BDEV_IS_DAX	/* Current block device support DAX */
>> +	EXT4_FLAGS_BDEV_IS_DAX,	/* Current block device support DAX */
>> +	EXT4_FLAGS_EMERGENCY_RO	/* Emergency read-only due to fs errors */
>>   };
>>   
>>   static inline int ext4_forced_shutdown(struct super_block *sb)
>> @@ -2243,6 +2244,11 @@ static inline int ext4_forced_shutdown(struct super_block *sb)
>>   	return test_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
>>   }
>>   
>> +static inline int ext4_emergency_ro(struct super_block *sb)
>> +{
>> +	return test_bit(EXT4_FLAGS_EMERGENCY_RO, &EXT4_SB(sb)->s_ext4_flags);
>> +}
>> +
>>   /*
>>    * Default values for user and/or group using reserved blocks
>>    */
>> -- 
>> 2.39.2
>>

Re: [PATCH 3/7] ext4: add ext4_is_emergency() helper function

[Baokun Li]

On 2025/1/21 20:14, Jan Kara wrote:
> On Fri 17-01-25 16:23:11, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> Since both SHUTDOWN and EMERGENCY_RO are emergency states of the ext4 file
>> system, and they are checked in similar locations, we have added a helper
>> function, ext4_is_emergency(), to determine whether the current file system
>> is in one of these two emergency states.
>>
>> Then, replace calls to ext4_forced_shutdown() with ext4_is_emergency() in
>> those functions that could potentially trigger write operations.
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Looks good, just one naming suggestion:
>
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index c5b775482897..ca01b476e42b 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -2249,6 +2249,15 @@ static inline int ext4_emergency_ro(struct super_block *sb)
>>   	return test_bit(EXT4_FLAGS_EMERGENCY_RO, &EXT4_SB(sb)->s_ext4_flags);
>>   }
>>   
>> +static inline int ext4_is_emergency(struct super_block *sb)
>> +{
>> +	if (unlikely(ext4_forced_shutdown(sb)))
>> +		return -EIO;
>> +	if (unlikely(ext4_emergency_ro(sb)))
>> +		return -EROFS;
>> +	return 0;
>> +}
> Since this actually returns error I'd call it ext4_emergency_state() or
> something like that. Otherwise feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> 								Honza
Yeah, ext4_emergency_state() sounds better than ext4_is_emergency().
We thought about ext4_sb_permission, ext4_check_writable, and
ext4_sb_access, but felt that none of them were quite suitable.

I'll use ext4_emergency_state() in the next version.
Thanks for your review and the naming suggestion!


Cheers,
Baokun

>
>> +
>>   /*
>>    * Default values for user and/or group using reserved blocks
>>    */
>> diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
>> index da4a82456383..2c4e976360f1 100644
>> --- a/fs/ext4/ext4_jbd2.c
>> +++ b/fs/ext4/ext4_jbd2.c
>> @@ -63,12 +63,14 @@ static void ext4_put_nojournal(handle_t *handle)
>>    */
>>   static int ext4_journal_check_start(struct super_block *sb)
>>   {
>> +	int ret;
>>   	journal_t *journal;
>>   
>>   	might_sleep();
>>   
>> -	if (unlikely(ext4_forced_shutdown(sb)))
>> -		return -EIO;
>> +	ret = ext4_is_emergency(sb);
>> +	if (unlikely(ret))
>> +		return ret;
>>   
>>   	if (WARN_ON_ONCE(sb_rdonly(sb)))
>>   		return -EROFS;
>> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
>> index a5205149adba..6db052a87b9b 100644
>> --- a/fs/ext4/file.c
>> +++ b/fs/ext4/file.c
>> @@ -688,10 +688,12 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
>>   static ssize_t
>>   ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>>   {
>> +	int ret;
>>   	struct inode *inode = file_inode(iocb->ki_filp);
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> -		return -EIO;
>> +	ret = ext4_is_emergency(inode->i_sb);
>> +	if (unlikely(ret))
>> +		return ret;
>>   
>>   #ifdef CONFIG_FS_DAX
>>   	if (IS_DAX(inode))
>> @@ -700,7 +702,6 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>>   
>>   	if (iocb->ki_flags & IOCB_ATOMIC) {
>>   		size_t len = iov_iter_count(from);
>> -		int ret;
>>   
>>   		if (len < EXT4_SB(inode->i_sb)->s_awu_min ||
>>   		    len > EXT4_SB(inode->i_sb)->s_awu_max)
>> @@ -803,11 +804,16 @@ static const struct vm_operations_struct ext4_file_vm_ops = {
>>   
>>   static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
>>   {
>> +	int ret;
>>   	struct inode *inode = file->f_mapping->host;
>>   	struct dax_device *dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> -		return -EIO;
>> +	if (file->f_mode & FMODE_WRITE)
>> +		ret = ext4_is_emergency(inode->i_sb);
>> +	else
>> +		ret = ext4_forced_shutdown(inode->i_sb) ? -EIO : 0;
>> +	if (unlikely(ret))
>> +		return ret;
>>   
>>   	/*
>>   	 * We don't support synchronous mappings for non-DAX files and
>> @@ -881,8 +887,12 @@ static int ext4_file_open(struct inode *inode, struct file *filp)
>>   {
>>   	int ret;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> -		return -EIO;
>> +	if (filp->f_mode & FMODE_WRITE)
>> +		ret = ext4_is_emergency(inode->i_sb);
>> +	else
>> +		ret = ext4_forced_shutdown(inode->i_sb) ? -EIO : 0;
>> +	if (unlikely(ret))
>> +		return ret;
>>   
>>   	ret = ext4_sample_last_mounted(inode->i_sb, filp->f_path.mnt);
>>   	if (ret)
>> diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
>> index b40d3b29f7e5..ee9078a5d098 100644
>> --- a/fs/ext4/fsync.c
>> +++ b/fs/ext4/fsync.c
>> @@ -132,20 +132,16 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>>   	bool needs_barrier = false;
>>   	struct inode *inode = file->f_mapping->host;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> -		return -EIO;
>> +	ret = ext4_is_emergency(inode->i_sb);
>> +	if (unlikely(ret))
>> +		return ret;
>>   
>>   	ASSERT(ext4_journal_current_handle() == NULL);
>>   
>>   	trace_ext4_sync_file_enter(file, datasync);
>>   
>> -	if (sb_rdonly(inode->i_sb)) {
>> -		/* Make sure that we read updated s_ext4_flags value */
>> -		smp_rmb();
>> -		if (ext4_forced_shutdown(inode->i_sb))
>> -			ret = -EROFS;
>> +	if (sb_rdonly(inode->i_sb))
>>   		goto out;
>> -	}
>>   
>>   	if (!EXT4_SB(inode->i_sb)->s_journal) {
>>   		ret = ext4_fsync_nojournal(file, start, end, datasync,
>> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
>> index 21d228073d79..4d0af20fa319 100644
>> --- a/fs/ext4/ialloc.c
>> +++ b/fs/ext4/ialloc.c
>> @@ -951,8 +951,9 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
>>   	sb = dir->i_sb;
>>   	sbi = EXT4_SB(sb);
>>   
>> -	if (unlikely(ext4_forced_shutdown(sb)))
>> -		return ERR_PTR(-EIO);
>> +	ret2 = ext4_is_emergency(sb);
>> +	if (unlikely(ret2))
>> +		return ERR_PTR(ret2);
>>   
>>   	ngroups = ext4_get_groups_count(sb);
>>   	trace_ext4_request_inode(dir, mode);
>> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
>> index 3536ca7e4fcc..d44cc9b5589e 100644
>> --- a/fs/ext4/inline.c
>> +++ b/fs/ext4/inline.c
>> @@ -228,7 +228,7 @@ static void ext4_write_inline_data(struct inode *inode, struct ext4_iloc *iloc,
>>   	struct ext4_inode *raw_inode;
>>   	int cp_len = 0;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> +	if (unlikely(ext4_is_emergency(inode->i_sb)))
>>   		return;
>>   
>>   	BUG_ON(!EXT4_I(inode)->i_inline_off);
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 7c54ae5fcbd4..3971e10874eb 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -1149,8 +1149,9 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
>>   	pgoff_t index;
>>   	unsigned from, to;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> -		return -EIO;
>> +	ret = ext4_is_emergency(inode->i_sb);
>> +	if (unlikely(ret))
>> +		return ret;
>>   
>>   	trace_ext4_write_begin(inode, pos, len);
>>   	/*
>> @@ -2273,7 +2274,7 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>>   		if (err < 0) {
>>   			struct super_block *sb = inode->i_sb;
>>   
>> -			if (ext4_forced_shutdown(sb))
>> +			if (ext4_is_emergency(sb))
>>   				goto invalidate_dirty_pages;
>>   			/*
>>   			 * Let the uper layers retry transient errors.
>> @@ -2599,10 +2600,9 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
>>   	 * *never* be called, so if that ever happens, we would want
>>   	 * the stack trace.
>>   	 */
>> -	if (unlikely(ext4_forced_shutdown(mapping->host->i_sb))) {
>> -		ret = -EROFS;
>> +	ret = ext4_is_emergency(mapping->host->i_sb);
>> +	if (unlikely(ret))
>>   		goto out_writepages;
>> -	}
>>   
>>   	/*
>>   	 * If we have inline data and arrive here, it means that
>> @@ -2817,8 +2817,9 @@ static int ext4_writepages(struct address_space *mapping,
>>   	int ret;
>>   	int alloc_ctx;
>>   
>> -	if (unlikely(ext4_forced_shutdown(sb)))
>> -		return -EIO;
>> +	ret = ext4_is_emergency(sb);
>> +	if (unlikely(ret))
>> +		return ret;
>>   
>>   	alloc_ctx = ext4_writepages_down_read(sb);
>>   	ret = ext4_do_writepages(&mpd);
>> @@ -2858,8 +2859,9 @@ static int ext4_dax_writepages(struct address_space *mapping,
>>   	struct inode *inode = mapping->host;
>>   	int alloc_ctx;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> -		return -EIO;
>> +	ret = ext4_is_emergency(inode->i_sb);
>> +	if (unlikely(ret))
>> +		return ret;
>>   
>>   	alloc_ctx = ext4_writepages_down_read(inode->i_sb);
>>   	trace_ext4_writepages(inode, wbc);
>> @@ -2915,8 +2917,9 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
>>   	pgoff_t index;
>>   	struct inode *inode = mapping->host;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> -		return -EIO;
>> +	ret = ext4_is_emergency(inode->i_sb);
>> +	if (unlikely(ret))
>> +		return ret;
>>   
>>   	index = pos >> PAGE_SHIFT;
>>   
>> @@ -5228,8 +5231,9 @@ int ext4_write_inode(struct inode *inode, struct writeback_control *wbc)
>>   	if (WARN_ON_ONCE(current->flags & PF_MEMALLOC))
>>   		return 0;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> -		return -EIO;
>> +	err = ext4_is_emergency(inode->i_sb);
>> +	if (unlikely(err))
>> +		return err;
>>   
>>   	if (EXT4_SB(inode->i_sb)->s_journal) {
>>   		if (ext4_journal_current_handle()) {
>> @@ -5351,8 +5355,9 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>>   	const unsigned int ia_valid = attr->ia_valid;
>>   	bool inc_ivers = true;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> -		return -EIO;
>> +	error = ext4_is_emergency(inode->i_sb);
>> +	if (unlikely(error))
>> +		return error;
>>   
>>   	if (unlikely(IS_IMMUTABLE(inode)))
>>   		return -EPERM;
>> @@ -5796,9 +5801,10 @@ int ext4_mark_iloc_dirty(handle_t *handle,
>>   {
>>   	int err = 0;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb))) {
>> +	err = ext4_is_emergency(inode->i_sb);
>> +	if (unlikely(err)) {
>>   		put_bh(iloc->bh);
>> -		return -EIO;
>> +		return err;
>>   	}
>>   	ext4_fc_track_inode(handle, inode);
>>   
>> @@ -5822,8 +5828,9 @@ ext4_reserve_inode_write(handle_t *handle, struct inode *inode,
>>   {
>>   	int err;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> -		return -EIO;
>> +	err = ext4_is_emergency(inode->i_sb);
>> +	if (unlikely(err))
>> +		return err;
>>   
>>   	err = ext4_get_inode_loc(inode, iloc);
>>   	if (!err) {
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index b25a27c86696..7c783cb2a1dc 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -5653,7 +5653,7 @@ static inline void ext4_mb_show_pa(struct super_block *sb)
>>   {
>>   	ext4_group_t i, ngroups;
>>   
>> -	if (ext4_forced_shutdown(sb))
>> +	if (ext4_is_emergency(sb))
>>   		return;
>>   
>>   	ngroups = ext4_get_groups_count(sb);
>> @@ -5687,7 +5687,7 @@ static void ext4_mb_show_ac(struct ext4_allocation_context *ac)
>>   {
>>   	struct super_block *sb = ac->ac_sb;
>>   
>> -	if (ext4_forced_shutdown(sb))
>> +	if (ext4_is_emergency(sb))
>>   		return;
>>   
>>   	mb_debug(sb, "Can't allocate:"
>> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
>> index d64c04ed061a..a3ae72ce3aa1 100644
>> --- a/fs/ext4/mmp.c
>> +++ b/fs/ext4/mmp.c
>> @@ -162,7 +162,7 @@ static int kmmpd(void *data)
>>   	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
>>   	       sizeof(mmp->mmp_nodename));
>>   
>> -	while (!kthread_should_stop() && !ext4_forced_shutdown(sb)) {
>> +	while (!kthread_should_stop() && !ext4_is_emergency(sb)) {
>>   		if (!ext4_has_feature_mmp(sb)) {
>>   			ext4_warning(sb, "kmmpd being stopped since MMP feature"
>>   				     " has been disabled.");
>> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
>> index 536d56d15072..72907dd96e6a 100644
>> --- a/fs/ext4/namei.c
>> +++ b/fs/ext4/namei.c
>> @@ -3151,8 +3151,9 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry)
>>   	struct ext4_dir_entry_2 *de;
>>   	handle_t *handle = NULL;
>>   
>> -	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
>> -		return -EIO;
>> +	retval = ext4_is_emergency(dir->i_sb);
>> +	if (unlikely(retval))
>> +		return retval;
>>   
>>   	/* Initialize quotas before so that eventual writes go in
>>   	 * separate transaction */
>> @@ -3309,8 +3310,9 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
>>   {
>>   	int retval;
>>   
>> -	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
>> -		return -EIO;
>> +	retval = ext4_is_emergency(dir->i_sb);
>> +	if (unlikely(retval))
>> +		return retval;
>>   
>>   	trace_ext4_unlink_enter(dir, dentry);
>>   	/*
>> @@ -3376,8 +3378,9 @@ static int ext4_symlink(struct mnt_idmap *idmap, struct inode *dir,
>>   	struct fscrypt_str disk_link;
>>   	int retries = 0;
>>   
>> -	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
>> -		return -EIO;
>> +	err = ext4_is_emergency(dir->i_sb);
>> +	if (unlikely(err))
>> +		return err;
>>   
>>   	err = fscrypt_prepare_symlink(dir, symname, len, dir->i_sb->s_blocksize,
>>   				      &disk_link);
>> @@ -4199,8 +4202,9 @@ static int ext4_rename2(struct mnt_idmap *idmap,
>>   {
>>   	int err;
>>   
>> -	if (unlikely(ext4_forced_shutdown(old_dir->i_sb)))
>> -		return -EIO;
>> +	err = ext4_is_emergency(old_dir->i_sb);
>> +	if (unlikely(err))
>> +		return err;
>>   
>>   	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE | RENAME_WHITEOUT))
>>   		return -EINVAL;
>> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
>> index 69b8a7221a2b..0e5e1de6b534 100644
>> --- a/fs/ext4/page-io.c
>> +++ b/fs/ext4/page-io.c
>> @@ -183,7 +183,7 @@ static int ext4_end_io_end(ext4_io_end_t *io_end)
>>   
>>   	io_end->handle = NULL;	/* Following call will use up the handle */
>>   	ret = ext4_convert_unwritten_io_end_vec(handle, io_end);
>> -	if (ret < 0 && !ext4_forced_shutdown(inode->i_sb)) {
>> +	if (ret < 0 && !ext4_is_emergency(inode->i_sb)) {
>>   		ext4_msg(inode->i_sb, KERN_EMERG,
>>   			 "failed to convert unwritten extents to written "
>>   			 "extents -- potential data loss!  "
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index a50e5c31b937..c12133628ee9 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -819,7 +819,7 @@ void __ext4_error(struct super_block *sb, const char *function,
>>   	struct va_format vaf;
>>   	va_list args;
>>   
>> -	if (unlikely(ext4_forced_shutdown(sb)))
>> +	if (unlikely(ext4_is_emergency(sb)))
>>   		return;
>>   
>>   	trace_ext4_error(sb, function, line);
>> @@ -844,7 +844,7 @@ void __ext4_error_inode(struct inode *inode, const char *function,
>>   	va_list args;
>>   	struct va_format vaf;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> +	if (unlikely(ext4_is_emergency(inode->i_sb)))
>>   		return;
>>   
>>   	trace_ext4_error(inode->i_sb, function, line);
>> @@ -879,7 +879,7 @@ void __ext4_error_file(struct file *file, const char *function,
>>   	struct inode *inode = file_inode(file);
>>   	char pathname[80], *path;
>>   
>> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
>> +	if (unlikely(ext4_is_emergency(inode->i_sb)))
>>   		return;
>>   
>>   	trace_ext4_error(inode->i_sb, function, line);
>> @@ -959,7 +959,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
>>   	char nbuf[16];
>>   	const char *errstr;
>>   
>> -	if (unlikely(ext4_forced_shutdown(sb)))
>> +	if (unlikely(ext4_is_emergency(sb)))
>>   		return;
>>   
>>   	/* Special case: if the error is EROFS, and we're not already
>> @@ -1053,7 +1053,7 @@ __acquires(bitlock)
>>   	struct va_format vaf;
>>   	va_list args;
>>   
>> -	if (unlikely(ext4_forced_shutdown(sb)))
>> +	if (unlikely(ext4_is_emergency(sb)))
>>   		return;
>>   
>>   	trace_ext4_error(sb, function, line);
>> @@ -6336,8 +6336,9 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
>>   	bool needs_barrier = false;
>>   	struct ext4_sb_info *sbi = EXT4_SB(sb);
>>   
>> -	if (unlikely(ext4_forced_shutdown(sb)))
>> -		return -EIO;
>> +	ret = ext4_is_emergency(sb);
>> +	if (unlikely(ret))
>> +		return ret;
>>   
>>   	trace_ext4_sync_fs(sb, wait);
>>   	flush_workqueue(sbi->rsv_conversion_wq);
>> @@ -6419,7 +6420,7 @@ static int ext4_freeze(struct super_block *sb)
>>    */
>>   static int ext4_unfreeze(struct super_block *sb)
>>   {
>> -	if (ext4_forced_shutdown(sb))
>> +	if (ext4_is_emergency(sb))
>>   		return 0;
>>   
>>   	if (EXT4_SB(sb)->s_journal) {
>> @@ -6575,7 +6576,7 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
>>   	flush_work(&sbi->s_sb_upd_work);
>>   
>>   	if ((bool)(fc->sb_flags & SB_RDONLY) != sb_rdonly(sb)) {
>> -		if (ext4_forced_shutdown(sb)) {
>> +		if (ext4_is_emergency(sb)) {
>>   			err = -EROFS;
>>   			goto restore_opts;
>>   		}
>> -- 
>> 2.39.2
>>

Re: [PATCH 4/7] ext4: add ext4_sb_rdonly() helper function

[Jan Kara]

On Fri 17-01-25 16:23:12, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> Because both SB_RDONLY and EXT4_FLAGS_EMERGENCY_RO indicate the file system
> is read-only, the ext4_sb_rdonly() helper function is added. This function
> returns true if either flag is set, signifying that the file system is
> read-only.
> 
> Then replace some sb_rdonly() with ext4_sb_rdonly() to avoid unexpected
> failures of some read-only operations or modification of the superblock
> after setting EXT4_FLAGS_EMERGENCY_RO.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

I'm not sure we really need this. I rather think more places need
additional ext4_emergency_state() checks. Look:

> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 6db052a87b9b..70b556c87b88 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -844,7 +844,7 @@ static int ext4_sample_last_mounted(struct super_block *sb,
>  	if (likely(ext4_test_mount_flag(sb, EXT4_MF_MNTDIR_SAMPLED)))
>  		return 0;
>  
> -	if (sb_rdonly(sb) || !sb_start_intwrite_trylock(sb))
> +	if (ext4_sb_rdonly(sb) || !sb_start_intwrite_trylock(sb))

We don't want to be modifying superblock if the filesystem is shutdown so I
think we should have here something like:

	if (ext4_emergency_state(sb) || sb_rdonly(sb) ||
	    !sb_start_intwrite_trylock(sb))

>  		return 0;
>  
>  	ext4_set_mount_flag(sb, EXT4_MF_MNTDIR_SAMPLED);
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 7b9ce71c1c81..0807ee8cbcdc 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1705,7 +1705,7 @@ int ext4_update_overhead(struct super_block *sb, bool force)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  
> -	if (sb_rdonly(sb))
> +	if (ext4_sb_rdonly(sb))
>  		return 0;

Similarly here I think we should have:

	if (ext4_emergency_state(sb) || sb_rdonly(sb))
		return 0;

> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c12133628ee9..fc5d30123f22 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -473,7 +473,7 @@ static void ext4_maybe_update_superblock(struct super_block *sb)
>  	__u64 lifetime_write_kbytes;
>  	__u64 diff_size;
>  
> -	if (sb_rdonly(sb) || !(sb->s_flags & SB_ACTIVE) ||
> +	if (ext4_sb_rdonly(sb) || !(sb->s_flags & SB_ACTIVE) ||
>  	    !journal || (journal->j_flags & JBD2_UNMOUNT))
>  		return;

And here we should add ext4_emergency_state() check as well.

> @@ -707,7 +707,7 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
>  	if (test_opt(sb, WARN_ON_ERROR))
>  		WARN_ON_ONCE(1);
>  
> -	if (!continue_fs && !sb_rdonly(sb)) {
> +	if (!continue_fs && !ext4_sb_rdonly(sb)) {

Here I actually think we should just drop the sb_rdonly() check completely?
Because callers have already checked we are not in emergency state yet and
we want to shutdown the fs (or later flag the emergency RO state) even if
the filesystem is mounted read only?

>  		set_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
>  		if (journal)
>  			jbd2_journal_abort(journal, -EIO);
> @@ -737,7 +737,7 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
>  			sb->s_id);
>  	}
>  
> -	if (sb_rdonly(sb) || continue_fs)
> +	if (ext4_sb_rdonly(sb) || continue_fs)
>  		return;

This will need a bit of reworking with the emergency ro flag anyway so for
now I'd leave it as is.

>  
>  	ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
> @@ -765,7 +765,7 @@ static void update_super_work(struct work_struct *work)
>  	 * We use directly jbd2 functions here to avoid recursing back into
>  	 * ext4 error handling code during handling of previous errors.
>  	 */
> -	if (!sb_rdonly(sbi->s_sb) && journal) {
> +	if (!ext4_sb_rdonly(sbi->s_sb) && journal) {
>  		struct buffer_head *sbh = sbi->s_sbh;
>  		bool call_notify_err = false;

Again here I think we should just add ext4_emergency_state() check because
we don't want to be modifying superblock on shutdown filesystem either. And
in the four cases below as well.

> @@ -1325,12 +1325,12 @@ static void ext4_put_super(struct super_block *sb)
>  	ext4_mb_release(sb);
>  	ext4_ext_release(sb);
>  
> -	if (!sb_rdonly(sb) && !aborted) {
> +	if (!ext4_sb_rdonly(sb) && !aborted) {
>  		ext4_clear_feature_journal_needs_recovery(sb);
>  		ext4_clear_feature_orphan_present(sb);
>  		es->s_state = cpu_to_le16(sbi->s_mount_state);
>  	}
> -	if (!sb_rdonly(sb))
> +	if (!ext4_sb_rdonly(sb))
>  		ext4_commit_super(sb);
>  
>  	ext4_group_desc_free(sbi);
> @@ -3693,7 +3693,8 @@ static int ext4_run_li_request(struct ext4_li_request *elr)
>  		if (group >= elr->lr_next_group) {
>  			ret = 1;
>  			if (elr->lr_first_not_zeroed != ngroups &&
> -			    !sb_rdonly(sb) && test_opt(sb, INIT_INODE_TABLE)) {
> +			    !ext4_sb_rdonly(sb) &&
> +			    test_opt(sb, INIT_INODE_TABLE)) {
>  				elr->lr_next_group = elr->lr_first_not_zeroed;
>  				elr->lr_mode = EXT4_LI_MODE_ITABLE;
>  				ret = 0;
> @@ -3998,7 +3999,7 @@ int ext4_register_li_request(struct super_block *sb,
>  		goto out;
>  	}
>  
> -	if (sb_rdonly(sb) ||
> +	if (ext4_sb_rdonly(sb) ||
>  	    (test_opt(sb, NO_PREFETCH_BLOCK_BITMAPS) &&
>  	     (first_not_zeroed == ngroups || !test_opt(sb, INIT_INODE_TABLE))))
>  		goto out;


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

Re: [PATCH 6/7] ext4: show 'emergency_ro' when EXT4_FLAGS_EMERGENCY_RO is set

[Jan Kara]

On Fri 17-01-25 16:23:14, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> After commit d3476f3dad4a ("ext4: don't set SB_RDONLY after filesystem
> errors") in v6.12-rc1, the 'errors=remount-ro' mode no longer sets
> SB_RDONLY on errors, which results in us seeing the filesystem is still
> in rw state after errors.
> 
> Therefore, after setting EXT4_FLAGS_EMERGENCY_RO, display the emergency_ro
> option so that users can query whether the current file system has become
> emergency read-only due to errors through commands such as 'mount' or
> 'cat /proc/fs/ext4/sdx/options'.
> 
> Fixes: d3476f3dad4a ("ext4: don't set SB_RDONLY after filesystem errors")
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/super.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 8d9ac8770764..2377ebf0aff1 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3029,6 +3029,9 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,
>  	if (nodefs && !test_opt(sb, NO_PREFETCH_BLOCK_BITMAPS))
>  		SEQ_OPTS_PUTS("prefetch_block_bitmaps");
>  
> +	if (ext4_emergency_ro(sb))
> +		SEQ_OPTS_PUTS("emergency_ro");
> +
>  	ext4_show_quota_options(seq, sb);
>  	return 0;
>  }
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 7/7] ext4: show 'shutdown' hint when ext4 is forced to shutdown

[Jan Kara]

On Fri 17-01-25 16:23:15, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> Now, if dmesg is cleared, we have no way of knowing if the file system has
> been shutdown. Moreover, ext4 allows directory reads even after the file
> system has been shutdown, so when reading a file returns -EIO, we cannot
> determine whether this is a hardware issue or if the file system has been
> shutdown.
> 
> Therefore, when ext4 file system is shutdown, we're adding a 'shutdown'
> hint to commands like mount so users can easily check the file system's
> status.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/super.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 2377ebf0aff1..b15c36df934c 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3032,6 +3032,9 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,
>  	if (ext4_emergency_ro(sb))
>  		SEQ_OPTS_PUTS("emergency_ro");
>  
> +	if (ext4_forced_shutdown(sb))
> +		SEQ_OPTS_PUTS("shutdown");
> +
>  	ext4_show_quota_options(seq, sb);
>  	return 0;
>  }
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/7] ext4: add ext4_sb_rdonly() helper function

[Baokun Li]

On 2025/1/21 21:11, Jan Kara wrote:
> On Fri 17-01-25 16:23:12, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> Because both SB_RDONLY and EXT4_FLAGS_EMERGENCY_RO indicate the file system
>> is read-only, the ext4_sb_rdonly() helper function is added. This function
>> returns true if either flag is set, signifying that the file system is
>> read-only.
>>
>> Then replace some sb_rdonly() with ext4_sb_rdonly() to avoid unexpected
>> failures of some read-only operations or modification of the superblock
>> after setting EXT4_FLAGS_EMERGENCY_RO.
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> I'm not sure we really need this. I rather think more places need
> additional ext4_emergency_state() checks. Look:
Make sense. 🤔
>
>> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
>> index 6db052a87b9b..70b556c87b88 100644
>> --- a/fs/ext4/file.c
>> +++ b/fs/ext4/file.c
>> @@ -844,7 +844,7 @@ static int ext4_sample_last_mounted(struct super_block *sb,
>>   	if (likely(ext4_test_mount_flag(sb, EXT4_MF_MNTDIR_SAMPLED)))
>>   		return 0;
>>   
>> -	if (sb_rdonly(sb) || !sb_start_intwrite_trylock(sb))
>> +	if (ext4_sb_rdonly(sb) || !sb_start_intwrite_trylock(sb))
> We don't want to be modifying superblock if the filesystem is shutdown so I
> think we should have here something like:
>
> 	if (ext4_emergency_state(sb) || sb_rdonly(sb) ||
> 	    !sb_start_intwrite_trylock(sb))
That's right, if the file system is already down, the caller will
return -EIO before calling ext4_sample_last_mounted().
>>   		return 0;
>>   
>>   	ext4_set_mount_flag(sb, EXT4_MF_MNTDIR_SAMPLED);
>> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>> index 7b9ce71c1c81..0807ee8cbcdc 100644
>> --- a/fs/ext4/ioctl.c
>> +++ b/fs/ext4/ioctl.c
>> @@ -1705,7 +1705,7 @@ int ext4_update_overhead(struct super_block *sb, bool force)
>>   {
>>   	struct ext4_sb_info *sbi = EXT4_SB(sb);
>>   
>> -	if (sb_rdonly(sb))
>> +	if (ext4_sb_rdonly(sb))
>>   		return 0;
> Similarly here I think we should have:
>
> 	if (ext4_emergency_state(sb) || sb_rdonly(sb))
> 		return 0;
Alright, even though there could be some overhead inconsistencies here,
we update s_overhead when mounting if bigalloc is not enabled.
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index c12133628ee9..fc5d30123f22 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -473,7 +473,7 @@ static void ext4_maybe_update_superblock(struct super_block *sb)
>>   	__u64 lifetime_write_kbytes;
>>   	__u64 diff_size;
>>   
>> -	if (sb_rdonly(sb) || !(sb->s_flags & SB_ACTIVE) ||
>> +	if (ext4_sb_rdonly(sb) || !(sb->s_flags & SB_ACTIVE) ||
>>   	    !journal || (journal->j_flags & JBD2_UNMOUNT))
>>   		return;
> And here we should add ext4_emergency_state() check as well.
Right, the return value doesn't matter here.
>> @@ -707,7 +707,7 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
>>   	if (test_opt(sb, WARN_ON_ERROR))
>>   		WARN_ON_ONCE(1);
>>   
>> -	if (!continue_fs && !sb_rdonly(sb)) {
>> +	if (!continue_fs && !ext4_sb_rdonly(sb)) {
> Here I actually think we should just drop the sb_rdonly() check completely?
> Because callers have already checked we are not in emergency state yet and
> we want to shutdown the fs (or later flag the emergency RO state) even if
> the filesystem is mounted read only?
Yeah, I totally agree, the error handling is now completely
independent of sb_rdonly(), so we can get rid of it.
>
>>   		set_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
>>   		if (journal)
>>   			jbd2_journal_abort(journal, -EIO);
>> @@ -737,7 +737,7 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
>>   			sb->s_id);
>>   	}
>>   
>> -	if (sb_rdonly(sb) || continue_fs)
>> +	if (ext4_sb_rdonly(sb) || continue_fs)
>>   		return;
> This will need a bit of reworking with the emergency ro flag anyway so for
> now I'd leave it as is.
Yes, we can keep it here for now and then replace it with
ext4_emergency_ro() later on.
>>   
>>   	ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
>> @@ -765,7 +765,7 @@ static void update_super_work(struct work_struct *work)
>>   	 * We use directly jbd2 functions here to avoid recursing back into
>>   	 * ext4 error handling code during handling of previous errors.
>>   	 */
>> -	if (!sb_rdonly(sbi->s_sb) && journal) {
>> +	if (!ext4_sb_rdonly(sbi->s_sb) && journal) {
>>   		struct buffer_head *sbh = sbi->s_sbh;
>>   		bool call_notify_err = false;
> Again here I think we should just add ext4_emergency_state() check because
> we don't want to be modifying superblock on shutdown filesystem either. And
> in the four cases below as well.
Yeah, let me just use ext4_emergency_state() directly instead of
a new helper function in the next version.

Thanks for your review and the detailed explanation!


Regards,
Baokun

>> @@ -1325,12 +1325,12 @@ static void ext4_put_super(struct super_block *sb)
>>   	ext4_mb_release(sb);
>>   	ext4_ext_release(sb);
>>   
>> -	if (!sb_rdonly(sb) && !aborted) {
>> +	if (!ext4_sb_rdonly(sb) && !aborted) {
>>   		ext4_clear_feature_journal_needs_recovery(sb);
>>   		ext4_clear_feature_orphan_present(sb);
>>   		es->s_state = cpu_to_le16(sbi->s_mount_state);
>>   	}
>> -	if (!sb_rdonly(sb))
>> +	if (!ext4_sb_rdonly(sb))
>>   		ext4_commit_super(sb);
>>   
>>   	ext4_group_desc_free(sbi);
>> @@ -3693,7 +3693,8 @@ static int ext4_run_li_request(struct ext4_li_request *elr)
>>   		if (group >= elr->lr_next_group) {
>>   			ret = 1;
>>   			if (elr->lr_first_not_zeroed != ngroups &&
>> -			    !sb_rdonly(sb) && test_opt(sb, INIT_INODE_TABLE)) {
>> +			    !ext4_sb_rdonly(sb) &&
>> +			    test_opt(sb, INIT_INODE_TABLE)) {
>>   				elr->lr_next_group = elr->lr_first_not_zeroed;
>>   				elr->lr_mode = EXT4_LI_MODE_ITABLE;
>>   				ret = 0;
>> @@ -3998,7 +3999,7 @@ int ext4_register_li_request(struct super_block *sb,
>>   		goto out;
>>   	}
>>   
>> -	if (sb_rdonly(sb) ||
>> +	if (ext4_sb_rdonly(sb) ||
>>   	    (test_opt(sb, NO_PREFETCH_BLOCK_BITMAPS) &&
>>   	     (first_not_zeroed == ngroups || !test_opt(sb, INIT_INODE_TABLE))))
>>   		goto out;
>
> 								Honza