This variable only indicates that we do checksum success, while
chksum_err can also do. Moreover, condition "!chksum_seen" in else
if bracket is pointless.
Signed-off-by: Shijie Luo <[email protected]>
---
fs/jbd2/recovery.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 2ed278f0dced..575bb6426bcc 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -690,14 +690,12 @@ static int do_one_pass(journal_t *journal,
* number. */
if (pass == PASS_SCAN &&
jbd2_has_feature_checksum(journal)) {
- int chksum_err, chksum_seen;
+ int chksum_err = 0;
struct commit_header *cbh =
(struct commit_header *)bh->b_data;
unsigned found_chksum =
be32_to_cpu(cbh->h_chksum[0]);
- chksum_err = chksum_seen = 0;
-
if (info->end_transaction) {
journal->j_failed_commit =
info->end_transaction;
@@ -709,11 +707,10 @@ static int do_one_pass(journal_t *journal,
cbh->h_chksum_type == JBD2_CRC32_CHKSUM &&
cbh->h_chksum_size ==
JBD2_CRC32_CHKSUM_SIZE)
- chksum_seen = 1;
+ chksum_err = 0;
else if (!(cbh->h_chksum_type == 0 &&
cbh->h_chksum_size == 0 &&
- found_chksum == 0 &&
- !chksum_seen))
+ found_chksum == 0))
/*
* If fs is mounted using an old kernel and then
* kernel with journal_chksum is used then we
--
2.19.1
On Mon 10-08-20 22:21:28, Shijie Luo wrote:
> This variable only indicates that we do checksum success, while
> chksum_err can also do. Moreover, condition "!chksum_seen" in else
> if bracket is pointless.
>
> Signed-off-by: Shijie Luo <[email protected]>
Thanks for the patch! Some comments below.
> @@ -709,11 +707,10 @@ static int do_one_pass(journal_t *journal,
> cbh->h_chksum_type == JBD2_CRC32_CHKSUM &&
> cbh->h_chksum_size ==
> JBD2_CRC32_CHKSUM_SIZE)
> - chksum_seen = 1;
> + chksum_err = 0;
> else if (!(cbh->h_chksum_type == 0 &&
> cbh->h_chksum_size == 0 &&
> - found_chksum == 0 &&
> - !chksum_seen))
> + found_chksum == 0))
> /*
> * If fs is mounted using an old kernel and then
> * kernel with journal_chksum is used then we
I agree the use of chksum_err & chksum_seen looks rather arbitrary. In fact
the code seems to be equivalent to:
/* Neither checksum match nor unused? */
if (!(crc32_sum == found_chksum &&
cbh->h_chksum_type == JBD2_CRC32_CHKSUM &&
cbh->h_chksum_size ==
JBD2_CRC32_CHKSUM_SIZE) &&
!(cbh->h_chksum_type == 0 &&
cbh->h_chksum_size == 0 &&
found_chksum == 0)) {
info->end_transaction = next_commit_ID;
if (jbd2_has_feature_async_commit(journal)) {
...
}
}
crc32_sum = ~0;
which would be even simpler...
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2020/8/18 18:48, Jan Kara wrote:
> On Mon 10-08-20 22:21:28, Shijie Luo wrote:
>> This variable only indicates that we do checksum success, while
>> chksum_err can also do. Moreover, condition "!chksum_seen" in else
>> if bracket is pointless.
>>
>> Signed-off-by: Shijie Luo <[email protected]>
> Thanks for the patch! Some comments below.
>
>> @@ -709,11 +707,10 @@ static int do_one_pass(journal_t *journal,
>> cbh->h_chksum_type == JBD2_CRC32_CHKSUM &&
>> cbh->h_chksum_size ==
>> JBD2_CRC32_CHKSUM_SIZE)
>> - chksum_seen = 1;
>> + chksum_err = 0;
>> else if (!(cbh->h_chksum_type == 0 &&
>> cbh->h_chksum_size == 0 &&
>> - found_chksum == 0 &&
>> - !chksum_seen))
>> + found_chksum == 0))
>> /*
>> * If fs is mounted using an old kernel and then
>> * kernel with journal_chksum is used then we
> I agree the use of chksum_err & chksum_seen looks rather arbitrary. In fact
> the code seems to be equivalent to:
>
> /* Neither checksum match nor unused? */
> if (!(crc32_sum == found_chksum &&
> cbh->h_chksum_type == JBD2_CRC32_CHKSUM &&
> cbh->h_chksum_size ==
> JBD2_CRC32_CHKSUM_SIZE) &&
> !(cbh->h_chksum_type == 0 &&
> cbh->h_chksum_size == 0 &&
> found_chksum == 0)) {
> info->end_transaction = next_commit_ID;
> if (jbd2_has_feature_async_commit(journal)) {
> ...
> }
> }
> crc32_sum = ~0;
>
> which would be even simpler...
>
> Honza
Thanks for your review，it 's definitely true to use these code as a
substitute, but I think these are
a little bit hard to read. By the way, I found a indentation error on
"chksum_err = 1".
Do you think which one is better? I will take your opinion into account
and send a v2 patch.
I wonder if this is even cleaner? What do folks think?
- Ted
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 2ed278f0dced..4373abbfd19a 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -690,14 +690,11 @@ static int do_one_pass(journal_t *journal,
* number. */
if (pass == PASS_SCAN &&
jbd2_has_feature_checksum(journal)) {
- int chksum_err, chksum_seen;
struct commit_header *cbh =
(struct commit_header *)bh->b_data;
unsigned found_chksum =
be32_to_cpu(cbh->h_chksum[0]);
- chksum_err = chksum_seen = 0;
-
if (info->end_transaction) {
journal->j_failed_commit =
info->end_transaction;
@@ -705,42 +702,23 @@ static int do_one_pass(journal_t *journal,
break;
}
- if (crc32_sum == found_chksum &&
- cbh->h_chksum_type == JBD2_CRC32_CHKSUM &&
- cbh->h_chksum_size ==
- JBD2_CRC32_CHKSUM_SIZE)
- chksum_seen = 1;
- else if (!(cbh->h_chksum_type == 0 &&
- cbh->h_chksum_size == 0 &&
- found_chksum == 0 &&
- !chksum_seen))
- /*
- * If fs is mounted using an old kernel and then
- * kernel with journal_chksum is used then we
- * get a situation where the journal flag has
- * checksum flag set but checksums are not
- * present i.e chksum = 0, in the individual
- * commit blocks.
- * Hence to avoid checksum failures, in this
- * situation, this extra check is added.
- */
- chksum_err = 1;
-
- if (chksum_err) {
- info->end_transaction = next_commit_ID;
-
- if (!jbd2_has_feature_async_commit(journal)) {
- journal->j_failed_commit =
- next_commit_ID;
- brelse(bh);
- break;
- }
- }
+ /* Neither checksum match nor unused? */
+ if (!((crc32_sum == found_chksum &&
+ cbh->h_chksum_type ==
+ JBD2_CRC32_CHKSUM &&
+ cbh->h_chksum_size ==
+ JBD2_CRC32_CHKSUM_SIZE) ||
+ (cbh->h_chksum_type == 0 &&
+ cbh->h_chksum_size == 0 &&
+ found_chksum == 0)))
+ goto chksum_error;
+
crc32_sum = ~0;
}
if (pass == PASS_SCAN &&
!jbd2_commit_block_csum_verify(journal,
bh->b_data)) {
+ chksum_error:
info->end_transaction = next_commit_ID;
if (!jbd2_has_feature_async_commit(journal)) {
Hi, Ted. Thanks for your reply, I think this one is perfect.
On 2020/8/19 3:14, Theodore Y. Ts'o wrote:
> I wonder if this is even cleaner? What do folks think?
>
> - Ted
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 2ed278f0dced..4373abbfd19a 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -690,14 +690,11 @@ static int do_one_pass(journal_t *journal,
> * number. */
> if (pass == PASS_SCAN &&
> jbd2_has_feature_checksum(journal)) {
> - int chksum_err, chksum_seen;
> struct commit_header *cbh =
> (struct commit_header *)bh->b_data;
> unsigned found_chksum =
> be32_to_cpu(cbh->h_chksum[0]);
>
> - chksum_err = chksum_seen = 0;
> -
> if (info->end_transaction) {
> journal->j_failed_commit =
> info->end_transaction;
> @@ -705,42 +702,23 @@ static int do_one_pass(journal_t *journal,
> break;
> }
>
> - if (crc32_sum == found_chksum &&
> - cbh->h_chksum_type == JBD2_CRC32_CHKSUM &&
> - cbh->h_chksum_size ==
> - JBD2_CRC32_CHKSUM_SIZE)
> - chksum_seen = 1;
> - else if (!(cbh->h_chksum_type == 0 &&
> - cbh->h_chksum_size == 0 &&
> - found_chksum == 0 &&
> - !chksum_seen))
> - /*
> - * If fs is mounted using an old kernel and then
> - * kernel with journal_chksum is used then we
> - * get a situation where the journal flag has
> - * checksum flag set but checksums are not
> - * present i.e chksum = 0, in the individual
> - * commit blocks.
> - * Hence to avoid checksum failures, in this
> - * situation, this extra check is added.
> - */
> - chksum_err = 1;
> -
> - if (chksum_err) {
> - info->end_transaction = next_commit_ID;
> -
> - if (!jbd2_has_feature_async_commit(journal)) {
> - journal->j_failed_commit =
> - next_commit_ID;
> - brelse(bh);
> - break;
> - }
> - }
> + /* Neither checksum match nor unused? */
> + if (!((crc32_sum == found_chksum &&
> + cbh->h_chksum_type ==
> + JBD2_CRC32_CHKSUM &&
> + cbh->h_chksum_size ==
> + JBD2_CRC32_CHKSUM_SIZE) ||
> + (cbh->h_chksum_type == 0 &&
> + cbh->h_chksum_size == 0 &&
> + found_chksum == 0)))
> + goto chksum_error;
> +
> crc32_sum = ~0;
> }
> if (pass == PASS_SCAN &&
> !jbd2_commit_block_csum_verify(journal,
> bh->b_data)) {
> + chksum_error:
> info->end_transaction = next_commit_ID;
>
> if (!jbd2_has_feature_async_commit(journal)) {
>
> .
>
On Tue 18-08-20 15:14:59, Theodore Y. Ts'o wrote:
> I wonder if this is even cleaner? What do folks think?
>
> - Ted
Yup, looks good to me. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
once you add proper changelog and signed-off-by.
Honza
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 2ed278f0dced..4373abbfd19a 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -690,14 +690,11 @@ static int do_one_pass(journal_t *journal,
> * number. */
> if (pass == PASS_SCAN &&
> jbd2_has_feature_checksum(journal)) {
> - int chksum_err, chksum_seen;
> struct commit_header *cbh =
> (struct commit_header *)bh->b_data;
> unsigned found_chksum =
> be32_to_cpu(cbh->h_chksum[0]);
>
> - chksum_err = chksum_seen = 0;
> -
> if (info->end_transaction) {
> journal->j_failed_commit =
> info->end_transaction;
> @@ -705,42 +702,23 @@ static int do_one_pass(journal_t *journal,
> break;
> }
>
> - if (crc32_sum == found_chksum &&
> - cbh->h_chksum_type == JBD2_CRC32_CHKSUM &&
> - cbh->h_chksum_size ==
> - JBD2_CRC32_CHKSUM_SIZE)
> - chksum_seen = 1;
> - else if (!(cbh->h_chksum_type == 0 &&
> - cbh->h_chksum_size == 0 &&
> - found_chksum == 0 &&
> - !chksum_seen))
> - /*
> - * If fs is mounted using an old kernel and then
> - * kernel with journal_chksum is used then we
> - * get a situation where the journal flag has
> - * checksum flag set but checksums are not
> - * present i.e chksum = 0, in the individual
> - * commit blocks.
> - * Hence to avoid checksum failures, in this
> - * situation, this extra check is added.
> - */
> - chksum_err = 1;
> -
> - if (chksum_err) {
> - info->end_transaction = next_commit_ID;
> -
> - if (!jbd2_has_feature_async_commit(journal)) {
> - journal->j_failed_commit =
> - next_commit_ID;
> - brelse(bh);
> - break;
> - }
> - }
> + /* Neither checksum match nor unused? */
> + if (!((crc32_sum == found_chksum &&
> + cbh->h_chksum_type ==
> + JBD2_CRC32_CHKSUM &&
> + cbh->h_chksum_size ==
> + JBD2_CRC32_CHKSUM_SIZE) ||
> + (cbh->h_chksum_type == 0 &&
> + cbh->h_chksum_size == 0 &&
> + found_chksum == 0)))
> + goto chksum_error;
> +
> crc32_sum = ~0;
> }
> if (pass == PASS_SCAN &&
> !jbd2_commit_block_csum_verify(journal,
> bh->b_data)) {
> + chksum_error:
> info->end_transaction = next_commit_ID;
>
> if (!jbd2_has_feature_async_commit(journal)) {
--
Jan Kara <[email protected]>
SUSE Labs, CR