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

[Ojaswin Mujoo]

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@xxxxxxxxx>
> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
> > ---
> >  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.


> 
> > +
> > +		$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
> > 
> >