[PATCH 00/12] ext4,jbd2: cleanup journal load and initialization process

[PATCH 00/12] ext4,jbd2: cleanup journal load and initialization process

[Zhang Yi]

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

Hello,

This patch set cleanup the journal load and initialization process
(discussed and suggested by Ted in [1]). Firstly, move reading of the
journal superblock from jbd2_journal_load() and jbd2_journal_wipe()
early to journal_init_common(), and completely drop the kludgy call of
journal_get_superblock() in jbd2_journal_check_used_features(). Then
cleanup the ext4_get_journal() and ext4_get_dev_journal(), making their
initialization process and error handling process more clear, and return
proper errno if some bad happens. Finally rename those two functions to
jbd2_open_{dev,inode}_journal. This patch set has passed
'kvm-xfstests -g auto'.

[1] https://lore.kernel.org/linux-ext4/20230617185057.GA343628@mit.edu/

Thanks,
Yi.

Zhang Yi (12):
  jbd2: move load_superblock() dependent functions
  jbd2: move load_superblock() into journal_init_common()
  jbd2: don't load superblock in jbd2_journal_check_used_features()
  jbd2: checking valid features early in journal_get_superblock()
  jbd2: open code jbd2_verify_csum_type() helper
  jbd2: cleanup load_superblock()
  jbd2: add fast_commit space check
  jbd2: cleanup journal_init_common()
  jbd2: drop useless error tag in jbd2_journal_wipe()
  jbd2: jbd2_journal_init_{dev,inode} return proper error return value
  ext4: cleanup ext4_get_dev_journal() and ext4_get_journal()
  ext4: ext4_get_{dev}_journal return proper error value

 fs/ext4/super.c    | 154 ++++++++-------
 fs/jbd2/journal.c  | 474 ++++++++++++++++++++++-----------------------
 fs/ocfs2/journal.c |   8 +-
 3 files changed, 308 insertions(+), 328 deletions(-)

-- 
2.39.2

[PATCH 01/12] jbd2: move load_superblock() dependent functions

[Zhang Yi]

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

Moving load_superblock() dependent functions before
journal_init_common(), just preparing for moving the call to
load_superblock() from jbd2_journal_load() and jbd2_journal_wipe() to
journal_init_common(), no functional changes.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/jbd2/journal.c | 337 +++++++++++++++++++++++-----------------------
 1 file changed, 168 insertions(+), 169 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index fbce16fedaa4..48c44c7fccf4 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1336,6 +1336,174 @@ static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
 	return count;
 }
 
+/*
+ * If the journal init or create aborts, we need to mark the journal
+ * superblock as being NULL to prevent the journal destroy from writing
+ * back a bogus superblock.
+ */
+static void journal_fail_superblock(journal_t *journal)
+{
+	struct buffer_head *bh = journal->j_sb_buffer;
+	brelse(bh);
+	journal->j_sb_buffer = NULL;
+}
+
+/*
+ * Read the superblock for a given journal, performing initial
+ * validation of the format.
+ */
+static int journal_get_superblock(journal_t *journal)
+{
+	struct buffer_head *bh;
+	journal_superblock_t *sb;
+	int err;
+
+	bh = journal->j_sb_buffer;
+
+	J_ASSERT(bh != NULL);
+	if (buffer_verified(bh))
+		return 0;
+
+	err = bh_read(bh, 0);
+	if (err < 0) {
+		printk(KERN_ERR
+			"JBD2: IO error reading journal superblock\n");
+		goto out;
+	}
+
+	sb = journal->j_superblock;
+
+	err = -EINVAL;
+
+	if (sb->s_header.h_magic != cpu_to_be32(JBD2_MAGIC_NUMBER) ||
+	    sb->s_blocksize != cpu_to_be32(journal->j_blocksize)) {
+		printk(KERN_WARNING "JBD2: no valid journal superblock found\n");
+		goto out;
+	}
+
+	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) {
+		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
+			"JBD2: Invalid start block of journal: %u\n",
+			be32_to_cpu(sb->s_first));
+		goto out;
+	}
+
+	if (jbd2_has_feature_csum2(journal) &&
+	    jbd2_has_feature_csum3(journal)) {
+		/* Can't have checksum v2 and v3 at the same time! */
+		printk(KERN_ERR "JBD2: Can't enable checksumming v2 and v3 "
+		       "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! */
+		printk(KERN_ERR "JBD2: Can't enable checksumming v1 and v2/3 "
+		       "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);
+		if (IS_ERR(journal->j_chksum_driver)) {
+			printk(KERN_ERR "JBD2: Cannot load crc32c driver.\n");
+			err = PTR_ERR(journal->j_chksum_driver);
+			journal->j_chksum_driver = NULL;
+			goto out;
+		}
+		/* Check superblock checksum */
+		if (sb->s_checksum != jbd2_superblock_csum(journal, sb)) {
+			printk(KERN_ERR "JBD2: journal checksum error\n");
+			err = -EFSBADCRC;
+			goto out;
+		}
+	}
+	set_buffer_verified(bh);
+	return 0;
+
+out:
+	journal_fail_superblock(journal);
+	return err;
+}
+
+static int journal_revoke_records_per_block(journal_t *journal)
+{
+	int record_size;
+	int space = journal->j_blocksize - sizeof(jbd2_journal_revoke_header_t);
+
+	if (jbd2_has_feature_64bit(journal))
+		record_size = 8;
+	else
+		record_size = 4;
+
+	if (jbd2_journal_has_csum_v2or3(journal))
+		space -= sizeof(struct jbd2_journal_block_tail);
+	return space / record_size;
+}
+
+/*
+ * Load the on-disk journal superblock and read the key fields into the
+ * journal_t.
+ */
+static int load_superblock(journal_t *journal)
+{
+	int err;
+	journal_superblock_t *sb;
+	int num_fc_blocks;
+
+	err = journal_get_superblock(journal);
+	if (err)
+		return err;
+
+	sb = journal->j_superblock;
+
+	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);
+		if (journal->j_last - num_fc_blocks >= JBD2_MIN_JOURNAL_BLOCKS)
+			journal->j_last = journal->j_fc_last - num_fc_blocks;
+		journal->j_fc_first = journal->j_last + 1;
+		journal->j_fc_off = 0;
+	}
+
+	return 0;
+}
+
+
 /*
  * Management for journal control blocks: functions to create and
  * destroy journal_t structures, and to initialise and read existing
@@ -1521,18 +1689,6 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
 	return journal;
 }
 
-/*
- * If the journal init or create aborts, we need to mark the journal
- * superblock as being NULL to prevent the journal destroy from writing
- * back a bogus superblock.
- */
-static void journal_fail_superblock(journal_t *journal)
-{
-	struct buffer_head *bh = journal->j_sb_buffer;
-	brelse(bh);
-	journal->j_sb_buffer = NULL;
-}
-
 /*
  * Given a journal_t structure, initialise the various fields for
  * startup of a new journaling session.  We use this both when creating
@@ -1889,163 +2045,6 @@ void jbd2_journal_update_sb_errno(journal_t *journal)
 }
 EXPORT_SYMBOL(jbd2_journal_update_sb_errno);
 
-static int journal_revoke_records_per_block(journal_t *journal)
-{
-	int record_size;
-	int space = journal->j_blocksize - sizeof(jbd2_journal_revoke_header_t);
-
-	if (jbd2_has_feature_64bit(journal))
-		record_size = 8;
-	else
-		record_size = 4;
-
-	if (jbd2_journal_has_csum_v2or3(journal))
-		space -= sizeof(struct jbd2_journal_block_tail);
-	return space / record_size;
-}
-
-/*
- * Read the superblock for a given journal, performing initial
- * validation of the format.
- */
-static int journal_get_superblock(journal_t *journal)
-{
-	struct buffer_head *bh;
-	journal_superblock_t *sb;
-	int err;
-
-	bh = journal->j_sb_buffer;
-
-	J_ASSERT(bh != NULL);
-	if (buffer_verified(bh))
-		return 0;
-
-	err = bh_read(bh, 0);
-	if (err < 0) {
-		printk(KERN_ERR
-			"JBD2: IO error reading journal superblock\n");
-		goto out;
-	}
-
-	sb = journal->j_superblock;
-
-	err = -EINVAL;
-
-	if (sb->s_header.h_magic != cpu_to_be32(JBD2_MAGIC_NUMBER) ||
-	    sb->s_blocksize != cpu_to_be32(journal->j_blocksize)) {
-		printk(KERN_WARNING "JBD2: no valid journal superblock found\n");
-		goto out;
-	}
-
-	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) {
-		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
-			"JBD2: Invalid start block of journal: %u\n",
-			be32_to_cpu(sb->s_first));
-		goto out;
-	}
-
-	if (jbd2_has_feature_csum2(journal) &&
-	    jbd2_has_feature_csum3(journal)) {
-		/* Can't have checksum v2 and v3 at the same time! */
-		printk(KERN_ERR "JBD2: Can't enable checksumming v2 and v3 "
-		       "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! */
-		printk(KERN_ERR "JBD2: Can't enable checksumming v1 and v2/3 "
-		       "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);
-		if (IS_ERR(journal->j_chksum_driver)) {
-			printk(KERN_ERR "JBD2: Cannot load crc32c driver.\n");
-			err = PTR_ERR(journal->j_chksum_driver);
-			journal->j_chksum_driver = NULL;
-			goto out;
-		}
-		/* Check superblock checksum */
-		if (sb->s_checksum != jbd2_superblock_csum(journal, sb)) {
-			printk(KERN_ERR "JBD2: journal checksum error\n");
-			err = -EFSBADCRC;
-			goto out;
-		}
-	}
-	set_buffer_verified(bh);
-	return 0;
-
-out:
-	journal_fail_superblock(journal);
-	return err;
-}
-
-/*
- * Load the on-disk journal superblock and read the key fields into the
- * journal_t.
- */
-
-static int load_superblock(journal_t *journal)
-{
-	int err;
-	journal_superblock_t *sb;
-	int num_fc_blocks;
-
-	err = journal_get_superblock(journal);
-	if (err)
-		return err;
-
-	sb = journal->j_superblock;
-
-	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);
-		if (journal->j_last - num_fc_blocks >= JBD2_MIN_JOURNAL_BLOCKS)
-			journal->j_last = journal->j_fc_last - num_fc_blocks;
-		journal->j_fc_first = journal->j_last + 1;
-		journal->j_fc_off = 0;
-	}
-
-	return 0;
-}
-
-
 /**
  * jbd2_journal_load() - Read journal from disk.
  * @journal: Journal to act on.
-- 
2.39.2

Re: [PATCH 01/12] jbd2: move load_superblock() dependent functions

[Jan Kara]

On Tue 04-07-23 21:42:22, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Moving load_superblock() dependent functions before
> journal_init_common(), just preparing for moving the call to
> load_superblock() from jbd2_journal_load() and jbd2_journal_wipe() to
> journal_init_common(), no functional changes.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

I'd just slightly rephrase the changelog:

Move load_superblock() declaration and the functions it calls before
journal_init_common(). This is a preparation for moving a call to
load_superblock() from jbd2_journal_load() and jbd2_journal_wipe() to
journal_init_common(). No functional changes.

Otherwise the patch looks good. Feel free to add:

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

								Honza

> ---
>  fs/jbd2/journal.c | 337 +++++++++++++++++++++++-----------------------
>  1 file changed, 168 insertions(+), 169 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index fbce16fedaa4..48c44c7fccf4 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1336,6 +1336,174 @@ static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
>  	return count;
>  }
>  
> +/*
> + * If the journal init or create aborts, we need to mark the journal
> + * superblock as being NULL to prevent the journal destroy from writing
> + * back a bogus superblock.
> + */
> +static void journal_fail_superblock(journal_t *journal)
> +{
> +	struct buffer_head *bh = journal->j_sb_buffer;
> +	brelse(bh);
> +	journal->j_sb_buffer = NULL;
> +}
> +
> +/*
> + * Read the superblock for a given journal, performing initial
> + * validation of the format.
> + */
> +static int journal_get_superblock(journal_t *journal)
> +{
> +	struct buffer_head *bh;
> +	journal_superblock_t *sb;
> +	int err;
> +
> +	bh = journal->j_sb_buffer;
> +
> +	J_ASSERT(bh != NULL);
> +	if (buffer_verified(bh))
> +		return 0;
> +
> +	err = bh_read(bh, 0);
> +	if (err < 0) {
> +		printk(KERN_ERR
> +			"JBD2: IO error reading journal superblock\n");
> +		goto out;
> +	}
> +
> +	sb = journal->j_superblock;
> +
> +	err = -EINVAL;
> +
> +	if (sb->s_header.h_magic != cpu_to_be32(JBD2_MAGIC_NUMBER) ||
> +	    sb->s_blocksize != cpu_to_be32(journal->j_blocksize)) {
> +		printk(KERN_WARNING "JBD2: no valid journal superblock found\n");
> +		goto out;
> +	}
> +
> +	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) {
> +		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
> +			"JBD2: Invalid start block of journal: %u\n",
> +			be32_to_cpu(sb->s_first));
> +		goto out;
> +	}
> +
> +	if (jbd2_has_feature_csum2(journal) &&
> +	    jbd2_has_feature_csum3(journal)) {
> +		/* Can't have checksum v2 and v3 at the same time! */
> +		printk(KERN_ERR "JBD2: Can't enable checksumming v2 and v3 "
> +		       "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! */
> +		printk(KERN_ERR "JBD2: Can't enable checksumming v1 and v2/3 "
> +		       "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);
> +		if (IS_ERR(journal->j_chksum_driver)) {
> +			printk(KERN_ERR "JBD2: Cannot load crc32c driver.\n");
> +			err = PTR_ERR(journal->j_chksum_driver);
> +			journal->j_chksum_driver = NULL;
> +			goto out;
> +		}
> +		/* Check superblock checksum */
> +		if (sb->s_checksum != jbd2_superblock_csum(journal, sb)) {
> +			printk(KERN_ERR "JBD2: journal checksum error\n");
> +			err = -EFSBADCRC;
> +			goto out;
> +		}
> +	}
> +	set_buffer_verified(bh);
> +	return 0;
> +
> +out:
> +	journal_fail_superblock(journal);
> +	return err;
> +}
> +
> +static int journal_revoke_records_per_block(journal_t *journal)
> +{
> +	int record_size;
> +	int space = journal->j_blocksize - sizeof(jbd2_journal_revoke_header_t);
> +
> +	if (jbd2_has_feature_64bit(journal))
> +		record_size = 8;
> +	else
> +		record_size = 4;
> +
> +	if (jbd2_journal_has_csum_v2or3(journal))
> +		space -= sizeof(struct jbd2_journal_block_tail);
> +	return space / record_size;
> +}
> +
> +/*
> + * Load the on-disk journal superblock and read the key fields into the
> + * journal_t.
> + */
> +static int load_superblock(journal_t *journal)
> +{
> +	int err;
> +	journal_superblock_t *sb;
> +	int num_fc_blocks;
> +
> +	err = journal_get_superblock(journal);
> +	if (err)
> +		return err;
> +
> +	sb = journal->j_superblock;
> +
> +	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);
> +		if (journal->j_last - num_fc_blocks >= JBD2_MIN_JOURNAL_BLOCKS)
> +			journal->j_last = journal->j_fc_last - num_fc_blocks;
> +		journal->j_fc_first = journal->j_last + 1;
> +		journal->j_fc_off = 0;
> +	}
> +
> +	return 0;
> +}
> +
> +
>  /*
>   * Management for journal control blocks: functions to create and
>   * destroy journal_t structures, and to initialise and read existing
> @@ -1521,18 +1689,6 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
>  	return journal;
>  }
>  
> -/*
> - * If the journal init or create aborts, we need to mark the journal
> - * superblock as being NULL to prevent the journal destroy from writing
> - * back a bogus superblock.
> - */
> -static void journal_fail_superblock(journal_t *journal)
> -{
> -	struct buffer_head *bh = journal->j_sb_buffer;
> -	brelse(bh);
> -	journal->j_sb_buffer = NULL;
> -}
> -
>  /*
>   * Given a journal_t structure, initialise the various fields for
>   * startup of a new journaling session.  We use this both when creating
> @@ -1889,163 +2045,6 @@ void jbd2_journal_update_sb_errno(journal_t *journal)
>  }
>  EXPORT_SYMBOL(jbd2_journal_update_sb_errno);
>  
> -static int journal_revoke_records_per_block(journal_t *journal)
> -{
> -	int record_size;
> -	int space = journal->j_blocksize - sizeof(jbd2_journal_revoke_header_t);
> -
> -	if (jbd2_has_feature_64bit(journal))
> -		record_size = 8;
> -	else
> -		record_size = 4;
> -
> -	if (jbd2_journal_has_csum_v2or3(journal))
> -		space -= sizeof(struct jbd2_journal_block_tail);
> -	return space / record_size;
> -}
> -
> -/*
> - * Read the superblock for a given journal, performing initial
> - * validation of the format.
> - */
> -static int journal_get_superblock(journal_t *journal)
> -{
> -	struct buffer_head *bh;
> -	journal_superblock_t *sb;
> -	int err;
> -
> -	bh = journal->j_sb_buffer;
> -
> -	J_ASSERT(bh != NULL);
> -	if (buffer_verified(bh))
> -		return 0;
> -
> -	err = bh_read(bh, 0);
> -	if (err < 0) {
> -		printk(KERN_ERR
> -			"JBD2: IO error reading journal superblock\n");
> -		goto out;
> -	}
> -
> -	sb = journal->j_superblock;
> -
> -	err = -EINVAL;
> -
> -	if (sb->s_header.h_magic != cpu_to_be32(JBD2_MAGIC_NUMBER) ||
> -	    sb->s_blocksize != cpu_to_be32(journal->j_blocksize)) {
> -		printk(KERN_WARNING "JBD2: no valid journal superblock found\n");
> -		goto out;
> -	}
> -
> -	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) {
> -		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
> -			"JBD2: Invalid start block of journal: %u\n",
> -			be32_to_cpu(sb->s_first));
> -		goto out;
> -	}
> -
> -	if (jbd2_has_feature_csum2(journal) &&
> -	    jbd2_has_feature_csum3(journal)) {
> -		/* Can't have checksum v2 and v3 at the same time! */
> -		printk(KERN_ERR "JBD2: Can't enable checksumming v2 and v3 "
> -		       "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! */
> -		printk(KERN_ERR "JBD2: Can't enable checksumming v1 and v2/3 "
> -		       "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);
> -		if (IS_ERR(journal->j_chksum_driver)) {
> -			printk(KERN_ERR "JBD2: Cannot load crc32c driver.\n");
> -			err = PTR_ERR(journal->j_chksum_driver);
> -			journal->j_chksum_driver = NULL;
> -			goto out;
> -		}
> -		/* Check superblock checksum */
> -		if (sb->s_checksum != jbd2_superblock_csum(journal, sb)) {
> -			printk(KERN_ERR "JBD2: journal checksum error\n");
> -			err = -EFSBADCRC;
> -			goto out;
> -		}
> -	}
> -	set_buffer_verified(bh);
> -	return 0;
> -
> -out:
> -	journal_fail_superblock(journal);
> -	return err;
> -}
> -
> -/*
> - * Load the on-disk journal superblock and read the key fields into the
> - * journal_t.
> - */
> -
> -static int load_superblock(journal_t *journal)
> -{
> -	int err;
> -	journal_superblock_t *sb;
> -	int num_fc_blocks;
> -
> -	err = journal_get_superblock(journal);
> -	if (err)
> -		return err;
> -
> -	sb = journal->j_superblock;
> -
> -	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);
> -		if (journal->j_last - num_fc_blocks >= JBD2_MIN_JOURNAL_BLOCKS)
> -			journal->j_last = journal->j_fc_last - num_fc_blocks;
> -		journal->j_fc_first = journal->j_last + 1;
> -		journal->j_fc_off = 0;
> -	}
> -
> -	return 0;
> -}
> -
> -
>  /**
>   * jbd2_journal_load() - Read journal from disk.
>   * @journal: Journal to act on.
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 02/12] jbd2: move load_superblock() into journal_init_common()

[Zhang Yi]

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

Moving the call to load_superblock() from jbd2_journal_load() and
jbd2_journal_wipe() eraly into journal_init_common(), the journal
superblock gets read and the in-memory jounal_t structure gets
initialised after calling jbd2_journal_init_{dev,inode}, it's safe to
do following initialization according to it.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/jbd2/journal.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 48c44c7fccf4..b247d374e7a6 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1582,6 +1582,10 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	journal->j_sb_buffer = bh;
 	journal->j_superblock = (journal_superblock_t *)bh->b_data;
 
+	err = load_superblock(journal);
+	if (err)
+		goto err_cleanup;
+
 	journal->j_shrink_transaction = NULL;
 	journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
 	journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
@@ -2056,13 +2060,7 @@ EXPORT_SYMBOL(jbd2_journal_update_sb_errno);
 int jbd2_journal_load(journal_t *journal)
 {
 	int err;
-	journal_superblock_t *sb;
-
-	err = load_superblock(journal);
-	if (err)
-		return err;
-
-	sb = journal->j_superblock;
+	journal_superblock_t *sb = journal->j_superblock;
 
 	/*
 	 * If this is a V2 superblock, then we have to check the
@@ -2521,10 +2519,6 @@ int jbd2_journal_wipe(journal_t *journal, int write)
 
 	J_ASSERT (!(journal->j_flags & JBD2_LOADED));
 
-	err = load_superblock(journal);
-	if (err)
-		return err;
-
 	if (!journal->j_tail)
 		goto no_recovery;
 
-- 
2.39.2

Re: [PATCH 02/12] jbd2: move load_superblock() into journal_init_common()

[Jan Kara]

On Tue 04-07-23 21:42:23, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Moving the call to load_superblock() from jbd2_journal_load() and
  ^^ Move

> jbd2_journal_wipe() eraly into journal_init_common(), the journal
                      ^^^ early

> superblock gets read and the in-memory jounal_t structure gets
					  ^^ journal_t

> initialised after calling jbd2_journal_init_{dev,inode}, it's safe to
> do following initialization according to it.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Otherwise the patch looks good to me. Feel free to add:

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

								Honza

> ---
>  fs/jbd2/journal.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 48c44c7fccf4..b247d374e7a6 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1582,6 +1582,10 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	journal->j_sb_buffer = bh;
>  	journal->j_superblock = (journal_superblock_t *)bh->b_data;
>  
> +	err = load_superblock(journal);
> +	if (err)
> +		goto err_cleanup;
> +
>  	journal->j_shrink_transaction = NULL;
>  	journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
>  	journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
> @@ -2056,13 +2060,7 @@ EXPORT_SYMBOL(jbd2_journal_update_sb_errno);
>  int jbd2_journal_load(journal_t *journal)
>  {
>  	int err;
> -	journal_superblock_t *sb;
> -
> -	err = load_superblock(journal);
> -	if (err)
> -		return err;
> -
> -	sb = journal->j_superblock;
> +	journal_superblock_t *sb = journal->j_superblock;
>  
>  	/*
>  	 * If this is a V2 superblock, then we have to check the
> @@ -2521,10 +2519,6 @@ int jbd2_journal_wipe(journal_t *journal, int write)
>  
>  	J_ASSERT (!(journal->j_flags & JBD2_LOADED));
>  
> -	err = load_superblock(journal);
> -	if (err)
> -		return err;
> -
>  	if (!journal->j_tail)
>  		goto no_recovery;
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 03/12] jbd2: don't load superblock in jbd2_journal_check_used_features()

[Zhang Yi]

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

Since load_superblock() has been moved to journal_init_common(), the
in-memory superblock structure is initialized and contains valid data
once the file system has a journal_t object, so it's safe to access it,
let's drop the call to journal_get_superblock() from
jbd2_journal_check_used_features() and also drop the setting/clearing of
the veirfy bit of the superblock buffer.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/jbd2/journal.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index b247d374e7a6..c7cdb434966f 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1361,8 +1361,6 @@ static int journal_get_superblock(journal_t *journal)
 	bh = journal->j_sb_buffer;
 
 	J_ASSERT(bh != NULL);
-	if (buffer_verified(bh))
-		return 0;
 
 	err = bh_read(bh, 0);
 	if (err < 0) {
@@ -1437,7 +1435,6 @@ static int journal_get_superblock(journal_t *journal)
 			goto out;
 		}
 	}
-	set_buffer_verified(bh);
 	return 0;
 
 out:
@@ -2224,8 +2221,6 @@ int jbd2_journal_check_used_features(journal_t *journal, unsigned long compat,
 
 	if (!compat && !ro && !incompat)
 		return 1;
-	if (journal_get_superblock(journal))
-		return 0;
 	if (!jbd2_format_support_feature(journal))
 		return 0;
 
-- 
2.39.2

Re: [PATCH 03/12] jbd2: don't load superblock in jbd2_journal_check_used_features()

[Jan Kara]

On Tue 04-07-23 21:42:24, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Since load_superblock() has been moved to journal_init_common(), the
> in-memory superblock structure is initialized and contains valid data
> once the file system has a journal_t object, so it's safe to access it,
> let's drop the call to journal_get_superblock() from
> jbd2_journal_check_used_features() and also drop the setting/clearing of
> the veirfy bit of the superblock buffer.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/jbd2/journal.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index b247d374e7a6..c7cdb434966f 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1361,8 +1361,6 @@ static int journal_get_superblock(journal_t *journal)
>  	bh = journal->j_sb_buffer;
>  
>  	J_ASSERT(bh != NULL);
> -	if (buffer_verified(bh))
> -		return 0;
>  
>  	err = bh_read(bh, 0);
>  	if (err < 0) {
> @@ -1437,7 +1435,6 @@ static int journal_get_superblock(journal_t *journal)
>  			goto out;
>  		}
>  	}
> -	set_buffer_verified(bh);
>  	return 0;
>  
>  out:
> @@ -2224,8 +2221,6 @@ int jbd2_journal_check_used_features(journal_t *journal, unsigned long compat,
>  
>  	if (!compat && !ro && !incompat)
>  		return 1;
> -	if (journal_get_superblock(journal))
> -		return 0;
>  	if (!jbd2_format_support_feature(journal))
>  		return 0;
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 04/12] jbd2: checking valid features early in journal_get_superblock()

[Zhang Yi]

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

journal_get_superblock() is used to check validity of the jounal
supberblock, so move the features checks from jbd2_journal_load() to
journal_get_superblock().

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

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index c7cdb434966f..d84f26b08315 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1398,6 +1398,21 @@ static int journal_get_superblock(journal_t *journal)
 		goto out;
 	}
 
+	/*
+	 * If this is a V2 superblock, then we have to check the
+	 * features flags on it.
+	 */
+	if (!jbd2_format_support_feature(journal))
+		return 0;
+
+	if ((sb->s_feature_ro_compat &
+			~cpu_to_be32(JBD2_KNOWN_ROCOMPAT_FEATURES)) ||
+	    (sb->s_feature_incompat &
+			~cpu_to_be32(JBD2_KNOWN_INCOMPAT_FEATURES))) {
+		printk(KERN_WARNING "JBD2: Unrecognised features on journal\n");
+		goto out;
+	}
+
 	if (jbd2_has_feature_csum2(journal) &&
 	    jbd2_has_feature_csum3(journal)) {
 		/* Can't have checksum v2 and v3 at the same time! */
@@ -2059,21 +2074,6 @@ int jbd2_journal_load(journal_t *journal)
 	int err;
 	journal_superblock_t *sb = journal->j_superblock;
 
-	/*
-	 * 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 &
-		     ~cpu_to_be32(JBD2_KNOWN_INCOMPAT_FEATURES))) {
-			printk(KERN_WARNING
-				"JBD2: Unrecognised features on journal\n");
-			return -EINVAL;
-		}
-	}
-
 	/*
 	 * Create a slab for this blocksize
 	 */
-- 
2.39.2

Re: [PATCH 04/12] jbd2: checking valid features early in journal_get_superblock()

[Jan Kara]

On Tue 04-07-23 21:42:25, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> journal_get_superblock() is used to check validity of the jounal
> supberblock, so move the features checks from jbd2_journal_load() to
> journal_get_superblock().
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/jbd2/journal.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index c7cdb434966f..d84f26b08315 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1398,6 +1398,21 @@ static int journal_get_superblock(journal_t *journal)
>  		goto out;
>  	}
>  
> +	/*
> +	 * If this is a V2 superblock, then we have to check the
> +	 * features flags on it.
> +	 */
> +	if (!jbd2_format_support_feature(journal))
> +		return 0;
> +
> +	if ((sb->s_feature_ro_compat &
> +			~cpu_to_be32(JBD2_KNOWN_ROCOMPAT_FEATURES)) ||
> +	    (sb->s_feature_incompat &
> +			~cpu_to_be32(JBD2_KNOWN_INCOMPAT_FEATURES))) {
> +		printk(KERN_WARNING "JBD2: Unrecognised features on journal\n");
> +		goto out;
> +	}
> +
>  	if (jbd2_has_feature_csum2(journal) &&
>  	    jbd2_has_feature_csum3(journal)) {
>  		/* Can't have checksum v2 and v3 at the same time! */
> @@ -2059,21 +2074,6 @@ int jbd2_journal_load(journal_t *journal)
>  	int err;
>  	journal_superblock_t *sb = journal->j_superblock;
>  
> -	/*
> -	 * 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 &
> -		     ~cpu_to_be32(JBD2_KNOWN_INCOMPAT_FEATURES))) {
> -			printk(KERN_WARNING
> -				"JBD2: Unrecognised features on journal\n");
> -			return -EINVAL;
> -		}
> -	}
> -
>  	/*
>  	 * Create a slab for this blocksize
>  	 */
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 05/12] jbd2: open code jbd2_verify_csum_type() helper

[Zhang Yi]

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

jbd2_verify_csum_type() helper check checksum type in the superblock for
v2 or v3 checksum feature, it always return true if these features are
not enabled, and it has only one user, so open code it is more clear.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/jbd2/journal.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index d84f26b08315..46ab47b4439e 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -115,14 +115,6 @@ void __jbd2_debug(int level, const char *file, const char *func,
 #endif
 
 /* Checksumming functions */
-static int jbd2_verify_csum_type(journal_t *j, journal_superblock_t *sb)
-{
-	if (!jbd2_journal_has_csum_v2or3_feature(j))
-		return 1;
-
-	return sb->s_checksum_type == JBD2_CRC32C_CHKSUM;
-}
-
 static __be32 jbd2_superblock_csum(journal_t *j, journal_superblock_t *sb)
 {
 	__u32 csum;
@@ -1429,13 +1421,13 @@ static int journal_get_superblock(journal_t *journal)
 		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)) {
+		if (sb->s_checksum_type != JBD2_CRC32C_CHKSUM) {
+			printk(KERN_ERR "JBD2: Unknown checksum type\n");
+			goto out;
+		}
+
 		journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
 		if (IS_ERR(journal->j_chksum_driver)) {
 			printk(KERN_ERR "JBD2: Cannot load crc32c driver.\n");
-- 
2.39.2

Re: [PATCH 05/12] jbd2: open code jbd2_verify_csum_type() helper

[Jan Kara]

On Tue 04-07-23 21:42:26, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> jbd2_verify_csum_type() helper check checksum type in the superblock for
> v2 or v3 checksum feature, it always return true if these features are
> not enabled, and it has only one user, so open code it is more clear.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/jbd2/journal.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index d84f26b08315..46ab47b4439e 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -115,14 +115,6 @@ void __jbd2_debug(int level, const char *file, const char *func,
>  #endif
>  
>  /* Checksumming functions */
> -static int jbd2_verify_csum_type(journal_t *j, journal_superblock_t *sb)
> -{
> -	if (!jbd2_journal_has_csum_v2or3_feature(j))
> -		return 1;
> -
> -	return sb->s_checksum_type == JBD2_CRC32C_CHKSUM;
> -}
> -
>  static __be32 jbd2_superblock_csum(journal_t *j, journal_superblock_t *sb)
>  {
>  	__u32 csum;
> @@ -1429,13 +1421,13 @@ static int journal_get_superblock(journal_t *journal)
>  		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)) {
> +		if (sb->s_checksum_type != JBD2_CRC32C_CHKSUM) {
> +			printk(KERN_ERR "JBD2: Unknown checksum type\n");
> +			goto out;
> +		}
> +
>  		journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
>  		if (IS_ERR(journal->j_chksum_driver)) {
>  			printk(KERN_ERR "JBD2: Cannot load crc32c driver.\n");
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 06/12] jbd2: cleanup load_superblock()

[Zhang Yi]

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

Rename load_superblock() to journal_load_superblock(), move getting and
reading superblock from journal_init_common() and
journal_get_superblock() to this function, and also rename
journal_get_superblock() to journal_check_superblock(), make it a pure
check helper to check superblock validity from disk.

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

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 46ab47b4439e..efdb8db3c06e 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1340,46 +1340,33 @@ static void journal_fail_superblock(journal_t *journal)
 	journal->j_sb_buffer = NULL;
 }
 
-/*
- * Read the superblock for a given journal, performing initial
+/**
+ * journal_check_superblock()
+ * @journal: journal to act on.
+ *
+ * Check the superblock for a given journal, performing initial
  * validation of the format.
  */
-static int journal_get_superblock(journal_t *journal)
+static int journal_check_superblock(journal_t *journal)
 {
-	struct buffer_head *bh;
-	journal_superblock_t *sb;
-	int err;
-
-	bh = journal->j_sb_buffer;
-
-	J_ASSERT(bh != NULL);
-
-	err = bh_read(bh, 0);
-	if (err < 0) {
-		printk(KERN_ERR
-			"JBD2: IO error reading journal superblock\n");
-		goto out;
-	}
-
-	sb = journal->j_superblock;
-
-	err = -EINVAL;
+	journal_superblock_t *sb = journal->j_superblock;
+	int err = -EINVAL;
 
 	if (sb->s_header.h_magic != cpu_to_be32(JBD2_MAGIC_NUMBER) ||
 	    sb->s_blocksize != cpu_to_be32(journal->j_blocksize)) {
 		printk(KERN_WARNING "JBD2: no valid journal superblock found\n");
-		goto out;
+		return err;
 	}
 
 	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;
+		return err;
 	}
 
 	if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) {
 		printk(KERN_WARNING "JBD2: journal file too short\n");
-		goto out;
+		return err;
 	}
 
 	if (be32_to_cpu(sb->s_first) == 0 ||
@@ -1387,7 +1374,7 @@ static int journal_get_superblock(journal_t *journal)
 		printk(KERN_WARNING
 			"JBD2: Invalid start block of journal: %u\n",
 			be32_to_cpu(sb->s_first));
-		goto out;
+		return err;
 	}
 
 	/*
@@ -1402,7 +1389,7 @@ static int journal_get_superblock(journal_t *journal)
 	    (sb->s_feature_incompat &
 			~cpu_to_be32(JBD2_KNOWN_INCOMPAT_FEATURES))) {
 		printk(KERN_WARNING "JBD2: Unrecognised features on journal\n");
-		goto out;
+		return err;
 	}
 
 	if (jbd2_has_feature_csum2(journal) &&
@@ -1410,7 +1397,7 @@ static int journal_get_superblock(journal_t *journal)
 		/* Can't have checksum v2 and v3 at the same time! */
 		printk(KERN_ERR "JBD2: Can't enable checksumming v2 and v3 "
 		       "at the same time!\n");
-		goto out;
+		return err;
 	}
 
 	if (jbd2_journal_has_csum_v2or3_feature(journal) &&
@@ -1418,14 +1405,14 @@ static int journal_get_superblock(journal_t *journal)
 		/* Can't have checksum v1 and v2 on at the same time! */
 		printk(KERN_ERR "JBD2: Can't enable checksumming v1 and v2/3 "
 		       "at the same time!\n");
-		goto out;
+		return err;
 	}
 
 	/* Load the checksum driver */
 	if (jbd2_journal_has_csum_v2or3_feature(journal)) {
 		if (sb->s_checksum_type != JBD2_CRC32C_CHKSUM) {
 			printk(KERN_ERR "JBD2: Unknown checksum type\n");
-			goto out;
+			return err;
 		}
 
 		journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
@@ -1433,20 +1420,17 @@ static int journal_get_superblock(journal_t *journal)
 			printk(KERN_ERR "JBD2: Cannot load crc32c driver.\n");
 			err = PTR_ERR(journal->j_chksum_driver);
 			journal->j_chksum_driver = NULL;
-			goto out;
+			return err;
 		}
 		/* Check superblock checksum */
 		if (sb->s_checksum != jbd2_superblock_csum(journal, sb)) {
 			printk(KERN_ERR "JBD2: journal checksum error\n");
 			err = -EFSBADCRC;
-			goto out;
+			return err;
 		}
 	}
-	return 0;
 
-out:
-	journal_fail_superblock(journal);
-	return err;
+	return 0;
 }
 
 static int journal_revoke_records_per_block(journal_t *journal)
@@ -1464,21 +1448,38 @@ static int journal_revoke_records_per_block(journal_t *journal)
 	return space / record_size;
 }
 
-/*
+/**
+ * journal_load_superblock()
+ * @journal: journal to act on.
+ *
  * Load the on-disk journal superblock and read the key fields into the
  * journal_t.
  */
-static int load_superblock(journal_t *journal)
+static int journal_load_superblock(journal_t *journal)
 {
 	int err;
+	struct buffer_head *bh;
 	journal_superblock_t *sb;
 	int num_fc_blocks;
 
-	err = journal_get_superblock(journal);
-	if (err)
-		return err;
+	bh = getblk_unmovable(journal->j_dev, journal->j_blk_offset,
+			      journal->j_blocksize);
+	if (bh)
+		err = bh_read(bh, 0);
+	if (!bh || err < 0) {
+		pr_err("%s: Cannot read journal superblock\n", __func__);
+		brelse(bh);
+		return -EIO;
+	}
 
-	sb = journal->j_superblock;
+	journal->j_sb_buffer = bh;
+	sb = (journal_superblock_t *)bh->b_data;
+	journal->j_superblock = sb;
+	err = journal_check_superblock(journal);
+	if (err) {
+		journal_fail_superblock(journal);
+		return err;
+	}
 
 	journal->j_tail_sequence = be32_to_cpu(sb->s_sequence);
 	journal->j_tail = be32_to_cpu(sb->s_start);
@@ -1524,7 +1525,6 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	static struct lock_class_key jbd2_trans_commit_key;
 	journal_t *journal;
 	int err;
-	struct buffer_head *bh;
 	int n;
 
 	journal = kzalloc(sizeof(*journal), GFP_KERNEL);
@@ -1577,16 +1577,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	if (!journal->j_wbuf)
 		goto err_cleanup;
 
-	bh = getblk_unmovable(journal->j_dev, start, journal->j_blocksize);
-	if (!bh) {
-		pr_err("%s: Cannot get buffer for journal superblock\n",
-			__func__);
-		goto err_cleanup;
-	}
-	journal->j_sb_buffer = bh;
-	journal->j_superblock = (journal_superblock_t *)bh->b_data;
-
-	err = load_superblock(journal);
+	err = journal_load_superblock(journal);
 	if (err)
 		goto err_cleanup;
 
-- 
2.39.2

Re: [PATCH 06/12] jbd2: cleanup load_superblock()

[Jan Kara]

On Tue 04-07-23 21:42:27, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Rename load_superblock() to journal_load_superblock(), move getting and
> reading superblock from journal_init_common() and
> journal_get_superblock() to this function, and also rename
> journal_get_superblock() to journal_check_superblock(), make it a pure
> check helper to check superblock validity from disk.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Two comments below:

> -/*
> - * Read the superblock for a given journal, performing initial
> +/**
> + * journal_check_superblock()
> + * @journal: journal to act on.
> + *
> + * Check the superblock for a given journal, performing initial
>   * validation of the format.
>   */

We rarely use kerneldoc style comments for local functions. In particular
in this place where there's only one user, it seems a bit superfluous. But
if you want to keep it, I'm not against it but then the proper kerneldoc
format should be used. In particular, it should be like:

/**
 * journal_check_superblock - check validity of journal superblock
 * @journal: journal to act on.
 *
 * ... description ... and include here also description of return values
 */


The same comment applies to journal_load_superblock() below.

								Honza

> -/*
> +/**
> + * journal_load_superblock()
> + * @journal: journal to act on.
> + *
>   * Load the on-disk journal superblock and read the key fields into the
>   * journal_t.
>   */
> -static int load_superblock(journal_t *journal)
> +static int journal_load_superblock(journal_t *journal)
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 07/12] jbd2: add fast_commit space check

[Zhang Yi]

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

If JBD2_FEATURE_INCOMPAT_FAST_COMMIT bit is set, it means the journal
have fast commit records need to recover, so the fast commit size
should not be zero, and also the leftover normal journal size should
never less than JBD2_MIN_JOURNAL_BLOCKS. Add a check into the
journal_check_superblock() and drop the pointless branch when
initializing in-memory fastcommit parameters.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/jbd2/journal.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index efdb8db3c06e..210b532a3673 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1392,6 +1392,18 @@ static int journal_check_superblock(journal_t *journal)
 		return err;
 	}
 
+	if (jbd2_has_feature_fast_commit(journal)) {
+		int num_fc_blks = be32_to_cpu(sb->s_num_fc_blks);
+
+		if (!num_fc_blks ||
+		    (be32_to_cpu(sb->s_maxlen) - num_fc_blks <
+		     JBD2_MIN_JOURNAL_BLOCKS)) {
+			printk(KERN_ERR "JBD2: Invalid fast commit size %d\n",
+			       num_fc_blks);
+			return err;
+		}
+	}
+
 	if (jbd2_has_feature_csum2(journal) &&
 	    jbd2_has_feature_csum3(journal)) {
 		/* Can't have checksum v2 and v3 at the same time! */
@@ -1460,7 +1472,6 @@ static int journal_load_superblock(journal_t *journal)
 	int err;
 	struct buffer_head *bh;
 	journal_superblock_t *sb;
-	int num_fc_blocks;
 
 	bh = getblk_unmovable(journal->j_dev, journal->j_blk_offset,
 			      journal->j_blocksize);
@@ -1498,9 +1509,8 @@ static int journal_load_superblock(journal_t *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);
-		if (journal->j_last - num_fc_blocks >= JBD2_MIN_JOURNAL_BLOCKS)
-			journal->j_last = journal->j_fc_last - num_fc_blocks;
+		journal->j_last = journal->j_fc_last -
+				  be32_to_cpu(sb->s_num_fc_blks);
 		journal->j_fc_first = journal->j_last + 1;
 		journal->j_fc_off = 0;
 	}
-- 
2.39.2

Re: [PATCH 07/12] jbd2: add fast_commit space check

[Jan Kara]

On Tue 04-07-23 21:42:28, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> If JBD2_FEATURE_INCOMPAT_FAST_COMMIT bit is set, it means the journal
> have fast commit records need to recover, so the fast commit size
> should not be zero, and also the leftover normal journal size should
> never less than JBD2_MIN_JOURNAL_BLOCKS. Add a check into the
> journal_check_superblock() and drop the pointless branch when
> initializing in-memory fastcommit parameters.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Some comments below.


> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index efdb8db3c06e..210b532a3673 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1392,6 +1392,18 @@ static int journal_check_superblock(journal_t *journal)
>  		return err;
>  	}
>  
> +	if (jbd2_has_feature_fast_commit(journal)) {
> +		int num_fc_blks = be32_to_cpu(sb->s_num_fc_blks);
> +
> +		if (!num_fc_blks ||
> +		    (be32_to_cpu(sb->s_maxlen) - num_fc_blks <
> +		     JBD2_MIN_JOURNAL_BLOCKS)) {
> +			printk(KERN_ERR "JBD2: Invalid fast commit size %d\n",
> +			       num_fc_blks);
> +			return err;
> +		}

This is wrong sb->s_num_fc_blks == 0 means that the fast-commit area should
have the default size of 256 blocks. At least that's how it behaves
currently and we should not change the behavior.

Similarly if the number of fastcommit blocks was too big (i.e. there was
not enough space left for ordinary journal), we effectively silently
disable fastcommit and you break this behavior in this patch.

								Honza

> +	}
> +
>  	if (jbd2_has_feature_csum2(journal) &&
>  	    jbd2_has_feature_csum3(journal)) {
>  		/* Can't have checksum v2 and v3 at the same time! */
> @@ -1460,7 +1472,6 @@ static int journal_load_superblock(journal_t *journal)
>  	int err;
>  	struct buffer_head *bh;
>  	journal_superblock_t *sb;
> -	int num_fc_blocks;
>  
>  	bh = getblk_unmovable(journal->j_dev, journal->j_blk_offset,
>  			      journal->j_blocksize);
> @@ -1498,9 +1509,8 @@ static int journal_load_superblock(journal_t *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);
> -		if (journal->j_last - num_fc_blocks >= JBD2_MIN_JOURNAL_BLOCKS)
> -			journal->j_last = journal->j_fc_last - num_fc_blocks;
> +		journal->j_last = journal->j_fc_last -
> +				  be32_to_cpu(sb->s_num_fc_blks);
>  		journal->j_fc_first = journal->j_last + 1;
>  		journal->j_fc_off = 0;
>  	}
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 07/12] jbd2: add fast_commit space check

[Zhang Yi]

On 2023/8/3 22:38, Jan Kara wrote:
> On Tue 04-07-23 21:42:28, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> If JBD2_FEATURE_INCOMPAT_FAST_COMMIT bit is set, it means the journal
>> have fast commit records need to recover, so the fast commit size
>> should not be zero, and also the leftover normal journal size should
>> never less than JBD2_MIN_JOURNAL_BLOCKS. Add a check into the
>> journal_check_superblock() and drop the pointless branch when
>> initializing in-memory fastcommit parameters.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> 
> Some comments below.
> 
> 
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index efdb8db3c06e..210b532a3673 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -1392,6 +1392,18 @@ static int journal_check_superblock(journal_t *journal)
>>  		return err;
>>  	}
>>  
>> +	if (jbd2_has_feature_fast_commit(journal)) {
>> +		int num_fc_blks = be32_to_cpu(sb->s_num_fc_blks);
>> +
>> +		if (!num_fc_blks ||
>> +		    (be32_to_cpu(sb->s_maxlen) - num_fc_blks <
>> +		     JBD2_MIN_JOURNAL_BLOCKS)) {
>> +			printk(KERN_ERR "JBD2: Invalid fast commit size %d\n",
>> +			       num_fc_blks);
>> +			return err;
>> +		}
> 
> This is wrong sb->s_num_fc_blks == 0 means that the fast-commit area should
> have the default size of 256 blocks. At least that's how it behaves
> currently and we should not change the behavior.

Thanks for the review and correcting me. I missed the fc_debug_force
mount option, this option enable fast commit feature without init
sb->s_num_fc_blks to disk, so it could left over an unclean image with
fast_commit feature but sb->s_num_fc_blks is still zero. And the mke2fs
could also set sb->s_num_fc_blks to 0.

> 
> Similarly if the number of fastcommit blocks was too big (i.e. there was
> not enough space left for ordinary journal), we effectively silently
> disable fastcommit and you break this behavior in this patch.
> 

If the fastcommit is too big, jbd2_journal_initialize_fast_commit()
will detect this corruption and refuse to mount.

[ 1213.810719] JBD2: Cannot enable fast commits.
[ 1213.812282] EXT4-fs (pmem1): Failed to set fast commit journal feature

It only silently disable fastcommit while recovering the journal, but it
doesn't seem to make much sense, because the journal->j_last is likely to
be wrong (not point to the correct end of normal journal range) and will
probably lead to incorrect recovery. It seems better to report the error
and exit as early as possible. So I suppose we could keep this "too big"
check in journal_check_superblock(). How does that sound ?

Thanks,
Yi.

> 
>> +	}
>> +
>>  	if (jbd2_has_feature_csum2(journal) &&
>>  	    jbd2_has_feature_csum3(journal)) {
>>  		/* Can't have checksum v2 and v3 at the same time! */
>> @@ -1460,7 +1472,6 @@ static int journal_load_superblock(journal_t *journal)
>>  	int err;
>>  	struct buffer_head *bh;
>>  	journal_superblock_t *sb;
>> -	int num_fc_blocks;
>>  
>>  	bh = getblk_unmovable(journal->j_dev, journal->j_blk_offset,
>>  			      journal->j_blocksize);
>> @@ -1498,9 +1509,8 @@ static int journal_load_superblock(journal_t *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);
>> -		if (journal->j_last - num_fc_blocks >= JBD2_MIN_JOURNAL_BLOCKS)
>> -			journal->j_last = journal->j_fc_last - num_fc_blocks;
>> +		journal->j_last = journal->j_fc_last -
>> +				  be32_to_cpu(sb->s_num_fc_blks);
>>  		journal->j_fc_first = journal->j_last + 1;
>>  		journal->j_fc_off = 0;
>>  	}
>> -- 
>> 2.39.2
>>

Re: [PATCH 07/12] jbd2: add fast_commit space check

[Jan Kara]

On Mon 07-08-23 18:53:09, Zhang Yi wrote:
> On 2023/8/3 22:38, Jan Kara wrote:
> > On Tue 04-07-23 21:42:28, Zhang Yi wrote:
> >> From: Zhang Yi <yi.zhang@huawei.com>
> >>
> >> If JBD2_FEATURE_INCOMPAT_FAST_COMMIT bit is set, it means the journal
> >> have fast commit records need to recover, so the fast commit size
> >> should not be zero, and also the leftover normal journal size should
> >> never less than JBD2_MIN_JOURNAL_BLOCKS. Add a check into the
> >> journal_check_superblock() and drop the pointless branch when
> >> initializing in-memory fastcommit parameters.
> >>
> >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> > 
> > Some comments below.
> > 
> > 
> >> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> >> index efdb8db3c06e..210b532a3673 100644
> >> --- a/fs/jbd2/journal.c
> >> +++ b/fs/jbd2/journal.c
> >> @@ -1392,6 +1392,18 @@ static int journal_check_superblock(journal_t *journal)
> >>  		return err;
> >>  	}
> >>  
> >> +	if (jbd2_has_feature_fast_commit(journal)) {
> >> +		int num_fc_blks = be32_to_cpu(sb->s_num_fc_blks);
> >> +
> >> +		if (!num_fc_blks ||
> >> +		    (be32_to_cpu(sb->s_maxlen) - num_fc_blks <
> >> +		     JBD2_MIN_JOURNAL_BLOCKS)) {
> >> +			printk(KERN_ERR "JBD2: Invalid fast commit size %d\n",
> >> +			       num_fc_blks);
> >> +			return err;
> >> +		}
> > 
> > This is wrong sb->s_num_fc_blks == 0 means that the fast-commit area should
> > have the default size of 256 blocks. At least that's how it behaves
> > currently and we should not change the behavior.
> 
> Thanks for the review and correcting me. I missed the fc_debug_force
> mount option, this option enable fast commit feature without init
> sb->s_num_fc_blks to disk, so it could left over an unclean image with
> fast_commit feature but sb->s_num_fc_blks is still zero. And the mke2fs
> could also set sb->s_num_fc_blks to 0.

Yes.

> > Similarly if the number of fastcommit blocks was too big (i.e. there was
> > not enough space left for ordinary journal), we effectively silently
> > disable fastcommit and you break this behavior in this patch.
> > 
> 
> If the fastcommit is too big, jbd2_journal_initialize_fast_commit()
> will detect this corruption and refuse to mount.
> 
> [ 1213.810719] JBD2: Cannot enable fast commits.
> [ 1213.812282] EXT4-fs (pmem1): Failed to set fast commit journal feature
> 
> It only silently disable fastcommit while recovering the journal, but it
> doesn't seem to make much sense, because the journal->j_last is likely to
> be wrong (not point to the correct end of normal journal range) and will
> probably lead to incorrect recovery. It seems better to report the error
> and exit as early as possible. So I suppose we could keep this "too big"
> check in journal_check_superblock(). How does that sound ?

Ah, you are right. So let's keep the "space for journal too small" check as
you suggest.

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

[PATCH 08/12] jbd2: cleanup journal_init_common()

[Zhang Yi]

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

Adjust the initialization sequence and error handle of journal_t, moving
load superblock to the begin, and classify others initialization.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/jbd2/journal.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 210b532a3673..065b5e789299 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1541,6 +1541,16 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	if (!journal)
 		return NULL;
 
+	journal->j_blocksize = blocksize;
+	journal->j_dev = bdev;
+	journal->j_fs_dev = fs_dev;
+	journal->j_blk_offset = start;
+	journal->j_total_len = len;
+
+	err = journal_load_superblock(journal);
+	if (err)
+		goto err_cleanup;
+
 	init_waitqueue_head(&journal->j_wait_transaction_locked);
 	init_waitqueue_head(&journal->j_wait_done_commit);
 	init_waitqueue_head(&journal->j_wait_commit);
@@ -1552,12 +1562,15 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	mutex_init(&journal->j_checkpoint_mutex);
 	spin_lock_init(&journal->j_revoke_lock);
 	spin_lock_init(&journal->j_list_lock);
+	spin_lock_init(&journal->j_history_lock);
 	rwlock_init(&journal->j_state_lock);
 
 	journal->j_commit_interval = (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE);
 	journal->j_min_batch_time = 0;
 	journal->j_max_batch_time = 15000; /* 15ms */
 	atomic_set(&journal->j_reserved_credits, 0);
+	lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle",
+			 &jbd2_trans_commit_key, 0);
 
 	/* The journal is marked for error until we succeed with recovery! */
 	journal->j_flags = JBD2_ABORT;
@@ -1567,18 +1580,10 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	if (err)
 		goto err_cleanup;
 
-	spin_lock_init(&journal->j_history_lock);
-
-	lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle",
-			 &jbd2_trans_commit_key, 0);
-
-	/* journal descriptor can store up to n blocks -bzzz */
-	journal->j_blocksize = blocksize;
-	journal->j_dev = bdev;
-	journal->j_fs_dev = fs_dev;
-	journal->j_blk_offset = start;
-	journal->j_total_len = len;
-	/* We need enough buffers to write out full descriptor block. */
+	/*
+	 * journal descriptor can store up to n blocks, we need enough
+	 * buffers to write out full descriptor block.
+	 */
 	n = journal->j_blocksize / jbd2_min_tag_size();
 	journal->j_wbufsize = n;
 	journal->j_fc_wbuf = NULL;
@@ -1587,7 +1592,8 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	if (!journal->j_wbuf)
 		goto err_cleanup;
 
-	err = journal_load_superblock(journal);
+	err = percpu_counter_init(&journal->j_checkpoint_jh_count, 0,
+				  GFP_KERNEL);
 	if (err)
 		goto err_cleanup;
 
@@ -1596,21 +1602,18 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
 	journal->j_shrinker.seeks = DEFAULT_SEEKS;
 	journal->j_shrinker.batch = journal->j_max_transaction_buffers;
-
-	if (percpu_counter_init(&journal->j_checkpoint_jh_count, 0, GFP_KERNEL))
+	err = register_shrinker(&journal->j_shrinker, "jbd2-journal:(%u:%u)",
+				MAJOR(bdev->bd_dev), MINOR(bdev->bd_dev));
+	if (err)
 		goto err_cleanup;
 
-	if (register_shrinker(&journal->j_shrinker, "jbd2-journal:(%u:%u)",
-			      MAJOR(bdev->bd_dev), MINOR(bdev->bd_dev))) {
-		percpu_counter_destroy(&journal->j_checkpoint_jh_count);
-		goto err_cleanup;
-	}
 	return journal;
 
 err_cleanup:
-	brelse(journal->j_sb_buffer);
+	percpu_counter_destroy(&journal->j_checkpoint_jh_count);
 	kfree(journal->j_wbuf);
 	jbd2_journal_destroy_revoke(journal);
+	journal_fail_superblock(journal);
 	kfree(journal);
 	return NULL;
 }
-- 
2.39.2

Re: [PATCH 08/12] jbd2: cleanup journal_init_common()

[Jan Kara]

On Tue 04-07-23 21:42:29, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Adjust the initialization sequence and error handle of journal_t, moving
> load superblock to the begin, and classify others initialization.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/jbd2/journal.c | 45 ++++++++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 210b532a3673..065b5e789299 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1541,6 +1541,16 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	if (!journal)
>  		return NULL;
>  
> +	journal->j_blocksize = blocksize;
> +	journal->j_dev = bdev;
> +	journal->j_fs_dev = fs_dev;
> +	journal->j_blk_offset = start;
> +	journal->j_total_len = len;
> +
> +	err = journal_load_superblock(journal);
> +	if (err)
> +		goto err_cleanup;
> +
>  	init_waitqueue_head(&journal->j_wait_transaction_locked);
>  	init_waitqueue_head(&journal->j_wait_done_commit);
>  	init_waitqueue_head(&journal->j_wait_commit);
> @@ -1552,12 +1562,15 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	mutex_init(&journal->j_checkpoint_mutex);
>  	spin_lock_init(&journal->j_revoke_lock);
>  	spin_lock_init(&journal->j_list_lock);
> +	spin_lock_init(&journal->j_history_lock);
>  	rwlock_init(&journal->j_state_lock);
>  
>  	journal->j_commit_interval = (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE);
>  	journal->j_min_batch_time = 0;
>  	journal->j_max_batch_time = 15000; /* 15ms */
>  	atomic_set(&journal->j_reserved_credits, 0);
> +	lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle",
> +			 &jbd2_trans_commit_key, 0);
>  
>  	/* The journal is marked for error until we succeed with recovery! */
>  	journal->j_flags = JBD2_ABORT;
> @@ -1567,18 +1580,10 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	if (err)
>  		goto err_cleanup;
>  
> -	spin_lock_init(&journal->j_history_lock);
> -
> -	lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle",
> -			 &jbd2_trans_commit_key, 0);
> -
> -	/* journal descriptor can store up to n blocks -bzzz */
> -	journal->j_blocksize = blocksize;
> -	journal->j_dev = bdev;
> -	journal->j_fs_dev = fs_dev;
> -	journal->j_blk_offset = start;
> -	journal->j_total_len = len;
> -	/* We need enough buffers to write out full descriptor block. */
> +	/*
> +	 * journal descriptor can store up to n blocks, we need enough
> +	 * buffers to write out full descriptor block.
> +	 */
>  	n = journal->j_blocksize / jbd2_min_tag_size();
>  	journal->j_wbufsize = n;
>  	journal->j_fc_wbuf = NULL;
> @@ -1587,7 +1592,8 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	if (!journal->j_wbuf)
>  		goto err_cleanup;
>  
> -	err = journal_load_superblock(journal);
> +	err = percpu_counter_init(&journal->j_checkpoint_jh_count, 0,
> +				  GFP_KERNEL);
>  	if (err)
>  		goto err_cleanup;
>  
> @@ -1596,21 +1602,18 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
>  	journal->j_shrinker.seeks = DEFAULT_SEEKS;
>  	journal->j_shrinker.batch = journal->j_max_transaction_buffers;
> -
> -	if (percpu_counter_init(&journal->j_checkpoint_jh_count, 0, GFP_KERNEL))
> +	err = register_shrinker(&journal->j_shrinker, "jbd2-journal:(%u:%u)",
> +				MAJOR(bdev->bd_dev), MINOR(bdev->bd_dev));
> +	if (err)
>  		goto err_cleanup;
>  
> -	if (register_shrinker(&journal->j_shrinker, "jbd2-journal:(%u:%u)",
> -			      MAJOR(bdev->bd_dev), MINOR(bdev->bd_dev))) {
> -		percpu_counter_destroy(&journal->j_checkpoint_jh_count);
> -		goto err_cleanup;
> -	}
>  	return journal;
>  
>  err_cleanup:
> -	brelse(journal->j_sb_buffer);
> +	percpu_counter_destroy(&journal->j_checkpoint_jh_count);
>  	kfree(journal->j_wbuf);
>  	jbd2_journal_destroy_revoke(journal);
> +	journal_fail_superblock(journal);
>  	kfree(journal);
>  	return NULL;
>  }
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 09/12] jbd2: drop useless error tag in jbd2_journal_wipe()

[Zhang Yi]

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

no_recovery is redundant, just drop it.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/jbd2/journal.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 065b5e789299..cc344b8d7476 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2506,12 +2506,12 @@ int jbd2_journal_flush(journal_t *journal, unsigned int flags)
 
 int jbd2_journal_wipe(journal_t *journal, int write)
 {
-	int err = 0;
+	int err;
 
 	J_ASSERT (!(journal->j_flags & JBD2_LOADED));
 
 	if (!journal->j_tail)
-		goto no_recovery;
+		return 0;
 
 	printk(KERN_WARNING "JBD2: %s recovery information on journal\n",
 		write ? "Clearing" : "Ignoring");
@@ -2524,7 +2524,6 @@ int jbd2_journal_wipe(journal_t *journal, int write)
 		mutex_unlock(&journal->j_checkpoint_mutex);
 	}
 
- no_recovery:
 	return err;
 }
 
-- 
2.39.2

Re: [PATCH 09/12] jbd2: drop useless error tag in jbd2_journal_wipe()

[Jan Kara]

On Tue 04-07-23 21:42:30, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> no_recovery is redundant, just drop it.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/jbd2/journal.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 065b5e789299..cc344b8d7476 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -2506,12 +2506,12 @@ int jbd2_journal_flush(journal_t *journal, unsigned int flags)
>  
>  int jbd2_journal_wipe(journal_t *journal, int write)
>  {
> -	int err = 0;
> +	int err;
>  
>  	J_ASSERT (!(journal->j_flags & JBD2_LOADED));
>  
>  	if (!journal->j_tail)
> -		goto no_recovery;
> +		return 0;
>  
>  	printk(KERN_WARNING "JBD2: %s recovery information on journal\n",
>  		write ? "Clearing" : "Ignoring");
> @@ -2524,7 +2524,6 @@ int jbd2_journal_wipe(journal_t *journal, int write)
>  		mutex_unlock(&journal->j_checkpoint_mutex);
>  	}
>  
> - no_recovery:
>  	return err;
>  }
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 10/12] jbd2: jbd2_journal_init_{dev,inode} return proper error return value

[Zhang Yi]

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

Current jbd2_journal_init_{dev,inode} return NULL if some error
happens, make them to pass out proper error return value.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/super.c    |  4 ++--
 fs/jbd2/journal.c  | 19 +++++++++----------
 fs/ocfs2/journal.c |  8 ++++----
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c94ebf704616..ce2e02b139af 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5827,7 +5827,7 @@ static journal_t *ext4_get_journal(struct super_block *sb,
 		return NULL;
 
 	journal = jbd2_journal_init_inode(journal_inode);
-	if (!journal) {
+	if (IS_ERR(journal)) {
 		ext4_msg(sb, KERN_ERR, "Could not load journal inode");
 		iput(journal_inode);
 		return NULL;
@@ -5906,7 +5906,7 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
 
 	journal = jbd2_journal_init_dev(bdev, sb->s_bdev,
 					start, len, blocksize);
-	if (!journal) {
+	if (IS_ERR(journal)) {
 		ext4_msg(sb, KERN_ERR, "failed to create device journal");
 		goto out_bdev;
 	}
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index cc344b8d7476..34dd65aa9f61 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1539,7 +1539,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
 
 	journal = kzalloc(sizeof(*journal), GFP_KERNEL);
 	if (!journal)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	journal->j_blocksize = blocksize;
 	journal->j_dev = bdev;
@@ -1584,6 +1584,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	 * journal descriptor can store up to n blocks, we need enough
 	 * buffers to write out full descriptor block.
 	 */
+	err = -ENOMEM;
 	n = journal->j_blocksize / jbd2_min_tag_size();
 	journal->j_wbufsize = n;
 	journal->j_fc_wbuf = NULL;
@@ -1615,7 +1616,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	jbd2_journal_destroy_revoke(journal);
 	journal_fail_superblock(journal);
 	kfree(journal);
-	return NULL;
+	return ERR_PTR(err);
 }
 
 /* jbd2_journal_init_dev and jbd2_journal_init_inode:
@@ -1648,8 +1649,8 @@ journal_t *jbd2_journal_init_dev(struct block_device *bdev,
 	journal_t *journal;
 
 	journal = journal_init_common(bdev, fs_dev, start, len, blocksize);
-	if (!journal)
-		return NULL;
+	if (IS_ERR(journal))
+		return ERR_CAST(journal);
 
 	snprintf(journal->j_devname, sizeof(journal->j_devname),
 		 "%pg", journal->j_dev);
@@ -1675,11 +1676,9 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
 
 	blocknr = 0;
 	err = bmap(inode, &blocknr);
-
 	if (err || !blocknr) {
-		pr_err("%s: Cannot locate journal superblock\n",
-			__func__);
-		return NULL;
+		pr_err("%s: Cannot locate journal superblock\n", __func__);
+		return err ? ERR_PTR(err) : ERR_PTR(-EINVAL);
 	}
 
 	jbd2_debug(1, "JBD2: inode %s/%ld, size %lld, bits %d, blksize %ld\n",
@@ -1689,8 +1688,8 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
 	journal = journal_init_common(inode->i_sb->s_bdev, inode->i_sb->s_bdev,
 			blocknr, inode->i_size >> inode->i_sb->s_blocksize_bits,
 			inode->i_sb->s_blocksize);
-	if (!journal)
-		return NULL;
+	if (IS_ERR(journal))
+		return ERR_CAST(journal);
 
 	journal->j_inode = inode;
 	snprintf(journal->j_devname, sizeof(journal->j_devname),
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index 25d8072ccfce..f35a1bbf52e2 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -911,9 +911,9 @@ int ocfs2_journal_init(struct ocfs2_super *osb, int *dirty)
 
 	/* call the kernels journal init function now */
 	j_journal = jbd2_journal_init_inode(inode);
-	if (j_journal == NULL) {
+	if (IS_ERR(j_journal)) {
 		mlog(ML_ERROR, "Linux journal layer error\n");
-		status = -EINVAL;
+		status = PTR_ERR(journal);
 		goto done;
 	}
 
@@ -1687,9 +1687,9 @@ static int ocfs2_replay_journal(struct ocfs2_super *osb,
 	}
 
 	journal = jbd2_journal_init_inode(inode);
-	if (journal == NULL) {
+	if (IS_ERR(journal)) {
 		mlog(ML_ERROR, "Linux journal layer error\n");
-		status = -EIO;
+		status = PTR_ERR(journal);
 		goto done;
 	}
 
-- 
2.39.2

Re: [PATCH 10/12] jbd2: jbd2_journal_init_{dev,inode} return proper error return value

[Jan Kara]

On Tue 04-07-23 21:42:31, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Current jbd2_journal_init_{dev,inode} return NULL if some error
> happens, make them to pass out proper error return value.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/super.c    |  4 ++--
>  fs/jbd2/journal.c  | 19 +++++++++----------
>  fs/ocfs2/journal.c |  8 ++++----
>  3 files changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c94ebf704616..ce2e02b139af 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5827,7 +5827,7 @@ static journal_t *ext4_get_journal(struct super_block *sb,
>  		return NULL;
>  
>  	journal = jbd2_journal_init_inode(journal_inode);
> -	if (!journal) {
> +	if (IS_ERR(journal)) {
>  		ext4_msg(sb, KERN_ERR, "Could not load journal inode");
>  		iput(journal_inode);
>  		return NULL;
> @@ -5906,7 +5906,7 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
>  
>  	journal = jbd2_journal_init_dev(bdev, sb->s_bdev,
>  					start, len, blocksize);
> -	if (!journal) {
> +	if (IS_ERR(journal)) {
>  		ext4_msg(sb, KERN_ERR, "failed to create device journal");
>  		goto out_bdev;
>  	}
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index cc344b8d7476..34dd65aa9f61 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1539,7 +1539,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  
>  	journal = kzalloc(sizeof(*journal), GFP_KERNEL);
>  	if (!journal)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	journal->j_blocksize = blocksize;
>  	journal->j_dev = bdev;
> @@ -1584,6 +1584,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	 * journal descriptor can store up to n blocks, we need enough
>  	 * buffers to write out full descriptor block.
>  	 */
> +	err = -ENOMEM;
>  	n = journal->j_blocksize / jbd2_min_tag_size();
>  	journal->j_wbufsize = n;
>  	journal->j_fc_wbuf = NULL;
> @@ -1615,7 +1616,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	jbd2_journal_destroy_revoke(journal);
>  	journal_fail_superblock(journal);
>  	kfree(journal);
> -	return NULL;
> +	return ERR_PTR(err);
>  }
>  
>  /* jbd2_journal_init_dev and jbd2_journal_init_inode:
> @@ -1648,8 +1649,8 @@ journal_t *jbd2_journal_init_dev(struct block_device *bdev,
>  	journal_t *journal;
>  
>  	journal = journal_init_common(bdev, fs_dev, start, len, blocksize);
> -	if (!journal)
> -		return NULL;
> +	if (IS_ERR(journal))
> +		return ERR_CAST(journal);
>  
>  	snprintf(journal->j_devname, sizeof(journal->j_devname),
>  		 "%pg", journal->j_dev);
> @@ -1675,11 +1676,9 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
>  
>  	blocknr = 0;
>  	err = bmap(inode, &blocknr);
> -
>  	if (err || !blocknr) {
> -		pr_err("%s: Cannot locate journal superblock\n",
> -			__func__);
> -		return NULL;
> +		pr_err("%s: Cannot locate journal superblock\n", __func__);
> +		return err ? ERR_PTR(err) : ERR_PTR(-EINVAL);
>  	}
>  
>  	jbd2_debug(1, "JBD2: inode %s/%ld, size %lld, bits %d, blksize %ld\n",
> @@ -1689,8 +1688,8 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
>  	journal = journal_init_common(inode->i_sb->s_bdev, inode->i_sb->s_bdev,
>  			blocknr, inode->i_size >> inode->i_sb->s_blocksize_bits,
>  			inode->i_sb->s_blocksize);
> -	if (!journal)
> -		return NULL;
> +	if (IS_ERR(journal))
> +		return ERR_CAST(journal);
>  
>  	journal->j_inode = inode;
>  	snprintf(journal->j_devname, sizeof(journal->j_devname),
> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> index 25d8072ccfce..f35a1bbf52e2 100644
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -911,9 +911,9 @@ int ocfs2_journal_init(struct ocfs2_super *osb, int *dirty)
>  
>  	/* call the kernels journal init function now */
>  	j_journal = jbd2_journal_init_inode(inode);
> -	if (j_journal == NULL) {
> +	if (IS_ERR(j_journal)) {
>  		mlog(ML_ERROR, "Linux journal layer error\n");
> -		status = -EINVAL;
> +		status = PTR_ERR(journal);
>  		goto done;
>  	}
>  
> @@ -1687,9 +1687,9 @@ static int ocfs2_replay_journal(struct ocfs2_super *osb,
>  	}
>  
>  	journal = jbd2_journal_init_inode(inode);
> -	if (journal == NULL) {
> +	if (IS_ERR(journal)) {
>  		mlog(ML_ERROR, "Linux journal layer error\n");
> -		status = -EIO;
> +		status = PTR_ERR(journal);
>  		goto done;
>  	}
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 11/12] ext4: cleanup ext4_get_dev_journal() and ext4_get_journal()

[Zhang Yi]

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

Factor out a new helper form ext4_get_dev_journal() to get external
journal bdev and check validation of this device, drop ext4_blkdev_get()
helper, and also remove duplicate check of journal feature. It makes
ext4_get_dev_journal() more clear than before.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/super.c | 109 ++++++++++++++++++++++--------------------------
 1 file changed, 49 insertions(+), 60 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ce2e02b139af..25ae536a370f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1105,26 +1105,6 @@ static const struct blk_holder_ops ext4_holder_ops = {
 	.mark_dead		= ext4_bdev_mark_dead,
 };
 
-/*
- * Open the external journal device
- */
-static struct block_device *ext4_blkdev_get(dev_t dev, struct super_block *sb)
-{
-	struct block_device *bdev;
-
-	bdev = blkdev_get_by_dev(dev, BLK_OPEN_READ | BLK_OPEN_WRITE, sb,
-				 &ext4_holder_ops);
-	if (IS_ERR(bdev))
-		goto fail;
-	return bdev;
-
-fail:
-	ext4_msg(sb, KERN_ERR,
-		 "failed to open journal device unknown-block(%u,%u) %ld",
-		 MAJOR(dev), MINOR(dev), PTR_ERR(bdev));
-	return NULL;
-}
-
 /*
  * Release the journal device
  */
@@ -5780,14 +5760,14 @@ static struct inode *ext4_get_journal_inode(struct super_block *sb,
 		ext4_msg(sb, KERN_ERR, "journal inode is deleted");
 		return NULL;
 	}
-
-	ext4_debug("Journal inode found at %p: %lld bytes\n",
-		  journal_inode, journal_inode->i_size);
 	if (!S_ISREG(journal_inode->i_mode) || IS_ENCRYPTED(journal_inode)) {
 		ext4_msg(sb, KERN_ERR, "invalid journal inode");
 		iput(journal_inode);
 		return NULL;
 	}
+
+	ext4_debug("Journal inode found at %p: %lld bytes\n",
+		  journal_inode, journal_inode->i_size);
 	return journal_inode;
 }
 
@@ -5819,9 +5799,6 @@ static journal_t *ext4_get_journal(struct super_block *sb,
 	struct inode *journal_inode;
 	journal_t *journal;
 
-	if (WARN_ON_ONCE(!ext4_has_feature_journal(sb)))
-		return NULL;
-
 	journal_inode = ext4_get_journal_inode(sb, journal_inum);
 	if (!journal_inode)
 		return NULL;
@@ -5838,25 +5815,25 @@ static journal_t *ext4_get_journal(struct super_block *sb,
 	return journal;
 }
 
-static journal_t *ext4_get_dev_journal(struct super_block *sb,
-				       dev_t j_dev)
+static struct block_device *ext4_get_journal_dev(struct super_block *sb,
+					dev_t j_dev, ext4_fsblk_t *j_start,
+					ext4_fsblk_t *j_len)
 {
 	struct buffer_head *bh;
-	journal_t *journal;
-	ext4_fsblk_t start;
-	ext4_fsblk_t len;
+	struct block_device *bdev;
 	int hblock, blocksize;
 	ext4_fsblk_t sb_block;
 	unsigned long offset;
 	struct ext4_super_block *es;
-	struct block_device *bdev;
 
-	if (WARN_ON_ONCE(!ext4_has_feature_journal(sb)))
-		return NULL;
-
-	bdev = ext4_blkdev_get(j_dev, sb);
-	if (bdev == NULL)
+	bdev = blkdev_get_by_dev(j_dev, BLK_OPEN_READ | BLK_OPEN_WRITE, sb,
+				 &ext4_holder_ops);
+	if (IS_ERR(bdev)) {
+		ext4_msg(sb, KERN_ERR,
+			 "failed to open journal device unknown-block(%u,%u) %ld",
+			 MAJOR(j_dev), MINOR(j_dev), PTR_ERR(bdev));
 		return NULL;
+	}
 
 	blocksize = sb->s_blocksize;
 	hblock = bdev_logical_block_size(bdev);
@@ -5869,7 +5846,8 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
 	sb_block = EXT4_MIN_BLOCK_SIZE / blocksize;
 	offset = EXT4_MIN_BLOCK_SIZE % blocksize;
 	set_blocksize(bdev, blocksize);
-	if (!(bh = __bread(bdev, sb_block, blocksize))) {
+	bh = __bread(bdev, sb_block, blocksize);
+	if (!bh) {
 		ext4_msg(sb, KERN_ERR, "couldn't read superblock of "
 		       "external journal");
 		goto out_bdev;
@@ -5879,56 +5857,67 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
 	if ((le16_to_cpu(es->s_magic) != EXT4_SUPER_MAGIC) ||
 	    !(le32_to_cpu(es->s_feature_incompat) &
 	      EXT4_FEATURE_INCOMPAT_JOURNAL_DEV)) {
-		ext4_msg(sb, KERN_ERR, "external journal has "
-					"bad superblock");
-		brelse(bh);
-		goto out_bdev;
+		ext4_msg(sb, KERN_ERR, "external journal has bad superblock");
+		goto out_bh;
 	}
 
 	if ((le32_to_cpu(es->s_feature_ro_compat) &
 	     EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
 	    es->s_checksum != ext4_superblock_csum(sb, es)) {
-		ext4_msg(sb, KERN_ERR, "external journal has "
-				       "corrupt superblock");
-		brelse(bh);
-		goto out_bdev;
+		ext4_msg(sb, KERN_ERR, "external journal has corrupt superblock");
+		goto out_bh;
 	}
 
 	if (memcmp(EXT4_SB(sb)->s_es->s_journal_uuid, es->s_uuid, 16)) {
 		ext4_msg(sb, KERN_ERR, "journal UUID does not match");
-		brelse(bh);
-		goto out_bdev;
+		goto out_bh;
 	}
 
-	len = ext4_blocks_count(es);
-	start = sb_block + 1;
-	brelse(bh);	/* we're done with the superblock */
+	brelse(bh);
+	*j_start = sb_block + 1;
+	*j_len = ext4_blocks_count(es);
+	return bdev;
+
+out_bh:
+	brelse(bh);
+out_bdev:
+	blkdev_put(bdev, sb);
+	return NULL;
+}
+
+static journal_t *ext4_get_dev_journal(struct super_block *sb,
+				       dev_t j_dev)
+{
+	journal_t *journal;
+	ext4_fsblk_t j_start;
+	ext4_fsblk_t j_len;
+	struct block_device *journal_bdev;
+
+	journal_bdev = ext4_get_journal_dev(sb, j_dev, &j_start, &j_len);
+	if (!journal_bdev)
+		return NULL;
 
-	journal = jbd2_journal_init_dev(bdev, sb->s_bdev,
-					start, len, blocksize);
+	journal = jbd2_journal_init_dev(journal_bdev, sb->s_bdev, j_start,
+					j_len, sb->s_blocksize);
 	if (IS_ERR(journal)) {
 		ext4_msg(sb, KERN_ERR, "failed to create device journal");
 		goto out_bdev;
 	}
-	journal->j_private = sb;
-	if (ext4_read_bh_lock(journal->j_sb_buffer, REQ_META | REQ_PRIO, true)) {
-		ext4_msg(sb, KERN_ERR, "I/O error on journal device");
-		goto out_journal;
-	}
 	if (be32_to_cpu(journal->j_superblock->s_nr_users) != 1) {
 		ext4_msg(sb, KERN_ERR, "External journal has more than one "
 					"user (unsupported) - %d",
 			be32_to_cpu(journal->j_superblock->s_nr_users));
 		goto out_journal;
 	}
-	EXT4_SB(sb)->s_journal_bdev = bdev;
+	journal->j_private = sb;
+	EXT4_SB(sb)->s_journal_bdev = journal_bdev;
 	ext4_init_journal_params(sb, journal);
 	return journal;
 
 out_journal:
 	jbd2_journal_destroy(journal);
 out_bdev:
-	blkdev_put(bdev, sb);
+	blkdev_put(journal_bdev, sb);
 	return NULL;
 }
 
-- 
2.39.2

Re: [PATCH 11/12] ext4: cleanup ext4_get_dev_journal() and ext4_get_journal()

[Jan Kara]

On Tue 04-07-23 21:42:32, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Factor out a new helper form ext4_get_dev_journal() to get external
> journal bdev and check validation of this device, drop ext4_blkdev_get()
> helper, and also remove duplicate check of journal feature. It makes
> ext4_get_dev_journal() more clear than before.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

One comment below:

> @@ -5838,25 +5815,25 @@ static journal_t *ext4_get_journal(struct super_block *sb,
>  	return journal;
>  }
>  
> -static journal_t *ext4_get_dev_journal(struct super_block *sb,
> -				       dev_t j_dev)
> +static struct block_device *ext4_get_journal_dev(struct super_block *sb,
> +					dev_t j_dev, ext4_fsblk_t *j_start,
> +					ext4_fsblk_t *j_len)
>  {
>  	struct buffer_head *bh;
> -	journal_t *journal;
> -	ext4_fsblk_t start;
> -	ext4_fsblk_t len;
> +	struct block_device *bdev;
>  	int hblock, blocksize;
>  	ext4_fsblk_t sb_block;
>  	unsigned long offset;
>  	struct ext4_super_block *es;
> -	struct block_device *bdev;
>  
> -	if (WARN_ON_ONCE(!ext4_has_feature_journal(sb)))
> -		return NULL;
> -
> -	bdev = ext4_blkdev_get(j_dev, sb);
> -	if (bdev == NULL)
> +	bdev = blkdev_get_by_dev(j_dev, BLK_OPEN_READ | BLK_OPEN_WRITE, sb,
> +				 &ext4_holder_ops);
> +	if (IS_ERR(bdev)) {
> +		ext4_msg(sb, KERN_ERR,
> +			 "failed to open journal device unknown-block(%u,%u) %ld",
> +			 MAJOR(j_dev), MINOR(j_dev), PTR_ERR(bdev));
>  		return NULL;
> +	}
>  
>  	blocksize = sb->s_blocksize;
>  	hblock = bdev_logical_block_size(bdev);
> @@ -5869,7 +5846,8 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
>  	sb_block = EXT4_MIN_BLOCK_SIZE / blocksize;
>  	offset = EXT4_MIN_BLOCK_SIZE % blocksize;
>  	set_blocksize(bdev, blocksize);
> -	if (!(bh = __bread(bdev, sb_block, blocksize))) {
> +	bh = __bread(bdev, sb_block, blocksize);
> +	if (!bh) {
>  		ext4_msg(sb, KERN_ERR, "couldn't read superblock of "
>  		       "external journal");
>  		goto out_bdev;
> @@ -5879,56 +5857,67 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
>  	if ((le16_to_cpu(es->s_magic) != EXT4_SUPER_MAGIC) ||
>  	    !(le32_to_cpu(es->s_feature_incompat) &
>  	      EXT4_FEATURE_INCOMPAT_JOURNAL_DEV)) {
> -		ext4_msg(sb, KERN_ERR, "external journal has "
> -					"bad superblock");
> -		brelse(bh);
> -		goto out_bdev;
> +		ext4_msg(sb, KERN_ERR, "external journal has bad superblock");
> +		goto out_bh;
>  	}
>  
>  	if ((le32_to_cpu(es->s_feature_ro_compat) &
>  	     EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
>  	    es->s_checksum != ext4_superblock_csum(sb, es)) {
> -		ext4_msg(sb, KERN_ERR, "external journal has "
> -				       "corrupt superblock");
> -		brelse(bh);
> -		goto out_bdev;
> +		ext4_msg(sb, KERN_ERR, "external journal has corrupt superblock");
> +		goto out_bh;
>  	}
>  
>  	if (memcmp(EXT4_SB(sb)->s_es->s_journal_uuid, es->s_uuid, 16)) {
>  		ext4_msg(sb, KERN_ERR, "journal UUID does not match");
> -		brelse(bh);
> -		goto out_bdev;
> +		goto out_bh;
>  	}
>  
> -	len = ext4_blocks_count(es);
> -	start = sb_block + 1;
> -	brelse(bh);	/* we're done with the superblock */
> +	brelse(bh);
> +	*j_start = sb_block + 1;
> +	*j_len = ext4_blocks_count(es);

Here the ext4_blocks_count() is a use-after-free since you've released the
bh a few lines above.

Otherwise the patch looks good to me.

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

Re: [PATCH 11/12] ext4: cleanup ext4_get_dev_journal() and ext4_get_journal()

[Zhang Yi]

On 2023/8/4 0:14, Jan Kara wrote:
> On Tue 04-07-23 21:42:32, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> Factor out a new helper form ext4_get_dev_journal() to get external
>> journal bdev and check validation of this device, drop ext4_blkdev_get()
>> helper, and also remove duplicate check of journal feature. It makes
>> ext4_get_dev_journal() more clear than before.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> 
> One comment below:
> 
>> @@ -5838,25 +5815,25 @@ static journal_t *ext4_get_journal(struct super_block *sb,
>>  	return journal;
>>  }
>>  
>> -static journal_t *ext4_get_dev_journal(struct super_block *sb,
>> -				       dev_t j_dev)
>> +static struct block_device *ext4_get_journal_dev(struct super_block *sb,
>> +					dev_t j_dev, ext4_fsblk_t *j_start,
>> +					ext4_fsblk_t *j_len)
>>  {
>>  	struct buffer_head *bh;
>> -	journal_t *journal;
>> -	ext4_fsblk_t start;
>> -	ext4_fsblk_t len;
>> +	struct block_device *bdev;
>>  	int hblock, blocksize;
>>  	ext4_fsblk_t sb_block;
>>  	unsigned long offset;
>>  	struct ext4_super_block *es;
>> -	struct block_device *bdev;
>>  
>> -	if (WARN_ON_ONCE(!ext4_has_feature_journal(sb)))
>> -		return NULL;
>> -
>> -	bdev = ext4_blkdev_get(j_dev, sb);
>> -	if (bdev == NULL)
>> +	bdev = blkdev_get_by_dev(j_dev, BLK_OPEN_READ | BLK_OPEN_WRITE, sb,
>> +				 &ext4_holder_ops);
>> +	if (IS_ERR(bdev)) {
>> +		ext4_msg(sb, KERN_ERR,
>> +			 "failed to open journal device unknown-block(%u,%u) %ld",
>> +			 MAJOR(j_dev), MINOR(j_dev), PTR_ERR(bdev));
>>  		return NULL;
>> +	}
>>  
>>  	blocksize = sb->s_blocksize;
>>  	hblock = bdev_logical_block_size(bdev);
>> @@ -5869,7 +5846,8 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
>>  	sb_block = EXT4_MIN_BLOCK_SIZE / blocksize;
>>  	offset = EXT4_MIN_BLOCK_SIZE % blocksize;
>>  	set_blocksize(bdev, blocksize);
>> -	if (!(bh = __bread(bdev, sb_block, blocksize))) {
>> +	bh = __bread(bdev, sb_block, blocksize);
>> +	if (!bh) {
>>  		ext4_msg(sb, KERN_ERR, "couldn't read superblock of "
>>  		       "external journal");
>>  		goto out_bdev;
>> @@ -5879,56 +5857,67 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
>>  	if ((le16_to_cpu(es->s_magic) != EXT4_SUPER_MAGIC) ||
>>  	    !(le32_to_cpu(es->s_feature_incompat) &
>>  	      EXT4_FEATURE_INCOMPAT_JOURNAL_DEV)) {
>> -		ext4_msg(sb, KERN_ERR, "external journal has "
>> -					"bad superblock");
>> -		brelse(bh);
>> -		goto out_bdev;
>> +		ext4_msg(sb, KERN_ERR, "external journal has bad superblock");
>> +		goto out_bh;
>>  	}
>>  
>>  	if ((le32_to_cpu(es->s_feature_ro_compat) &
>>  	     EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
>>  	    es->s_checksum != ext4_superblock_csum(sb, es)) {
>> -		ext4_msg(sb, KERN_ERR, "external journal has "
>> -				       "corrupt superblock");
>> -		brelse(bh);
>> -		goto out_bdev;
>> +		ext4_msg(sb, KERN_ERR, "external journal has corrupt superblock");
>> +		goto out_bh;
>>  	}
>>  
>>  	if (memcmp(EXT4_SB(sb)->s_es->s_journal_uuid, es->s_uuid, 16)) {
>>  		ext4_msg(sb, KERN_ERR, "journal UUID does not match");
>> -		brelse(bh);
>> -		goto out_bdev;
>> +		goto out_bh;
>>  	}
>>  
>> -	len = ext4_blocks_count(es);
>> -	start = sb_block + 1;
>> -	brelse(bh);	/* we're done with the superblock */
>> +	brelse(bh);
>> +	*j_start = sb_block + 1;
>> +	*j_len = ext4_blocks_count(es);
> 
> Here the ext4_blocks_count() is a use-after-free since you've released the
> bh a few lines above.
> 

Indeed, will move it before the brelse(bh).

Thanks,
Yi.

[PATCH 12/12] ext4: ext4_get_{dev}_journal return proper error value

[Zhang Yi]

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

ext4_get_journal() and ext4_get_dev_journal() return NULL if they failed
to init journal, making them return proper error value instead, also
rename them to ext4_open_{inode,dev}_journal().

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/super.c | 51 +++++++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 25ae536a370f..ae8a964e493d 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5752,18 +5752,18 @@ static struct inode *ext4_get_journal_inode(struct super_block *sb,
 	journal_inode = ext4_iget(sb, journal_inum, EXT4_IGET_SPECIAL);
 	if (IS_ERR(journal_inode)) {
 		ext4_msg(sb, KERN_ERR, "no journal found");
-		return NULL;
+		return ERR_CAST(journal_inode);
 	}
 	if (!journal_inode->i_nlink) {
 		make_bad_inode(journal_inode);
 		iput(journal_inode);
 		ext4_msg(sb, KERN_ERR, "journal inode is deleted");
-		return NULL;
+		return ERR_PTR(-EFSCORRUPTED);
 	}
 	if (!S_ISREG(journal_inode->i_mode) || IS_ENCRYPTED(journal_inode)) {
 		ext4_msg(sb, KERN_ERR, "invalid journal inode");
 		iput(journal_inode);
-		return NULL;
+		return ERR_PTR(-EFSCORRUPTED);
 	}
 
 	ext4_debug("Journal inode found at %p: %lld bytes\n",
@@ -5793,21 +5793,21 @@ static int ext4_journal_bmap(journal_t *journal, sector_t *block)
 	return 0;
 }
 
-static journal_t *ext4_get_journal(struct super_block *sb,
-				   unsigned int journal_inum)
+static journal_t *ext4_open_inode_journal(struct super_block *sb,
+					  unsigned int journal_inum)
 {
 	struct inode *journal_inode;
 	journal_t *journal;
 
 	journal_inode = ext4_get_journal_inode(sb, journal_inum);
-	if (!journal_inode)
-		return NULL;
+	if (IS_ERR(journal_inode))
+		return ERR_CAST(journal_inode);
 
 	journal = jbd2_journal_init_inode(journal_inode);
 	if (IS_ERR(journal)) {
 		ext4_msg(sb, KERN_ERR, "Could not load journal inode");
 		iput(journal_inode);
-		return NULL;
+		return ERR_CAST(journal);
 	}
 	journal->j_private = sb;
 	journal->j_bmap = ext4_journal_bmap;
@@ -5825,6 +5825,7 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
 	ext4_fsblk_t sb_block;
 	unsigned long offset;
 	struct ext4_super_block *es;
+	int errno;
 
 	bdev = blkdev_get_by_dev(j_dev, BLK_OPEN_READ | BLK_OPEN_WRITE, sb,
 				 &ext4_holder_ops);
@@ -5832,7 +5833,7 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
 		ext4_msg(sb, KERN_ERR,
 			 "failed to open journal device unknown-block(%u,%u) %ld",
 			 MAJOR(j_dev), MINOR(j_dev), PTR_ERR(bdev));
-		return NULL;
+		return ERR_CAST(bdev);
 	}
 
 	blocksize = sb->s_blocksize;
@@ -5840,6 +5841,7 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
 	if (blocksize < hblock) {
 		ext4_msg(sb, KERN_ERR,
 			"blocksize too small for journal device");
+		errno = -EINVAL;
 		goto out_bdev;
 	}
 
@@ -5850,6 +5852,7 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
 	if (!bh) {
 		ext4_msg(sb, KERN_ERR, "couldn't read superblock of "
 		       "external journal");
+		errno = -EINVAL;
 		goto out_bdev;
 	}
 
@@ -5858,6 +5861,7 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
 	    !(le32_to_cpu(es->s_feature_incompat) &
 	      EXT4_FEATURE_INCOMPAT_JOURNAL_DEV)) {
 		ext4_msg(sb, KERN_ERR, "external journal has bad superblock");
+		errno = -EFSCORRUPTED;
 		goto out_bh;
 	}
 
@@ -5865,11 +5869,13 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
 	     EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
 	    es->s_checksum != ext4_superblock_csum(sb, es)) {
 		ext4_msg(sb, KERN_ERR, "external journal has corrupt superblock");
+		errno = -EFSCORRUPTED;
 		goto out_bh;
 	}
 
 	if (memcmp(EXT4_SB(sb)->s_es->s_journal_uuid, es->s_uuid, 16)) {
 		ext4_msg(sb, KERN_ERR, "journal UUID does not match");
+		errno = -EFSCORRUPTED;
 		goto out_bh;
 	}
 
@@ -5882,31 +5888,34 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
 	brelse(bh);
 out_bdev:
 	blkdev_put(bdev, sb);
-	return NULL;
+	return ERR_PTR(errno);
 }
 
-static journal_t *ext4_get_dev_journal(struct super_block *sb,
-				       dev_t j_dev)
+static journal_t *ext4_open_dev_journal(struct super_block *sb,
+					dev_t j_dev)
 {
 	journal_t *journal;
 	ext4_fsblk_t j_start;
 	ext4_fsblk_t j_len;
 	struct block_device *journal_bdev;
+	int errno = 0;
 
 	journal_bdev = ext4_get_journal_dev(sb, j_dev, &j_start, &j_len);
-	if (!journal_bdev)
-		return NULL;
+	if (IS_ERR(journal_bdev))
+		return ERR_CAST(journal_bdev);
 
 	journal = jbd2_journal_init_dev(journal_bdev, sb->s_bdev, j_start,
 					j_len, sb->s_blocksize);
 	if (IS_ERR(journal)) {
 		ext4_msg(sb, KERN_ERR, "failed to create device journal");
+		errno = PTR_ERR(journal);
 		goto out_bdev;
 	}
 	if (be32_to_cpu(journal->j_superblock->s_nr_users) != 1) {
 		ext4_msg(sb, KERN_ERR, "External journal has more than one "
 					"user (unsupported) - %d",
 			be32_to_cpu(journal->j_superblock->s_nr_users));
+		errno = -EINVAL;
 		goto out_journal;
 	}
 	journal->j_private = sb;
@@ -5918,7 +5927,7 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
 	jbd2_journal_destroy(journal);
 out_bdev:
 	blkdev_put(journal_bdev, sb);
-	return NULL;
+	return ERR_PTR(errno);
 }
 
 static int ext4_load_journal(struct super_block *sb,
@@ -5950,13 +5959,13 @@ static int ext4_load_journal(struct super_block *sb,
 	}
 
 	if (journal_inum) {
-		journal = ext4_get_journal(sb, journal_inum);
-		if (!journal)
-			return -EINVAL;
+		journal = ext4_open_inode_journal(sb, journal_inum);
+		if (IS_ERR(journal))
+			return PTR_ERR(journal);
 	} else {
-		journal = ext4_get_dev_journal(sb, journal_dev);
-		if (!journal)
-			return -EINVAL;
+		journal = ext4_open_dev_journal(sb, journal_dev);
+		if (IS_ERR(journal))
+			return PTR_ERR(journal);
 	}
 
 	journal_dev_ro = bdev_read_only(journal->j_dev);
-- 
2.39.2

Re: [PATCH 12/12] ext4: ext4_get_{dev}_journal return proper error value

[Jan Kara]

On Tue 04-07-23 21:42:33, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> ext4_get_journal() and ext4_get_dev_journal() return NULL if they failed
> to init journal, making them return proper error value instead, also
> rename them to ext4_open_{inode,dev}_journal().
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good to me. Feel free to add:

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

								Honza

> ---
>  fs/ext4/super.c | 51 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 25ae536a370f..ae8a964e493d 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5752,18 +5752,18 @@ static struct inode *ext4_get_journal_inode(struct super_block *sb,
>  	journal_inode = ext4_iget(sb, journal_inum, EXT4_IGET_SPECIAL);
>  	if (IS_ERR(journal_inode)) {
>  		ext4_msg(sb, KERN_ERR, "no journal found");
> -		return NULL;
> +		return ERR_CAST(journal_inode);
>  	}
>  	if (!journal_inode->i_nlink) {
>  		make_bad_inode(journal_inode);
>  		iput(journal_inode);
>  		ext4_msg(sb, KERN_ERR, "journal inode is deleted");
> -		return NULL;
> +		return ERR_PTR(-EFSCORRUPTED);
>  	}
>  	if (!S_ISREG(journal_inode->i_mode) || IS_ENCRYPTED(journal_inode)) {
>  		ext4_msg(sb, KERN_ERR, "invalid journal inode");
>  		iput(journal_inode);
> -		return NULL;
> +		return ERR_PTR(-EFSCORRUPTED);
>  	}
>  
>  	ext4_debug("Journal inode found at %p: %lld bytes\n",
> @@ -5793,21 +5793,21 @@ static int ext4_journal_bmap(journal_t *journal, sector_t *block)
>  	return 0;
>  }
>  
> -static journal_t *ext4_get_journal(struct super_block *sb,
> -				   unsigned int journal_inum)
> +static journal_t *ext4_open_inode_journal(struct super_block *sb,
> +					  unsigned int journal_inum)
>  {
>  	struct inode *journal_inode;
>  	journal_t *journal;
>  
>  	journal_inode = ext4_get_journal_inode(sb, journal_inum);
> -	if (!journal_inode)
> -		return NULL;
> +	if (IS_ERR(journal_inode))
> +		return ERR_CAST(journal_inode);
>  
>  	journal = jbd2_journal_init_inode(journal_inode);
>  	if (IS_ERR(journal)) {
>  		ext4_msg(sb, KERN_ERR, "Could not load journal inode");
>  		iput(journal_inode);
> -		return NULL;
> +		return ERR_CAST(journal);
>  	}
>  	journal->j_private = sb;
>  	journal->j_bmap = ext4_journal_bmap;
> @@ -5825,6 +5825,7 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
>  	ext4_fsblk_t sb_block;
>  	unsigned long offset;
>  	struct ext4_super_block *es;
> +	int errno;
>  
>  	bdev = blkdev_get_by_dev(j_dev, BLK_OPEN_READ | BLK_OPEN_WRITE, sb,
>  				 &ext4_holder_ops);
> @@ -5832,7 +5833,7 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
>  		ext4_msg(sb, KERN_ERR,
>  			 "failed to open journal device unknown-block(%u,%u) %ld",
>  			 MAJOR(j_dev), MINOR(j_dev), PTR_ERR(bdev));
> -		return NULL;
> +		return ERR_CAST(bdev);
>  	}
>  
>  	blocksize = sb->s_blocksize;
> @@ -5840,6 +5841,7 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
>  	if (blocksize < hblock) {
>  		ext4_msg(sb, KERN_ERR,
>  			"blocksize too small for journal device");
> +		errno = -EINVAL;
>  		goto out_bdev;
>  	}
>  
> @@ -5850,6 +5852,7 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
>  	if (!bh) {
>  		ext4_msg(sb, KERN_ERR, "couldn't read superblock of "
>  		       "external journal");
> +		errno = -EINVAL;
>  		goto out_bdev;
>  	}
>  
> @@ -5858,6 +5861,7 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
>  	    !(le32_to_cpu(es->s_feature_incompat) &
>  	      EXT4_FEATURE_INCOMPAT_JOURNAL_DEV)) {
>  		ext4_msg(sb, KERN_ERR, "external journal has bad superblock");
> +		errno = -EFSCORRUPTED;
>  		goto out_bh;
>  	}
>  
> @@ -5865,11 +5869,13 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
>  	     EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
>  	    es->s_checksum != ext4_superblock_csum(sb, es)) {
>  		ext4_msg(sb, KERN_ERR, "external journal has corrupt superblock");
> +		errno = -EFSCORRUPTED;
>  		goto out_bh;
>  	}
>  
>  	if (memcmp(EXT4_SB(sb)->s_es->s_journal_uuid, es->s_uuid, 16)) {
>  		ext4_msg(sb, KERN_ERR, "journal UUID does not match");
> +		errno = -EFSCORRUPTED;
>  		goto out_bh;
>  	}
>  
> @@ -5882,31 +5888,34 @@ static struct block_device *ext4_get_journal_dev(struct super_block *sb,
>  	brelse(bh);
>  out_bdev:
>  	blkdev_put(bdev, sb);
> -	return NULL;
> +	return ERR_PTR(errno);
>  }
>  
> -static journal_t *ext4_get_dev_journal(struct super_block *sb,
> -				       dev_t j_dev)
> +static journal_t *ext4_open_dev_journal(struct super_block *sb,
> +					dev_t j_dev)
>  {
>  	journal_t *journal;
>  	ext4_fsblk_t j_start;
>  	ext4_fsblk_t j_len;
>  	struct block_device *journal_bdev;
> +	int errno = 0;
>  
>  	journal_bdev = ext4_get_journal_dev(sb, j_dev, &j_start, &j_len);
> -	if (!journal_bdev)
> -		return NULL;
> +	if (IS_ERR(journal_bdev))
> +		return ERR_CAST(journal_bdev);
>  
>  	journal = jbd2_journal_init_dev(journal_bdev, sb->s_bdev, j_start,
>  					j_len, sb->s_blocksize);
>  	if (IS_ERR(journal)) {
>  		ext4_msg(sb, KERN_ERR, "failed to create device journal");
> +		errno = PTR_ERR(journal);
>  		goto out_bdev;
>  	}
>  	if (be32_to_cpu(journal->j_superblock->s_nr_users) != 1) {
>  		ext4_msg(sb, KERN_ERR, "External journal has more than one "
>  					"user (unsupported) - %d",
>  			be32_to_cpu(journal->j_superblock->s_nr_users));
> +		errno = -EINVAL;
>  		goto out_journal;
>  	}
>  	journal->j_private = sb;
> @@ -5918,7 +5927,7 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
>  	jbd2_journal_destroy(journal);
>  out_bdev:
>  	blkdev_put(journal_bdev, sb);
> -	return NULL;
> +	return ERR_PTR(errno);
>  }
>  
>  static int ext4_load_journal(struct super_block *sb,
> @@ -5950,13 +5959,13 @@ static int ext4_load_journal(struct super_block *sb,
>  	}
>  
>  	if (journal_inum) {
> -		journal = ext4_get_journal(sb, journal_inum);
> -		if (!journal)
> -			return -EINVAL;
> +		journal = ext4_open_inode_journal(sb, journal_inum);
> +		if (IS_ERR(journal))
> +			return PTR_ERR(journal);
>  	} else {
> -		journal = ext4_get_dev_journal(sb, journal_dev);
> -		if (!journal)
> -			return -EINVAL;
> +		journal = ext4_open_dev_journal(sb, journal_dev);
> +		if (IS_ERR(journal))
> +			return PTR_ERR(journal);
>  	}
>  
>  	journal_dev_ro = bdev_read_only(journal->j_dev);
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR