| Message ID | 20200811022128.32690-1-luoshijie1@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | jbd2: remove useless variable chksum_seen in do_one_pass | expand |
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 <luoshijie1@huawei.com> 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
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 <luoshijie1@huawei.com> > 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 <jack@suse.cz> 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)) {
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
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 <luoshijie1@huawei.com> --- fs/jbd2/recovery.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)