[PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal

[PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal

[Kai Li]

Fixes: 85e0c4e89c1b "jbd2: if the journal is aborted then don't allow update of the log tail"

If journal is dirty when mount, it will be replayed but jbd2 sb
log tail cannot be updated to mark a new start because
journal->j_flags has already been set with JBD2_ABORT first
in journal_init_common.
When a new transaction is committed, it will be recorded in block 1
first(journal->j_tail is set to 1 in journal_reset). If emergency
restart again before journal super block is updated unfortunately,
the new recorded trans will not be replayed in the next mount.
It is danerous which may lead to metadata corruption for file system.

Signed-off-by: Kai Li <li.kai4@h3c.com>
---
 fs/jbd2/journal.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 5e408ee24a1a..069b22eba795 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1710,6 +1710,11 @@ int jbd2_journal_load(journal_t *journal)
 		       journal->j_devname);
 		return -EFSCORRUPTED;
 	}
+	/*
+	 * clear JBD2_ABORT flag initialized in journal_init_common
+	 * here to update log tail information with the newest seq.
+	 */
+	journal->j_flags &= ~JBD2_ABORT;
 
 	/* OK, we've finished with the dynamic journal bits:
 	 * reinitialise the dynamic contents of the superblock in memory
@@ -1717,7 +1722,6 @@ int jbd2_journal_load(journal_t *journal)
 	if (journal_reset(journal))
 		goto recovery_error;
 
-	journal->j_flags &= ~JBD2_ABORT;
 	journal->j_flags |= JBD2_LOADED;
 	return 0;
 
-- 
2.24.0.windows.2

Re: [PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal

[Jan Kara]

Thanks for the patch! Just some small comments below:

On Sat 11-01-20 10:25:42, Kai Li wrote:
> Fixes: 85e0c4e89c1b "jbd2: if the journal is aborted then don't allow update of the log tail"

This tag should come at the bottom of the changelog (close to your
Signed-off-by).

> If journal is dirty when mount, it will be replayed but jbd2 sb
> log tail cannot be updated to mark a new start because
> journal->j_flags has already been set with JBD2_ABORT first
> in journal_init_common.
> When a new transaction is committed, it will be recorded in block 1
> first(journal->j_tail is set to 1 in journal_reset). If emergency
> restart again before journal super block is updated unfortunately,
> the new recorded trans will not be replayed in the next mount.
> It is danerous which may lead to metadata corruption for file system.

I'd slightly rephrase the text here so that it is more easily readable and
correct some grammar mistakes. Something like:

If the journal is dirty when the filesystem is mounted, jbd2 will replay
the journal but the journal superblock will not be updated by
journal_reset() because JBD2_ABORT flag is still set (it was set in
journal_init_common()). This is problematic because when a new transaction
is then committed, it will be recorded in block 1 (journal->j_tail was set
to 1 in journal_reset()). If unclean shutdown happens again before the
journal superblock is updated, the new recorded transaction will not be
replayed during the next mount (because of stale sb->s_start and
sb->s_sequence values) which can lead to filesystem corruption.

Otherwise the patch looks good to me so feel free to add:

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

(again this is added to the bottom of the changelog like the 'Fixes' tag or
'Signed-off-by' tag).

								Honza

> 
> Signed-off-by: Kai Li <li.kai4@h3c.com>
> ---
>  fs/jbd2/journal.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 5e408ee24a1a..069b22eba795 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1710,6 +1710,11 @@ int jbd2_journal_load(journal_t *journal)
>  		       journal->j_devname);
>  		return -EFSCORRUPTED;
>  	}
> +	/*
> +	 * clear JBD2_ABORT flag initialized in journal_init_common
> +	 * here to update log tail information with the newest seq.
> +	 */
> +	journal->j_flags &= ~JBD2_ABORT;
>  
>  	/* OK, we've finished with the dynamic journal bits:
>  	 * reinitialise the dynamic contents of the superblock in memory
> @@ -1717,7 +1722,6 @@ int jbd2_journal_load(journal_t *journal)
>  	if (journal_reset(journal))
>  		goto recovery_error;
>  
> -	journal->j_flags &= ~JBD2_ABORT;
>  	journal->j_flags |= JBD2_LOADED;
>  	return 0;
>  
> -- 
> 2.24.0.windows.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal

[Theodore Y. Ts'o]

On Tue, Jan 14, 2020 at 11:31:19AM +0100, Jan Kara wrote:
> Thanks for the patch! Just some small comments below:
> 
> On Sat 11-01-20 10:25:42, Kai Li wrote:
> > Fixes: 85e0c4e89c1b "jbd2: if the journal is aborted then don't allow update of the log tail"
> 
> This tag should come at the bottom of the changelog (close to your
> Signed-off-by).
> 
> > If journal is dirty when mount, it will be replayed but jbd2 sb
> > log tail cannot be updated to mark a new start because
> > journal->j_flags has already been set with JBD2_ABORT first
> > in journal_init_common.
> > When a new transaction is committed, it will be recorded in block 1
> > first(journal->j_tail is set to 1 in journal_reset). If emergency
> > restart again before journal super block is updated unfortunately,
> > the new recorded trans will not be replayed in the next mount.
> > It is danerous which may lead to metadata corruption for file system.
> 
> I'd slightly rephrase the text here so that it is more easily readable and
> correct some grammar mistakes. Something like:
> 
> If the journal is dirty when the filesystem is mounted, jbd2 will replay
> the journal but the journal superblock will not be updated by
> journal_reset() because JBD2_ABORT flag is still set (it was set in
> journal_init_common()). This is problematic because when a new transaction
> is then committed, it will be recorded in block 1 (journal->j_tail was set
> to 1 in journal_reset()). If unclean shutdown happens again before the
> journal superblock is updated, the new recorded transaction will not be
> replayed during the next mount (because of stale sb->s_start and
> sb->s_sequence values) which can lead to filesystem corruption.
> 
> Otherwise the patch looks good to me so feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> (again this is added to the bottom of the changelog like the 'Fixes' tag or
> 'Signed-off-by' tag).

Thanks, applied with a fixed up commit description.

		       	     	       - Ted

Re: [PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal

[Likai]

On 2020/1/18 5:27, Theodore Y. Ts'o wrote:
> On Tue, Jan 14, 2020 at 11:31:19AM +0100, Jan Kara wrote:
>> Thanks for the patch! Just some small comments below:
>>
>> On Sat 11-01-20 10:25:42, Kai Li wrote:
>>> Fixes: 85e0c4e89c1b "jbd2: if the journal is aborted then don't allow update of the log tail"
>> This tag should come at the bottom of the changelog (close to your
>> Signed-off-by).
>>
>>> If journal is dirty when mount, it will be replayed but jbd2 sb
>>> log tail cannot be updated to mark a new start because
>>> journal->j_flags has already been set with JBD2_ABORT first
>>> in journal_init_common.
>>> When a new transaction is committed, it will be recorded in block 1
>>> first(journal->j_tail is set to 1 in journal_reset). If emergency
>>> restart again before journal super block is updated unfortunately,
>>> the new recorded trans will not be replayed in the next mount.
>>> It is danerous which may lead to metadata corruption for file system.
>> I'd slightly rephrase the text here so that it is more easily readable and
>> correct some grammar mistakes. Something like:
>>
>> If the journal is dirty when the filesystem is mounted, jbd2 will replay
>> the journal but the journal superblock will not be updated by
>> journal_reset() because JBD2_ABORT flag is still set (it was set in
>> journal_init_common()). This is problematic because when a new transaction
>> is then committed, it will be recorded in block 1 (journal->j_tail was set
>> to 1 in journal_reset()). If unclean shutdown happens again before the
>> journal superblock is updated, the new recorded transaction will not be
>> replayed during the next mount (because of stale sb->s_start and
>> sb->s_sequence values) which can lead to filesystem corruption.
>>
>> Otherwise the patch looks good to me so feel free to add:
>>
>> Reviewed-by: Jan Kara <jack@suse.cz>
>>
>> (again this is added to the bottom of the changelog like the 'Fixes' tag or
>> 'Signed-off-by' tag).
> Thanks, applied with a fixed up commit description.
>
> 		       	     	       - Ted
>
Sorry for reply so late due to my business trip recently.  This new
comment is ok and more clear. Thanks.

                                                                       
                                                                       
      - Kai

Re: [PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal

[yebin]

Have you really encountered the problem of file system inconsistency 
caused by non-replay of logs?

(1) if (needs_recovery==0)
      jbd2_journal_wipe will call jbd2_mark_journal_empty to update 
s_start and s_sequence.

(2) if (needs_recovery == 1)
   ext4_fill_super:
......
   if (needs_recovery) {
          ext4_msg(sb, KERN_INFO, "recovery complete");
          ext4_mark_recovery_complete(sb, es);    -->Will update s_start 
and s_sequence
  }
......
   Recently, I encountered the problem of inconsistent file system data, 
which may be
caused by the log not replaying during fsck. But according to the 
description of your
patch, the s_start and the  s_sequence will all be updated.
   Therefore, I doubt whether the problem you described really exists.


On 2020/1/20 14:30, Likai wrote:
> On 2020/1/18 5:27, Theodore Y. Ts'o wrote:
>> On Tue, Jan 14, 2020 at 11:31:19AM +0100, Jan Kara wrote:
>>> Thanks for the patch! Just some small comments below:
>>>
>>> On Sat 11-01-20 10:25:42, Kai Li wrote:
>>>> Fixes: 85e0c4e89c1b "jbd2: if the journal is aborted then don't allow update of the log tail"
>>> This tag should come at the bottom of the changelog (close to your
>>> Signed-off-by).
>>>
>>>> If journal is dirty when mount, it will be replayed but jbd2 sb
>>>> log tail cannot be updated to mark a new start because
>>>> journal->j_flags has already been set with JBD2_ABORT first
>>>> in journal_init_common.
>>>> When a new transaction is committed, it will be recorded in block 1
>>>> first(journal->j_tail is set to 1 in journal_reset). If emergency
>>>> restart again before journal super block is updated unfortunately,
>>>> the new recorded trans will not be replayed in the next mount.
>>>> It is danerous which may lead to metadata corruption for file system.
>>> I'd slightly rephrase the text here so that it is more easily readable and
>>> correct some grammar mistakes. Something like:
>>>
>>> If the journal is dirty when the filesystem is mounted, jbd2 will replay
>>> the journal but the journal superblock will not be updated by
>>> journal_reset() because JBD2_ABORT flag is still set (it was set in
>>> journal_init_common()). This is problematic because when a new transaction
>>> is then committed, it will be recorded in block 1 (journal->j_tail was set
>>> to 1 in journal_reset()). If unclean shutdown happens again before the
>>> journal superblock is updated, the new recorded transaction will not be
>>> replayed during the next mount (because of stale sb->s_start and
>>> sb->s_sequence values) which can lead to filesystem corruption.
>>>
>>> Otherwise the patch looks good to me so feel free to add:
>>>
>>> Reviewed-by: Jan Kara <jack@suse.cz>
>>>
>>> (again this is added to the bottom of the changelog like the 'Fixes' tag or
>>> 'Signed-off-by' tag).
>> Thanks, applied with a fixed up commit description.
>>
>> 		       	     	       - Ted
>>
> Sorry for reply so late due to my business trip recently.  This new
> comment is ok and more clear. Thanks.
>
>                                                                         
>                                                                         
>        - Kai
>
> .
>