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);
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.
> 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?
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
