Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 18 Aug 2020 12:48:26 +0200
From:   Jan Kara <jack@suse.cz>
To:     Shijie Luo <luoshijie1@huawei.com>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu, jack@suse.cz,
        linfeilong@huawei.com
Subject: Re: [PATCH] jbd2: remove useless variable chksum_seen in do_one_pass
Message-ID: <20200818104826.GA1902@quack2.suse.cz>
References: <20200811022128.32690-1-luoshijie1@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200811022128.32690-1-luoshijie1@huawei.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR