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

[Brian Foster <bfoster@redhat.com>]

On Sun, Jan 21, 2024 at 12:00:47PM +0800, Su Yue wrote:
> 
> 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
> 

Ah, good point.

So generic/466 clears MKFS_OPTIONS so the function will use the provided
block size over whatever exists in the config, yet generic/466 is the
only user of this param in the first place.

I wonder if a more useful change might be to change the
_scratch_mkfs_sized logic to prefer the provided block size over
whatever is in the config, but then again that might require further
changes for any of the places that continue to use MKFS_OPTIONS. :/

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

That seems reasonable to me. The only caveat is this was assuming the
blocksize param would go away and so could be ignored for bcachefs.

Without getting too far into the weeds on the higher level parameter
handling, I wonder if the blocksize parameter was initially put into a
separate variable (i.e. blocksize_param or some such), if that would at
least help bcachefs determine between the case where 4096 was requested
by the caller vs. picked out of a hat by the default handling logic
earlier in the function, and thus correctly determine when to set a
blocksize vs. use the mkfs (non-4k) default.

So then IOW the logic might look something like this..?

	[ -n $blocksize_param ] && blocksize_opt="--block_size=$blocksize_param"
	[ -n $def_blksize ] && blocksize_opt="--block_size=$def_blksize"
	mkfs ...

Brian

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