On 2023/2/27 19:20, Jan Kara wrote:
On Sat 18-02-23 10:18:42, yebin (H) wrote:
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?
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.
But my code above should be handling this situation you describe - the
check:
error_fs = es->s_state & cpu_to_le16(EXT4_ERROR_FS);
Here, we do not need to backup 'error_fs', as
'EXT4_SB(sb)->s_mount_state' already
record this flag when fs has error flag before do journal replay.
will check the newest state in the superblock before journal replay. Then
we replay the journal - es->s_state may loose the EXT4_ERROR_FS flag if the
superblock version in the journal didn't have it. So we do:
if (error_fs)
es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
which makes sure EXT4_ERROR_FS is set either if it was set in the journal
or in the newest superblock version before journal replay.
My modification is to deal with the situation we missed, and I don't
want to introduce
an invalid super block submission.
If you think my judgment is too obscure, I can send another version
according to your
suggestion.
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.
Why do you think that if es->s_state has EXT4_ERROR_FS, the super block in
the journal has that flag? During mount, we load the superblock directly
from the first block in the filesystem and until we call
jbd2_journal_load(), es->s_state contains this newest value of the
superblock state. What am I missing?
After jbd2_journal_load() 'es->s_state' is already covered by the super
block in journal.
If there is super block in journal and 'es->s_state' has error flag this
means super block
in journal has error flag. If there is no super block in journal and
'es->s_state' has error
flag, this means super block already has error flag.In both cases we can
do nothing.
Honza
