Re: [PATCH v4 15/16] btrfs: handle discarding fully-remapped block groups

[Philip Li <philip.li@intel.com>]

On Mon, Nov 03, 2025 at 04:49:26PM +0000, Mark Harmstone wrote:
> On 31/10/2025 10.12 pm, Boris Burkov wrote:
> > On Tue, Oct 28, 2025 at 12:04:11AM +0800, kernel test robot wrote:
> > > Hi Mark,
> > > 
> > > kernel test robot noticed the following build warnings:
> > > 
> > > [auto build test WARNING on kdave/for-next]
> > > [also build test WARNING on next-20251027]
> > > [cannot apply to linus/master v6.18-rc3]
> > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > And when submitting patch, we suggest to use '--base' as documented in
> > > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > > 
> > > url:    https://github.com/intel-lab-lkp/linux/commits/Mark-Harmstone/btrfs-add-definitions-and-constants-for-remap-tree/20251025-021910
> > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> > > patch link:    https://lore.kernel.org/r/20251024181227.32228-16-mark%40harmstone.com
> > > patch subject: [PATCH v4 15/16] btrfs: handle discarding fully-remapped block groups
> > > config: arm-randconfig-003-20251027 (https://download.01.org/0day-ci/archive/20251027/202510272322.N1S5rdDc-lkp@intel.com/config)
> > > compiler: arm-linux-gnueabi-gcc (GCC) 8.5.0
> > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251027/202510272322.N1S5rdDc-lkp@intel.com/reproduce)
> > > 
> > > 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/202510272322.N1S5rdDc-lkp@intel.com/
> > > 
> > > Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
> > > http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
> > > 
> > > All warnings (new ones prefixed by >>):
> > > 
> > >     fs/btrfs/discard.c: In function 'btrfs_discard_workfn':
> > > > > fs/btrfs/discard.c:596:6: warning: 'discard_state' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > >        if (discard_state == BTRFS_DISCARD_BITMAPS ||
> > >           ^
> > 
> > I think this gets set by peek_discard_list() so I don't think this is
> > a valid warning.
> 
> You are correct. discard_state gets initialized if the return value of
> peek_discard_list() is not NULL, and if it is NULL we return before we use
> it.
> 
> This is an ancient version of GCC, the warning doesn't trigger on GCC 15 -
> presumably it has better control flow analysis. I don't think the robot
> should be compiling with this warning turned on for old compiler versions,
> if it's prone to false positives.

Thanks for the suggestion, I will update the bot to avoid sending out this
directly. Sorry for the false positive.

> 
> > > 
> > > 
> > > vim +/discard_state +596 fs/btrfs/discard.c
> > > 
> > >     513	
> > >     514	/*
> > >     515	 * Discard work queue callback
> > >     516	 *
> > >     517	 * @work: work
> > >     518	 *
> > >     519	 * Find the next block_group to start discarding and then discard a single
> > >     520	 * region.  It does this in a two-pass fashion: first extents and second
> > >     521	 * bitmaps.  Completely discarded block groups are sent to the unused_bgs path.
> > >     522	 */
> > >     523	static void btrfs_discard_workfn(struct work_struct *work)
> > >     524	{
> > >     525		struct btrfs_discard_ctl *discard_ctl;
> > >     526		struct btrfs_block_group *block_group;
> > >     527		enum btrfs_discard_state discard_state;
> > >     528		int discard_index = 0;
> > >     529		u64 trimmed = 0;
> > >     530		u64 minlen = 0;
> > >     531		u64 now = ktime_get_ns();
> > >     532	
> > >     533		discard_ctl = container_of(work, struct btrfs_discard_ctl, work.work);
> > >     534	
> > >     535		block_group = peek_discard_list(discard_ctl, &discard_state,
> > >     536						&discard_index, now);
> > >     537		if (!block_group)
> > >     538			return;
> > >     539		if (!btrfs_run_discard_work(discard_ctl)) {
> > >     540			spin_lock(&discard_ctl->lock);
> > >     541			btrfs_put_block_group(block_group);
> > >     542			discard_ctl->block_group = NULL;
> > >     543			spin_unlock(&discard_ctl->lock);
> > >     544			return;
> > >     545		}
> > >     546		if (now < block_group->discard_eligible_time) {
> > >     547			spin_lock(&discard_ctl->lock);
> > >     548			btrfs_put_block_group(block_group);
> > >     549			discard_ctl->block_group = NULL;
> > >     550			spin_unlock(&discard_ctl->lock);
> > >     551			btrfs_discard_schedule_work(discard_ctl, false);
> > >     552			return;
> > >     553		}
> > >     554	
> > >     555		/* Perform discarding */
> > >     556		minlen = discard_minlen[discard_index];
> > >     557	
> > >     558		switch (discard_state) {
> > >     559		case BTRFS_DISCARD_BITMAPS: {
> > >     560			u64 maxlen = 0;
> > >     561	
> > >     562			/*
> > >     563			 * Use the previous levels minimum discard length as the max
> > >     564			 * length filter.  In the case something is added to make a
> > >     565			 * region go beyond the max filter, the entire bitmap is set
> > >     566			 * back to BTRFS_TRIM_STATE_UNTRIMMED.
> > >     567			 */
> > >     568			if (discard_index != BTRFS_DISCARD_INDEX_UNUSED)
> > >     569				maxlen = discard_minlen[discard_index - 1];
> > >     570	
> > >     571			btrfs_trim_block_group_bitmaps(block_group, &trimmed,
> > >     572					       block_group->discard_cursor,
> > >     573					       btrfs_block_group_end(block_group),
> > >     574					       minlen, maxlen, true);
> > >     575			discard_ctl->discard_bitmap_bytes += trimmed;
> > >     576	
> > >     577			break;
> > >     578		}
> > >     579	
> > >     580		case BTRFS_DISCARD_FULLY_REMAPPED:
> > >     581			btrfs_trim_fully_remapped_block_group(block_group);
> > >     582			break;
> > >     583	
> > >     584		default:
> > >     585			btrfs_trim_block_group_extents(block_group, &trimmed,
> > >     586					       block_group->discard_cursor,
> > >     587					       btrfs_block_group_end(block_group),
> > >     588					       minlen, true);
> > >     589			discard_ctl->discard_extent_bytes += trimmed;
> > >     590	
> > >     591			break;
> > >     592		}
> > >     593	
> > >     594		/* Determine next steps for a block_group */
> > >     595		if (block_group->discard_cursor >= btrfs_block_group_end(block_group)) {
> > >   > 596			if (discard_state == BTRFS_DISCARD_BITMAPS ||
> > >     597			    discard_state == BTRFS_DISCARD_FULLY_REMAPPED) {
> > >     598				btrfs_finish_discard_pass(discard_ctl, block_group);
> > >     599			} else {
> > >     600				block_group->discard_cursor = block_group->start;
> > >     601				spin_lock(&discard_ctl->lock);
> > >     602				if (block_group->discard_state !=
> > >     603				    BTRFS_DISCARD_RESET_CURSOR)
> > >     604					block_group->discard_state =
> > >     605								BTRFS_DISCARD_BITMAPS;
> > >     606				spin_unlock(&discard_ctl->lock);
> > >     607			}
> > >     608		}
> > >     609	
> > >     610		now = ktime_get_ns();
> > >     611		spin_lock(&discard_ctl->lock);
> > >     612		discard_ctl->prev_discard = trimmed;
> > >     613		discard_ctl->prev_discard_time = now;
> > >     614		btrfs_put_block_group(block_group);
> > >     615		discard_ctl->block_group = NULL;
> > >     616		__btrfs_discard_schedule_work(discard_ctl, now, false);
> > >     617		spin_unlock(&discard_ctl->lock);
> > >     618	}
> > >     619	
> > > 
> > > -- 
> > > 0-DAY CI Kernel Test Service
> > > https://github.com/intel/lkp-tests/wiki
> 
>