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

Su Yue
2024-01-21T04:12:14Z

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