Re: [PATCH v3 2/2] common/rc: improve block_size support for bcachefs

[Su Yue <l@damenly.org>]

On Mon 29 Jan 2024 at 10:44, Brian Foster <bfoster@redhat.com> 
wrote:

> On Mon, Jan 29, 2024 at 10:01:01PM +0800, Su Yue wrote:
>> From: Su Yue <glass.su@suse.com>
>>
>> mkfs.bcachefs now supports option '--block_size' to allow
>> custom block_size.
>>
>> Add the pattern to set def_blksz if MKFS_OPTIONS contains the
>> option in _scratch_mkfs_sized.
>> Also let mkfs.bcachefs decide blocksize if no option is given 
>> in
>> local.config or _scratch_mkfs_sized parameter.
>>
>> Signed-off-by: Su Yue <glass.su@suse.com>
>> ---
>> changelog:
>> v3:
>>     Add logic to Let mkfs.bcachefs decide blocksize if no 
>>     option is given in
>>     local.config or _scratch_mkfs_sized parameter.
>> v2:
>>     Born.
>> ---
>
> Looks like the series duplicates the patches for some reason..
>
Yeah..I sent two patches but 4 patches in
https://lore.kernel.org/fstests/20240129140101.4259-3-l@damenly.org/T/#t

>
>>  common/rc | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 31c21d2a8360..315a2413f963 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -930,6 +930,7 @@ _scratch_mkfs_sized()
>>  	local fssize=$1
>>  	local blocksize=$2
>>  	local def_blksz
>> +	local blocksize_opt
>>
>>  	case $FSTYP in
>>  	xfs)
>> @@ -950,6 +951,13 @@ _scratch_mkfs_sized()
>>  	jfs)
>>  		def_blksz=4096
>>  		;;
>> +	bcachefs)
>> +		def_blksz=`echo $MKFS_OPTIONS | sed -rn 
>> 's/.*(--block_size)[ =]?+([0-9]+).*/\2/p'`
>> +		[ -n "$def_blksize" ] && 
>> blocksize_opt="--block_size=$def_blksize"
>> +		[ -n "$blocksize" ] && 
>> blocksize_opt="--block_size=$blocksize"
>
> This seems reasonable to me, but if I follow this function 
> correctly the
> behavior when both the param ($blocksize) and MKFS_OPTIONS 
> specify a
> block size is that MKFS_OPTIONS overrides the former. For 
> bcachefs it
> looks like the above does the opposite. Should we switch around 
> the
> above two statements?
>
Thanks for the catch. Nights turned my brain to mush.

V4 was sent.
--
Su
> Brian
>
>> +		# If no block size is given by local.confg or 
>> parameter, blocksize_opt is empty.
>> +		# Let MKFS_BCACHEFS_PROG decide block size on its own.
>> +		;;
>>  	esac
>>
>>  	[ -n "$def_blksz" ] && blocksize=$def_blksz
>> @@ -1051,7 +1059,7 @@ _scratch_mkfs_sized()
>>  		export MOUNT_OPTIONS="-o size=$fssize 
>>  $TMPFS_MOUNT_OPTIONS"
>>  		;;
>>  	bcachefs)
>> -		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize 
>> --block_size=$blocksize $SCRATCH_DEV
>> +		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize 
>> $blocksize_opt $SCRATCH_DEV
>>  		;;
>>  	*)
>>  		_notrun "Filesystem $FSTYP not supported in 
>>  _scratch_mkfs_sized"
>> --
>> 2.43.0
>>
>>