On Thu, Jan 25, 2024 at 10:10:48AM +0800, Su Yue wrote: > > 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. > No worries, same.. > > 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? > My concern was more that I think the original logic I posted didn't properly handle the case where a blocksize param is passed in (and thus my version removed the parameter, which was incorrect due to not realizing that generic/466 used it), but it's also possible I confused myself between the various options that have been discussed or am just not expressing the concern clearly enough. It's probably not worth getting further hung up on this point here. If you have a patch along the lines of your most recent example, that sounds like a good direction to me and so I can just take a fresh look from there... Brian > -- > 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 > > > > > > > > > > > > > > > >
