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

[Baokun Li <libaokun1@huawei.com>]

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