Message-ID: <ad80d4a4-6bc7-47ab-bca1-569d77c868a1@huawei.com>
Date: Sat, 4 Jan 2025 11:15:33 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Jan Kara <jack@...e.cz>
CC: "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>, Theodore Ts'o
	<tytso@....edu>, Christian Brauner <brauner@...nel.org>,
	<sunyongjian1@...wei.com>, Yang Erkun <yangerkun@...wei.com>,
	<linux-fsdevel@...r.kernel.org>, <linux-xfs@...r.kernel.org>, Baokun Li
	<libaokun1@...wei.com>
Subject: Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?

On 2025/1/3 22:34, Jan Kara wrote:
> On Fri 03-01-25 21:19:27, Baokun Li wrote:
>> On 2025/1/3 18:42, Jan Kara wrote:
>>>>>> 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.
>>>>> Here I don't understand what is really the problem with EXT4_MF_FS_ABORTED
>>>>> removal. What do you exactly mean by "causes the file system to not be
>>>>> read-only"? We still return EROFS where we used to, we disallow writing as
>>>>> we used to. Can you perhaps give an example what changed with this commit?
>>>> Sorry for the lack of clarity in my previous explanation. The key point
>>>> is not about removing EXT4_MF_FS_ABORTED, but rather we will set
>>>> EXT4_FLAGS_SHUTDOWN bit, which not only prevents writes but also prevents
>>>> reads. Therefore, saying it's not read-only actually means it's completely
>>>> unreadable.
>>> Ah, I see. I didn't think about that. Is it that you really want reading to
>>> work from a filesystem after error? Can you share why (I'm just trying to
>>> understand the usecase)? Or is this mostly a theoretical usecase?
>> Switching to read-only mode after an error is a common practice for most
>> file systems (ext4/btrfs/affs/fat/jfs/nilfs/nilfs2/ocfs2/ubifs/ufs, etc.).
>> There are two main benefits to doing this:
>>   * Read-only processes can continue to run unaffected after the error.
>>   * Shutting down after an error would lose some data in memory that has
>>     not been written to disk. If the file system is read-only, we can back
>>     up these data to another location in time and then exit gracefully.
>>> I think we could introduce "shutdown modifications" state which would still
>>> allow pure reads to succeed if there's a usecase for such functionality.
>> I agree that maintaining a flag like EXT4_FLAGS_RDONLY within ext4 seems
>> to be a good solution at this point. It avoids both introducing mechanism
>> changes and VFS coupling. If no one has a better idea, I will implement it.
> Yeah, let's go with a separate "emergency RO" ext4 flag then. I think we
> could just enhance the ext4_forced_shutdown() checks to take a flag whether
> the operation is a modification or not and when it is a modification, it
> would additionally trigger also when EMERGENCY_RO flag is set (which would
> get set by ext4_handle_error()).
>
> Thanks for having a look into this.
>
> 								Honza
Sounds very nice, this way the changes will be minimal and the code won't
look messy.Thank you for your suggestion!

Cheers,
Baokun
>>>>> So how does your framework detect that the filesystem has failed with
>>>>> errors=remount-ro? By parsing /proc/mounts or otherwise querying current
>>>>> filesystem mount options?
>>>> In most cases, run the mount command and filter related options.
>>>>> Would it be acceptable for you to look at some
>>>>> other mount option (e.g. "shutdown") to detect that state? We could easily
>>>>> implement that.
>>>> We do need to add a shutdown hint, but that's not the point.
>>>>
>>>> We've discussed this internally, and now if the logs are flushed,
>>>> we have no way of knowing if the current filesystem is shutdown. We don't
>>>> know if the -EIO from the filesystem is a hardware problem or if the
>>>> filesystem is already shutdown. So even if there is no current problem,
>>>> we should add some kind of hint to let the user know that the current
>>>> filesystem is shutdown.
>>>>
>>>> The changes to display shutdown are as follows, so that we can see if the
>>>> current filesystem has been shutdown in the mount command.
>>> Yes, I think this would be a good addition regardless of other changes we
>>> might need to do. It would be preferable to be able to come up with
>>> something that's acceptable for querying of shutdown state also for other
>>> filesystems - I've CCed fsdevel and XFS in particular since it has much
>>> longer history of fs shutdown implementation.
>>>
>>> 								Honza
>>>
>>>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>>>> index 3955bec9245d..ba28ef0f662e 100644
>>>> --- a/fs/ext4/super.c
>>>> +++ b/fs/ext4/super.c
>>>> @@ -3157,6 +3157,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 (!nodefs && ext4_forced_shutdown(sb))
>>>> +               SEQ_OPTS_PUTS("shutdown");
>>>> +
>>>>           ext4_show_quota_options(seq, sb);
>>>>           return 0;
>>>>    }
>>>>> I'm sorry again for causing you trouble.
>>>> Never mind, thank you for your reply!
>>>>

Powered by blists - more mailing lists