[PATCH] jbd2: correct the end of the journal recovery scan range

[PATCH] jbd2: correct the end of the journal recovery scan range

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

We got a filesystem inconsistency issue below while running generic/475
I/O failure pressure test with fast_commit feature enabled.

 Symlink /p3/d3/d1c/d6c/dd6/dce/l101 (inode #132605) is invalid.

If fast_commit feature is enabled, a special fast_commit journal area is
appended to the end of the normal journal area. The journal->j_last
point to the first unused block behind the normal journal area instead
of the whole log area, and the journal->j_fc_last point to the first
unused block behind the fast_commit journal area. While doing journal
recovery, do_one_pass(PASS_SCAN) should first scan the normal journal
area and turn around to the first block once it meet journal->j_last,
but the wrap() macro misuse the journal->j_fc_last, so the recovering
could not read the next magic block (commit block perhaps) and would end
early mistakenly and missing tN and every transaction after it in the
following example. Finally, it could lead to filesystem inconsistency.

 | normal journal area                             | fast commit area |
 +-------------------------------------------------+------------------+
 | tN(rere) | tN+1 |~| tN-x |...| tN-1 | tN(front) |       ....       |
 +-------------------------------------------------+------------------+
                     /                             /                  /
                start               journal->j_last journal->j_fc_last

This patch fix it by use the correct ending journal->j_last.

Fixes: 5b849b5f96b4 ("jbd2: fast commit recovery path")
Reported-by: Theodore Ts'o <tytso@mit.edu>
Link: https://lore.kernel.org/linux-ext4/20230613043120.GB1584772@mit.edu/
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/jbd2/recovery.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 0184931d47f7..c269a7d29a46 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -230,12 +230,8 @@ static int count_tags(journal_t *journal, struct buffer_head *bh)
 /* Make sure we wrap around the log correctly! */
 #define wrap(journal, var)						\
 do {									\
-	unsigned long _wrap_last =					\
-		jbd2_has_feature_fast_commit(journal) ?			\
-			(journal)->j_fc_last : (journal)->j_last;	\
-									\
-	if (var >= _wrap_last)						\
-		var -= (_wrap_last - (journal)->j_first);		\
+	if (var >= (journal)->j_last)					\
+		var -= ((journal)->j_last - (journal)->j_first);	\
 } while (0)
 
 static int fc_do_one_pass(journal_t *journal,
@@ -524,9 +520,7 @@ static int do_one_pass(journal_t *journal,
 				break;
 
 		jbd2_debug(2, "Scanning for sequence ID %u at %lu/%lu\n",
-			  next_commit_ID, next_log_block,
-			  jbd2_has_feature_fast_commit(journal) ?
-			  journal->j_fc_last : journal->j_last);
+			  next_commit_ID, next_log_block, journal->j_last);
 
 		/* Skip over each chunk of the transaction looking
 		 * either the next descriptor block or the final commit
-- 
2.31.1

Re: [PATCH] jbd2: correct the end of the journal recovery scan range

[Jan Kara]

On Mon 26-06-23 15:33:22, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> We got a filesystem inconsistency issue below while running generic/475
> I/O failure pressure test with fast_commit feature enabled.
> 
>  Symlink /p3/d3/d1c/d6c/dd6/dce/l101 (inode #132605) is invalid.
> 
> If fast_commit feature is enabled, a special fast_commit journal area is
> appended to the end of the normal journal area. The journal->j_last
> point to the first unused block behind the normal journal area instead
> of the whole log area, and the journal->j_fc_last point to the first
> unused block behind the fast_commit journal area. While doing journal
> recovery, do_one_pass(PASS_SCAN) should first scan the normal journal
> area and turn around to the first block once it meet journal->j_last,
> but the wrap() macro misuse the journal->j_fc_last, so the recovering
> could not read the next magic block (commit block perhaps) and would end
> early mistakenly and missing tN and every transaction after it in the
> following example. Finally, it could lead to filesystem inconsistency.
> 
>  | normal journal area                             | fast commit area |
>  +-------------------------------------------------+------------------+
>  | tN(rere) | tN+1 |~| tN-x |...| tN-1 | tN(front) |       ....       |
>  +-------------------------------------------------+------------------+
>                      /                             /                  /
>                 start               journal->j_last journal->j_fc_last
> 
> This patch fix it by use the correct ending journal->j_last.
> 
> Fixes: 5b849b5f96b4 ("jbd2: fast commit recovery path")
> Reported-by: Theodore Ts'o <tytso@mit.edu>
> Link: https://lore.kernel.org/linux-ext4/20230613043120.GB1584772@mit.edu/
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Ah, great catch! The patch looks good to me. Feel free to add:

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

								Honza

> ---
>  fs/jbd2/recovery.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 0184931d47f7..c269a7d29a46 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -230,12 +230,8 @@ static int count_tags(journal_t *journal, struct buffer_head *bh)
>  /* Make sure we wrap around the log correctly! */
>  #define wrap(journal, var)						\
>  do {									\
> -	unsigned long _wrap_last =					\
> -		jbd2_has_feature_fast_commit(journal) ?			\
> -			(journal)->j_fc_last : (journal)->j_last;	\
> -									\
> -	if (var >= _wrap_last)						\
> -		var -= (_wrap_last - (journal)->j_first);		\
> +	if (var >= (journal)->j_last)					\
> +		var -= ((journal)->j_last - (journal)->j_first);	\
>  } while (0)
>  
>  static int fc_do_one_pass(journal_t *journal,
> @@ -524,9 +520,7 @@ static int do_one_pass(journal_t *journal,
>  				break;
>  
>  		jbd2_debug(2, "Scanning for sequence ID %u at %lu/%lu\n",
> -			  next_commit_ID, next_log_block,
> -			  jbd2_has_feature_fast_commit(journal) ?
> -			  journal->j_fc_last : journal->j_last);
> +			  next_commit_ID, next_log_block, journal->j_last);
>  
>  		/* Skip over each chunk of the transaction looking
>  		 * either the next descriptor block or the final commit
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] jbd2: correct the end of the journal recovery scan range

[Theodore Ts'o]

On Mon, 26 Jun 2023 15:33:22 +0800, Zhang Yi wrote:
> We got a filesystem inconsistency issue below while running generic/475
> I/O failure pressure test with fast_commit feature enabled.
> 
>  Symlink /p3/d3/d1c/d6c/dd6/dce/l101 (inode #132605) is invalid.
> 
> If fast_commit feature is enabled, a special fast_commit journal area is
> appended to the end of the normal journal area. The journal->j_last
> point to the first unused block behind the normal journal area instead
> of the whole log area, and the journal->j_fc_last point to the first
> unused block behind the fast_commit journal area. While doing journal
> recovery, do_one_pass(PASS_SCAN) should first scan the normal journal
> area and turn around to the first block once it meet journal->j_last,
> but the wrap() macro misuse the journal->j_fc_last, so the recovering
> could not read the next magic block (commit block perhaps) and would end
> early mistakenly and missing tN and every transaction after it in the
> following example. Finally, it could lead to filesystem inconsistency.
> 
> [...]

Applied, thanks!

[1/1] jbd2: correct the end of the journal recovery scan range
      commit: 3aab2c3d9ef0b30dd41e20c336377c43b4ca513e

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>