Skip to contentSkip to conversation

jbd2: discard last transaction when commit block checksum broken in v2v3

PATCH In review Ye Bin 2 participants 2 messages Sep 29, 2021 – Nov 22, 2021 Journalling layer for block devices (JBD2)

lore
Share & tools
lore

Ye Bin

author
#0 lore raw

PATCH 1/1 jbd2: discard last transaction when commit block checksum broken in v2v3

Now, we meet an issue that commit block has broken checksum when cold reboot device, that lead to mount failed.
The reason maybe only some sector store on disk, and then device power off. But we calculate checksum with whole logic block.The data stored on disk can only ensure the atomicity of sector level.
Actually, we already replay previous transactions. We can just discard last transaction. As now, descriptor/revocation/commit/superblock has it's own checksum.

Fixes 80b3767fbe15("jbd2: don't wipe the journal on a failed journal checksum")
Signed-off-by Ye Bin <[email protected]>
---
 fs/jbd2/journal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/jbd2/journal.c +1 -1
index 35302bc192eb..a3dd7b757b3d 100644
@@ -2080,7 +2080,7 @@ int jbd2_journal_load(journal_t *journal)
 	if (jbd2_journal_recover(journal))
 		goto recovery_error;
 
-	if (journal->j_failed_commit) {
+	if (journal->j_failed_commit && !jbd2_journal_has_csum_v2or3(journal)) {
 		printk(KERN_ERR "JBD2: journal transaction %u on %s "
 		       "is corrupt.\n", journal->j_failed_commit,
 		       journal->j_devname);
Signature · 1 line
2.31.1
#1 lore raw
Quoted 11 lines · Ye Bin

Now, we meet an issue that commit block has broken checksum when cold reboot device, that lead to mount failed.
The reason maybe only some sector store on disk, and then device power off. But we calculate checksum with whole logic block.The data stored on disk can only ensure the atomicity of sector level.
Actually, we already replay previous transactions. We can just discard last transaction. As now, descriptor/revocation/commit/superblock has it's own checksum.

Fixes 80b3767fbe15("jbd2: don't wipe the journal on a failed journal checksum")
Signed-off-by Ye Bin <[email protected]>

Thanks for the patch. It seems to have fallen through the cracks. Sorry for that.

Quoted 9 lines
---
 fs/jbd2/journal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/jbd2/journal.c +0 -0
index 35302bc192eb..a3dd7b757b3d 100644

fs/jbd2/journal.c int jbd2_journal_load(journal_t *journal) · line 2083

Quoted 5 lines
fs/jbd2/journal.c +1 -1
@@ -2080,7 +2080,7 @@ int jbd2_journal_load(journal_t *journal)
 	if (jbd2_journal_recover(journal))
 		goto recovery_error;
 
-	if (journal->j_failed_commit) {
+	if (journal->j_failed_commit && !jbd2_journal_has_csum_v2or3(journal)) {

I guess this decision somewhat questionable. If the failed commit was indeed the last one, I guess loosing the last transaction as you suggest is a sensible thing to do. However if the checksum failed somewhere in the middle of the journal because of a bitflip or something like that, we probably don't want to loose that many transactions and rather want to do fsck and try to recover as much data as possible... What do others think?

Honza

Quoted 6 lines
fs/jbd2/journal.c +0 -0
 		printk(KERN_ERR "JBD2: journal transaction %u on %s "
 		       "is corrupt.\n", journal->j_failed_commit,
 		       journal->j_devname);
Signature · 1 line
2.31.1
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR