[PATCH v2 1/2] fstests: introduce MKFS_BCACHEFS_PROG for bcachefs

[PATCH v2 1/2] fstests: introduce MKFS_BCACHEFS_PROG for bcachefs

[Su Yue]

mkfs.bcachefs supports force overwrite when option '-f' is given:
$ mkfs.bcachefs --help | grep force
  -f, --force

There are some tests which call _scratch_mkfs multiple times
e.g. tests/generic/171. Without '-f' in MKFS_OPTIONS,
these tests just hang in overwrite confirmation.
After this commit, MKFS_BCACHEFS_PROG will contains ' -f' so
we don't have to add '-f' to MKFS_OPTIONS manually to make
these tests pass.

It also fixes generic/466 which unsets MKFS_OPTIONS causing
that test hangs in mfks.bcachefs waiting for confirmation of
the force overwrite.

Signed-off-by: Su Yue <glass.su@suse.com>
---
changelog:
v2:
    Add more details about why MKFS_BCACHEFS_PROG should contain '-f'.
---
 common/config |  3 ++-
 common/rc     | 12 +++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/common/config b/common/config
index c9771ff934cb..1f9edceec57a 100644
--- a/common/config
+++ b/common/config
@@ -105,7 +105,7 @@ set_mkfs_prog_path_with_opts()
 	# Note: mkfs.f2fs doesn't support the --help option yet, but it doesn't
 	# matter since it also prints the help when an invalid option is given.
 	if [ "$p" != "" ] && \
-		$p --help |& grep -q "[[:space:]]-f[[:space:]|]"; then
+		$p --help |& grep -q "[[:space:]]-f[[:space:]|,]"; then
 		echo "$p -f"
 	else
 		echo $p
@@ -313,6 +313,7 @@ export MKFS_REISER4_PROG=$(type -P mkfs.reiser4)
 export E2FSCK_PROG=$(type -P e2fsck)
 export TUNE2FS_PROG=$(type -P tune2fs)
 export FSCK_OVERLAY_PROG=$(type -P fsck.overlay)
+export MKFS_BCACHEFS_PROG=$(set_mkfs_prog_path_with_opts bcachefs)
 
 # SELinux adds extra xattrs which can mess up our expected output.
 # So, mount with a context, and they won't be created.
diff --git a/common/rc b/common/rc
index 524ffa02aa6a..31c21d2a8360 100644
--- a/common/rc
+++ b/common/rc
@@ -611,6 +611,9 @@ _test_mkfs()
     xfs)
 	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $* $TEST_DEV
 	;;
+    bcachefs)
+    $MKFS_BCACHEFS_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
+	;;
     *)
 	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* $TEST_DEV
 	;;
@@ -753,6 +756,10 @@ _scratch_mkfs()
 		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
 		mkfs_filter="grep -v -e ^mkfs\.ocfs2"
 		;;
+    bcachefs)
+		mkfs_cmd="$MKFS_BCACHEFS_PROG"
+		mkfs_filter="cat"
+		;;
 	*)
 		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
 		mkfs_filter="cat"
@@ -1044,7 +1051,7 @@ _scratch_mkfs_sized()
 		export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
 		;;
 	bcachefs)
-		$MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS --fs_size=$fssize --block_size=$blocksize $SCRATCH_DEV
+		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize --block_size=$blocksize $SCRATCH_DEV
 		;;
 	*)
 		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized"
@@ -1128,8 +1135,7 @@ _scratch_mkfs_blocksized()
 						-C $blocksize $SCRATCH_DEV
 		;;
 	bcachefs)
-		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS --block_size=$blocksize \
-								$SCRATCH_DEV
+		_scratch_mkfs --block_size=$blocksize
 		;;
 	udf)
 		_scratch_mkfs -b $blocksize
-- 
2.43.0

[PATCH v2 2/2] common/rc: improve block size support for bcachefs

[Su Yue]

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@suse.com>
---
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'`
+		;;
 	esac
 
 	[ -n "$def_blksz" ] && blocksize=$def_blksz
-- 
2.43.0

Re: [PATCH v2 1/2] fstests: introduce MKFS_BCACHEFS_PROG for bcachefs

[Brian Foster]

On Wed, Jan 17, 2024 at 05:23:08PM +0800, Su Yue wrote:
> mkfs.bcachefs supports force overwrite when option '-f' is given:
> $ mkfs.bcachefs --help | grep force
>   -f, --force
> 
> There are some tests which call _scratch_mkfs multiple times
> e.g. tests/generic/171. Without '-f' in MKFS_OPTIONS,
> these tests just hang in overwrite confirmation.
> After this commit, MKFS_BCACHEFS_PROG will contains ' -f' so
> we don't have to add '-f' to MKFS_OPTIONS manually to make
> these tests pass.
> 
> It also fixes generic/466 which unsets MKFS_OPTIONS causing
> that test hangs in mfks.bcachefs waiting for confirmation of
> the force overwrite.
> 
> Signed-off-by: Su Yue <glass.su@suse.com>
> ---
> changelog:
> v2:
>     Add more details about why MKFS_BCACHEFS_PROG should contain '-f'.
> ---
>  common/config |  3 ++-
>  common/rc     | 12 +++++++++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/common/config b/common/config
> index c9771ff934cb..1f9edceec57a 100644
> --- a/common/config
> +++ b/common/config
> @@ -105,7 +105,7 @@ set_mkfs_prog_path_with_opts()
>  	# Note: mkfs.f2fs doesn't support the --help option yet, but it doesn't
>  	# matter since it also prints the help when an invalid option is given.
>  	if [ "$p" != "" ] && \
> -		$p --help |& grep -q "[[:space:]]-f[[:space:]|]"; then
> +		$p --help |& grep -q "[[:space:]]-f[[:space:]|,]"; then
>  		echo "$p -f"
>  	else
>  		echo $p
> @@ -313,6 +313,7 @@ export MKFS_REISER4_PROG=$(type -P mkfs.reiser4)
>  export E2FSCK_PROG=$(type -P e2fsck)
>  export TUNE2FS_PROG=$(type -P tune2fs)
>  export FSCK_OVERLAY_PROG=$(type -P fsck.overlay)
> +export MKFS_BCACHEFS_PROG=$(set_mkfs_prog_path_with_opts bcachefs)
>  
>  # SELinux adds extra xattrs which can mess up our expected output.
>  # So, mount with a context, and they won't be created.
> diff --git a/common/rc b/common/rc
> index 524ffa02aa6a..31c21d2a8360 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -611,6 +611,9 @@ _test_mkfs()
>      xfs)
>  	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $* $TEST_DEV
>  	;;
> +    bcachefs)
> +    $MKFS_BCACHEFS_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null

Whitespace looks off on this line, but I suppose the maintainer can fix
that up if desired. That aside this LGTM:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +	;;
>      *)
>  	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* $TEST_DEV
>  	;;
> @@ -753,6 +756,10 @@ _scratch_mkfs()
>  		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
>  		mkfs_filter="grep -v -e ^mkfs\.ocfs2"
>  		;;
> +    bcachefs)
> +		mkfs_cmd="$MKFS_BCACHEFS_PROG"
> +		mkfs_filter="cat"
> +		;;
>  	*)
>  		mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
>  		mkfs_filter="cat"
> @@ -1044,7 +1051,7 @@ _scratch_mkfs_sized()
>  		export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
>  		;;
>  	bcachefs)
> -		$MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS --fs_size=$fssize --block_size=$blocksize $SCRATCH_DEV
> +		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize --block_size=$blocksize $SCRATCH_DEV
>  		;;
>  	*)
>  		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized"
> @@ -1128,8 +1135,7 @@ _scratch_mkfs_blocksized()
>  						-C $blocksize $SCRATCH_DEV
>  		;;
>  	bcachefs)
> -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS --block_size=$blocksize \
> -								$SCRATCH_DEV
> +		_scratch_mkfs --block_size=$blocksize
>  		;;
>  	udf)
>  		_scratch_mkfs -b $blocksize
> -- 
> 2.43.0
> 

Re: [PATCH v2 2/2] common/rc: improve block size support for bcachefs

[Brian Foster]

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@suse.com>
> ---
> 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
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?

Brian

>  	esac
>  
>  	[ -n "$def_blksz" ] && blocksize=$def_blksz
> -- 
> 2.43.0
> 

Re: [PATCH v2 2/2] common/rc: improve block size support for bcachefs

[Su Yue]

On Wed 17 Jan 2024 at 12:55, Brian Foster <bfoster@redhat.com> 
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@suse.com>
>> ---
>> 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.

--
Su
>
> Brian
>
>>  	esac
>>
>>  	[ -n "$def_blksz" ] && blocksize=$def_blksz
>> --
>> 2.43.0
>>

Re: [PATCH v2 2/2] common/rc: improve block size support for bcachefs

[Brian Foster]

On Thu, Jan 18, 2024 at 10:59:14AM +0800, Su Yue wrote:
> 
> On Wed 17 Jan 2024 at 12:55, Brian Foster <bfoster@redhat.com> 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@suse.com>
> > > ---
> > > 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
> > > 
> 

Re: [PATCH v2 2/2] common/rc: improve block size support for bcachefs

[Su Yue]

On Fri 19 Jan 2024 at 11:09, Brian Foster <bfoster@redhat.com> 
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@redhat.com> 
>> 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@suse.com>
>> > > ---
>> > > 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
>> > >
>>

Re: [PATCH v2 2/2] common/rc: improve block size support for bcachefs

[Brian Foster]

On Sun, Jan 21, 2024 at 12:00:47PM +0800, Su Yue wrote:
> 
> On Fri 19 Jan 2024 at 11:09, Brian Foster <bfoster@redhat.com> 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@redhat.com>
> > > 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@suse.com>
> > > > > ---
> > > > > 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.

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. :/

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

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

Re: [PATCH v2 2/2] common/rc: improve block size support for bcachefs

[Su Yue]

On Mon 22 Jan 2024 at 10:20, Brian Foster <bfoster@redhat.com> 
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@redhat.com> 
>> 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@redhat.com>
>> > > 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@suse.com>
>> > > > > ---
>> > > > > 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
>> > > > >
>> > >
>>

Re: [PATCH v2 2/2] common/rc: improve block size support for bcachefs

[Brian Foster]

On Thu, Jan 25, 2024 at 10:10:48AM +0800, Su Yue wrote:
> 
> On Mon 22 Jan 2024 at 10:20, Brian Foster <bfoster@redhat.com> 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@redhat.com>
> > > 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@redhat.com>
> > > > > 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@suse.com>
> > > > > > > ---
> > > > > > > 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
> > > > > > >
> > > > >
> > > 
>