[PATCH v3 0/2] fix error flag covered by journal recovery

[PATCH v3 0/2] fix error flag covered by journal recovery

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Diff v3 Vs v2:
Only fix fs error flag lost when previous journal errno is not record
in disk. As this may lead to drop orphan list, however fs not record
error flag, then fsck will not repair deeply.

Diff v2 vs v1:
Move call 'j_replay_prepare_callback' and 'j_replay_end_callback' from
ext4_load_journal() to jbd2_journal_recover().

When do fault injection test, got issue as follows:
EXT4-fs (dm-5): warning: mounting fs with errors, running e2fsck is recommended
EXT4-fs (dm-5): Errors on filesystem, clearing orphan list.
EXT4-fs (dm-5): recovery complete
EXT4-fs (dm-5): mounted filesystem with ordered data mode. Opts: data_err=abort,errors=remount-ro

EXT4-fs (dm-5): recovery complete
EXT4-fs (dm-5): mounted filesystem with ordered data mode. Opts: data_err=abort,errors=remount-ro

Without do file system check, file system is clean when do second mount.
Theoretically, the kernel will not clear fs error flag. In errors=remount-ro
mode the last super block is commit directly. So super block in journal is
not uptodate. When do jounral recovery, the uptodate super block will be
covered by jounral data. If super block submit all failed after recover
journal, then file system error flag is lost. When do "fsck -a" couldn't
repair file system deeply.
To solve above issue we need to do extra handle when do super block journal
recovery.


Ye Bin (2):
  ext4: commit super block if fs record error when journal record
    without error
  ext4: make sure fs error flag setted before clear journal error

 fs/ext4/super.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

-- 
2.31.1

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

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

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@huawei.com>
---
 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;
+		}
+	}
+
 	EXT4_SB(sb)->s_journal = journal;
 	err = ext4_clear_journal_err(sb, es);
 	if (err) {
-- 
2.31.1

[PATCH v3 2/2] ext4: make sure fs error flag setted before clear journal error

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Now, jounral error number maybe cleared even though ext4_commit_super()
failed. This may lead to error flag miss, then fsck will miss to check
file system deeply.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/super.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index b94754ba8556..619ef6d021d4 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6163,11 +6163,13 @@ static int ext4_clear_journal_err(struct super_block *sb,
 		errstr = ext4_decode_error(sb, j_errno, nbuf);
 		ext4_warning(sb, "Filesystem error recorded "
 			     "from previous mount: %s", errstr);
-		ext4_warning(sb, "Marking fs in need of filesystem check.");
 
 		EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
 		es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
-		ext4_commit_super(sb);
+		j_errno = ext4_commit_super(sb);
+		if (j_errno)
+			return j_errno;
+		ext4_warning(sb, "Marked fs in need of filesystem check.");
 
 		jbd2_journal_clear_err(journal);
 		jbd2_journal_update_sb_errno(journal);
-- 
2.31.1

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

[Baokun Li]

On 2023/2/14 10:29, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> 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@huawei.com>
> ---
>   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;
> +		}
> +	}
> +
>   	EXT4_SB(sb)->s_journal = journal;
>   	err = ext4_clear_journal_err(sb, es);
>   	if (err) {
I think we don't need such a complicated judgment, after the journal 
replay and saving the error info,
if there is EXT4_ERROR_FS flag in ext4_sb_info->s_mount_state, just add 
this flag directly to es->s_state.
This way the EXT4_ERROR_FS flag and the error message will be written to 
disk the next time
ext4_commit_super() is executed. The code change is as follows:

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 260c1b3e3ef2..341b11c589b3 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5935,6 +5935,7 @@ static int ext4_load_journal(struct super_block *sb,
                         memcpy(((char *) es) + EXT4_S_ERR_START,
                                save, EXT4_S_ERR_LEN);
                 kfree(save);
+               es->s_state |= cpu_to_le16(EXT4_SB(sb)->s_mount_state & 
EXT4_ERROR_FS);
         }

         if (err) {

-- 
With Best Regards,
Baokun Li
.

Re: [PATCH v3 2/2] ext4: make sure fs error flag setted before clear journal error

[Baokun Li]

On 2023/2/14 10:29, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> Now, jounral error number maybe cleared even though ext4_commit_super()
> failed. This may lead to error flag miss, then fsck will miss to check
> file system deeply.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
Looks good to me.

Reviewed-by: Baokun Li <libaokun1@huawei.com>
> ---
>   fs/ext4/super.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b94754ba8556..619ef6d021d4 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -6163,11 +6163,13 @@ static int ext4_clear_journal_err(struct super_block *sb,
>   		errstr = ext4_decode_error(sb, j_errno, nbuf);
>   		ext4_warning(sb, "Filesystem error recorded "
>   			     "from previous mount: %s", errstr);
> -		ext4_warning(sb, "Marking fs in need of filesystem check.");
>   
>   		EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
>   		es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
> -		ext4_commit_super(sb);
> +		j_errno = ext4_commit_super(sb);
> +		if (j_errno)
> +			return j_errno;
> +		ext4_warning(sb, "Marked fs in need of filesystem check.");
>   
>   		jbd2_journal_clear_err(journal);
>   		jbd2_journal_update_sb_errno(journal);
-- 
With Best Regards,
Baokun Li
.

Re: [PATCH v3 0/2] fix error flag covered by journal recovery

[Baokun Li]

On 2023/2/14 10:29, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> Diff v3 Vs v2:
> Only fix fs error flag lost when previous journal errno is not record
> in disk. As this may lead to drop orphan list, however fs not record
> error flag, then fsck will not repair deeply.
>
> Diff v2 vs v1:
> Move call 'j_replay_prepare_callback' and 'j_replay_end_callback' from
> ext4_load_journal() to jbd2_journal_recover().
>
> When do fault injection test, got issue as follows:
> EXT4-fs (dm-5): warning: mounting fs with errors, running e2fsck is recommended
> EXT4-fs (dm-5): Errors on filesystem, clearing orphan list.
> EXT4-fs (dm-5): recovery complete
> EXT4-fs (dm-5): mounted filesystem with ordered data mode. Opts: data_err=abort,errors=remount-ro
>
> EXT4-fs (dm-5): recovery complete
> EXT4-fs (dm-5): mounted filesystem with ordered data mode. Opts: data_err=abort,errors=remount-ro
>
> Without do file system check, file system is clean when do second mount.
> Theoretically, the kernel will not clear fs error flag. In errors=remount-ro
> mode the last super block is commit directly. So super block in journal is
> not uptodate. When do jounral recovery, the uptodate super block will be
> covered by jounral data. If super block submit all failed after recover
> journal, then file system error flag is lost. When do "fsck -a" couldn't
> repair file system deeply.
> To solve above issue we need to do extra handle when do super block journal
> recovery.
>
>
> Ye Bin (2):
>    ext4: commit super block if fs record error when journal record
>      without error
>    ext4: make sure fs error flag setted before clear journal error
>
>   fs/ext4/super.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
When we proceed in the flow of ( uninstall after injecting fault 
triggered error -> mount
kernel replay journal -> umount to view fsck info ), there are three cases:

1. When an injection fault causes the ERROR_FS flag to not be saved to 
disk, but j_errno
is successfully saved to disk, PATCH 2/2 effectively ensures that 
ERROR_FS is saved to disk
so that fsck performs a force check to discover the error correctly.

2. When j_errno is lost and the ERROR_FS flag is saved, after the 
journal replay：
     a. The ext4_super_block on disk has neither error info nor ERROR_FS 
flag;
     b. The ext4_super_block in memory contains error info but no 
ERROR_FS flag
         because the error info is copied additionally during journal 
replay;
     c. The ext4_sb_info in memory contains both error info and ERROR_FS 
flag.
This means that the ext4_super_block in memory will be written to disk 
the next time
ext4_commit_super is executed, while the ERROR_FS flag in ext4_sb_info 
will not be written
to disk until ext4_put_super is called. So if there is a disk 
deletion/power failure/disk offline,
we will lose the ERROR_FS flag or even the error info.

(In this case, repairing directly with e2fsck will not do a force check 
either, because it
relies on j_errno to recover the ERROR_FS flag after the journal replay. 
And it reloads
the information from the disk into memory after the journal replay, so the
ERROR_FS flag and error info are completely lost.)

3. If neither the ERROR_FS flag nor j_errno are saved to disk, we seem 
to be unable to
determine if a deep sweep is currently needed. But I think when journal 
replay is needed
it means that the file system exits abnormally,
*Is it possible to consider e2fsck to do a force check after the journal 
replay?*

-- 
With Best Regards,
Baokun Li
.

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

[yebin (H)]

On 2023/2/16 15:17, Baokun Li wrote:
> On 2023/2/14 10:29, Ye Bin wrote:
>> From: Ye Bin <yebin10@huawei.com>
>>
>> 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@huawei.com>
>> ---
>>   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;
>> +        }
>> +    }
>> +
>>       EXT4_SB(sb)->s_journal = journal;
>>       err = ext4_clear_journal_err(sb, es);
>>       if (err) {
> I think we don't need such a complicated judgment, after the journal 
> replay and saving the error info,
> if there is EXT4_ERROR_FS flag in ext4_sb_info->s_mount_state, just 
> add this flag directly to es->s_state.
> This way the EXT4_ERROR_FS flag and the error message will be written 
> to disk the next time

Thanks for your suggestion. There are two reasons for this:
1. We want to write the error mark to the disk as soon as possible.
2. Here we deal with the case where there is no error mark bit but there 
is an error record.
In this case, the file system should be marked with an error and the 
user should be prompted.
> ext4_commit_super() is executed. The code change is as follows:
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 260c1b3e3ef2..341b11c589b3 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5935,6 +5935,7 @@ static int ext4_load_journal(struct super_block 
> *sb,
>                         memcpy(((char *) es) + EXT4_S_ERR_START,
>                                save, EXT4_S_ERR_LEN);
>                 kfree(save);
> +               es->s_state |= cpu_to_le16(EXT4_SB(sb)->s_mount_state 
> & EXT4_ERROR_FS);
>         }
>
>         if (err) {
>

Re: [PATCH v3 0/2] fix error flag covered by journal recovery

[yebin (H)]

On 2023/2/16 15:18, Baokun Li wrote:
> On 2023/2/14 10:29, Ye Bin wrote:
>> From: Ye Bin <yebin10@huawei.com>
>>
>> Diff v3 Vs v2:
>> Only fix fs error flag lost when previous journal errno is not record
>> in disk. As this may lead to drop orphan list, however fs not record
>> error flag, then fsck will not repair deeply.
>>
>> Diff v2 vs v1:
>> Move call 'j_replay_prepare_callback' and 'j_replay_end_callback' from
>> ext4_load_journal() to jbd2_journal_recover().
>>
>> When do fault injection test, got issue as follows:
>> EXT4-fs (dm-5): warning: mounting fs with errors, running e2fsck is 
>> recommended
>> EXT4-fs (dm-5): Errors on filesystem, clearing orphan list.
>> EXT4-fs (dm-5): recovery complete
>> EXT4-fs (dm-5): mounted filesystem with ordered data mode. Opts: 
>> data_err=abort,errors=remount-ro
>>
>> EXT4-fs (dm-5): recovery complete
>> EXT4-fs (dm-5): mounted filesystem with ordered data mode. Opts: 
>> data_err=abort,errors=remount-ro
>>
>> Without do file system check, file system is clean when do second mount.
>> Theoretically, the kernel will not clear fs error flag. In 
>> errors=remount-ro
>> mode the last super block is commit directly. So super block in 
>> journal is
>> not uptodate. When do jounral recovery, the uptodate super block will be
>> covered by jounral data. If super block submit all failed after recover
>> journal, then file system error flag is lost. When do "fsck -a" couldn't
>> repair file system deeply.
>> To solve above issue we need to do extra handle when do super block 
>> journal
>> recovery.
>>
>>
>> Ye Bin (2):
>>    ext4: commit super block if fs record error when journal record
>>      without error
>>    ext4: make sure fs error flag setted before clear journal error
>>
>>   fs/ext4/super.c | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
> When we proceed in the flow of ( uninstall after injecting fault 
> triggered error -> mount
> kernel replay journal -> umount to view fsck info ), there are three 
> cases:
>
> 1. When an injection fault causes the ERROR_FS flag to not be saved to 
> disk, but j_errno
> is successfully saved to disk, PATCH 2/2 effectively ensures that 
> ERROR_FS is saved to disk
> so that fsck performs a force check to discover the error correctly.
>
> 2. When j_errno is lost and the ERROR_FS flag is saved, after the 
> journal replay：
>     a. The ext4_super_block on disk has neither error info nor 
> ERROR_FS flag;
>     b. The ext4_super_block in memory contains error info but no 
> ERROR_FS flag
>         because the error info is copied additionally during journal 
> replay;
>     c. The ext4_sb_info in memory contains both error info and 
> ERROR_FS flag.
> This means that the ext4_super_block in memory will be written to disk 
> the next time
> ext4_commit_super is executed, while the ERROR_FS flag in ext4_sb_info 
> will not be written
> to disk until ext4_put_super is called. So if there is a disk 
> deletion/power failure/disk offline,
> we will lose the ERROR_FS flag or even the error info.
>
> (In this case, repairing directly with e2fsck will not do a force 
> check either, because it
> relies on j_errno to recover the ERROR_FS flag after the journal 
> replay. And it reloads
> the information from the disk into memory after the journal replay, so 
> the
> ERROR_FS flag and error info are completely lost.)
>
> 3. If neither the ERROR_FS flag nor j_errno are saved to disk, we seem 
> to be unable to
> determine if a deep sweep is currently needed. But I think when 
> journal replay is needed
> it means that the file system exits abnormally,
> *Is it possible to consider e2fsck to do a force check after the 
> journal replay?*
Perhaps e2fsck can provide a command parameter, because it is 
unacceptable to
do so in scenarios with requirements for startup time.

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

[Baokun Li]

On 2023/2/16 15:44, yebin (H) wrote:
>
>
> On 2023/2/16 15:17, Baokun Li wrote:
>> On 2023/2/14 10:29, Ye Bin wrote:
>>> From: Ye Bin <yebin10@huawei.com>
>>>
>>> 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@huawei.com>
>>> ---
>>>   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;
>>> +        }
>>> +    }
>>> +
>>>       EXT4_SB(sb)->s_journal = journal;
>>>       err = ext4_clear_journal_err(sb, es);
>>>       if (err) {
>> I think we don't need such a complicated judgment, after the journal 
>> replay and saving the error info,
>> if there is EXT4_ERROR_FS flag in ext4_sb_info->s_mount_state, just 
>> add this flag directly to es->s_state.
>> This way the EXT4_ERROR_FS flag and the error message will be written 
>> to disk the next time
>
> Thanks for your suggestion. There are two reasons for this:
> 1. We want to write the error mark to the disk as soon as possible.
> 2. Here we deal with the case where there is no error mark bit but 
> there is an error record.
> In this case, the file system should be marked with an error and the 
> user should be prompted.
The EXT4_ERROR_FS flag is always written to disk at the same time as the 
error info,
except when the journal is replayed. During journal replay the error 
info is additionally
copied to disk first, and the EXT4_ERROR_FS flag in the sbi is not 
written to disk until
the ext4_put_super() is called. It is only when a failure occurs during 
this time that
there is an error info but no EXT4_ERROR_FS flag. So we just need to 
make sure that
the EXT4_ERROR_FS flag is also written to disk at the same time as the 
error info
after the journal replay.
>> ext4_commit_super() is executed. The code change is as follows:
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 260c1b3e3ef2..341b11c589b3 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -5935,6 +5935,7 @@ static int ext4_load_journal(struct super_block 
>> *sb,
>>                         memcpy(((char *) es) + EXT4_S_ERR_START,
>>                                save, EXT4_S_ERR_LEN);
>>                 kfree(save);
>> +               es->s_state |= cpu_to_le16(EXT4_SB(sb)->s_mount_state 
>> & EXT4_ERROR_FS);
>>         }
>>
>>         if (err) {
>>
>
>
>
-- 
With Best Regards,
Baokun Li
.

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

[yebin (H)]

On 2023/2/16 17:17, Baokun Li wrote:
> On 2023/2/16 15:44, yebin (H) wrote:
>>
>>
>> On 2023/2/16 15:17, Baokun Li wrote:
>>> On 2023/2/14 10:29, Ye Bin wrote:
>>>> From: Ye Bin <yebin10@huawei.com>
>>>>
>>>> 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@huawei.com>
>>>> ---
>>>>   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;
>>>> +        }
>>>> +    }
>>>> +
>>>>       EXT4_SB(sb)->s_journal = journal;
>>>>       err = ext4_clear_journal_err(sb, es);
>>>>       if (err) {
>>> I think we don't need such a complicated judgment, after the journal 
>>> replay and saving the error info,
>>> if there is EXT4_ERROR_FS flag in ext4_sb_info->s_mount_state, just 
>>> add this flag directly to es->s_state.
>>> This way the EXT4_ERROR_FS flag and the error message will be 
>>> written to disk the next time
>>
>> Thanks for your suggestion. There are two reasons for this:
>> 1. We want to write the error mark to the disk as soon as possible.
>> 2. Here we deal with the case where there is no error mark bit but 
>> there is an error record.
>> In this case, the file system should be marked with an error and the 
>> user should be prompted.
> The EXT4_ERROR_FS flag is always written to disk at the same time as 
> the error info,
> except when the journal is replayed. During journal replay the error 
> info is additionally
> copied to disk first, and the EXT4_ERROR_FS flag in the sbi is not 
> written to disk until
> the ext4_put_super() is called. It is only when a failure occurs 
> during this time that
> there is an error info but no EXT4_ERROR_FS flag. So we just need to 
> make sure that
> the EXT4_ERROR_FS flag is also written to disk at the same time as the 
> error info
> after the journal replay.
The situation you said is based on the situation after the repair. What 
about the existing
image with such inconsistency?
>>> ext4_commit_super() is executed. The code change is as follows:
>>>
>>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>>> index 260c1b3e3ef2..341b11c589b3 100644
>>> --- a/fs/ext4/super.c
>>> +++ b/fs/ext4/super.c
>>> @@ -5935,6 +5935,7 @@ static int ext4_load_journal(struct 
>>> super_block *sb,
>>>                         memcpy(((char *) es) + EXT4_S_ERR_START,
>>>                                save, EXT4_S_ERR_LEN);
>>>                 kfree(save);
>>> +               es->s_state |= 
>>> cpu_to_le16(EXT4_SB(sb)->s_mount_state & EXT4_ERROR_FS);
>>>         }
>>>
>>>         if (err) {
>>>
>>
>>
>>

Re: [PATCH v3 2/2] ext4: make sure fs error flag setted before clear journal error

[Jan Kara]

On Tue 14-02-23 10:29:05, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> Now, jounral error number maybe cleared even though ext4_commit_super()
> failed. This may lead to error flag miss, then fsck will miss to check
> file system deeply.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/super.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b94754ba8556..619ef6d021d4 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -6163,11 +6163,13 @@ static int ext4_clear_journal_err(struct super_block *sb,
>  		errstr = ext4_decode_error(sb, j_errno, nbuf);
>  		ext4_warning(sb, "Filesystem error recorded "
>  			     "from previous mount: %s", errstr);
> -		ext4_warning(sb, "Marking fs in need of filesystem check.");
>  
>  		EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
>  		es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
> -		ext4_commit_super(sb);
> +		j_errno = ext4_commit_super(sb);
> +		if (j_errno)
> +			return j_errno;
> +		ext4_warning(sb, "Marked fs in need of filesystem check.");
>  
>  		jbd2_journal_clear_err(journal);
>  		jbd2_journal_update_sb_errno(journal);
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Tue 14-02-23 10:29:04, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> 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@huawei.com>
> ---
>  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 - if EXT4_ERROR_FS was set, set it
now as well, take max of old and new s_error_count, set s_first_error_* if
it is now unset, set s_last_error_* if stored timestamp is newer than
current timestamp.

Or am I overengineering it now? :)

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Baokun Li]

On 2023/2/17 1:31, Jan Kara wrote:
> On Tue 14-02-23 10:29:04, Ye Bin wrote:
>> From: Ye Bin <yebin10@huawei.com>
>>
>> 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@huawei.com>
>> ---
>>   fs/ext4/super.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
> 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 - if EXT4_ERROR_FS was set, set it
> now as well, take max of old and new s_error_count, set s_first_error_* if
> it is now unset, set s_last_error_* if stored timestamp is newer than
> current timestamp.
>
> Or am I overengineering it now? :)
>
> 								Honza
This is exactly how the code is designed now！
The code has now saved all the above information except EXT4_ERROR_FS by
the following two pieces of logic, as follows：

---------------- In struct ext4_super_block ----------------
1412 #define EXT4_S_ERR_START offsetof(struct ext4_super_block, 
s_error_count)
1413         __le32  s_error_count;          /* number of fs errors */
1414         __le32  s_first_error_time;     /* first time an error 
happened */
1415         __le32  s_first_error_ino;      /* inode involved in first 
error */
1416         __le64  s_first_error_block;    /* block involved of first 
error */
1417         __u8    s_first_error_func[32] __nonstring;     /* function 
where the error happened */
1418         __le32  s_first_error_line;     /* line number where error 
happened */
1419         __le32  s_last_error_time;      /* most recent time of an 
error */
1420         __le32  s_last_error_ino;       /* inode involved in last 
error */
1421         __le32  s_last_error_line;      /* line number where error 
happened */
1422         __le64  s_last_error_block;     /* block involved of last 
error */
1423         __u8    s_last_error_func[32] __nonstring;      /* function 
where the error happened */
1424 #define EXT4_S_ERR_END offsetof(struct ext4_super_block, s_mount_opts)
-----------------------------------------------------------

---------------- In ext4_load_journal() ----------------
5929                 char *save = kmalloc(EXT4_S_ERR_LEN, GFP_KERNEL);
5930                 if (save)
5931                         memcpy(save, ((char *) es) +
5932                                EXT4_S_ERR_START, EXT4_S_ERR_LEN);
5933                 err = jbd2_journal_load(journal);
5934                 if (save)
5935                         memcpy(((char *) es) + EXT4_S_ERR_START,
5936                                save, EXT4_S_ERR_LEN);
5937                 kfree(save);
--------------------------------------------------------

As you said, we should also save EXT4_ERROR_FS to es->s_state.
But we are not saving this now, so I think we just need to add

  `es->s_state |= cpu_to_le16(EXT4_SB(sb)->s_mount_state & EXT4_ERROR_FS);`

to save the possible EXT4_ERROR_FS flag after copying the error area 
data to es.


-- 
With Best Regards,
Baokun Li
.

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

[yebin (H)]

On 2023/2/17 1:31, Jan Kara wrote:
> On Tue 14-02-23 10:29:04, Ye Bin wrote:
>> From: Ye Bin <yebin10@huawei.com>
>>
>> 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@huawei.com>
>> ---
>>   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.
> - if EXT4_ERROR_FS was set, set it
> now as well, take max of old and new s_error_count, set s_first_error_* if
> it is now unset, set s_last_error_* if stored timestamp is newer than
> current timestamp.
>
> Or am I overengineering it now? :)
>
> 								Honza

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

[Jan Kara]

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@huawei.com>
> > > 
> > > 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@huawei.com>
> > > ---
> > >   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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[yebin (H)]

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@huawei.com>
>>>>
>>>> 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@huawei.com>
>>>> ---
>>>>    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.

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@huawei.com>
> > > > > 
> > > > > 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@huawei.com>
> > > > > ---
> > > > >    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@suse.com>
SUSE Labs, CR

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

[yebin (H)]

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@huawei.com>
>>>>>>
>>>>>> 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@huawei.com>
>>>>>> ---
>>>>>>     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

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

[Jan Kara]

On Tue 28-02-23 10:24:26, yebin (H) wrote:
> 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@huawei.com>
> > > > > > > 
> > > > > > > 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@huawei.com>
> > > > > > > ---
> > > > > > >     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.

Yeah, right. We don't need error_fs variable and can just look at
EXT4_SB(sb)->s_mount_state.

> > 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 is the extra superblock write your only concern? Honestly, I prefer code
simplicity over saved one superblock write in case we had to replay the
journal (which should be rare anyway). If you look at the code, we can
writeout superblock several times in that mount path anyway.

> > > 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.

If what you wanted to say: "It is not necessary to write the superblock if
EXT4_ERROR_FS is already set." I tend to agree although not 100% because
journal replay could result in rewinding 's_last_error_*' fields and so
writing superblock would still make sense even if EXT4_ERROR_FS is set in
es->s_state after journal reply. That's why I wouldn't complicate things
and just always write the superblock after journal replay.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR