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

[Brian Foster <bfoster@redhat.com>]

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?

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?

Brian

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