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, 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?

Cheers,
-- 
Luís

>    364 ext4_set_mount_flag(sb, EXT4_MF_FC_INELIGIBLE); 365
>    spin_unlock(&sbi->s_fc_lock); 366 WARN_ON(reason >=
>    EXT4_FC_REASON_MAX); 367
>    sbi->s_fc_stats.fc_ineligible_reason_count[reason]++; 368 } 369
>
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki