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
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: <[email protected]>
Signed-off-by: Zhihao Cheng <[email protected]>
---
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
From: Zhang Yi <[email protected]>
JBD2_HAS_[IN|RO_]COMPAT_FEATURE macros are no longer used, just remove
them.
Signed-off-by: Zhang Yi <[email protected]>
Signed-off-by: Zhihao Cheng <[email protected]>
---
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
From: Zhang Yi <[email protected]>
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 <[email protected]>
Signed-off-by: Zhihao Cheng <[email protected]>
---
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
From: Zhang Yi <[email protected]>
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 <[email protected]>
Signed-off-by: Zhihao Cheng <[email protected]>
---
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
From: Zhang Yi <[email protected]>
journal->j_format_version is no longer used, remove it.
Signed-off-by: Zhang Yi <[email protected]>
Signed-off-by: Zhihao Cheng <[email protected]>
---
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
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: <[email protected]>
> Signed-off-by: Zhihao Cheng <[email protected]>
The fix looks good to me. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
On Fri 10-03-23 20:52:03, Zhihao Cheng wrote:
> From: Zhang Yi <[email protected]>
>
> JBD2_HAS_[IN|RO_]COMPAT_FEATURE macros are no longer used, just remove
> them.
>
> Signed-off-by: Zhang Yi <[email protected]>
> Signed-off-by: Zhihao Cheng <[email protected]>
> ---
> include/linux/jbd2.h | 11 -----------
> 1 file changed, 11 deletions(-)
Nice. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
On Fri 10-03-23 20:52:04, Zhihao Cheng wrote:
> From: Zhang Yi <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Zhihao Cheng <[email protected]>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
On Fri 10-03-23 20:52:05, Zhihao Cheng wrote:
> From: Zhang Yi <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Zhihao Cheng <[email protected]>
> ---
> 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 <[email protected]>
SUSE Labs, CR
On Fri 10-03-23 20:52:06, Zhihao Cheng wrote:
> From: Zhang Yi <[email protected]>
>
> journal->j_format_version is no longer used, remove it.
>
> Signed-off-by: Zhang Yi <[email protected]>
> Signed-off-by: Zhihao Cheng <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
Hi, Zhihao,
On 3/10/23 12:52, Zhihao Cheng wrote:
cut
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217171
> Cc: <[email protected]>
Shouldn't have been [email protected] 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
On 2023/3/13 19:12, Jan Kara wrote:
> On Fri 10-03-23 20:52:05, Zhihao Cheng wrote:
>> From: Zhang Yi <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Zhihao Cheng <[email protected]>
>> ---
>> 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.