Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal
@ 2020-01-11  2:25 Kai Li
  2020-01-14 10:31 ` Jan Kara
  0 siblings, 1 reply; 4+ messages in thread
From: Kai Li @ 2020-01-11  2:25 UTC (permalink / raw)
  To: jack
  Cc: tytso, linux-ext4, linux-kernel, joseph.qi, gechangwei,
	wang.yongd, wang.xibo, 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal
  2020-01-11  2:25 [PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal Kai Li
@ 2020-01-14 10:31 ` Jan Kara
  2020-01-17 21:26   ` Theodore Y. Ts'o
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Kara @ 2020-01-14 10:31 UTC (permalink / raw)
  To: Kai Li
  Cc: jack, tytso, linux-ext4, linux-kernel, joseph.qi, gechangwei,
	wang.yongd, wang.xibo

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal
  2020-01-14 10:31 ` Jan Kara
@ 2020-01-17 21:26   ` Theodore Y. Ts'o
  2020-01-20  6:30     ` Likai
  0 siblings, 1 reply; 4+ messages in thread
From: Theodore Y. Ts'o @ 2020-01-17 21:26 UTC (permalink / raw)
  To: Jan Kara
  Cc: Kai Li, linux-ext4, linux-kernel, joseph.qi, gechangwei,
	wang.yongd, wang.xibo

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal
  2020-01-17 21:26   ` Theodore Y. Ts'o
@ 2020-01-20  6:30     ` Likai
  0 siblings, 0 replies; 4+ messages in thread
From: Likai @ 2020-01-20  6:30 UTC (permalink / raw)
  To: Theodore Y. Ts'o, Jan Kara
  Cc: linux-ext4, linux-kernel, joseph.qi, gechangwei, Wangyong, Wangxibo

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-11  2:25 [PATCH] jbd2: clear JBD2_ABORT flag before journal_reset to update log tail info when load journal Kai Li
2020-01-14 10:31 ` Jan Kara
2020-01-17 21:26   ` Theodore Y. Ts'o
2020-01-20  6:30     ` Likai

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git