On Mon 22 Jan 2024 at 10:20, Brian Foster <bfoster@xxxxxxxxxx>
wrote:
On Sun, Jan 21, 2024 at 12:00:47PM +0800, Su Yue wrote:
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
Ah, good point.
Sorry for the late reply, was busy with other works.
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.
:/
I am wondering too...
> 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.
After going through the call stack of _scratch_mkfs quickly, I did
not
found something global to describe blocksize.
So I think blocksize_param is unnecessary?
--
Su
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
> > > >
> >
