On 2023/2/17 18:56, Jan Kara wrote:
On Fri 17-02-23 09:44:57, yebin (H) wrote:
On 2023/2/17 1:31, Jan Kara wrote:
On Tue 14-02-23 10:29:04, Ye Bin wrote:
From: Ye Bin <yebin10@xxxxxxxxxx>
Now, 'es->s_state' maybe covered by recover journal. And journal errno
maybe not recorded in journal sb as IO error. ext4_update_super() only
update error information when 'sbi->s_add_error_count' large than zero.
Then 'EXT4_ERROR_FS' flag maybe lost.
To solve above issue commit error information after recover journal.
Signed-off-by: Ye Bin <yebin10@xxxxxxxxxx>
---
fs/ext4/super.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index dc3907dff13a..b94754ba8556 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5932,6 +5932,18 @@ static int ext4_load_journal(struct super_block *sb,
goto err_out;
}
+ if (unlikely(es->s_error_count && !jbd2_journal_errno(journal) &&
+ !(le16_to_cpu(es->s_state) & EXT4_ERROR_FS))) {
+ EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
+ es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
+ err = ext4_commit_super(sb);
+ if (err) {
+ ext4_msg(sb, KERN_ERR,
+ "Failed to commit error information, please repair fs force!");
+ goto err_out;
+ }
+ }
+
Hum, I'm not sure I follow here. If journal replay has overwritten the
superblock (and thus the stored error info), then I'd expect
es->s_error_count got overwritten (possibly to 0) as well. And this is
actually relatively realistic scenario with errors=remount-ro behavior when
the first fs error happens.
What I intended in my original suggestion was to save es->s_error_count,
es->s_state & EXT4_ERROR_FS, es->s_first_error_*, es->s_last_error_* before
doing journal replay in ext4_load_journal() and then after journal replay
merge this info back to the superblock
Actually，commit 1c13d5c08728 ("ext4: Save error information to the
superblock for analysis")
already merged error info back to the superblock after journal replay except
'es->s_state'.
The problem I have now is that the error flag in the journal superblock was
not recorded,
but the error message was recorded in the superblock. So it leads to
ext4_clear_journal_err()
does not detect errors and marks the file system as an error. Because
ext4_update_super() is
only set error flag when 'sbi->s_add_error_count > 0'. Although
'sbi->s_mount_state' is
written to the super block when umount, but it is also conditional.
So I handle the scenario "es->s_error_count && !jbd2_journal_errno(journal)
&&
!(le16_to_cpu(es->s_state) & EXT4_ERROR_FS)". Maybe we can just store
'EXT4_SB(sb)->s_mount_state & EXT4_ERROR_FS' back to the superblock. But i
prefer to mark fs as error if it contain detail error info without
EXT4_ERROR_FS.
Aha, thanks for explanation! So now I finally understand what the problem
exactly is. I'm not sure relying on es->s_error_count is too good. Probably
it works but I'd be calmer if when saving error info we also did:
bool error_fs = es->s_state & cpu_to_le16(EXT4_ERROR_FS);
copy other info
err = jbd2_journal_load(journal);
restore other info
if (error_fs)
es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
/* Write out restored error information to the superblock */
err2 = ext4_commit_super(sb);
and be done with this. I don't think trying to optimize away the committing
of the superblock when we had to replay the journal is really worth it.
Does this solve your concerns?
Honza
Thanks for your suggestion.
I think if journal super block has 'j_errno' ext4_clear_journal_err()
will commit error info.
The scenario we need to deal with is:(1) journal super block has no
'j_errno'; (2) super
block has detail error info, but 'es->s_state' has no 'EXT4_ERROR_FS',
It means super
block in journal has no error flag and the newest super block has error
flag. so we
need to store error flag to 'es->s_state', and commit it to disk.If
'es->s_state' has
'EXT4_ERROR_FS', it means super block in journal has error flag, so we
do not need
to store error flag in super block.
I don't know if I can explain my idea of repair.
