2023-03-15 01:32:55

by Zhihao Cheng

[permalink] [raw]
Subject: [PATCH v3 0/6] ext4: Fix stale buffer loading from last failed

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

v1->v2:
Adopt suggestions from Tudor, add fix tag and corrupt 'stable' field
in patch 1.
Reserve empty lines in patch 4.
v2->v3:
Split block device checking cleanup into a new patch (2th).
Add 'Reviewed-by' tag in patch 3-6.


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 (2):
ext4: Fix reusing stale buffer heads from last failed mounting
ext4: ext4_put_super: Remove redundant checking for
'sbi->s_journal_bdev'

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

--
2.31.1



2023-03-15 01:32:58

by Zhihao Cheng

[permalink] [raw]
Subject: [PATCH v3 1/6] ext4: Fix reusing stale buffer heads from last failed mounting

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
Fixes: 25ed6e8a54df ("jbd2: enable journal clients to enable v2 checksumming")
Cc: [email protected] # v3.5
Signed-off-by: Zhihao Cheng <[email protected]>
---
fs/ext4/super.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index f43e526112ae..61511b7ba017 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;
}
@@ -1272,13 +1278,7 @@ 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.
- */
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


2023-03-15 01:33:00

by Zhihao Cheng

[permalink] [raw]
Subject: [PATCH v3 2/6] ext4: ext4_put_super: Remove redundant checking for 'sbi->s_journal_bdev'

As discussed in [1], 'sbi->s_journal_bdev != sb->s_bdev' will always
become true if sbi->s_journal_bdev exists. Filesystem block device and
journal block device are both opened with 'FMODE_EXCL' mode, so these
two devices can't be same one. Then we can remove the redundant checking
'sbi->s_journal_bdev != sb->s_bdev' if 'sbi->s_journal_bdev' exists.

[1] https://lore.kernel.org/lkml/[email protected]/

Signed-off-by: Zhihao Cheng <[email protected]>
---
fs/ext4/super.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 61511b7ba017..a22417d113ca 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1277,7 +1277,7 @@ 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) {
+ if (sbi->s_journal_bdev) {
sync_blockdev(sbi->s_journal_bdev);
ext4_blkdev_remove(sbi);
}
--
2.31.1


2023-03-15 01:33:01

by Zhihao Cheng

[permalink] [raw]
Subject: [PATCH v3 3/6] jbd2: remove unused feature macros

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]>
Reviewed-by: Jan Kara <[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 f619bae1dcc5..a91cf9c7a94b 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


2023-03-15 01:33:03

by Zhihao Cheng

[permalink] [raw]
Subject: [PATCH v3 4/6] jbd2: switch to check format version in superblock directly

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]>
Reviewed-by: Jan Kara <[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 8ae419152ff6..8d5fe6738cc4 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2062,10 +2062,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 &
@@ -2227,7 +2229,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;
@@ -2257,11 +2259,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 a91cf9c7a94b..1ffcea5c024e 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1313,11 +1313,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); \
} \
@@ -1335,7 +1346,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); \
} \
@@ -1353,7 +1364,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


2023-03-15 01:33:07

by Zhihao Cheng

[permalink] [raw]
Subject: [PATCH v3 5/6] jbd2: factor out journal initialization from journal_get_superblock()

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]>
Reviewed-by: Jan Kara <[email protected]>
---
fs/jbd2/journal.c | 46 ++++++++++++++++++++++------------------------
1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 8d5fe6738cc4..ee678f9e40c4 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1925,21 +1925,13 @@ static int journal_get_superblock(journal_t *journal)
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;
}
@@ -1982,25 +1974,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:
@@ -2025,12 +2006,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);
@@ -2226,8 +2225,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


2023-03-15 01:33:19

by Zhihao Cheng

[permalink] [raw]
Subject: [PATCH v3 6/6] jbd2: remove j_format_version

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]>
Reviewed-by: Jan Kara <[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 ee678f9e40c4..837a9a85e585 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2006,15 +2006,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 1ffcea5c024e..6990fc891612 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


2023-03-15 08:43:46

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] ext4: Fix reusing stale buffer heads from last failed mounting

On Wed 15-03-23 09:31:23, 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
> Fixes: 25ed6e8a54df ("jbd2: enable journal clients to enable v2 checksumming")
> Cc: [email protected] # v3.5
> Signed-off-by: Zhihao Cheng <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

Honza

> ---
> fs/ext4/super.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index f43e526112ae..61511b7ba017 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;
> }
> @@ -1272,13 +1278,7 @@ 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.
> - */
> 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

2023-03-15 08:44:22

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH v3 2/6] ext4: ext4_put_super: Remove redundant checking for 'sbi->s_journal_bdev'

On Wed 15-03-23 09:31:24, Zhihao Cheng wrote:
> As discussed in [1], 'sbi->s_journal_bdev != sb->s_bdev' will always
> become true if sbi->s_journal_bdev exists. Filesystem block device and
> journal block device are both opened with 'FMODE_EXCL' mode, so these
> two devices can't be same one. Then we can remove the redundant checking
> 'sbi->s_journal_bdev != sb->s_bdev' if 'sbi->s_journal_bdev' exists.
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> Signed-off-by: Zhihao Cheng <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

Honza

> ---
> fs/ext4/super.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 61511b7ba017..a22417d113ca 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1277,7 +1277,7 @@ 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) {
> + if (sbi->s_journal_bdev) {
> sync_blockdev(sbi->s_journal_bdev);
> ext4_blkdev_remove(sbi);
> }
> --
> 2.31.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR

2023-06-09 08:17:50

by Zhihao Cheng

[permalink] [raw]
Subject: Re: [PATCH v3 0/6] ext4: Fix stale buffer loading from last failed

?? 2023/3/15 9:31, 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.
>
> v1->v2:
> Adopt suggestions from Tudor, add fix tag and corrupt 'stable' field
> in patch 1.
> Reserve empty lines in patch 4.
> v2->v3:
> Split block device checking cleanup into a new patch (2th).
> Add 'Reviewed-by' tag in patch 3-6.
>
>
> 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 (2):
> ext4: Fix reusing stale buffer heads from last failed mounting
> ext4: ext4_put_super: Remove redundant checking for
> 'sbi->s_journal_bdev'
>
> fs/ext4/super.c | 15 +++++++------
> fs/jbd2/journal.c | 53 +++++++++++++++++---------------------------
> include/linux/jbd2.h | 33 ++++++++++++---------------
> 3 files changed, 42 insertions(+), 59 deletions(-)
>

Hi Ted, will this patchset be merged in next window?

2023-06-11 04:43:33

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH v3 0/6] ext4: Fix stale buffer loading from last failed

On Fri, Jun 09, 2023 at 04:04:47PM +0800, Zhihao Cheng wrote:
>
> Hi Ted, will this patchset be merged in next window?

It's currently in the dev branch. I haven't set the ack out for it
yet because I'm still seeing some test failures, including some test
hangs in my regression tests. There are a number of patches series
submission that I'm currently working through, so we'll have to see
what the "guilty" patch set might be, and whether there is an obvious
fix for it or not. (I've found one such problem that was missed by
code review[1], and unfortunately, there is at least one more issue
that I'm currently trying to pin down.)

[1] https://lore.kernel.org/r/[email protected]

Cheers,

- Ted

2023-06-15 15:24:30

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH v3 0/6] ext4: Fix stale buffer loading from last failed


On Wed, 15 Mar 2023 09:31:22 +0800, Zhihao Cheng wrote:
> Patch 1 fixes reusing stale buffer heads from last failed mounting.
> Patch 2~4 reconstructs 'j_format_version' initialization and checking
> in loading process.
>
> v1->v2:
> Adopt suggestions from Tudor, add fix tag and corrupt 'stable' field
> in patch 1.
> Reserve empty lines in patch 4.
> v2->v3:
> Split block device checking cleanup into a new patch (2th).
> Add 'Reviewed-by' tag in patch 3-6.
>
> [...]

Applied, thanks!

[1/6] ext4: Fix reusing stale buffer heads from last failed mounting
commit: ffea255f4052e6416a4b5738925337afbccd795a
[2/6] ext4: ext4_put_super: Remove redundant checking for 'sbi->s_journal_bdev'
commit: a8f17d78525adf325c80f9dd1db469d337a5ce49
[3/6] jbd2: remove unused feature macros
commit: 870a42846c1055c4ff9dfd492a0929c52a367d63
[4/6] jbd2: switch to check format version in superblock directly
commit: 6014c2204f10b1199e15ab61aa30274a14999b1d
[5/6] jbd2: factor out journal initialization from journal_get_superblock()
commit: 51bacdba23d85af2a9a145d97bfb77e6e85c98ad
[6/6] jbd2: remove j_format_version
commit: 1f15ee267c0498016cc4aee2cdcc18e56ff42bae

Best regards,
--
Theodore Ts'o <[email protected]>