[PATCH 0/5] ext4: Fix stale buffer loading from last failed

[PATCH 0/5] ext4: Fix stale buffer loading from last failed

[Zhihao Cheng]

Patch 1 fixes reusing stale buffer heads from last failed mounting.
Patch 2~4 reconstructs 'j_format_version' initialization and checking
in loading process.

Zhang Yi (4):
  jbd2: remove unused feature macros
  jbd2: switch to check format version in superblock directly
  jbd2: factor out journal initialization from journal_get_superblock()
  jbd2: remove j_format_version

Zhihao Cheng (1):
  ext4: Fix reusing stale buffer heads from last failed mounting

 fs/ext4/super.c      | 15 +++++------
 fs/jbd2/journal.c    | 59 +++++++++++++++-----------------------------
 include/linux/jbd2.h | 33 +++++++++++--------------
 3 files changed, 42 insertions(+), 65 deletions(-)

-- 
2.31.1

[PATCH 1/5] ext4: Fix reusing stale buffer heads from last failed mounting

[Zhihao Cheng]

Following process makes ext4 load stale buffer heads from last failed
mounting in a new mounting operation:
mount_bdev
 ext4_fill_super
 | ext4_load_and_init_journal
 |  ext4_load_journal
 |   jbd2_journal_load
 |    load_superblock
 |     journal_get_superblock
 |      set_buffer_verified(bh) // buffer head is verified
 |   jbd2_journal_recover // failed caused by EIO
 | goto failed_mount3a // skip 'sb->s_root' initialization
 deactivate_locked_super
  kill_block_super
   generic_shutdown_super
    if (sb->s_root)
    // false, skip ext4_put_super->invalidate_bdev->
    // invalidate_mapping_pages->mapping_evict_folio->
    // filemap_release_folio->try_to_free_buffers, which
    // cannot drop buffer head.
   blkdev_put
    blkdev_put_whole
     if (atomic_dec_and_test(&bdev->bd_openers))
     // false, systemd-udev happens to open the device. Then
     // blkdev_flush_mapping->kill_bdev->truncate_inode_pages->
     // truncate_inode_folio->truncate_cleanup_folio->
     // folio_invalidate->block_invalidate_folio->
     // filemap_release_folio->try_to_free_buffers will be skipped,
     // dropping buffer head is missed again.

Second mount:
ext4_fill_super
 ext4_load_and_init_journal
  ext4_load_journal
   ext4_get_journal
    jbd2_journal_init_inode
     journal_init_common
      bh = getblk_unmovable
       bh = __find_get_block // Found stale bh in last failed mounting
      journal->j_sb_buffer = bh
   jbd2_journal_load
    load_superblock
     journal_get_superblock
      if (buffer_verified(bh))
      // true, skip journal->j_format_version = 2, value is 0
    jbd2_journal_recover
     do_one_pass
      next_log_block += count_tags(journal, bh)
      // According to journal_tag_bytes(), 'tag_bytes' calculating is
      // affected by jbd2_has_feature_csum3(), jbd2_has_feature_csum3()
      // returns false because 'j->j_format_version >= 2' is not true,
      // then we get wrong next_log_block. The do_one_pass may exit
      // early whenoccuring non JBD2_MAGIC_NUMBER in 'next_log_block'.

The filesystem is corrupted here, journal is partially replayed, and
new journal sequence number actually is already used by last mounting.

The invalidate_bdev() can drop all buffer heads even racing with bare
reading block device(eg. systemd-udev), so we can fix it by invalidating
bdev in error handling path in __ext4_fill_super().

Fetch a reproducer in [Link].

Link: https://bugzilla.kernel.org/show_bug.cgi?id=217171
Cc: <stable@kernel.org>
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/ext4/super.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 88f7b8a88c76..7e990637bc48 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1126,6 +1126,12 @@ static void ext4_blkdev_remove(struct ext4_sb_info *sbi)
 	struct block_device *bdev;
 	bdev = sbi->s_journal_bdev;
 	if (bdev) {
+		/*
+		 * Invalidate the journal device's buffers.  We don't want them
+		 * floating about in memory - the physical journal device may
+		 * hotswapped, and it breaks the `ro-after' testing code.
+		 */
+		invalidate_bdev(bdev);
 		ext4_blkdev_put(bdev);
 		sbi->s_journal_bdev = NULL;
 	}
@@ -1271,14 +1277,8 @@ static void ext4_put_super(struct super_block *sb)
 
 	sync_blockdev(sb->s_bdev);
 	invalidate_bdev(sb->s_bdev);
-	if (sbi->s_journal_bdev && sbi->s_journal_bdev != sb->s_bdev) {
-		/*
-		 * Invalidate the journal device's buffers.  We don't want them
-		 * floating about in memory - the physical journal device may
-		 * hotswapped, and it breaks the `ro-after' testing code.
-		 */
+	if (sbi->s_journal_bdev) {
 		sync_blockdev(sbi->s_journal_bdev);
-		invalidate_bdev(sbi->s_journal_bdev);
 		ext4_blkdev_remove(sbi);
 	}
 
@@ -5610,6 +5610,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	brelse(sbi->s_sbh);
 	ext4_blkdev_remove(sbi);
 out_fail:
+	invalidate_bdev(sb->s_bdev);
 	sb->s_fs_info = NULL;
 	return err ? err : ret;
 }
-- 
2.31.1

Re: [PATCH 1/5] ext4: Fix reusing stale buffer heads from last failed mounting

[Jan Kara]

On Fri 10-03-23 20:52:02, Zhihao Cheng wrote:
> Following process makes ext4 load stale buffer heads from last failed
> mounting in a new mounting operation:
> mount_bdev
>  ext4_fill_super
>  | ext4_load_and_init_journal
>  |  ext4_load_journal
>  |   jbd2_journal_load
>  |    load_superblock
>  |     journal_get_superblock
>  |      set_buffer_verified(bh) // buffer head is verified
>  |   jbd2_journal_recover // failed caused by EIO
>  | goto failed_mount3a // skip 'sb->s_root' initialization
>  deactivate_locked_super
>   kill_block_super
>    generic_shutdown_super
>     if (sb->s_root)
>     // false, skip ext4_put_super->invalidate_bdev->
>     // invalidate_mapping_pages->mapping_evict_folio->
>     // filemap_release_folio->try_to_free_buffers, which
>     // cannot drop buffer head.
>    blkdev_put
>     blkdev_put_whole
>      if (atomic_dec_and_test(&bdev->bd_openers))
>      // false, systemd-udev happens to open the device. Then
>      // blkdev_flush_mapping->kill_bdev->truncate_inode_pages->
>      // truncate_inode_folio->truncate_cleanup_folio->
>      // folio_invalidate->block_invalidate_folio->
>      // filemap_release_folio->try_to_free_buffers will be skipped,
>      // dropping buffer head is missed again.
> 
> Second mount:
> ext4_fill_super
>  ext4_load_and_init_journal
>   ext4_load_journal
>    ext4_get_journal
>     jbd2_journal_init_inode
>      journal_init_common
>       bh = getblk_unmovable
>        bh = __find_get_block // Found stale bh in last failed mounting
>       journal->j_sb_buffer = bh
>    jbd2_journal_load
>     load_superblock
>      journal_get_superblock
>       if (buffer_verified(bh))
>       // true, skip journal->j_format_version = 2, value is 0
>     jbd2_journal_recover
>      do_one_pass
>       next_log_block += count_tags(journal, bh)
>       // According to journal_tag_bytes(), 'tag_bytes' calculating is
>       // affected by jbd2_has_feature_csum3(), jbd2_has_feature_csum3()
>       // returns false because 'j->j_format_version >= 2' is not true,
>       // then we get wrong next_log_block. The do_one_pass may exit
>       // early whenoccuring non JBD2_MAGIC_NUMBER in 'next_log_block'.
> 
> The filesystem is corrupted here, journal is partially replayed, and
> new journal sequence number actually is already used by last mounting.
> 
> The invalidate_bdev() can drop all buffer heads even racing with bare
> reading block device(eg. systemd-udev), so we can fix it by invalidating
> bdev in error handling path in __ext4_fill_super().
> 
> Fetch a reproducer in [Link].
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217171
> Cc: <stable@kernel.org>
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>

The fix looks good to me. Feel free to add:

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

								Honza

> ---
>  fs/ext4/super.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 88f7b8a88c76..7e990637bc48 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1126,6 +1126,12 @@ static void ext4_blkdev_remove(struct ext4_sb_info *sbi)
>  	struct block_device *bdev;
>  	bdev = sbi->s_journal_bdev;
>  	if (bdev) {
> +		/*
> +		 * Invalidate the journal device's buffers.  We don't want them
> +		 * floating about in memory - the physical journal device may
> +		 * hotswapped, and it breaks the `ro-after' testing code.
> +		 */
> +		invalidate_bdev(bdev);
>  		ext4_blkdev_put(bdev);
>  		sbi->s_journal_bdev = NULL;
>  	}
> @@ -1271,14 +1277,8 @@ static void ext4_put_super(struct super_block *sb)
>  
>  	sync_blockdev(sb->s_bdev);
>  	invalidate_bdev(sb->s_bdev);
> -	if (sbi->s_journal_bdev && sbi->s_journal_bdev != sb->s_bdev) {
> -		/*
> -		 * Invalidate the journal device's buffers.  We don't want them
> -		 * floating about in memory - the physical journal device may
> -		 * hotswapped, and it breaks the `ro-after' testing code.
> -		 */
> +	if (sbi->s_journal_bdev) {
>  		sync_blockdev(sbi->s_journal_bdev);
> -		invalidate_bdev(sbi->s_journal_bdev);
>  		ext4_blkdev_remove(sbi);
>  	}
>  
> @@ -5610,6 +5610,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	brelse(sbi->s_sbh);
>  	ext4_blkdev_remove(sbi);
>  out_fail:
> +	invalidate_bdev(sb->s_bdev);
>  	sb->s_fs_info = NULL;
>  	return err ? err : ret;
>  }
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/5] ext4: Fix reusing stale buffer heads from last failed mounting

[Tudor Ambarus]

Hi, Zhihao,

On 3/10/23 12:52, Zhihao Cheng wrote:

cut

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217171
> Cc: <stable@kernel.org>

Shouldn't have been stable@vger.kernel.org instead? That's what is
advertised at:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

A Fixes tag would be helpful. It would assist the stable kernel team, or
people that have to backport your patch, in determining which stable
versions should receive your fix. Same suggestion is made in
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Thanks!
ta

[PATCH 2/5] jbd2: remove unused feature macros

[Zhihao Cheng]

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

JBD2_HAS_[IN|RO_]COMPAT_FEATURE macros are no longer used, just remove
them.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 include/linux/jbd2.h | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 5962072a4b19..ad7bb6861143 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -274,17 +274,6 @@ typedef struct journal_superblock_s
 /* 0x0400 */
 } journal_superblock_t;
 
-/* Use the jbd2_{has,set,clear}_feature_* helpers; these will be removed */
-#define JBD2_HAS_COMPAT_FEATURE(j,mask)					\
-	((j)->j_format_version >= 2 &&					\
-	 ((j)->j_superblock->s_feature_compat & cpu_to_be32((mask))))
-#define JBD2_HAS_RO_COMPAT_FEATURE(j,mask)				\
-	((j)->j_format_version >= 2 &&					\
-	 ((j)->j_superblock->s_feature_ro_compat & cpu_to_be32((mask))))
-#define JBD2_HAS_INCOMPAT_FEATURE(j,mask)				\
-	((j)->j_format_version >= 2 &&					\
-	 ((j)->j_superblock->s_feature_incompat & cpu_to_be32((mask))))
-
 #define JBD2_FEATURE_COMPAT_CHECKSUM		0x00000001
 
 #define JBD2_FEATURE_INCOMPAT_REVOKE		0x00000001
-- 
2.31.1

Re: [PATCH 2/5] jbd2: remove unused feature macros

[Jan Kara]

On Fri 10-03-23 20:52:03, Zhihao Cheng wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> JBD2_HAS_[IN|RO_]COMPAT_FEATURE macros are no longer used, just remove
> them.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
>  include/linux/jbd2.h | 11 -----------
>  1 file changed, 11 deletions(-)

Nice. Feel free to add:

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

								Honza

> 
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 5962072a4b19..ad7bb6861143 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -274,17 +274,6 @@ typedef struct journal_superblock_s
>  /* 0x0400 */
>  } journal_superblock_t;
>  
> -/* Use the jbd2_{has,set,clear}_feature_* helpers; these will be removed */
> -#define JBD2_HAS_COMPAT_FEATURE(j,mask)					\
> -	((j)->j_format_version >= 2 &&					\
> -	 ((j)->j_superblock->s_feature_compat & cpu_to_be32((mask))))
> -#define JBD2_HAS_RO_COMPAT_FEATURE(j,mask)				\
> -	((j)->j_format_version >= 2 &&					\
> -	 ((j)->j_superblock->s_feature_ro_compat & cpu_to_be32((mask))))
> -#define JBD2_HAS_INCOMPAT_FEATURE(j,mask)				\
> -	((j)->j_format_version >= 2 &&					\
> -	 ((j)->j_superblock->s_feature_incompat & cpu_to_be32((mask))))
> -
>  #define JBD2_FEATURE_COMPAT_CHECKSUM		0x00000001
>  
>  #define JBD2_FEATURE_INCOMPAT_REVOKE		0x00000001
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/5] jbd2: switch to check format version in superblock directly

[Zhihao Cheng]

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

We should only check and set extented features if journal format version
is 2, and now we check the in memory copy of the superblock
'journal->j_format_version', which relys on the parameter initialization
sequence, switch to use the h_blocktype in superblock cloud be more
clear.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/jbd2/journal.c    | 16 +++++++---------
 include/linux/jbd2.h | 17 ++++++++++++++---
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index e80c781731f8..b991d5c21d16 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2059,10 +2059,12 @@ int jbd2_journal_load(journal_t *journal)
 		return err;
 
 	sb = journal->j_superblock;
-	/* If this is a V2 superblock, then we have to check the
-	 * features flags on it. */
 
-	if (journal->j_format_version >= 2) {
+	/*
+	 * If this is a V2 superblock, then we have to check the
+	 * features flags on it.
+	 */
+	if (jbd2_format_support_feature(journal)) {
 		if ((sb->s_feature_ro_compat &
 		     ~cpu_to_be32(JBD2_KNOWN_ROCOMPAT_FEATURES)) ||
 		    (sb->s_feature_incompat &
@@ -2224,7 +2226,7 @@ int jbd2_journal_check_used_features(journal_t *journal, unsigned long compat,
 	if (journal->j_format_version == 0 &&
 	    journal_get_superblock(journal) != 0)
 		return 0;
-	if (journal->j_format_version == 1)
+	if (!jbd2_format_support_feature(journal))
 		return 0;
 
 	sb = journal->j_superblock;
@@ -2254,11 +2256,7 @@ int jbd2_journal_check_available_features(journal_t *journal, unsigned long comp
 	if (!compat && !ro && !incompat)
 		return 1;
 
-	/* We can support any known requested features iff the
-	 * superblock is in version 2.  Otherwise we fail to support any
-	 * extended sb features. */
-
-	if (journal->j_format_version != 2)
+	if (!jbd2_format_support_feature(journal))
 		return 0;
 
 	if ((compat   & JBD2_KNOWN_COMPAT_FEATURES) == compat &&
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index ad7bb6861143..7095c0f17ad0 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1305,11 +1305,22 @@ struct journal_s
 		rwsem_release(&j->j_trans_commit_map, _THIS_IP_); \
 	} while (0)
 
+/*
+ * We can support any known requested features iff the
+ * superblock is not in version 1.  Otherwise we fail to support any
+ * extended sb features.
+ */
+static inline bool jbd2_format_support_feature(journal_t *j)
+{
+	return j->j_superblock->s_header.h_blocktype !=
+					cpu_to_be32(JBD2_SUPERBLOCK_V1);
+}
+
 /* journal feature predicate functions */
 #define JBD2_FEATURE_COMPAT_FUNCS(name, flagname) \
 static inline bool jbd2_has_feature_##name(journal_t *j) \
 { \
-	return ((j)->j_format_version >= 2 && \
+	return (jbd2_format_support_feature(j) && \
 		((j)->j_superblock->s_feature_compat & \
 		 cpu_to_be32(JBD2_FEATURE_COMPAT_##flagname)) != 0); \
 } \
@@ -1327,7 +1338,7 @@ static inline void jbd2_clear_feature_##name(journal_t *j) \
 #define JBD2_FEATURE_RO_COMPAT_FUNCS(name, flagname) \
 static inline bool jbd2_has_feature_##name(journal_t *j) \
 { \
-	return ((j)->j_format_version >= 2 && \
+	return (jbd2_format_support_feature(j) && \
 		((j)->j_superblock->s_feature_ro_compat & \
 		 cpu_to_be32(JBD2_FEATURE_RO_COMPAT_##flagname)) != 0); \
 } \
@@ -1345,7 +1356,7 @@ static inline void jbd2_clear_feature_##name(journal_t *j) \
 #define JBD2_FEATURE_INCOMPAT_FUNCS(name, flagname) \
 static inline bool jbd2_has_feature_##name(journal_t *j) \
 { \
-	return ((j)->j_format_version >= 2 && \
+	return (jbd2_format_support_feature(j) && \
 		((j)->j_superblock->s_feature_incompat & \
 		 cpu_to_be32(JBD2_FEATURE_INCOMPAT_##flagname)) != 0); \
 } \
-- 
2.31.1

Re: [PATCH 3/5] jbd2: switch to check format version in superblock directly

[Jan Kara]

On Fri 10-03-23 20:52:04, Zhihao Cheng wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> We should only check and set extented features if journal format version
> is 2, and now we check the in memory copy of the superblock
> 'journal->j_format_version', which relys on the parameter initialization
> sequence, switch to use the h_blocktype in superblock cloud be more
> clear.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>

Looks good to me. Feel free to add:

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

								Honza

> ---
>  fs/jbd2/journal.c    | 16 +++++++---------
>  include/linux/jbd2.h | 17 ++++++++++++++---
>  2 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index e80c781731f8..b991d5c21d16 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -2059,10 +2059,12 @@ int jbd2_journal_load(journal_t *journal)
>  		return err;
>  
>  	sb = journal->j_superblock;
> -	/* If this is a V2 superblock, then we have to check the
> -	 * features flags on it. */
>  
> -	if (journal->j_format_version >= 2) {
> +	/*
> +	 * If this is a V2 superblock, then we have to check the
> +	 * features flags on it.
> +	 */
> +	if (jbd2_format_support_feature(journal)) {
>  		if ((sb->s_feature_ro_compat &
>  		     ~cpu_to_be32(JBD2_KNOWN_ROCOMPAT_FEATURES)) ||
>  		    (sb->s_feature_incompat &
> @@ -2224,7 +2226,7 @@ int jbd2_journal_check_used_features(journal_t *journal, unsigned long compat,
>  	if (journal->j_format_version == 0 &&
>  	    journal_get_superblock(journal) != 0)
>  		return 0;
> -	if (journal->j_format_version == 1)
> +	if (!jbd2_format_support_feature(journal))
>  		return 0;
>  
>  	sb = journal->j_superblock;
> @@ -2254,11 +2256,7 @@ int jbd2_journal_check_available_features(journal_t *journal, unsigned long comp
>  	if (!compat && !ro && !incompat)
>  		return 1;
>  
> -	/* We can support any known requested features iff the
> -	 * superblock is in version 2.  Otherwise we fail to support any
> -	 * extended sb features. */
> -
> -	if (journal->j_format_version != 2)
> +	if (!jbd2_format_support_feature(journal))
>  		return 0;
>  
>  	if ((compat   & JBD2_KNOWN_COMPAT_FEATURES) == compat &&
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index ad7bb6861143..7095c0f17ad0 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1305,11 +1305,22 @@ struct journal_s
>  		rwsem_release(&j->j_trans_commit_map, _THIS_IP_); \
>  	} while (0)
>  
> +/*
> + * We can support any known requested features iff the
> + * superblock is not in version 1.  Otherwise we fail to support any
> + * extended sb features.
> + */
> +static inline bool jbd2_format_support_feature(journal_t *j)
> +{
> +	return j->j_superblock->s_header.h_blocktype !=
> +					cpu_to_be32(JBD2_SUPERBLOCK_V1);
> +}
> +
>  /* journal feature predicate functions */
>  #define JBD2_FEATURE_COMPAT_FUNCS(name, flagname) \
>  static inline bool jbd2_has_feature_##name(journal_t *j) \
>  { \
> -	return ((j)->j_format_version >= 2 && \
> +	return (jbd2_format_support_feature(j) && \
>  		((j)->j_superblock->s_feature_compat & \
>  		 cpu_to_be32(JBD2_FEATURE_COMPAT_##flagname)) != 0); \
>  } \
> @@ -1327,7 +1338,7 @@ static inline void jbd2_clear_feature_##name(journal_t *j) \
>  #define JBD2_FEATURE_RO_COMPAT_FUNCS(name, flagname) \
>  static inline bool jbd2_has_feature_##name(journal_t *j) \
>  { \
> -	return ((j)->j_format_version >= 2 && \
> +	return (jbd2_format_support_feature(j) && \
>  		((j)->j_superblock->s_feature_ro_compat & \
>  		 cpu_to_be32(JBD2_FEATURE_RO_COMPAT_##flagname)) != 0); \
>  } \
> @@ -1345,7 +1356,7 @@ static inline void jbd2_clear_feature_##name(journal_t *j) \
>  #define JBD2_FEATURE_INCOMPAT_FUNCS(name, flagname) \
>  static inline bool jbd2_has_feature_##name(journal_t *j) \
>  { \
> -	return ((j)->j_format_version >= 2 && \
> +	return (jbd2_format_support_feature(j) && \
>  		((j)->j_superblock->s_feature_incompat & \
>  		 cpu_to_be32(JBD2_FEATURE_INCOMPAT_##flagname)) != 0); \
>  } \
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 4/5] jbd2: factor out journal initialization from journal_get_superblock()

[Zhihao Cheng]

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

Current journal_get_superblock() couple journal superblock checking and
partial journal initialization, factor out initialization part from it
to make things clear.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/jbd2/journal.c | 52 ++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 30 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index b991d5c21d16..cd94d068b4e6 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1921,26 +1921,15 @@ static int journal_get_superblock(journal_t *journal)
 		printk(KERN_WARNING "JBD2: no valid journal superblock found\n");
 		goto out;
 	}
-
-	switch(be32_to_cpu(sb->s_header.h_blocktype)) {
-	case JBD2_SUPERBLOCK_V1:
-		journal->j_format_version = 1;
-		break;
-	case JBD2_SUPERBLOCK_V2:
-		journal->j_format_version = 2;
-		break;
-	default:
+	if (be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V1 &&
+	    be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V2) {
 		printk(KERN_WARNING "JBD2: unrecognised superblock format ID\n");
 		goto out;
 	}
-
-	if (be32_to_cpu(sb->s_maxlen) < journal->j_total_len)
-		journal->j_total_len = be32_to_cpu(sb->s_maxlen);
-	else if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) {
+	if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) {
 		printk(KERN_WARNING "JBD2: journal file too short\n");
 		goto out;
 	}
-
 	if (be32_to_cpu(sb->s_first) == 0 ||
 	    be32_to_cpu(sb->s_first) >= journal->j_total_len) {
 		printk(KERN_WARNING
@@ -1956,7 +1945,6 @@ static int journal_get_superblock(journal_t *journal)
 		       "at the same time!\n");
 		goto out;
 	}
-
 	if (jbd2_journal_has_csum_v2or3_feature(journal) &&
 	    jbd2_has_feature_checksum(journal)) {
 		/* Can't have checksum v1 and v2 on at the same time! */
@@ -1964,12 +1952,10 @@ static int journal_get_superblock(journal_t *journal)
 		       "at the same time!\n");
 		goto out;
 	}
-
 	if (!jbd2_verify_csum_type(journal, sb)) {
 		printk(KERN_ERR "JBD2: Unknown checksum type\n");
 		goto out;
 	}
-
 	/* Load the checksum driver */
 	if (jbd2_journal_has_csum_v2or3_feature(journal)) {
 		journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
@@ -1979,25 +1965,14 @@ static int journal_get_superblock(journal_t *journal)
 			journal->j_chksum_driver = NULL;
 			goto out;
 		}
-	}
-
-	if (jbd2_journal_has_csum_v2or3(journal)) {
 		/* Check superblock checksum */
 		if (sb->s_checksum != jbd2_superblock_csum(journal, sb)) {
 			printk(KERN_ERR "JBD2: journal checksum error\n");
 			err = -EFSBADCRC;
 			goto out;
 		}
-
-		/* Precompute checksum seed for all metadata */
-		journal->j_csum_seed = jbd2_chksum(journal, ~0, sb->s_uuid,
-						   sizeof(sb->s_uuid));
 	}
-
-	journal->j_revoke_records_per_block =
-				journal_revoke_records_per_block(journal);
 	set_buffer_verified(bh);
-
 	return 0;
 
 out:
@@ -2022,12 +1997,30 @@ static int load_superblock(journal_t *journal)
 
 	sb = journal->j_superblock;
 
+	switch (be32_to_cpu(sb->s_header.h_blocktype)) {
+	case JBD2_SUPERBLOCK_V1:
+		journal->j_format_version = 1;
+		break;
+	case JBD2_SUPERBLOCK_V2:
+		journal->j_format_version = 2;
+		break;
+	}
+
 	journal->j_tail_sequence = be32_to_cpu(sb->s_sequence);
 	journal->j_tail = be32_to_cpu(sb->s_start);
 	journal->j_first = be32_to_cpu(sb->s_first);
 	journal->j_errno = be32_to_cpu(sb->s_errno);
 	journal->j_last = be32_to_cpu(sb->s_maxlen);
 
+	if (be32_to_cpu(sb->s_maxlen) < journal->j_total_len)
+		journal->j_total_len = be32_to_cpu(sb->s_maxlen);
+	/* Precompute checksum seed for all metadata */
+	if (jbd2_journal_has_csum_v2or3(journal))
+		journal->j_csum_seed = jbd2_chksum(journal, ~0, sb->s_uuid,
+						   sizeof(sb->s_uuid));
+	journal->j_revoke_records_per_block =
+				journal_revoke_records_per_block(journal);
+
 	if (jbd2_has_feature_fast_commit(journal)) {
 		journal->j_fc_last = be32_to_cpu(sb->s_maxlen);
 		num_fc_blocks = jbd2_journal_get_num_fc_blks(sb);
@@ -2223,8 +2216,7 @@ int jbd2_journal_check_used_features(journal_t *journal, unsigned long compat,
 	if (!compat && !ro && !incompat)
 		return 1;
 	/* Load journal superblock if it is not loaded yet. */
-	if (journal->j_format_version == 0 &&
-	    journal_get_superblock(journal) != 0)
+	if (journal_get_superblock(journal))
 		return 0;
 	if (!jbd2_format_support_feature(journal))
 		return 0;
-- 
2.31.1

Re: [PATCH 4/5] jbd2: factor out journal initialization from journal_get_superblock()

[Jan Kara]

On Fri 10-03-23 20:52:05, Zhihao Cheng wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Current journal_get_superblock() couple journal superblock checking and
> partial journal initialization, factor out initialization part from it
> to make things clear.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
>  fs/jbd2/journal.c | 52 ++++++++++++++++++++---------------------------
>  1 file changed, 22 insertions(+), 30 deletions(-)

The patch looks mostly good. Just one style nit below.

> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index b991d5c21d16..cd94d068b4e6 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1921,26 +1921,15 @@ static int journal_get_superblock(journal_t *journal)
>  		printk(KERN_WARNING "JBD2: no valid journal superblock found\n");
>  		goto out;
>  	}
> -
> -	switch(be32_to_cpu(sb->s_header.h_blocktype)) {
> -	case JBD2_SUPERBLOCK_V1:
> -		journal->j_format_version = 1;
> -		break;
> -	case JBD2_SUPERBLOCK_V2:
> -		journal->j_format_version = 2;
> -		break;
> -	default:
> +	if (be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V1 &&
> +	    be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V2) {
>  		printk(KERN_WARNING "JBD2: unrecognised superblock format ID\n");
>  		goto out;
>  	}
> -
> -	if (be32_to_cpu(sb->s_maxlen) < journal->j_total_len)
> -		journal->j_total_len = be32_to_cpu(sb->s_maxlen);
> -	else if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) {
> +	if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) {
>  		printk(KERN_WARNING "JBD2: journal file too short\n");
>  		goto out;
>  	}
> -

Please keep this empty line here. It actually makes the code more readable,
separating logically different checks. Same with three empty lines below...

>  	if (be32_to_cpu(sb->s_first) == 0 ||
>  	    be32_to_cpu(sb->s_first) >= journal->j_total_len) {
>  		printk(KERN_WARNING
> @@ -1956,7 +1945,6 @@ static int journal_get_superblock(journal_t *journal)
>  		       "at the same time!\n");
>  		goto out;
>  	}
> -
>  	if (jbd2_journal_has_csum_v2or3_feature(journal) &&
>  	    jbd2_has_feature_checksum(journal)) {
>  		/* Can't have checksum v1 and v2 on at the same time! */
> @@ -1964,12 +1952,10 @@ static int journal_get_superblock(journal_t *journal)
>  		       "at the same time!\n");
>  		goto out;
>  	}
> -
>  	if (!jbd2_verify_csum_type(journal, sb)) {
>  		printk(KERN_ERR "JBD2: Unknown checksum type\n");
>  		goto out;
>  	}
> -
>  	/* Load the checksum driver */
>  	if (jbd2_journal_has_csum_v2or3_feature(journal)) {
>  		journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);

Otherwise the patch looks good.

									Honza

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

Re: [PATCH 4/5] jbd2: factor out journal initialization from journal_get_superblock()

[Zhang Yi]

On 2023/3/13 19:12, Jan Kara wrote:
> On Fri 10-03-23 20:52:05, Zhihao Cheng wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> Current journal_get_superblock() couple journal superblock checking and
>> partial journal initialization, factor out initialization part from it
>> to make things clear.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
>> ---
>>  fs/jbd2/journal.c | 52 ++++++++++++++++++++---------------------------
>>  1 file changed, 22 insertions(+), 30 deletions(-)
> 
> The patch looks mostly good. Just one style nit below.
> 
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index b991d5c21d16..cd94d068b4e6 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -1921,26 +1921,15 @@ static int journal_get_superblock(journal_t *journal)
>>  		printk(KERN_WARNING "JBD2: no valid journal superblock found\n");
>>  		goto out;
>>  	}
>> -
>> -	switch(be32_to_cpu(sb->s_header.h_blocktype)) {
>> -	case JBD2_SUPERBLOCK_V1:
>> -		journal->j_format_version = 1;
>> -		break;
>> -	case JBD2_SUPERBLOCK_V2:
>> -		journal->j_format_version = 2;
>> -		break;
>> -	default:
>> +	if (be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V1 &&
>> +	    be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V2) {
>>  		printk(KERN_WARNING "JBD2: unrecognised superblock format ID\n");
>>  		goto out;
>>  	}
>> -
>> -	if (be32_to_cpu(sb->s_maxlen) < journal->j_total_len)
>> -		journal->j_total_len = be32_to_cpu(sb->s_maxlen);
>> -	else if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) {
>> +	if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) {
>>  		printk(KERN_WARNING "JBD2: journal file too short\n");
>>  		goto out;
>>  	}
>> -
> 
> Please keep this empty line here. It actually makes the code more readable,
> separating logically different checks. Same with three empty lines below...

Thanks for the comments, I will add them back.

Yi.

[PATCH 5/5] jbd2: remove j_format_version

[Zhihao Cheng]

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

journal->j_format_version is no longer used, remove it.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/jbd2/journal.c    | 9 ---------
 include/linux/jbd2.h | 5 -----
 2 files changed, 14 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index cd94d068b4e6..fbada835b6b7 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1997,15 +1997,6 @@ static int load_superblock(journal_t *journal)
 
 	sb = journal->j_superblock;
 
-	switch (be32_to_cpu(sb->s_header.h_blocktype)) {
-	case JBD2_SUPERBLOCK_V1:
-		journal->j_format_version = 1;
-		break;
-	case JBD2_SUPERBLOCK_V2:
-		journal->j_format_version = 2;
-		break;
-	}
-
 	journal->j_tail_sequence = be32_to_cpu(sb->s_sequence);
 	journal->j_tail = be32_to_cpu(sb->s_start);
 	journal->j_first = be32_to_cpu(sb->s_first);
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 7095c0f17ad0..b7c79f68f7ca 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -792,11 +792,6 @@ struct journal_s
 	 */
 	journal_superblock_t	*j_superblock;
 
-	/**
-	 * @j_format_version: Version of the superblock format.
-	 */
-	int			j_format_version;
-
 	/**
 	 * @j_state_lock: Protect the various scalars in the journal.
 	 */
-- 
2.31.1

Re: [PATCH 5/5] jbd2: remove j_format_version

[Jan Kara]

On Fri 10-03-23 20:52:06, Zhihao Cheng wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> journal->j_format_version is no longer used, remove it.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/jbd2/journal.c    | 9 ---------
>  include/linux/jbd2.h | 5 -----
>  2 files changed, 14 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index cd94d068b4e6..fbada835b6b7 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1997,15 +1997,6 @@ static int load_superblock(journal_t *journal)
>  
>  	sb = journal->j_superblock;
>  
> -	switch (be32_to_cpu(sb->s_header.h_blocktype)) {
> -	case JBD2_SUPERBLOCK_V1:
> -		journal->j_format_version = 1;
> -		break;
> -	case JBD2_SUPERBLOCK_V2:
> -		journal->j_format_version = 2;
> -		break;
> -	}
> -
>  	journal->j_tail_sequence = be32_to_cpu(sb->s_sequence);
>  	journal->j_tail = be32_to_cpu(sb->s_start);
>  	journal->j_first = be32_to_cpu(sb->s_first);
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 7095c0f17ad0..b7c79f68f7ca 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -792,11 +792,6 @@ struct journal_s
>  	 */
>  	journal_superblock_t	*j_superblock;
>  
> -	/**
> -	 * @j_format_version: Version of the superblock format.
> -	 */
> -	int			j_format_version;
> -
>  	/**
>  	 * @j_state_lock: Protect the various scalars in the journal.
>  	 */
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR