* [PATCH 1/5] jbd2: remove redundant judgments for check v1 checksum
2024-09-18 11:35 [PATCH 0/5] some cleanup and refactor for jbd2 journal recover Ye Bin
@ 2024-09-18 11:36 ` Ye Bin
2024-09-26 12:52 ` Jan Kara
2024-09-18 11:36 ` [PATCH 2/5] jbd2: unified release of buffer_head in do_one_pass() Ye Bin
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Ye Bin @ 2024-09-18 11:36 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>
---
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] 12+ messages in thread* Re: [PATCH 1/5] jbd2: remove redundant judgments for check v1 checksum
2024-09-18 11:36 ` [PATCH 1/5] jbd2: remove redundant judgments for check v1 checksum Ye Bin
@ 2024-09-26 12:52 ` Jan Kara
0 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2024-09-26 12:52 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack, zhangxiaoxu5
On Wed 18-09-24 19:36:00, 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>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> 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
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/5] jbd2: unified release of buffer_head in do_one_pass()
2024-09-18 11:35 [PATCH 0/5] some cleanup and refactor for jbd2 journal recover Ye Bin
2024-09-18 11:36 ` [PATCH 1/5] jbd2: remove redundant judgments for check v1 checksum Ye Bin
@ 2024-09-18 11:36 ` Ye Bin
2024-09-26 12:57 ` Jan Kara
2024-09-18 11:36 ` [PATCH 3/5] jbd2: refactor JBD2_COMMIT_BLOCK process " Ye Bin
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Ye Bin @ 2024-09-18 11:36 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>
---
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] 12+ messages in thread* Re: [PATCH 2/5] jbd2: unified release of buffer_head in do_one_pass()
2024-09-18 11:36 ` [PATCH 2/5] jbd2: unified release of buffer_head in do_one_pass() Ye Bin
@ 2024-09-26 12:57 ` Jan Kara
0 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2024-09-26 12:57 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack, zhangxiaoxu5
On Wed 18-09-24 19:36:01, 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>
Indeed there are many places releasing bh. The patch looks good. Feel free
to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> 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
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/5] jbd2: refactor JBD2_COMMIT_BLOCK process in do_one_pass()
2024-09-18 11:35 [PATCH 0/5] some cleanup and refactor for jbd2 journal recover Ye Bin
2024-09-18 11:36 ` [PATCH 1/5] jbd2: remove redundant judgments for check v1 checksum Ye Bin
2024-09-18 11:36 ` [PATCH 2/5] jbd2: unified release of buffer_head in do_one_pass() Ye Bin
@ 2024-09-18 11:36 ` Ye Bin
2024-09-26 13:01 ` Jan Kara
2024-09-18 11:36 ` [PATCH 4/5] jbd2: factor out jbd2_do_replay() Ye Bin
2024-09-18 11:36 ` [PATCH 5/5] jbd2: remove useless 'block_error' variable Ye Bin
4 siblings, 1 reply; 12+ messages in thread
From: Ye Bin @ 2024-09-18 11:36 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>
---
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] 12+ messages in thread* Re: [PATCH 3/5] jbd2: refactor JBD2_COMMIT_BLOCK process in do_one_pass()
2024-09-18 11:36 ` [PATCH 3/5] jbd2: refactor JBD2_COMMIT_BLOCK process " Ye Bin
@ 2024-09-26 13:01 ` Jan Kara
0 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2024-09-26 13:01 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack, zhangxiaoxu5
On Wed 18-09-24 19:36:02, 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>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> 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
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/5] jbd2: factor out jbd2_do_replay()
2024-09-18 11:35 [PATCH 0/5] some cleanup and refactor for jbd2 journal recover Ye Bin
` (2 preceding siblings ...)
2024-09-18 11:36 ` [PATCH 3/5] jbd2: refactor JBD2_COMMIT_BLOCK process " Ye Bin
@ 2024-09-18 11:36 ` Ye Bin
2024-09-26 13:24 ` Jan Kara
2024-09-18 11:36 ` [PATCH 5/5] jbd2: remove useless 'block_error' variable Ye Bin
4 siblings, 1 reply; 12+ messages in thread
From: Ye Bin @ 2024-09-18 11:36 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..05ea449b95c4 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] 12+ messages in thread* Re: [PATCH 4/5] jbd2: factor out jbd2_do_replay()
2024-09-18 11:36 ` [PATCH 4/5] jbd2: factor out jbd2_do_replay() Ye Bin
@ 2024-09-26 13:24 ` Jan Kara
0 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2024-09-26 13:24 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack, zhangxiaoxu5
On Wed 18-09-24 19:36:03, 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>
The patch looks good but I have a few comments below:
> +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)
So you remove block_error in the later patch so that is good. But I also
wonder if we need the 'success' parameter. I'd think that jbd2_do_replay()
can just keep success internally to track if any error happened and then
return it at the end? do_one_pass() will then incorporate the returned
value in its 'success' variable. That way the error handling will be more
standard...
> +{
> + 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) <=
^^ no need for braces here...
> + journal->j_blocksize - descr_csum_size) {
^^ when the indentation level is this close to the code
block indentation, I actually prefer indenting it one tab more like:
journal->j_blocksize - descr_csum_size) {
But this is just a personal preference so feel free to ignore it :)
> +
> + 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)) {
^^^
Indentation like this is really confusing. I'd add one more tab here.
Otherwise the patch looks good.
Honza
> + 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
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 5/5] jbd2: remove useless 'block_error' variable
2024-09-18 11:35 [PATCH 0/5] some cleanup and refactor for jbd2 journal recover Ye Bin
` (3 preceding siblings ...)
2024-09-18 11:36 ` [PATCH 4/5] jbd2: factor out jbd2_do_replay() Ye Bin
@ 2024-09-18 11:36 ` Ye Bin
2024-09-18 13:19 ` Zhang Yi
2024-09-26 13:19 ` Jan Kara
4 siblings, 2 replies; 12+ messages in thread
From: Ye Bin @ 2024-09-18 11:36 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>
---
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 05ea449b95c4..0bcbb58d634b 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] 12+ messages in thread* Re: [PATCH 5/5] jbd2: remove useless 'block_error' variable
2024-09-18 11:36 ` [PATCH 5/5] jbd2: remove useless 'block_error' variable Ye Bin
@ 2024-09-18 13:19 ` Zhang Yi
2024-09-26 13:19 ` Jan Kara
1 sibling, 0 replies; 12+ messages in thread
From: Zhang Yi @ 2024-09-18 13:19 UTC (permalink / raw)
To: Ye Bin, tytso, adilger.kernel, linux-ext4; +Cc: jack, zhangxiaoxu5
On 2024/9/18 19:36, 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>
> ---
> 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 05ea449b95c4..0bcbb58d634b 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)
I wonder an unrelated question, why do we need this 'success' variable? Now we
keep on replaying later log blocks even if some bad things happened(read I/O
error or checksum error) instead of just quit, is it to replay as many log
blocks as possible, and then to reduce the data lose and also reduce e2fsck's
workload?
Thanks,
Yi.
> {
> 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] 12+ messages in thread* Re: [PATCH 5/5] jbd2: remove useless 'block_error' variable
2024-09-18 11:36 ` [PATCH 5/5] jbd2: remove useless 'block_error' variable Ye Bin
2024-09-18 13:19 ` Zhang Yi
@ 2024-09-26 13:19 ` Jan Kara
1 sibling, 0 replies; 12+ messages in thread
From: Jan Kara @ 2024-09-26 13:19 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack, zhangxiaoxu5
On Wed 18-09-24 19:36:04, 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>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> 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 05ea449b95c4..0bcbb58d634b 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
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 12+ messages in thread