Re: [PATCH v2 2/2] common/rc: improve block size support for bcachefs

[Su Yue <l@damenly.org>]

On Fri 19 Jan 2024 at 11:09, Brian Foster <bfoster@redhat.com> 
wrote:

> On Thu, Jan 18, 2024 at 10:59:14AM +0800, Su Yue wrote:
>>
>> On Wed 17 Jan 2024 at 12:55, Brian Foster <bfoster@redhat.com> 
>> wrote:
>>
>> > On Wed, Jan 17, 2024 at 05:23:09PM +0800, Su Yue wrote:
>> > > For bcachefs, def_blksz is never assigned even MKFS_OPTIONS 
>> > > contains
>> > > option
>> > > '--block_size'. So block size of bcachefs on scratch dev is 
>> > > always
>> > > 4096
>> > > if _scratch_mkfs_sized is called without second parameter.
>> > >
>> > > Add the pattern to set def_blksz if '--block_size' is given 
>> > > in
>> > > MKFS_OPTIONS.
>> > >
>> > > Signed-off-by: Su Yue <glass.su@suse.com>
>> > > ---
>> > > changelog:
>> > > v2:
>> > >     Born.
>> > > ---
>> > >  common/rc | 3 +++
>> > >  1 file changed, 3 insertions(+)
>> > >
>> > > diff --git a/common/rc b/common/rc
>> > > index 31c21d2a8360..6a01de69cf05 100644
>> > > --- a/common/rc
>> > > +++ b/common/rc
>> > > @@ -950,6 +950,9 @@ _scratch_mkfs_sized()
>> > >  	jfs)
>> > >  		def_blksz=4096
>> > >  		;;
>> > > +	bcachefs)
>> > > +		def_blksz=`echo $MKFS_OPTIONS | sed -rn 
>> > > 's/.*(--block_size)[
>> > > =]?+([0-9]+).*/\2/p'`
>> > > +		;;
>> >
>> > So if the default bcachefs block size is 512b, I wonder if 
>> > this should
>> > do something like what the udf branch does a few lines above 
>> > and
>> >
>> mkfs.bcachefs decides block size by querying statbuf.st_blksize 
>> or
>> BLKPBSZGET from the device if the option is not given.
>>
>> > override the hardcoded default of 4k. ISTM this whole thing 
>> > would be
>> > more robust if it just elided the param in the default cases 
>> > and let the
>> > associated mkfs tool use its own default, but that's probably 
>> > a separate
>> > issue. Hm?
>> >
>> Since there is no default block size of bcachefs, maybe we can 
>> let
>> mkfs.bcachefs decide on its own but it will make chaos when 
>> somebody
>> reports an unreproducible BUG due to the different block_size 
>> even
>> we have same local.config. It just happened...
>> So for now, I think 4096 is a resonable value of bcachefs block 
>> size.
>>
>
> I think we run into this no matter what if we pick a size out of 
> a hat,
> regardless of what the value is. If somebody is testing with 
> default
> blocksize (i.e. based on mkfs) on a filesystem where the default 
> isn't
> actually 4k, then it seems quite unexpected that 
> _scratch_mkfs_sized
> would use something different from _scratch_mkfs when a block 
> size isn't
> explicitly specified. That's exactly the situation we ran into 
> with the
> generic/361 thing where I would have expected this test to use 
> 512b
> blocks.
>
> ISTM that the 4k value in _scratch_mkfs_sized() is mainly a last 
> resort
> default value so the $blocks calculation can work for any 
> filesystem
> that might not be properly supported by the function. The 
> function looks
> a little wonky overall, but I think there are at least a couple 
> options
> to improve things for bcachefs. FWIW, it also looks to me that 
> nothing
> actually passes a blocksize param to _scratch_mkfs_sized, so 
> perhaps we
> could just drop that blocksize=$2 parameter across the board as 
> a
> simplification?
>
Maybe it can be dropped. The only user is generic/466

> With that, I think bcache could either:
>
> 1. Do something like def_blksize=`blockdev --getpbsz 
> $SCRATCH_DEV` in
> the first switch if no block size is specified in MKFS_OPTIONS 
> (or
> whatever best mimics mkfs logic).
>
> OR
>
> 2. Do something like the following in the last switch:
>
> 	[ -n $def_blksize ] && def_blksize="--block_size=$def_blksize"
> 	$MKFS_BCACHEFS_PROG ... $def_blksize $SCRATCH_DEV
>
> ... to allow mkfs to determine the block size. I _think_ that 
> works
> because the bcachefs format doesn't depend on $blocks at all, so
> whatever $blocksize was set to is irrelevent unless $def_blksize 
> was set
> above, but I could be missing something. That also assumes 
> blocksize
> wasn't set to something by the caller.
>
> If correct, option #2 seems a little cleaner to me, but other
> thoughts/ideas?
>
Option 2 is preferable. I would code it as(on more varaiable for 
readability):

local blocksize_opt

[ -n $def_blksize ] && blockzie_opt="--block_size=$def_blksize"
$MKFS_BCACHEFS_PROG ... $blocksize_opt $SCRATCH_DEV

--
Su
>
> Brian
>
>> --
>> Su
>> >
>> > Brian
>> >
>> > >  	esac
>> > >
>> > >  	[ -n "$def_blksz" ] && blocksize=$def_blksz
>> > > --
>> > > 2.43.0
>> > >
>>