On Fri 19 Jan 2024 at 11:09, Brian Foster <bfoster@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxx>
> > ---
> > 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
> >
