Re: [PATCH v3 02/13] common/rc: Fix fsx for ext4 with bigalloc

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jul 22, 2025 at 03:23:02PM +0530, Ojaswin Mujoo wrote:
> On Thu, Jul 17, 2025 at 09:11:54AM -0700, Darrick J. Wong wrote:
> > On Sat, Jul 12, 2025 at 07:42:44PM +0530, Ojaswin Mujoo wrote:
> > > Insert range and collapse range only works with bigalloc in case
> > > the range is cluster size aligned, which fsx doesnt take care. To
> > > work past this, disable insert range and collapse range on ext4, if
> > > bigalloc is enabled.
> > > 
> > > This is achieved by defining a new function _set_default_fsx_avoid
> > > called via run_fsx helper. This can be used to selectively disable
> > > fsx options based on the configuration.
> > > 
> > > Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> > > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > > ---
> > >  common/rc | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 9a9d3cc8..218cf253 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -5113,10 +5113,37 @@ _require_hugepage_fsx()
> > >  		_notrun "fsx binary does not support MADV_COLLAPSE"
> > >  }
> > >  
> > > +_set_default_fsx_avoid() {
> > > +	local file=$1
> > > +
> > > +	case "$FSTYP" in
> > > +	"ext4")
> > > +		local dev=$(findmnt -n -o SOURCE --target $file)
> > > +
> > > +		# open code instead of _require_dumpe2fs cause we don't
> > > +		# want to _notrun if dumpe2fs is not available
> > > +		if [ -z "$DUMPE2FS_PROG" ]; then
> > > +			echo "_set_default_fsx_avoid: dumpe2fs not found, skipping bigalloc check." >> $seqres.full
> > > +			return
> > > +		fi
> > 
> > I hate to be the guy who says one thing and then another, but ...
> > 
> > If we extended _get_file_block_size to report the ext4 bigalloc cluster
> > size, would that be sufficient to keep testing collapse/insert range?
> > 
> > I guess the tricky part here is that bigalloc allows sub-cluster
> > mappings and we might not want to do all file IO testing in such big
> > units.
> 
> Hmm, so maybe a better way is to just add a parameter like alloc_unit in
> fsx where we can pass the cluster_size to which INSERT/COLLAPSE range be
> aligned to. For now we can pass it explicitly in the tests if needed.
> 
> I do plan on working on your suggestion of exposing alloc unit via
> statx(). Once we have that in the kernel, fsx can use that as well.
> 
> If this approach sounds okay I can try to maybe send the whole "fixing
> of insert/collpase range in fsx" as a patchset separate from atomic
> writes.

Yeah, that sounds like a good longer-term solution to me. :)

--D

> > 
> > > +
> > > +		$DUMPE2FS_PROG -h $dev 2>&1 | grep -q bigalloc && {
> > > +			export FSX_AVOID+=" -I -C"
> > 
> > No need to export FSX_AVOID to subprocesses.
> > 
> > --D
> 
> Got it, will fix. Thanks for review!
> 
> 
> Regards,
> ojaswin
> > 
> > > +		}
> > > +		;;
> > > +	# Add other filesystem types here as needed
> > > +	*)
> > > +		;;
> > > +	esac
> > > +}
> > > +
> > >  _run_fsx()
> > >  {
> > >  	echo "fsx $*"
> > >  	local args=`echo $@ | sed -e "s/ BSIZE / $bsize /g" -e "s/ PSIZE / $psize /g"`
> > > +
> > > +	_set_default_fsx_avoid $testfile
> > > +
> > >  	set -- $FSX_PROG $args $FSX_AVOID $TEST_DIR/junk
> > >  	echo "$@" >>$seqres.full
> > >  	rm -f $TEST_DIR/junk
> > > -- 
> > > 2.49.0
> > > 
> > > 
>