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