Re: [PATCH v3 1/2] ext4: commit super block if fs record error when journal record without error

[Jan Kara]

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