Re: [linux-next:master 5690/11210] fs/ext4/fast_commit.c:362:21-23: WARNING !A || A && B is equivalent to !A || B

[Luis Henriques <luis.henriques@linux.dev>]

On Thu, Sep 12 2024, Jan Kara wrote:

> On Thu 12-09-24 09:19:18, Luis Henriques wrote:
>> On Thu, Sep 12 2024, kernel test robot wrote:
>> 
>> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
>> > head:   32ffa5373540a8d1c06619f52d019c6cdc948bb4
>> > commit: ebc4b2c1ac92fc0f8bf3f5a9c285a871d5084a6b [5690/11210] ext4: fix incorrect tid assumption in ext4_fc_mark_ineligible()
>> > config: loongarch-randconfig-r063-20240911 (https://download.01.org/0day-ci/archive/20240912/202409120149.GdjqoVYQ-lkp@intel.com/config)
>> > compiler: loongarch64-linux-gcc (GCC) 14.1.0
>> >
>> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> > the same patch/commit), kindly add following tags
>> > | Reported-by: kernel test robot <lkp@intel.com>
>> > | Closes: https://lore.kernel.org/oe-kbuild-all/202409120149.GdjqoVYQ-lkp@intel.com/
>> >
>> > cocci warnings: (new ones prefixed by >>)
>> >>> fs/ext4/fast_commit.c:362:21-23: WARNING !A || A && B is equivalent to !A || B
>> >
>> > vim +362 fs/ext4/fast_commit.c
>> >
>> >    332	
>> >    333	/*
>> >    334	 * Mark file system as fast commit ineligible, and record latest
>> >    335	 * ineligible transaction tid. This means until the recorded
>> >    336	 * transaction, commit operation would result in a full jbd2 commit.
>> >    337	 */
>> >    338	void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handle)
>> >    339	{
>> >    340		struct ext4_sb_info *sbi = EXT4_SB(sb);
>> >    341		tid_t tid;
>> >    342		bool has_transaction = true;
>> >    343		bool is_ineligible;
>> >    344	
>> >    345		if (ext4_fc_disabled(sb))
>> >    346			return;
>> >    347	
>> >    348		if (handle && !IS_ERR(handle))
>> >    349			tid = handle->h_transaction->t_tid;
>> >    350		else {
>> >    351			read_lock(&sbi->s_journal->j_state_lock);
>> >    352			if (sbi->s_journal->j_running_transaction)
>> >    353				tid = sbi->s_journal->j_running_transaction->t_tid;
>> >    354			else
>> >    355				has_transaction = false;
>> >    356			read_unlock(&sbi->s_journal->j_state_lock);
>> >    357		}
>> >    358		spin_lock(&sbi->s_fc_lock);
>> >    359		is_ineligible = ext4_test_mount_flag(sb, EXT4_MF_FC_INELIGIBLE);
>> >    360		if (has_transaction &&
>> >    361		    (!is_ineligible ||
>> >  > 362		     (is_ineligible && tid_gt(tid, sbi->s_fc_ineligible_tid))))
>> >    363			sbi->s_fc_ineligible_tid = tid;
>> 
>> This suggestion is obviously correct.  However, my brain found it much
>> easier to write (and understand) this logic if written this way.
>> 
>> Ted, want me to re-send this patch (or a fix for it), or are you happy
>> leaving it as is?
>
> I think I've already seen a patch for this. Yes [1]. Frankly I don't care
> much but I slightly prefer the shorter variant.

Oh! I totally missed that patch on the mailing-list.  OK, so no action for
me then.  Thanks!

> [1] https://lore.kernel.org/all/20240912090722.4e7o4l462y6hccau@quack3

(Minor nit: looks like this patch has a 'Closes:' tag pointing to a
bugzilla link which isn't accessible at the moment.  But maybe it's just a
temporary '502'.)

Cheers,
-- 
Luís