On Fri 10-03-23 20:52:05, Zhihao Cheng wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
>
> Current journal_get_superblock() couple journal superblock checking and
> partial journal initialization, factor out initialization part from it
> to make things clear.
>
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> Signed-off-by: Zhihao Cheng <chengzhihao1@xxxxxxxxxx>
> ---
> fs/jbd2/journal.c | 52 ++++++++++++++++++++---------------------------
> 1 file changed, 22 insertions(+), 30 deletions(-)
The patch looks mostly good. Just one style nit below.
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index b991d5c21d16..cd94d068b4e6 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1921,26 +1921,15 @@ static int journal_get_superblock(journal_t *journal)
> printk(KERN_WARNING "JBD2: no valid journal superblock found\n");
> goto out;
> }
> -
> - switch(be32_to_cpu(sb->s_header.h_blocktype)) {
> - case JBD2_SUPERBLOCK_V1:
> - journal->j_format_version = 1;
> - break;
> - case JBD2_SUPERBLOCK_V2:
> - journal->j_format_version = 2;
> - break;
> - default:
> + if (be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V1 &&
> + be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V2) {
> printk(KERN_WARNING "JBD2: unrecognised superblock format ID\n");
> goto out;
> }
> -
> - if (be32_to_cpu(sb->s_maxlen) < journal->j_total_len)
> - journal->j_total_len = be32_to_cpu(sb->s_maxlen);
> - else if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) {
> + if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) {
> printk(KERN_WARNING "JBD2: journal file too short\n");
> goto out;
> }
> -
Please keep this empty line here. It actually makes the code more readable,
separating logically different checks. Same with three empty lines below...
> if (be32_to_cpu(sb->s_first) == 0 ||
> be32_to_cpu(sb->s_first) >= journal->j_total_len) {
> printk(KERN_WARNING
> @@ -1956,7 +1945,6 @@ static int journal_get_superblock(journal_t *journal)
> "at the same time!\n");
> goto out;
> }
> -
> if (jbd2_journal_has_csum_v2or3_feature(journal) &&
> jbd2_has_feature_checksum(journal)) {
> /* Can't have checksum v1 and v2 on at the same time! */
> @@ -1964,12 +1952,10 @@ static int journal_get_superblock(journal_t *journal)
> "at the same time!\n");
> goto out;
> }
> -
> if (!jbd2_verify_csum_type(journal, sb)) {
> printk(KERN_ERR "JBD2: Unknown checksum type\n");
> goto out;
> }
> -
> /* Load the checksum driver */
> if (jbd2_journal_has_csum_v2or3_feature(journal)) {
> journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
Otherwise the patch looks good.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
