Messages in this thread | | | Subject | Re: [PATCH v2] jbd2: avoid mount failed when commit block is partial submitted | From | "yebin (H)" <> | Date | Wed, 24 Apr 2024 15:59:29 +0800 |
| |
On 2024/4/14 7:27, Andreas Dilger wrote: > On Apr 12, 2024, at 7:30 PM, Ye Bin <yebin10@huawei.com> wrote: >> We encountered a problem that the file system could not be mounted in >> the power-off scenario. The analysis of the file system mirror shows that >> only part of the data is written to the last commit block. >> The valid data of the commit block is concentrated in the first sector. >> However, the data of the entire block is involved in the checksum calculation. >> For different hardware, the minimum atomic unit may be different. >> If the checksum of a committed block is incorrect, clear the data except the >> 'commit_header' and then calculate the checksum. If the checkusm is correct, >> it is considered that the block is partially committed. > I think this is a clever solution to the problem, thanks for submitting > the patch. > >> However, if there are valid description/revoke blocks, it is considered >> that the data is abnormal and the log replay is stopped. > It would be possible to use the r_count of records in the revoke block > to determine how much of the revoke block is unused and could be zeroed > out to recompute the partial checksum? That should be relatively safe > to try, as long as r_count is itself checked to fit within the block > before the memory is zeroed, to avoid overflowing the temporary buffer size: > > r_count <= journal_revoke_records_per_block(journal) > > > It is open for discussion how much corruption should be allowed in the > journal, since it can be very destructive to copy corrupted blocks from > one place in the journal exactly into important metadata blocks across > the whole filesystem. That said, the checksums *should* avoid this kind > of problem, and revoke blocks do not contain "metadata" that is copied > into the filesystem but only block numbers to skip. It is "less bad" if > this was wrong, and having an incomplete journal replay due to minor > corruption that is causing boot failure is also a problem that should be > avoided if it can safely be done. > > > Additional comments inline below: > >> Signed-off-by: Ye Bin <yebin10@huawei.com> >> --- >> fs/jbd2/recovery.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> >> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c >> index 1f7664984d6e..eb0e026f3109 100644 >> --- a/fs/jbd2/recovery.c >> +++ b/fs/jbd2/recovery.c >> @@ -443,6 +443,27 @@ static int jbd2_commit_block_csum_verify(journal_t *j, void *buf) >> return provided == cpu_to_be32(calculated); >> } >> >> +static bool jbd2_commit_block_csum_partial_verify(journal_t *j, void *buf) >> +{ > (style) if this is named jbd2_commit_block_csum_verify_partial() then > it would sort together with jbd2_commit_block_csum_verify() and would > be easier to find with tag completion and grep in the future. > >> + struct commit_header *h; >> + __be32 provided; >> + __u32 calculated; >> + void *tmpbuf; >> + >> + tmpbuf = kzalloc(j->j_blocksize, GFP_KERNEL); >> + if (!tmpbuf) >> + return false; >> + >> + memcpy(tmpbuf, buf, sizeof(struct commit_header)); >> + h = tmpbuf; >> + provided = h->h_chksum[0]; >> + h->h_chksum[0] = 0; >> + calculated = jbd2_chksum(j, j->j_csum_seed, tmpbuf, j->j_blocksize); >> + kfree(tmpbuf); >> + >> + return provided == cpu_to_be32(calculated); >> +} >> + >> static int jbd2_block_tag_csum_verify(journal_t *j, journal_block_tag_t *tag, >> journal_block_tag3_t *tag3, >> void *buf, __u32 sequence) >> @@ -479,6 +500,7 @@ static int do_one_pass(journal_t *journal, >> int descr_csum_size = 0; >> int block_error = 0; >> bool need_check_commit_time = false; >> + bool has_partial_commit = false; >> __u64 last_trans_commit_time = 0, commit_time; >> >> /* >> @@ -590,6 +612,14 @@ static int do_one_pass(journal_t *journal, >> next_log_block); >> } >> >> + if (pass == PASS_SCAN && has_partial_commit) { >> + pr_err("JBD2: Detect validate descriptor block %lu after incomplete commit block\n", > (minor) it isn't clear to me what this error message is trying to say? > Should it be something like "detected invalid descriptor block ..."? > >> + next_log_block); >> + err = -EFSBADCRC; >> + brelse(bh); >> + goto failed; >> + } >> + >> /* If it is a valid descriptor block, replay it >> * in pass REPLAY; if journal_checksums enabled, then >> * calculate checksums in PASS_SCAN, otherwise, >> @@ -810,6 +840,14 @@ static int do_one_pass(journal_t *journal, >> if (pass == PASS_SCAN && >> !jbd2_commit_block_csum_verify(journal, >> bh->b_data)) { >> + if (jbd2_commit_block_csum_partial_verify( > If this function was restructured a bit then the code flow would not need > to get more complex than it already is. Something like: > > if (pass == PASS_SCAN && > !(jbd2_commit_block_csum_verify(journal, > bh->b_data) || > (has_partial_commit = > jbd2_commit_block_csum_verify_partial(journal, > bh->b_data))) { > > The pr_notice() can be printed by jbd2_commit_block_csum_partial_verify() > if the partial checksum is valid, so no need for goto and chksum_ok label. I modified it according to your idea, and found that the logic will be faulty when the checksum is not enabled. > >> + pr_notice("JBD2: Find incomplete commit block in transaction %u block %lu\n", >> + next_commit_ID, next_log_block); >> + has_partial_commit = true; >> + goto chksum_ok; >> + } >> chksum_error: >> if (commit_time < last_trans_commit_time) >> goto ignore_crc_mismatch; >> @@ -824,6 +862,7 @@ static int do_one_pass(journal_t *journal, >> } >> } >> if (pass == PASS_SCAN) { >> + chksum_ok: >> last_trans_commit_time = commit_time; >> head_block = next_log_block; >> } >> @@ -843,6 +882,15 @@ static int do_one_pass(journal_t *journal, >> next_log_block); >> need_check_commit_time = true; >> } >> + >> + if (pass == PASS_SCAN && has_partial_commit) { >> + pr_err("JBD2: Detect validate revoke block %lu after incomplete commit block\n", > Similarly, I find this error message hard to understand. Maybe "detected invalid revoke block ..."? > >> + next_log_block); >> + err = -EFSBADCRC; >> + brelse(bh); >> + goto failed; >> + } >> + >> /* If we aren't in the REVOKE pass, then we can >> * just skip over this block. */ >> if (pass != PASS_REVOKE) { >> -- >> 2.31.1 >> > > Cheers, Andreas > > > > >
| |