On Thu, Jan 18, 2024 at 10:59:14AM +0800, Su Yue wrote:
>
> On Wed 17 Jan 2024 at 12:55, Brian Foster <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
> > >
>
