* [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover
@ 2024-09-30 0:59 Ye Bin
2024-09-30 0:59 ` [PATCH v2 1/6] jbd2: remove redundant judgments for check v1 checksum Ye Bin
` (6 more replies)
0 siblings, 7 replies; 16+ messages in thread
From: Ye Bin @ 2024-09-30 0:59 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4; +Cc: jack, zhangxiaoxu5
Diff v2 vs v1:
1. Modify the indentation problem and remove unnecessary braces in PATCH[4];
2. Add PATCH[6] to remove the 'success' parameter from the jbd2_do_replay();
Ye Bin (6):
jbd2: remove redundant judgments for check v1 checksum
jbd2: unified release of buffer_head in do_one_pass()
jbd2: refactor JBD2_COMMIT_BLOCK process in do_one_pass()
jbd2: factor out jbd2_do_replay()
jbd2: remove useless 'block_error' variable
jbd2: remove the 'success' parameter from the jbd2_do_replay()
function
fs/jbd2/recovery.c | 311 +++++++++++++++++++++------------------------
1 file changed, 148 insertions(+), 163 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/6] jbd2: remove redundant judgments for check v1 checksum
2024-09-30 0:59 [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover Ye Bin
@ 2024-09-30 0:59 ` Ye Bin
2024-09-30 2:36 ` Zhang Yi
2024-09-30 0:59 ` [PATCH v2 2/6] jbd2: unified release of buffer_head in do_one_pass() Ye Bin
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Ye Bin @ 2024-09-30 0:59 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4; +Cc: jack, zhangxiaoxu5
From: Ye Bin <yebin10@huawei.com>
'need_check_commit_time' is only used by v2/v3 checksum, so there isn't
need to add 'need_check_commit_time' judegement for v1 checksum logic.
Signed-off-by: Ye Bin <yebin10@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
fs/jbd2/recovery.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 667f67342c52..5efbca6a98c4 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -619,7 +619,6 @@ static int do_one_pass(journal_t *journal,
if (pass != PASS_REPLAY) {
if (pass == PASS_SCAN &&
jbd2_has_feature_checksum(journal) &&
- !need_check_commit_time &&
!info->end_transaction) {
if (calc_chksums(journal, bh,
&next_log_block,
--
2.31.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/6] jbd2: unified release of buffer_head in do_one_pass()
2024-09-30 0:59 [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover Ye Bin
2024-09-30 0:59 ` [PATCH v2 1/6] jbd2: remove redundant judgments for check v1 checksum Ye Bin
@ 2024-09-30 0:59 ` Ye Bin
2024-09-30 2:36 ` Zhang Yi
2024-09-30 0:59 ` [PATCH v2 3/6] jbd2: refactor JBD2_COMMIT_BLOCK process " Ye Bin
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Ye Bin @ 2024-09-30 0:59 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4; +Cc: jack, zhangxiaoxu5
From: Ye Bin <yebin10@huawei.com>
Now buffer_head free is very fragmented in do_one_pass(), unified release
of buffer_head in do_one_pass()
Signed-off-by: Ye Bin <yebin10@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
fs/jbd2/recovery.c | 34 +++++++++-------------------------
1 file changed, 9 insertions(+), 25 deletions(-)
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 5efbca6a98c4..0adf0cb31a03 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -493,7 +493,7 @@ static int do_one_pass(journal_t *journal,
int err, success = 0;
journal_superblock_t * sb;
journal_header_t * tmp;
- struct buffer_head * bh;
+ struct buffer_head *bh = NULL;
unsigned int sequence;
int blocktype;
int tag_bytes = journal_tag_bytes(journal);
@@ -552,6 +552,8 @@ static int do_one_pass(journal_t *journal,
* record. */
jbd2_debug(3, "JBD2: checking block %ld\n", next_log_block);
+ brelse(bh);
+ bh = NULL;
err = jread(&bh, journal, next_log_block);
if (err)
goto failed;
@@ -567,20 +569,16 @@ static int do_one_pass(journal_t *journal,
tmp = (journal_header_t *)bh->b_data;
- if (tmp->h_magic != cpu_to_be32(JBD2_MAGIC_NUMBER)) {
- brelse(bh);
+ if (tmp->h_magic != cpu_to_be32(JBD2_MAGIC_NUMBER))
break;
- }
blocktype = be32_to_cpu(tmp->h_blocktype);
sequence = be32_to_cpu(tmp->h_sequence);
jbd2_debug(3, "Found magic %d, sequence %d\n",
blocktype, sequence);
- if (sequence != next_commit_ID) {
- brelse(bh);
+ if (sequence != next_commit_ID)
break;
- }
/* OK, we have a valid descriptor block which matches
* all of the sequence number checks. What are we going
@@ -603,7 +601,6 @@ static int do_one_pass(journal_t *journal,
pr_err("JBD2: Invalid checksum recovering block %lu in log\n",
next_log_block);
err = -EFSBADCRC;
- brelse(bh);
goto failed;
}
need_check_commit_time = true;
@@ -622,16 +619,12 @@ static int do_one_pass(journal_t *journal,
!info->end_transaction) {
if (calc_chksums(journal, bh,
&next_log_block,
- &crc32_sum)) {
- put_bh(bh);
+ &crc32_sum))
break;
- }
- put_bh(bh);
continue;
}
next_log_block += count_tags(journal, bh);
wrap(journal, next_log_block);
- put_bh(bh);
continue;
}
@@ -701,7 +694,6 @@ static int do_one_pass(journal_t *journal,
"JBD2: Out of memory "
"during recovery.\n");
err = -ENOMEM;
- brelse(bh);
brelse(obh);
goto failed;
}
@@ -733,7 +725,6 @@ static int do_one_pass(journal_t *journal,
break;
}
- brelse(bh);
continue;
case JBD2_COMMIT_BLOCK:
@@ -781,7 +772,6 @@ static int do_one_pass(journal_t *journal,
pr_err("JBD2: Invalid checksum found in transaction %u\n",
next_commit_ID);
err = -EFSBADCRC;
- brelse(bh);
goto failed;
}
ignore_crc_mismatch:
@@ -791,7 +781,6 @@ static int do_one_pass(journal_t *journal,
*/
jbd2_debug(1, "JBD2: Invalid checksum ignored in transaction %u, likely stale data\n",
next_commit_ID);
- brelse(bh);
goto done;
}
@@ -811,7 +800,6 @@ static int do_one_pass(journal_t *journal,
if (info->end_transaction) {
journal->j_failed_commit =
info->end_transaction;
- brelse(bh);
break;
}
@@ -847,7 +835,6 @@ static int do_one_pass(journal_t *journal,
if (!jbd2_has_feature_async_commit(journal)) {
journal->j_failed_commit =
next_commit_ID;
- brelse(bh);
break;
}
}
@@ -856,7 +843,6 @@ static int do_one_pass(journal_t *journal,
last_trans_commit_time = commit_time;
head_block = next_log_block;
}
- brelse(bh);
next_commit_ID++;
continue;
@@ -875,14 +861,11 @@ static int do_one_pass(journal_t *journal,
/* If we aren't in the REVOKE pass, then we can
* just skip over this block. */
- if (pass != PASS_REVOKE) {
- brelse(bh);
+ if (pass != PASS_REVOKE)
continue;
- }
err = scan_revoke_records(journal, bh,
next_commit_ID, info);
- brelse(bh);
if (err)
goto failed;
continue;
@@ -890,12 +873,12 @@ static int do_one_pass(journal_t *journal,
default:
jbd2_debug(3, "Unrecognised magic %d, end of scan.\n",
blocktype);
- brelse(bh);
goto done;
}
}
done:
+ brelse(bh);
/*
* We broke out of the log scan loop: either we came to the
* known end of the log or we found an unexpected block in the
@@ -931,6 +914,7 @@ static int do_one_pass(journal_t *journal,
return success;
failed:
+ brelse(bh);
return err;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/6] jbd2: refactor JBD2_COMMIT_BLOCK process in do_one_pass()
2024-09-30 0:59 [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover Ye Bin
2024-09-30 0:59 ` [PATCH v2 1/6] jbd2: remove redundant judgments for check v1 checksum Ye Bin
2024-09-30 0:59 ` [PATCH v2 2/6] jbd2: unified release of buffer_head in do_one_pass() Ye Bin
@ 2024-09-30 0:59 ` Ye Bin
2024-09-30 2:37 ` Zhang Yi
2024-09-30 0:59 ` [PATCH v2 4/6] jbd2: factor out jbd2_do_replay() Ye Bin
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Ye Bin @ 2024-09-30 0:59 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4; +Cc: jack, zhangxiaoxu5
From: Ye Bin <yebin10@huawei.com>
To make JBD2_COMMIT_BLOCK process more clean, no functional change.
Signed-off-by: Ye Bin <yebin10@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
fs/jbd2/recovery.c | 55 ++++++++++++++++++++++++----------------------
1 file changed, 29 insertions(+), 26 deletions(-)
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 0adf0cb31a03..0d697979d83e 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -728,6 +728,11 @@ static int do_one_pass(journal_t *journal,
continue;
case JBD2_COMMIT_BLOCK:
+ if (pass != PASS_SCAN) {
+ next_commit_ID++;
+ continue;
+ }
+
/* How to differentiate between interrupted commit
* and journal corruption ?
*
@@ -790,8 +795,7 @@ static int do_one_pass(journal_t *journal,
* much to do other than move on to the next sequence
* number.
*/
- if (pass == PASS_SCAN &&
- jbd2_has_feature_checksum(journal)) {
+ if (jbd2_has_feature_checksum(journal)) {
struct commit_header *cbh =
(struct commit_header *)bh->b_data;
unsigned found_chksum =
@@ -815,34 +819,33 @@ static int do_one_pass(journal_t *journal,
goto chksum_error;
crc32_sum = ~0;
+ goto chksum_ok;
}
- if (pass == PASS_SCAN &&
- !jbd2_commit_block_csum_verify(journal,
- bh->b_data)) {
- if (jbd2_commit_block_csum_verify_partial(
- journal,
- bh->b_data)) {
- pr_notice("JBD2: Find incomplete commit block in transaction %u block %lu\n",
- next_commit_ID, next_log_block);
- goto chksum_ok;
- }
- chksum_error:
- if (commit_time < last_trans_commit_time)
- goto ignore_crc_mismatch;
- info->end_transaction = next_commit_ID;
- info->head_block = head_block;
- if (!jbd2_has_feature_async_commit(journal)) {
- journal->j_failed_commit =
- next_commit_ID;
- break;
- }
+ if (jbd2_commit_block_csum_verify(journal, bh->b_data))
+ goto chksum_ok;
+
+ if (jbd2_commit_block_csum_verify_partial(journal,
+ bh->b_data)) {
+ pr_notice("JBD2: Find incomplete commit block in transaction %u block %lu\n",
+ next_commit_ID, next_log_block);
+ goto chksum_ok;
}
- if (pass == PASS_SCAN) {
- chksum_ok:
- last_trans_commit_time = commit_time;
- head_block = next_log_block;
+
+chksum_error:
+ if (commit_time < last_trans_commit_time)
+ goto ignore_crc_mismatch;
+ info->end_transaction = next_commit_ID;
+ info->head_block = head_block;
+
+ if (!jbd2_has_feature_async_commit(journal)) {
+ journal->j_failed_commit = next_commit_ID;
+ break;
}
+
+chksum_ok:
+ last_trans_commit_time = commit_time;
+ head_block = next_log_block;
next_commit_ID++;
continue;
--
2.31.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 4/6] jbd2: factor out jbd2_do_replay()
2024-09-30 0:59 [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover Ye Bin
` (2 preceding siblings ...)
2024-09-30 0:59 ` [PATCH v2 3/6] jbd2: refactor JBD2_COMMIT_BLOCK process " Ye Bin
@ 2024-09-30 0:59 ` Ye Bin
2024-09-30 2:40 ` Zhang Yi
2024-09-30 10:30 ` Jan Kara
2024-09-30 0:59 ` [PATCH v2 5/6] jbd2: remove useless 'block_error' variable Ye Bin
` (2 subsequent siblings)
6 siblings, 2 replies; 16+ messages in thread
From: Ye Bin @ 2024-09-30 0:59 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4; +Cc: jack, zhangxiaoxu5
From: Ye Bin <yebin10@huawei.com>
Factor out jbd2_do_replay() no funtional change.
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
fs/jbd2/recovery.c | 219 +++++++++++++++++++++++----------------------
1 file changed, 110 insertions(+), 109 deletions(-)
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 0d697979d83e..046744d6239c 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -485,6 +485,105 @@ static int jbd2_block_tag_csum_verify(journal_t *j, journal_block_tag_t *tag,
return tag->t_checksum == cpu_to_be16(csum32);
}
+static __always_inline int jbd2_do_replay(journal_t *journal,
+ struct recovery_info *info,
+ struct buffer_head *bh,
+ unsigned long *next_log_block,
+ unsigned int next_commit_ID,
+ int *success, int *block_error)
+{
+ char *tagp;
+ int flags;
+ int err;
+ int tag_bytes = journal_tag_bytes(journal);
+ int descr_csum_size = 0;
+ unsigned long io_block;
+ journal_block_tag_t tag;
+ struct buffer_head *obh;
+ struct buffer_head *nbh;
+
+ if (jbd2_journal_has_csum_v2or3(journal))
+ descr_csum_size = sizeof(struct jbd2_journal_block_tail);
+
+ tagp = &bh->b_data[sizeof(journal_header_t)];
+ while (tagp - bh->b_data + tag_bytes <=
+ journal->j_blocksize - descr_csum_size) {
+
+ memcpy(&tag, tagp, sizeof(tag));
+ flags = be16_to_cpu(tag.t_flags);
+
+ io_block = (*next_log_block)++;
+ wrap(journal, *next_log_block);
+ err = jread(&obh, journal, io_block);
+ if (err) {
+ /* Recover what we can, but report failure at the end. */
+ *success = err;
+ pr_err("JBD2: IO error %d recovering block %lu in log\n",
+ err, io_block);
+ } else {
+ unsigned long long blocknr;
+
+ J_ASSERT(obh != NULL);
+ blocknr = read_tag_block(journal, &tag);
+
+ /* If the block has been revoked, then we're all done here. */
+ if (jbd2_journal_test_revoke(journal, blocknr,
+ next_commit_ID)) {
+ brelse(obh);
+ ++info->nr_revoke_hits;
+ goto skip_write;
+ }
+
+ /* Look for block corruption */
+ if (!jbd2_block_tag_csum_verify(journal, &tag,
+ (journal_block_tag3_t *)tagp,
+ obh->b_data, next_commit_ID)) {
+ brelse(obh);
+ *success = -EFSBADCRC;
+ pr_err("JBD2: Invalid checksum recovering data block %llu in journal block %lu\n",
+ blocknr, io_block);
+ *block_error = 1;
+ goto skip_write;
+ }
+
+ /* Find a buffer for the new data being restored */
+ nbh = __getblk(journal->j_fs_dev, blocknr,
+ journal->j_blocksize);
+ if (nbh == NULL) {
+ pr_err("JBD2: Out of memory during recovery.\n");
+ brelse(obh);
+ return -ENOMEM;
+ }
+
+ lock_buffer(nbh);
+ memcpy(nbh->b_data, obh->b_data, journal->j_blocksize);
+ if (flags & JBD2_FLAG_ESCAPE) {
+ *((__be32 *)nbh->b_data) =
+ cpu_to_be32(JBD2_MAGIC_NUMBER);
+ }
+
+ BUFFER_TRACE(nbh, "marking dirty");
+ set_buffer_uptodate(nbh);
+ mark_buffer_dirty(nbh);
+ BUFFER_TRACE(nbh, "marking uptodate");
+ ++info->nr_replays;
+ unlock_buffer(nbh);
+ brelse(obh);
+ brelse(nbh);
+ }
+
+skip_write:
+ tagp += tag_bytes;
+ if (!(flags & JBD2_FLAG_SAME_UUID))
+ tagp += 16;
+
+ if (flags & JBD2_FLAG_LAST_TAG)
+ break;
+ }
+
+ return 0;
+}
+
static int do_one_pass(journal_t *journal,
struct recovery_info *info, enum passtype pass)
{
@@ -496,9 +595,7 @@ static int do_one_pass(journal_t *journal,
struct buffer_head *bh = NULL;
unsigned int sequence;
int blocktype;
- int tag_bytes = journal_tag_bytes(journal);
__u32 crc32_sum = ~0; /* Transactional Checksums */
- int descr_csum_size = 0;
int block_error = 0;
bool need_check_commit_time = false;
__u64 last_trans_commit_time = 0, commit_time;
@@ -528,12 +625,6 @@ static int do_one_pass(journal_t *journal,
*/
while (1) {
- int flags;
- char * tagp;
- journal_block_tag_t tag;
- struct buffer_head * obh;
- struct buffer_head * nbh;
-
cond_resched();
/* If we already know where to stop the log traversal,
@@ -587,11 +678,7 @@ static int do_one_pass(journal_t *journal,
switch(blocktype) {
case JBD2_DESCRIPTOR_BLOCK:
/* Verify checksum first */
- if (jbd2_journal_has_csum_v2or3(journal))
- descr_csum_size =
- sizeof(struct jbd2_journal_block_tail);
- if (descr_csum_size > 0 &&
- !jbd2_descriptor_block_csum_verify(journal,
+ if (!jbd2_descriptor_block_csum_verify(journal,
bh->b_data)) {
/*
* PASS_SCAN can see stale blocks due to lazy
@@ -628,102 +715,16 @@ static int do_one_pass(journal_t *journal,
continue;
}
- /* A descriptor block: we can now write all of
- * the data blocks. Yay, useful work is finally
- * getting done here! */
-
- tagp = &bh->b_data[sizeof(journal_header_t)];
- while ((tagp - bh->b_data + tag_bytes)
- <= journal->j_blocksize - descr_csum_size) {
- unsigned long io_block;
-
- memcpy(&tag, tagp, sizeof(tag));
- flags = be16_to_cpu(tag.t_flags);
-
- io_block = next_log_block++;
- wrap(journal, next_log_block);
- err = jread(&obh, journal, io_block);
- if (err) {
- /* Recover what we can, but
- * report failure at the end. */
- success = err;
- printk(KERN_ERR
- "JBD2: IO error %d recovering "
- "block %lu in log\n",
- err, io_block);
- } else {
- unsigned long long blocknr;
-
- J_ASSERT(obh != NULL);
- blocknr = read_tag_block(journal,
- &tag);
-
- /* If the block has been
- * revoked, then we're all done
- * here. */
- if (jbd2_journal_test_revoke
- (journal, blocknr,
- next_commit_ID)) {
- brelse(obh);
- ++info->nr_revoke_hits;
- goto skip_write;
- }
-
- /* Look for block corruption */
- if (!jbd2_block_tag_csum_verify(
- journal, &tag, (journal_block_tag3_t *)tagp,
- obh->b_data, be32_to_cpu(tmp->h_sequence))) {
- brelse(obh);
- success = -EFSBADCRC;
- printk(KERN_ERR "JBD2: Invalid "
- "checksum recovering "
- "data block %llu in "
- "journal block %lu\n",
- blocknr, io_block);
- block_error = 1;
- goto skip_write;
- }
-
- /* Find a buffer for the new
- * data being restored */
- nbh = __getblk(journal->j_fs_dev,
- blocknr,
- journal->j_blocksize);
- if (nbh == NULL) {
- printk(KERN_ERR
- "JBD2: Out of memory "
- "during recovery.\n");
- err = -ENOMEM;
- brelse(obh);
- goto failed;
- }
-
- lock_buffer(nbh);
- memcpy(nbh->b_data, obh->b_data,
- journal->j_blocksize);
- if (flags & JBD2_FLAG_ESCAPE) {
- *((__be32 *)nbh->b_data) =
- cpu_to_be32(JBD2_MAGIC_NUMBER);
- }
-
- BUFFER_TRACE(nbh, "marking dirty");
- set_buffer_uptodate(nbh);
- mark_buffer_dirty(nbh);
- BUFFER_TRACE(nbh, "marking uptodate");
- ++info->nr_replays;
- unlock_buffer(nbh);
- brelse(obh);
- brelse(nbh);
- }
-
- skip_write:
- tagp += tag_bytes;
- if (!(flags & JBD2_FLAG_SAME_UUID))
- tagp += 16;
-
- if (flags & JBD2_FLAG_LAST_TAG)
- break;
- }
+ /*
+ * A descriptor block: we can now write all of the
+ * data blocks. Yay, useful work is finally getting
+ * done here!
+ */
+ err = jbd2_do_replay(journal, info, bh, &next_log_block,
+ next_commit_ID, &success,
+ &block_error);
+ if (err)
+ goto failed;
continue;
--
2.31.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 5/6] jbd2: remove useless 'block_error' variable
2024-09-30 0:59 [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover Ye Bin
` (3 preceding siblings ...)
2024-09-30 0:59 ` [PATCH v2 4/6] jbd2: factor out jbd2_do_replay() Ye Bin
@ 2024-09-30 0:59 ` Ye Bin
2024-09-30 2:41 ` Zhang Yi
2024-09-30 0:59 ` [PATCH v2 6/6] jbd2: remove the 'success' parameter from the jbd2_do_replay() function Ye Bin
2024-11-07 15:13 ` [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover Theodore Ts'o
6 siblings, 1 reply; 16+ messages in thread
From: Ye Bin @ 2024-09-30 0:59 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4; +Cc: jack, zhangxiaoxu5
From: Ye Bin <yebin10@huawei.com>
The judgement 'if (block_error && success == 0)' is never valid. Just
remove useless 'block_error' variable.
Signed-off-by: Ye Bin <yebin10@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
fs/jbd2/recovery.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 046744d6239c..4f1e9ca34503 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -490,7 +490,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
struct buffer_head *bh,
unsigned long *next_log_block,
unsigned int next_commit_ID,
- int *success, int *block_error)
+ int *success)
{
char *tagp;
int flags;
@@ -542,7 +542,6 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
*success = -EFSBADCRC;
pr_err("JBD2: Invalid checksum recovering data block %llu in journal block %lu\n",
blocknr, io_block);
- *block_error = 1;
goto skip_write;
}
@@ -596,7 +595,6 @@ static int do_one_pass(journal_t *journal,
unsigned int sequence;
int blocktype;
__u32 crc32_sum = ~0; /* Transactional Checksums */
- int block_error = 0;
bool need_check_commit_time = false;
__u64 last_trans_commit_time = 0, commit_time;
@@ -721,8 +719,7 @@ static int do_one_pass(journal_t *journal,
* done here!
*/
err = jbd2_do_replay(journal, info, bh, &next_log_block,
- next_commit_ID, &success,
- &block_error);
+ next_commit_ID, &success);
if (err)
goto failed;
@@ -913,8 +910,6 @@ static int do_one_pass(journal_t *journal,
success = err;
}
- if (block_error && success == 0)
- success = -EIO;
return success;
failed:
--
2.31.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 6/6] jbd2: remove the 'success' parameter from the jbd2_do_replay() function
2024-09-30 0:59 [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover Ye Bin
` (4 preceding siblings ...)
2024-09-30 0:59 ` [PATCH v2 5/6] jbd2: remove useless 'block_error' variable Ye Bin
@ 2024-09-30 0:59 ` Ye Bin
2024-09-30 2:43 ` Zhang Yi
2024-09-30 10:32 ` Jan Kara
2024-11-07 15:13 ` [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover Theodore Ts'o
6 siblings, 2 replies; 16+ messages in thread
From: Ye Bin @ 2024-09-30 0:59 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4; +Cc: jack, zhangxiaoxu5
From: Ye Bin <yebin10@huawei.com>
Keep 'success' internally to track if any error happened and then
return it at the end in do_one_pass(). If jbd2_do_replay() return
-ENOMEM then stop replay journal.
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
fs/jbd2/recovery.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 4f1e9ca34503..9192be7c19d8 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -489,12 +489,11 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
struct recovery_info *info,
struct buffer_head *bh,
unsigned long *next_log_block,
- unsigned int next_commit_ID,
- int *success)
+ unsigned int next_commit_ID)
{
char *tagp;
int flags;
- int err;
+ int ret = 0;
int tag_bytes = journal_tag_bytes(journal);
int descr_csum_size = 0;
unsigned long io_block;
@@ -508,6 +507,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
tagp = &bh->b_data[sizeof(journal_header_t)];
while (tagp - bh->b_data + tag_bytes <=
journal->j_blocksize - descr_csum_size) {
+ int err;
memcpy(&tag, tagp, sizeof(tag));
flags = be16_to_cpu(tag.t_flags);
@@ -517,7 +517,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
err = jread(&obh, journal, io_block);
if (err) {
/* Recover what we can, but report failure at the end. */
- *success = err;
+ ret = err;
pr_err("JBD2: IO error %d recovering block %lu in log\n",
err, io_block);
} else {
@@ -539,7 +539,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
(journal_block_tag3_t *)tagp,
obh->b_data, next_commit_ID)) {
brelse(obh);
- *success = -EFSBADCRC;
+ ret = -EFSBADCRC;
pr_err("JBD2: Invalid checksum recovering data block %llu in journal block %lu\n",
blocknr, io_block);
goto skip_write;
@@ -580,7 +580,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
break;
}
- return 0;
+ return ret;
}
static int do_one_pass(journal_t *journal,
@@ -719,9 +719,12 @@ static int do_one_pass(journal_t *journal,
* done here!
*/
err = jbd2_do_replay(journal, info, bh, &next_log_block,
- next_commit_ID, &success);
- if (err)
- goto failed;
+ next_commit_ID);
+ if (err) {
+ if (err == -ENOMEM)
+ goto failed;
+ success = err;
+ }
continue;
--
2.31.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/6] jbd2: remove redundant judgments for check v1 checksum
2024-09-30 0:59 ` [PATCH v2 1/6] jbd2: remove redundant judgments for check v1 checksum Ye Bin
@ 2024-09-30 2:36 ` Zhang Yi
0 siblings, 0 replies; 16+ messages in thread
From: Zhang Yi @ 2024-09-30 2:36 UTC (permalink / raw)
To: Ye Bin; +Cc: jack, zhangxiaoxu5, tytso, adilger.kernel, linux-ext4
On 2024/9/30 8:59, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> 'need_check_commit_time' is only used by v2/v3 checksum, so there isn't
> need to add 'need_check_commit_time' judegement for v1 checksum logic.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
Looks good to me.
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> ---
> fs/jbd2/recovery.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 667f67342c52..5efbca6a98c4 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -619,7 +619,6 @@ static int do_one_pass(journal_t *journal,
> if (pass != PASS_REPLAY) {
> if (pass == PASS_SCAN &&
> jbd2_has_feature_checksum(journal) &&
> - !need_check_commit_time &&
> !info->end_transaction) {
> if (calc_chksums(journal, bh,
> &next_log_block,
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/6] jbd2: unified release of buffer_head in do_one_pass()
2024-09-30 0:59 ` [PATCH v2 2/6] jbd2: unified release of buffer_head in do_one_pass() Ye Bin
@ 2024-09-30 2:36 ` Zhang Yi
0 siblings, 0 replies; 16+ messages in thread
From: Zhang Yi @ 2024-09-30 2:36 UTC (permalink / raw)
To: Ye Bin; +Cc: jack, zhangxiaoxu5, tytso, adilger.kernel, linux-ext4
On 2024/9/30 8:59, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> Now buffer_head free is very fragmented in do_one_pass(), unified release
> of buffer_head in do_one_pass()
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
Looks good to me.
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> ---
> fs/jbd2/recovery.c | 34 +++++++++-------------------------
> 1 file changed, 9 insertions(+), 25 deletions(-)
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 5efbca6a98c4..0adf0cb31a03 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -493,7 +493,7 @@ static int do_one_pass(journal_t *journal,
> int err, success = 0;
> journal_superblock_t * sb;
> journal_header_t * tmp;
> - struct buffer_head * bh;
> + struct buffer_head *bh = NULL;
> unsigned int sequence;
> int blocktype;
> int tag_bytes = journal_tag_bytes(journal);
> @@ -552,6 +552,8 @@ static int do_one_pass(journal_t *journal,
> * record. */
>
> jbd2_debug(3, "JBD2: checking block %ld\n", next_log_block);
> + brelse(bh);
> + bh = NULL;
> err = jread(&bh, journal, next_log_block);
> if (err)
> goto failed;
> @@ -567,20 +569,16 @@ static int do_one_pass(journal_t *journal,
>
> tmp = (journal_header_t *)bh->b_data;
>
> - if (tmp->h_magic != cpu_to_be32(JBD2_MAGIC_NUMBER)) {
> - brelse(bh);
> + if (tmp->h_magic != cpu_to_be32(JBD2_MAGIC_NUMBER))
> break;
> - }
>
> blocktype = be32_to_cpu(tmp->h_blocktype);
> sequence = be32_to_cpu(tmp->h_sequence);
> jbd2_debug(3, "Found magic %d, sequence %d\n",
> blocktype, sequence);
>
> - if (sequence != next_commit_ID) {
> - brelse(bh);
> + if (sequence != next_commit_ID)
> break;
> - }
>
> /* OK, we have a valid descriptor block which matches
> * all of the sequence number checks. What are we going
> @@ -603,7 +601,6 @@ static int do_one_pass(journal_t *journal,
> pr_err("JBD2: Invalid checksum recovering block %lu in log\n",
> next_log_block);
> err = -EFSBADCRC;
> - brelse(bh);
> goto failed;
> }
> need_check_commit_time = true;
> @@ -622,16 +619,12 @@ static int do_one_pass(journal_t *journal,
> !info->end_transaction) {
> if (calc_chksums(journal, bh,
> &next_log_block,
> - &crc32_sum)) {
> - put_bh(bh);
> + &crc32_sum))
> break;
> - }
> - put_bh(bh);
> continue;
> }
> next_log_block += count_tags(journal, bh);
> wrap(journal, next_log_block);
> - put_bh(bh);
> continue;
> }
>
> @@ -701,7 +694,6 @@ static int do_one_pass(journal_t *journal,
> "JBD2: Out of memory "
> "during recovery.\n");
> err = -ENOMEM;
> - brelse(bh);
> brelse(obh);
> goto failed;
> }
> @@ -733,7 +725,6 @@ static int do_one_pass(journal_t *journal,
> break;
> }
>
> - brelse(bh);
> continue;
>
> case JBD2_COMMIT_BLOCK:
> @@ -781,7 +772,6 @@ static int do_one_pass(journal_t *journal,
> pr_err("JBD2: Invalid checksum found in transaction %u\n",
> next_commit_ID);
> err = -EFSBADCRC;
> - brelse(bh);
> goto failed;
> }
> ignore_crc_mismatch:
> @@ -791,7 +781,6 @@ static int do_one_pass(journal_t *journal,
> */
> jbd2_debug(1, "JBD2: Invalid checksum ignored in transaction %u, likely stale data\n",
> next_commit_ID);
> - brelse(bh);
> goto done;
> }
>
> @@ -811,7 +800,6 @@ static int do_one_pass(journal_t *journal,
> if (info->end_transaction) {
> journal->j_failed_commit =
> info->end_transaction;
> - brelse(bh);
> break;
> }
>
> @@ -847,7 +835,6 @@ static int do_one_pass(journal_t *journal,
> if (!jbd2_has_feature_async_commit(journal)) {
> journal->j_failed_commit =
> next_commit_ID;
> - brelse(bh);
> break;
> }
> }
> @@ -856,7 +843,6 @@ static int do_one_pass(journal_t *journal,
> last_trans_commit_time = commit_time;
> head_block = next_log_block;
> }
> - brelse(bh);
> next_commit_ID++;
> continue;
>
> @@ -875,14 +861,11 @@ static int do_one_pass(journal_t *journal,
>
> /* If we aren't in the REVOKE pass, then we can
> * just skip over this block. */
> - if (pass != PASS_REVOKE) {
> - brelse(bh);
> + if (pass != PASS_REVOKE)
> continue;
> - }
>
> err = scan_revoke_records(journal, bh,
> next_commit_ID, info);
> - brelse(bh);
> if (err)
> goto failed;
> continue;
> @@ -890,12 +873,12 @@ static int do_one_pass(journal_t *journal,
> default:
> jbd2_debug(3, "Unrecognised magic %d, end of scan.\n",
> blocktype);
> - brelse(bh);
> goto done;
> }
> }
>
> done:
> + brelse(bh);
> /*
> * We broke out of the log scan loop: either we came to the
> * known end of the log or we found an unexpected block in the
> @@ -931,6 +914,7 @@ static int do_one_pass(journal_t *journal,
> return success;
>
> failed:
> + brelse(bh);
> return err;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/6] jbd2: refactor JBD2_COMMIT_BLOCK process in do_one_pass()
2024-09-30 0:59 ` [PATCH v2 3/6] jbd2: refactor JBD2_COMMIT_BLOCK process " Ye Bin
@ 2024-09-30 2:37 ` Zhang Yi
0 siblings, 0 replies; 16+ messages in thread
From: Zhang Yi @ 2024-09-30 2:37 UTC (permalink / raw)
To: Ye Bin; +Cc: jack, zhangxiaoxu5, tytso, adilger.kernel, linux-ext4
On 2024/9/30 8:59, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> To make JBD2_COMMIT_BLOCK process more clean, no functional change.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
Looks good to me.
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> ---
> fs/jbd2/recovery.c | 55 ++++++++++++++++++++++++----------------------
> 1 file changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 0adf0cb31a03..0d697979d83e 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -728,6 +728,11 @@ static int do_one_pass(journal_t *journal,
> continue;
>
> case JBD2_COMMIT_BLOCK:
> + if (pass != PASS_SCAN) {
> + next_commit_ID++;
> + continue;
> + }
> +
> /* How to differentiate between interrupted commit
> * and journal corruption ?
> *
> @@ -790,8 +795,7 @@ static int do_one_pass(journal_t *journal,
> * much to do other than move on to the next sequence
> * number.
> */
> - if (pass == PASS_SCAN &&
> - jbd2_has_feature_checksum(journal)) {
> + if (jbd2_has_feature_checksum(journal)) {
> struct commit_header *cbh =
> (struct commit_header *)bh->b_data;
> unsigned found_chksum =
> @@ -815,34 +819,33 @@ static int do_one_pass(journal_t *journal,
> goto chksum_error;
>
> crc32_sum = ~0;
> + goto chksum_ok;
> }
> - if (pass == PASS_SCAN &&
> - !jbd2_commit_block_csum_verify(journal,
> - bh->b_data)) {
> - if (jbd2_commit_block_csum_verify_partial(
> - journal,
> - bh->b_data)) {
> - pr_notice("JBD2: Find incomplete commit block in transaction %u block %lu\n",
> - next_commit_ID, next_log_block);
> - goto chksum_ok;
> - }
> - chksum_error:
> - if (commit_time < last_trans_commit_time)
> - goto ignore_crc_mismatch;
> - info->end_transaction = next_commit_ID;
> - info->head_block = head_block;
>
> - if (!jbd2_has_feature_async_commit(journal)) {
> - journal->j_failed_commit =
> - next_commit_ID;
> - break;
> - }
> + if (jbd2_commit_block_csum_verify(journal, bh->b_data))
> + goto chksum_ok;
> +
> + if (jbd2_commit_block_csum_verify_partial(journal,
> + bh->b_data)) {
> + pr_notice("JBD2: Find incomplete commit block in transaction %u block %lu\n",
> + next_commit_ID, next_log_block);
> + goto chksum_ok;
> }
> - if (pass == PASS_SCAN) {
> - chksum_ok:
> - last_trans_commit_time = commit_time;
> - head_block = next_log_block;
> +
> +chksum_error:
> + if (commit_time < last_trans_commit_time)
> + goto ignore_crc_mismatch;
> + info->end_transaction = next_commit_ID;
> + info->head_block = head_block;
> +
> + if (!jbd2_has_feature_async_commit(journal)) {
> + journal->j_failed_commit = next_commit_ID;
> + break;
> }
> +
> +chksum_ok:
> + last_trans_commit_time = commit_time;
> + head_block = next_log_block;
> next_commit_ID++;
> continue;
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/6] jbd2: factor out jbd2_do_replay()
2024-09-30 0:59 ` [PATCH v2 4/6] jbd2: factor out jbd2_do_replay() Ye Bin
@ 2024-09-30 2:40 ` Zhang Yi
2024-09-30 10:30 ` Jan Kara
1 sibling, 0 replies; 16+ messages in thread
From: Zhang Yi @ 2024-09-30 2:40 UTC (permalink / raw)
To: Ye Bin; +Cc: jack, zhangxiaoxu5, tytso, adilger.kernel, linux-ext4
On 2024/9/30 8:59, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> Factor out jbd2_do_replay() no funtional change.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
Looks good to me.
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> ---
> fs/jbd2/recovery.c | 219 +++++++++++++++++++++++----------------------
> 1 file changed, 110 insertions(+), 109 deletions(-)
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 0d697979d83e..046744d6239c 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -485,6 +485,105 @@ static int jbd2_block_tag_csum_verify(journal_t *j, journal_block_tag_t *tag,
> return tag->t_checksum == cpu_to_be16(csum32);
> }
>
> +static __always_inline int jbd2_do_replay(journal_t *journal,
> + struct recovery_info *info,
> + struct buffer_head *bh,
> + unsigned long *next_log_block,
> + unsigned int next_commit_ID,
> + int *success, int *block_error)
> +{
> + char *tagp;
> + int flags;
> + int err;
> + int tag_bytes = journal_tag_bytes(journal);
> + int descr_csum_size = 0;
> + unsigned long io_block;
> + journal_block_tag_t tag;
> + struct buffer_head *obh;
> + struct buffer_head *nbh;
> +
> + if (jbd2_journal_has_csum_v2or3(journal))
> + descr_csum_size = sizeof(struct jbd2_journal_block_tail);
> +
> + tagp = &bh->b_data[sizeof(journal_header_t)];
> + while (tagp - bh->b_data + tag_bytes <=
> + journal->j_blocksize - descr_csum_size) {
> +
> + memcpy(&tag, tagp, sizeof(tag));
> + flags = be16_to_cpu(tag.t_flags);
> +
> + io_block = (*next_log_block)++;
> + wrap(journal, *next_log_block);
> + err = jread(&obh, journal, io_block);
> + if (err) {
> + /* Recover what we can, but report failure at the end. */
> + *success = err;
> + pr_err("JBD2: IO error %d recovering block %lu in log\n",
> + err, io_block);
> + } else {
> + unsigned long long blocknr;
> +
> + J_ASSERT(obh != NULL);
> + blocknr = read_tag_block(journal, &tag);
> +
> + /* If the block has been revoked, then we're all done here. */
> + if (jbd2_journal_test_revoke(journal, blocknr,
> + next_commit_ID)) {
> + brelse(obh);
> + ++info->nr_revoke_hits;
> + goto skip_write;
> + }
> +
> + /* Look for block corruption */
> + if (!jbd2_block_tag_csum_verify(journal, &tag,
> + (journal_block_tag3_t *)tagp,
> + obh->b_data, next_commit_ID)) {
> + brelse(obh);
> + *success = -EFSBADCRC;
> + pr_err("JBD2: Invalid checksum recovering data block %llu in journal block %lu\n",
> + blocknr, io_block);
> + *block_error = 1;
> + goto skip_write;
> + }
> +
> + /* Find a buffer for the new data being restored */
> + nbh = __getblk(journal->j_fs_dev, blocknr,
> + journal->j_blocksize);
> + if (nbh == NULL) {
> + pr_err("JBD2: Out of memory during recovery.\n");
> + brelse(obh);
> + return -ENOMEM;
> + }
> +
> + lock_buffer(nbh);
> + memcpy(nbh->b_data, obh->b_data, journal->j_blocksize);
> + if (flags & JBD2_FLAG_ESCAPE) {
> + *((__be32 *)nbh->b_data) =
> + cpu_to_be32(JBD2_MAGIC_NUMBER);
> + }
> +
> + BUFFER_TRACE(nbh, "marking dirty");
> + set_buffer_uptodate(nbh);
> + mark_buffer_dirty(nbh);
> + BUFFER_TRACE(nbh, "marking uptodate");
> + ++info->nr_replays;
> + unlock_buffer(nbh);
> + brelse(obh);
> + brelse(nbh);
> + }
> +
> +skip_write:
> + tagp += tag_bytes;
> + if (!(flags & JBD2_FLAG_SAME_UUID))
> + tagp += 16;
> +
> + if (flags & JBD2_FLAG_LAST_TAG)
> + break;
> + }
> +
> + return 0;
> +}
> +
> static int do_one_pass(journal_t *journal,
> struct recovery_info *info, enum passtype pass)
> {
> @@ -496,9 +595,7 @@ static int do_one_pass(journal_t *journal,
> struct buffer_head *bh = NULL;
> unsigned int sequence;
> int blocktype;
> - int tag_bytes = journal_tag_bytes(journal);
> __u32 crc32_sum = ~0; /* Transactional Checksums */
> - int descr_csum_size = 0;
> int block_error = 0;
> bool need_check_commit_time = false;
> __u64 last_trans_commit_time = 0, commit_time;
> @@ -528,12 +625,6 @@ static int do_one_pass(journal_t *journal,
> */
>
> while (1) {
> - int flags;
> - char * tagp;
> - journal_block_tag_t tag;
> - struct buffer_head * obh;
> - struct buffer_head * nbh;
> -
> cond_resched();
>
> /* If we already know where to stop the log traversal,
> @@ -587,11 +678,7 @@ static int do_one_pass(journal_t *journal,
> switch(blocktype) {
> case JBD2_DESCRIPTOR_BLOCK:
> /* Verify checksum first */
> - if (jbd2_journal_has_csum_v2or3(journal))
> - descr_csum_size =
> - sizeof(struct jbd2_journal_block_tail);
> - if (descr_csum_size > 0 &&
> - !jbd2_descriptor_block_csum_verify(journal,
> + if (!jbd2_descriptor_block_csum_verify(journal,
> bh->b_data)) {
> /*
> * PASS_SCAN can see stale blocks due to lazy
> @@ -628,102 +715,16 @@ static int do_one_pass(journal_t *journal,
> continue;
> }
>
> - /* A descriptor block: we can now write all of
> - * the data blocks. Yay, useful work is finally
> - * getting done here! */
> -
> - tagp = &bh->b_data[sizeof(journal_header_t)];
> - while ((tagp - bh->b_data + tag_bytes)
> - <= journal->j_blocksize - descr_csum_size) {
> - unsigned long io_block;
> -
> - memcpy(&tag, tagp, sizeof(tag));
> - flags = be16_to_cpu(tag.t_flags);
> -
> - io_block = next_log_block++;
> - wrap(journal, next_log_block);
> - err = jread(&obh, journal, io_block);
> - if (err) {
> - /* Recover what we can, but
> - * report failure at the end. */
> - success = err;
> - printk(KERN_ERR
> - "JBD2: IO error %d recovering "
> - "block %lu in log\n",
> - err, io_block);
> - } else {
> - unsigned long long blocknr;
> -
> - J_ASSERT(obh != NULL);
> - blocknr = read_tag_block(journal,
> - &tag);
> -
> - /* If the block has been
> - * revoked, then we're all done
> - * here. */
> - if (jbd2_journal_test_revoke
> - (journal, blocknr,
> - next_commit_ID)) {
> - brelse(obh);
> - ++info->nr_revoke_hits;
> - goto skip_write;
> - }
> -
> - /* Look for block corruption */
> - if (!jbd2_block_tag_csum_verify(
> - journal, &tag, (journal_block_tag3_t *)tagp,
> - obh->b_data, be32_to_cpu(tmp->h_sequence))) {
> - brelse(obh);
> - success = -EFSBADCRC;
> - printk(KERN_ERR "JBD2: Invalid "
> - "checksum recovering "
> - "data block %llu in "
> - "journal block %lu\n",
> - blocknr, io_block);
> - block_error = 1;
> - goto skip_write;
> - }
> -
> - /* Find a buffer for the new
> - * data being restored */
> - nbh = __getblk(journal->j_fs_dev,
> - blocknr,
> - journal->j_blocksize);
> - if (nbh == NULL) {
> - printk(KERN_ERR
> - "JBD2: Out of memory "
> - "during recovery.\n");
> - err = -ENOMEM;
> - brelse(obh);
> - goto failed;
> - }
> -
> - lock_buffer(nbh);
> - memcpy(nbh->b_data, obh->b_data,
> - journal->j_blocksize);
> - if (flags & JBD2_FLAG_ESCAPE) {
> - *((__be32 *)nbh->b_data) =
> - cpu_to_be32(JBD2_MAGIC_NUMBER);
> - }
> -
> - BUFFER_TRACE(nbh, "marking dirty");
> - set_buffer_uptodate(nbh);
> - mark_buffer_dirty(nbh);
> - BUFFER_TRACE(nbh, "marking uptodate");
> - ++info->nr_replays;
> - unlock_buffer(nbh);
> - brelse(obh);
> - brelse(nbh);
> - }
> -
> - skip_write:
> - tagp += tag_bytes;
> - if (!(flags & JBD2_FLAG_SAME_UUID))
> - tagp += 16;
> -
> - if (flags & JBD2_FLAG_LAST_TAG)
> - break;
> - }
> + /*
> + * A descriptor block: we can now write all of the
> + * data blocks. Yay, useful work is finally getting
> + * done here!
> + */
> + err = jbd2_do_replay(journal, info, bh, &next_log_block,
> + next_commit_ID, &success,
> + &block_error);
> + if (err)
> + goto failed;
>
> continue;
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/6] jbd2: remove useless 'block_error' variable
2024-09-30 0:59 ` [PATCH v2 5/6] jbd2: remove useless 'block_error' variable Ye Bin
@ 2024-09-30 2:41 ` Zhang Yi
0 siblings, 0 replies; 16+ messages in thread
From: Zhang Yi @ 2024-09-30 2:41 UTC (permalink / raw)
To: Ye Bin; +Cc: jack, zhangxiaoxu5, tytso, adilger.kernel, linux-ext4
On 2024/9/30 8:59, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> The judgement 'if (block_error && success == 0)' is never valid. Just
> remove useless 'block_error' variable.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
Looks good to me.
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> ---
> fs/jbd2/recovery.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 046744d6239c..4f1e9ca34503 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -490,7 +490,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> struct buffer_head *bh,
> unsigned long *next_log_block,
> unsigned int next_commit_ID,
> - int *success, int *block_error)
> + int *success)
> {
> char *tagp;
> int flags;
> @@ -542,7 +542,6 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> *success = -EFSBADCRC;
> pr_err("JBD2: Invalid checksum recovering data block %llu in journal block %lu\n",
> blocknr, io_block);
> - *block_error = 1;
> goto skip_write;
> }
>
> @@ -596,7 +595,6 @@ static int do_one_pass(journal_t *journal,
> unsigned int sequence;
> int blocktype;
> __u32 crc32_sum = ~0; /* Transactional Checksums */
> - int block_error = 0;
> bool need_check_commit_time = false;
> __u64 last_trans_commit_time = 0, commit_time;
>
> @@ -721,8 +719,7 @@ static int do_one_pass(journal_t *journal,
> * done here!
> */
> err = jbd2_do_replay(journal, info, bh, &next_log_block,
> - next_commit_ID, &success,
> - &block_error);
> + next_commit_ID, &success);
> if (err)
> goto failed;
>
> @@ -913,8 +910,6 @@ static int do_one_pass(journal_t *journal,
> success = err;
> }
>
> - if (block_error && success == 0)
> - success = -EIO;
> return success;
>
> failed:
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 6/6] jbd2: remove the 'success' parameter from the jbd2_do_replay() function
2024-09-30 0:59 ` [PATCH v2 6/6] jbd2: remove the 'success' parameter from the jbd2_do_replay() function Ye Bin
@ 2024-09-30 2:43 ` Zhang Yi
2024-09-30 10:32 ` Jan Kara
1 sibling, 0 replies; 16+ messages in thread
From: Zhang Yi @ 2024-09-30 2:43 UTC (permalink / raw)
To: Ye Bin; +Cc: jack, zhangxiaoxu5, tytso, adilger.kernel, linux-ext4
On 2024/9/30 8:59, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> Keep 'success' internally to track if any error happened and then
> return it at the end in do_one_pass(). If jbd2_do_replay() return
> -ENOMEM then stop replay journal.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
Looks good to me.
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> ---
> fs/jbd2/recovery.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 4f1e9ca34503..9192be7c19d8 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -489,12 +489,11 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> struct recovery_info *info,
> struct buffer_head *bh,
> unsigned long *next_log_block,
> - unsigned int next_commit_ID,
> - int *success)
> + unsigned int next_commit_ID)
> {
> char *tagp;
> int flags;
> - int err;
> + int ret = 0;
> int tag_bytes = journal_tag_bytes(journal);
> int descr_csum_size = 0;
> unsigned long io_block;
> @@ -508,6 +507,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> tagp = &bh->b_data[sizeof(journal_header_t)];
> while (tagp - bh->b_data + tag_bytes <=
> journal->j_blocksize - descr_csum_size) {
> + int err;
>
> memcpy(&tag, tagp, sizeof(tag));
> flags = be16_to_cpu(tag.t_flags);
> @@ -517,7 +517,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> err = jread(&obh, journal, io_block);
> if (err) {
> /* Recover what we can, but report failure at the end. */
> - *success = err;
> + ret = err;
> pr_err("JBD2: IO error %d recovering block %lu in log\n",
> err, io_block);
> } else {
> @@ -539,7 +539,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> (journal_block_tag3_t *)tagp,
> obh->b_data, next_commit_ID)) {
> brelse(obh);
> - *success = -EFSBADCRC;
> + ret = -EFSBADCRC;
> pr_err("JBD2: Invalid checksum recovering data block %llu in journal block %lu\n",
> blocknr, io_block);
> goto skip_write;
> @@ -580,7 +580,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> break;
> }
>
> - return 0;
> + return ret;
> }
>
> static int do_one_pass(journal_t *journal,
> @@ -719,9 +719,12 @@ static int do_one_pass(journal_t *journal,
> * done here!
> */
> err = jbd2_do_replay(journal, info, bh, &next_log_block,
> - next_commit_ID, &success);
> - if (err)
> - goto failed;
> + next_commit_ID);
> + if (err) {
> + if (err == -ENOMEM)
> + goto failed;
> + success = err;
> + }
>
> continue;
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/6] jbd2: factor out jbd2_do_replay()
2024-09-30 0:59 ` [PATCH v2 4/6] jbd2: factor out jbd2_do_replay() Ye Bin
2024-09-30 2:40 ` Zhang Yi
@ 2024-09-30 10:30 ` Jan Kara
1 sibling, 0 replies; 16+ messages in thread
From: Jan Kara @ 2024-09-30 10:30 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack, zhangxiaoxu5
On Mon 30-09-24 08:59:40, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> Factor out jbd2_do_replay() no funtional change.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Just one style nit below:
> + lock_buffer(nbh);
> + memcpy(nbh->b_data, obh->b_data, journal->j_blocksize);
> + if (flags & JBD2_FLAG_ESCAPE) {
> + *((__be32 *)nbh->b_data) =
> + cpu_to_be32(JBD2_MAGIC_NUMBER);
^^ this needs one more indent
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 6/6] jbd2: remove the 'success' parameter from the jbd2_do_replay() function
2024-09-30 0:59 ` [PATCH v2 6/6] jbd2: remove the 'success' parameter from the jbd2_do_replay() function Ye Bin
2024-09-30 2:43 ` Zhang Yi
@ 2024-09-30 10:32 ` Jan Kara
1 sibling, 0 replies; 16+ messages in thread
From: Jan Kara @ 2024-09-30 10:32 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack, zhangxiaoxu5
On Mon 30-09-24 08:59:42, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> Keep 'success' internally to track if any error happened and then
> return it at the end in do_one_pass(). If jbd2_do_replay() return
> -ENOMEM then stop replay journal.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
Nice. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/jbd2/recovery.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 4f1e9ca34503..9192be7c19d8 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -489,12 +489,11 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> struct recovery_info *info,
> struct buffer_head *bh,
> unsigned long *next_log_block,
> - unsigned int next_commit_ID,
> - int *success)
> + unsigned int next_commit_ID)
> {
> char *tagp;
> int flags;
> - int err;
> + int ret = 0;
> int tag_bytes = journal_tag_bytes(journal);
> int descr_csum_size = 0;
> unsigned long io_block;
> @@ -508,6 +507,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> tagp = &bh->b_data[sizeof(journal_header_t)];
> while (tagp - bh->b_data + tag_bytes <=
> journal->j_blocksize - descr_csum_size) {
> + int err;
>
> memcpy(&tag, tagp, sizeof(tag));
> flags = be16_to_cpu(tag.t_flags);
> @@ -517,7 +517,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> err = jread(&obh, journal, io_block);
> if (err) {
> /* Recover what we can, but report failure at the end. */
> - *success = err;
> + ret = err;
> pr_err("JBD2: IO error %d recovering block %lu in log\n",
> err, io_block);
> } else {
> @@ -539,7 +539,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> (journal_block_tag3_t *)tagp,
> obh->b_data, next_commit_ID)) {
> brelse(obh);
> - *success = -EFSBADCRC;
> + ret = -EFSBADCRC;
> pr_err("JBD2: Invalid checksum recovering data block %llu in journal block %lu\n",
> blocknr, io_block);
> goto skip_write;
> @@ -580,7 +580,7 @@ static __always_inline int jbd2_do_replay(journal_t *journal,
> break;
> }
>
> - return 0;
> + return ret;
> }
>
> static int do_one_pass(journal_t *journal,
> @@ -719,9 +719,12 @@ static int do_one_pass(journal_t *journal,
> * done here!
> */
> err = jbd2_do_replay(journal, info, bh, &next_log_block,
> - next_commit_ID, &success);
> - if (err)
> - goto failed;
> + next_commit_ID);
> + if (err) {
> + if (err == -ENOMEM)
> + goto failed;
> + success = err;
> + }
>
> continue;
>
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover
2024-09-30 0:59 [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover Ye Bin
` (5 preceding siblings ...)
2024-09-30 0:59 ` [PATCH v2 6/6] jbd2: remove the 'success' parameter from the jbd2_do_replay() function Ye Bin
@ 2024-11-07 15:13 ` Theodore Ts'o
6 siblings, 0 replies; 16+ messages in thread
From: Theodore Ts'o @ 2024-11-07 15:13 UTC (permalink / raw)
To: adilger.kernel, linux-ext4, Ye Bin; +Cc: Theodore Ts'o, jack, zhangxiaoxu5
On Mon, 30 Sep 2024 08:59:36 +0800, Ye Bin wrote:
> Diff v2 vs v1:
> 1. Modify the indentation problem and remove unnecessary braces in PATCH[4];
> 2. Add PATCH[6] to remove the 'success' parameter from the jbd2_do_replay();
>
> Ye Bin (6):
> jbd2: remove redundant judgments for check v1 checksum
> jbd2: unified release of buffer_head in do_one_pass()
> jbd2: refactor JBD2_COMMIT_BLOCK process in do_one_pass()
> jbd2: factor out jbd2_do_replay()
> jbd2: remove useless 'block_error' variable
> jbd2: remove the 'success' parameter from the jbd2_do_replay()
> function
>
> [...]
Applied, thanks!
[1/6] jbd2: remove redundant judgments for check v1 checksum
commit: 1438c95d4541fb7283c13d3d65b95fd91010a903
[2/6] jbd2: unified release of buffer_head in do_one_pass()
commit: ed6eb7b7fc53405e57c988b0ff60ae845470ca04
[3/6] jbd2: refactor JBD2_COMMIT_BLOCK process in do_one_pass()
commit: 21e90d4ca19fd9353ff5e02182ae80ef926afc84
[4/6] jbd2: factor out jbd2_do_replay()
commit: 0b85400756de5678f9e581eb7698c6e35f578a16
[5/6] jbd2: remove useless 'block_error' variable
commit: 4b3703e8200e0d8dcf46411f148bbb1d22d59525
[6/6] jbd2: remove the 'success' parameter from the jbd2_do_replay() function
commit: 1c77bb71e581d101df54df95c30407621d87ba68
Best regards,
--
Theodore Ts'o <tytso@mit.edu>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2024-11-07 15:13 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-30 0:59 [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover Ye Bin
2024-09-30 0:59 ` [PATCH v2 1/6] jbd2: remove redundant judgments for check v1 checksum Ye Bin
2024-09-30 2:36 ` Zhang Yi
2024-09-30 0:59 ` [PATCH v2 2/6] jbd2: unified release of buffer_head in do_one_pass() Ye Bin
2024-09-30 2:36 ` Zhang Yi
2024-09-30 0:59 ` [PATCH v2 3/6] jbd2: refactor JBD2_COMMIT_BLOCK process " Ye Bin
2024-09-30 2:37 ` Zhang Yi
2024-09-30 0:59 ` [PATCH v2 4/6] jbd2: factor out jbd2_do_replay() Ye Bin
2024-09-30 2:40 ` Zhang Yi
2024-09-30 10:30 ` Jan Kara
2024-09-30 0:59 ` [PATCH v2 5/6] jbd2: remove useless 'block_error' variable Ye Bin
2024-09-30 2:41 ` Zhang Yi
2024-09-30 0:59 ` [PATCH v2 6/6] jbd2: remove the 'success' parameter from the jbd2_do_replay() function Ye Bin
2024-09-30 2:43 ` Zhang Yi
2024-09-30 10:32 ` Jan Kara
2024-11-07 15:13 ` [PATCH v2 0/6] some cleanup and refactor for jbd2 journal recover Theodore Ts'o
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).