On 2025/1/21 21:11, Jan Kara wrote:
On Fri 17-01-25 16:23:12, libaokun@xxxxxxxxxxxxxxx wrote:
From: Baokun Li <libaokun1@xxxxxxxxxx>
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@xxxxxxxxxx>
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
