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
> > > > 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!
> > >
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
