Message ID | 20230310125206.2867822-5-chengzhihao1@huawei.com |
---|---|
State | Superseded |
Headers | show |
Series | ext4: Fix stale buffer loading from last failed | expand |
On Fri 10-03-23 20:52:05, Zhihao Cheng wrote: > From: Zhang Yi <yi.zhang@huawei.com> > > Current journal_get_superblock() couple journal superblock checking and > partial journal initialization, factor out initialization part from it > to make things clear. > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com> > --- > fs/jbd2/journal.c | 52 ++++++++++++++++++++--------------------------- > 1 file changed, 22 insertions(+), 30 deletions(-) The patch looks mostly good. Just one style nit below. > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > index b991d5c21d16..cd94d068b4e6 100644 > --- a/fs/jbd2/journal.c > +++ b/fs/jbd2/journal.c > @@ -1921,26 +1921,15 @@ static int journal_get_superblock(journal_t *journal) > printk(KERN_WARNING "JBD2: no valid journal superblock found\n"); > goto out; > } > - > - switch(be32_to_cpu(sb->s_header.h_blocktype)) { > - case JBD2_SUPERBLOCK_V1: > - journal->j_format_version = 1; > - break; > - case JBD2_SUPERBLOCK_V2: > - journal->j_format_version = 2; > - break; > - default: > + if (be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V1 && > + be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V2) { > printk(KERN_WARNING "JBD2: unrecognised superblock format ID\n"); > goto out; > } > - > - if (be32_to_cpu(sb->s_maxlen) < journal->j_total_len) > - journal->j_total_len = be32_to_cpu(sb->s_maxlen); > - else if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) { > + if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) { > printk(KERN_WARNING "JBD2: journal file too short\n"); > goto out; > } > - Please keep this empty line here. It actually makes the code more readable, separating logically different checks. Same with three empty lines below... > if (be32_to_cpu(sb->s_first) == 0 || > be32_to_cpu(sb->s_first) >= journal->j_total_len) { > printk(KERN_WARNING > @@ -1956,7 +1945,6 @@ static int journal_get_superblock(journal_t *journal) > "at the same time!\n"); > goto out; > } > - > if (jbd2_journal_has_csum_v2or3_feature(journal) && > jbd2_has_feature_checksum(journal)) { > /* Can't have checksum v1 and v2 on at the same time! */ > @@ -1964,12 +1952,10 @@ static int journal_get_superblock(journal_t *journal) > "at the same time!\n"); > goto out; > } > - > if (!jbd2_verify_csum_type(journal, sb)) { > printk(KERN_ERR "JBD2: Unknown checksum type\n"); > goto out; > } > - > /* Load the checksum driver */ > if (jbd2_journal_has_csum_v2or3_feature(journal)) { > journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0); Otherwise the patch looks good. Honza
On 2023/3/13 19:12, Jan Kara wrote: > On Fri 10-03-23 20:52:05, Zhihao Cheng wrote: >> From: Zhang Yi <yi.zhang@huawei.com> >> >> Current journal_get_superblock() couple journal superblock checking and >> partial journal initialization, factor out initialization part from it >> to make things clear. >> >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> >> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com> >> --- >> fs/jbd2/journal.c | 52 ++++++++++++++++++++--------------------------- >> 1 file changed, 22 insertions(+), 30 deletions(-) > > The patch looks mostly good. Just one style nit below. > >> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c >> index b991d5c21d16..cd94d068b4e6 100644 >> --- a/fs/jbd2/journal.c >> +++ b/fs/jbd2/journal.c >> @@ -1921,26 +1921,15 @@ static int journal_get_superblock(journal_t *journal) >> printk(KERN_WARNING "JBD2: no valid journal superblock found\n"); >> goto out; >> } >> - >> - switch(be32_to_cpu(sb->s_header.h_blocktype)) { >> - case JBD2_SUPERBLOCK_V1: >> - journal->j_format_version = 1; >> - break; >> - case JBD2_SUPERBLOCK_V2: >> - journal->j_format_version = 2; >> - break; >> - default: >> + if (be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V1 && >> + be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V2) { >> printk(KERN_WARNING "JBD2: unrecognised superblock format ID\n"); >> goto out; >> } >> - >> - if (be32_to_cpu(sb->s_maxlen) < journal->j_total_len) >> - journal->j_total_len = be32_to_cpu(sb->s_maxlen); >> - else if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) { >> + if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) { >> printk(KERN_WARNING "JBD2: journal file too short\n"); >> goto out; >> } >> - > > Please keep this empty line here. It actually makes the code more readable, > separating logically different checks. Same with three empty lines below... Thanks for the comments, I will add them back. Yi.
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index b991d5c21d16..cd94d068b4e6 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -1921,26 +1921,15 @@ static int journal_get_superblock(journal_t *journal) printk(KERN_WARNING "JBD2: no valid journal superblock found\n"); goto out; } - - switch(be32_to_cpu(sb->s_header.h_blocktype)) { - case JBD2_SUPERBLOCK_V1: - journal->j_format_version = 1; - break; - case JBD2_SUPERBLOCK_V2: - journal->j_format_version = 2; - break; - default: + if (be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V1 && + be32_to_cpu(sb->s_header.h_blocktype) != JBD2_SUPERBLOCK_V2) { printk(KERN_WARNING "JBD2: unrecognised superblock format ID\n"); goto out; } - - if (be32_to_cpu(sb->s_maxlen) < journal->j_total_len) - journal->j_total_len = be32_to_cpu(sb->s_maxlen); - else if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) { + if (be32_to_cpu(sb->s_maxlen) > journal->j_total_len) { printk(KERN_WARNING "JBD2: journal file too short\n"); goto out; } - if (be32_to_cpu(sb->s_first) == 0 || be32_to_cpu(sb->s_first) >= journal->j_total_len) { printk(KERN_WARNING @@ -1956,7 +1945,6 @@ static int journal_get_superblock(journal_t *journal) "at the same time!\n"); goto out; } - if (jbd2_journal_has_csum_v2or3_feature(journal) && jbd2_has_feature_checksum(journal)) { /* Can't have checksum v1 and v2 on at the same time! */ @@ -1964,12 +1952,10 @@ static int journal_get_superblock(journal_t *journal) "at the same time!\n"); goto out; } - if (!jbd2_verify_csum_type(journal, sb)) { printk(KERN_ERR "JBD2: Unknown checksum type\n"); goto out; } - /* Load the checksum driver */ if (jbd2_journal_has_csum_v2or3_feature(journal)) { journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0); @@ -1979,25 +1965,14 @@ static int journal_get_superblock(journal_t *journal) journal->j_chksum_driver = NULL; goto out; } - } - - if (jbd2_journal_has_csum_v2or3(journal)) { /* Check superblock checksum */ if (sb->s_checksum != jbd2_superblock_csum(journal, sb)) { printk(KERN_ERR "JBD2: journal checksum error\n"); err = -EFSBADCRC; goto out; } - - /* Precompute checksum seed for all metadata */ - journal->j_csum_seed = jbd2_chksum(journal, ~0, sb->s_uuid, - sizeof(sb->s_uuid)); } - - journal->j_revoke_records_per_block = - journal_revoke_records_per_block(journal); set_buffer_verified(bh); - return 0; out: @@ -2022,12 +1997,30 @@ static int load_superblock(journal_t *journal) sb = journal->j_superblock; + switch (be32_to_cpu(sb->s_header.h_blocktype)) { + case JBD2_SUPERBLOCK_V1: + journal->j_format_version = 1; + break; + case JBD2_SUPERBLOCK_V2: + journal->j_format_version = 2; + break; + } + journal->j_tail_sequence = be32_to_cpu(sb->s_sequence); journal->j_tail = be32_to_cpu(sb->s_start); journal->j_first = be32_to_cpu(sb->s_first); journal->j_errno = be32_to_cpu(sb->s_errno); journal->j_last = be32_to_cpu(sb->s_maxlen); + if (be32_to_cpu(sb->s_maxlen) < journal->j_total_len) + journal->j_total_len = be32_to_cpu(sb->s_maxlen); + /* Precompute checksum seed for all metadata */ + if (jbd2_journal_has_csum_v2or3(journal)) + journal->j_csum_seed = jbd2_chksum(journal, ~0, sb->s_uuid, + sizeof(sb->s_uuid)); + journal->j_revoke_records_per_block = + journal_revoke_records_per_block(journal); + if (jbd2_has_feature_fast_commit(journal)) { journal->j_fc_last = be32_to_cpu(sb->s_maxlen); num_fc_blocks = jbd2_journal_get_num_fc_blks(sb); @@ -2223,8 +2216,7 @@ int jbd2_journal_check_used_features(journal_t *journal, unsigned long compat, if (!compat && !ro && !incompat) return 1; /* Load journal superblock if it is not loaded yet. */ - if (journal->j_format_version == 0 && - journal_get_superblock(journal) != 0) + if (journal_get_superblock(journal)) return 0; if (!jbd2_format_support_feature(journal)) return 0;