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

[Jan Kara <jack@suse.cz>]

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.

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

								Honza

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