[PATCH] xfs/205,432,516: read sysfs for mkfs block size instead of hardcoded values

[PATCH] xfs/205,432,516: read sysfs for mkfs block size instead of hardcoded values

[Nitesh Shetty]

At present block size of 1024 is hardcoded for mkfs. This creates
problem when device block size is more than 1024. So we use sysfs values
of SCRATCH_DEV, if not found then default to 1024.

Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
---
 tests/xfs/205 | 3 ++-
 tests/xfs/432 | 3 ++-
 tests/xfs/516 | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/xfs/205 b/tests/xfs/205
index 104f1f45..73e32c4d 100755
--- a/tests/xfs/205
+++ b/tests/xfs/205
@@ -22,7 +22,8 @@ _require_scratch_nocheck
 # bitmap consuming all the free space in our small data device.
 unset SCRATCH_RTDEV
 
-fsblksz=1024
+physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
+fsblksz=${physical:-1024}
 _scratch_mkfs_xfs -d size=$((32768*fsblksz)) -b size=$fsblksz >> $seqres.full 2>&1
 _scratch_mount
 
diff --git a/tests/xfs/432 b/tests/xfs/432
index 86012f0b..cd6367e2 100755
--- a/tests/xfs/432
+++ b/tests/xfs/432
@@ -49,7 +49,8 @@ echo "Format and mount"
 # block.  8187 hashes/dablk / 248 dirents/dirblock = ~33 dirblocks per
 # dablock.  33 dirblocks * 64k mean that we can expand a directory by
 # 2112k before we have to allocate another da btree block.
-_scratch_mkfs -b size=1k -n size=64k > "$seqres.full" 2>&1
+physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
+_scratch_mkfs -b size=${physical:-1k} -n size=64k > "$seqres.full" 2>&1
 _scratch_mount >> "$seqres.full" 2>&1
 
 metadump_file="$TEST_DIR/meta-$seq"
diff --git a/tests/xfs/516 b/tests/xfs/516
index 9e1b9931..b9d4f0c9 100755
--- a/tests/xfs/516
+++ b/tests/xfs/516
@@ -84,7 +84,8 @@ test_su_opts()
 	local mounted=0
 
 	echo "Format with 256k stripe unit; 4x stripe width" >> $seqres.full
-	_scratch_mkfs -b size=1k -d su=256k,sw=4 >> $seqres.full 2>&1
+	physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
+	_scratch_mkfs -b size=${physical:-1k} -d su=256k,sw=4 >> $seqres.full 2>&1
 
 	__test_mount_opts "$@"
 }

base-commit: 2ea74ba4e70b546279896e2a733c8c7f4b206193
-- 
2.25.1

Re: [PATCH] xfs/205,432,516: read sysfs for mkfs block size instead of hardcoded values

[Luis Chamberlain]

On Wed, Mar 02, 2022 at 03:00:22AM +0530, Nitesh Shetty wrote:
> At present block size of 1024 is hardcoded for mkfs. This creates
> problem when device block size is more than 1024. So we use sysfs values
> of SCRATCH_DEV, if not found then default to 1024.
> 
> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH] xfs/205,432,516: read sysfs for mkfs block size instead of hardcoded values

[Darrick J. Wong]

On Wed, Mar 02, 2022 at 03:00:22AM +0530, Nitesh Shetty wrote:
> At present block size of 1024 is hardcoded for mkfs. This creates
> problem when device block size is more than 1024. So we use sysfs values

What kinds of problems?

> of SCRATCH_DEV, if not found then default to 1024.
> 
> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
> ---
>  tests/xfs/205 | 3 ++-
>  tests/xfs/432 | 3 ++-
>  tests/xfs/516 | 3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/xfs/205 b/tests/xfs/205
> index 104f1f45..73e32c4d 100755
> --- a/tests/xfs/205
> +++ b/tests/xfs/205
> @@ -22,7 +22,8 @@ _require_scratch_nocheck
>  # bitmap consuming all the free space in our small data device.
>  unset SCRATCH_RTDEV
>  
> -fsblksz=1024
> +physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)

For a disk with 512 byte sectors, does this set physical=512?

The code within the $() really ought to be turned into a helper
_scratch_dev_phys_block_size or something.

> +fsblksz=${physical:-1024}

Filesystem blocksize isn't supposed to be smaller than the physical
blocksize, but why do you change the fs block size to the physical size?


>  _scratch_mkfs_xfs -d size=$((32768*fsblksz)) -b size=$fsblksz >> $seqres.full 2>&1
>  _scratch_mount
>  
> diff --git a/tests/xfs/432 b/tests/xfs/432
> index 86012f0b..cd6367e2 100755
> --- a/tests/xfs/432
> +++ b/tests/xfs/432
> @@ -49,7 +49,8 @@ echo "Format and mount"
>  # block.  8187 hashes/dablk / 248 dirents/dirblock = ~33 dirblocks per
>  # dablock.  33 dirblocks * 64k mean that we can expand a directory by
>  # 2112k before we have to allocate another da btree block.
> -_scratch_mkfs -b size=1k -n size=64k > "$seqres.full" 2>&1
> +physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
> +_scratch_mkfs -b size=${physical:-1k} -n size=64k > "$seqres.full" 2>&1

This test formats a very specific geometry because it needs precise
calculations to generate a directory with 1000 consecutively mapped
blocks.  Does it still do that if the blocksize isn't 1k?

>  _scratch_mount >> "$seqres.full" 2>&1
>  
>  metadump_file="$TEST_DIR/meta-$seq"
> diff --git a/tests/xfs/516 b/tests/xfs/516
> index 9e1b9931..b9d4f0c9 100755
> --- a/tests/xfs/516
> +++ b/tests/xfs/516
> @@ -84,7 +84,8 @@ test_su_opts()
>  	local mounted=0
>  
>  	echo "Format with 256k stripe unit; 4x stripe width" >> $seqres.full
> -	_scratch_mkfs -b size=1k -d su=256k,sw=4 >> $seqres.full 2>&1
> +	physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
> +	_scratch_mkfs -b size=${physical:-1k} -d su=256k,sw=4 >> $seqres.full 2>&1

This is a test of sunit/swidth.  Do you need to scale those up as well?

--D

>  
>  	__test_mount_opts "$@"
>  }
> 
> base-commit: 2ea74ba4e70b546279896e2a733c8c7f4b206193
> -- 
> 2.25.1
> 

Re: [PATCH] xfs/205,432,516: read sysfs for mkfs block size instead of hardcoded values

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 4195 bytes --]

On Wed, Mar 02, 2022 at 02:57:00PM -0800, Darrick J. Wong wrote:
> On Wed, Mar 02, 2022 at 03:00:22AM +0530, Nitesh Shetty wrote:
> > At present block size of 1024 is hardcoded for mkfs. This creates
> > problem when device block size is more than 1024. So we use sysfs values
> 
> What kinds of problems?
>

I was running the test on NVMe device, which has a LBA of 4096 by default,
so these test fails, not becuase of functionality issues, mainly because of
mkfs failure. mkfs failure is mainly because of assumption of device
LBA is 512 bytes, which is mostly right incase of scsi device.

> > of SCRATCH_DEV, if not found then default to 1024.
> > 
> > Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
> > ---
> >  tests/xfs/205 | 3 ++-
> >  tests/xfs/432 | 3 ++-
> >  tests/xfs/516 | 3 ++-
> >  3 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/xfs/205 b/tests/xfs/205
> > index 104f1f45..73e32c4d 100755
> > --- a/tests/xfs/205
> > +++ b/tests/xfs/205
> > @@ -22,7 +22,8 @@ _require_scratch_nocheck
> >  # bitmap consuming all the free space in our small data device.
> >  unset SCRATCH_RTDEV
> >  
> > -fsblksz=1024
> > +physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
> 
> For a disk with 512 byte sectors, does this set physical=512?
> 
> The code within the $() really ought to be turned into a helper
> _scratch_dev_phys_block_size or something.
> 

Yes it does, you are right, I see a new failure, since minimum block size for CRC
enabled filesystem is 1K. I will make it minimum of 1024 and actual
device LBA and send v2.

> > +fsblksz=${physical:-1024}
> 
> Filesystem blocksize isn't supposed to be smaller than the physical
> blocksize, but why do you change the fs block size to the physical size?
> 
> 
adressed above. next patch will have fsblksz=min(1024, physical blocksize)

> >  _scratch_mkfs_xfs -d size=$((32768*fsblksz)) -b size=$fsblksz >> $seqres.full 2>&1
> >  _scratch_mount
> >  
> > diff --git a/tests/xfs/432 b/tests/xfs/432
> > index 86012f0b..cd6367e2 100755
> > --- a/tests/xfs/432
> > +++ b/tests/xfs/432
> > @@ -49,7 +49,8 @@ echo "Format and mount"
> >  # block.  8187 hashes/dablk / 248 dirents/dirblock = ~33 dirblocks per
> >  # dablock.  33 dirblocks * 64k mean that we can expand a directory by
> >  # 2112k before we have to allocate another da btree block.
> > -_scratch_mkfs -b size=1k -n size=64k > "$seqres.full" 2>&1
> > +physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
> > +_scratch_mkfs -b size=${physical:-1k} -n size=64k > "$seqres.full" 2>&1
> 
> This test formats a very specific geometry because it needs precise
> calculations to generate a directory with 1000 consecutively mapped
> blocks.  Does it still do that if the blocksize isn't 1k?
> 

yes, agree its geometric specific. But with above change test exits as
not run, instead of failure. Reason being unable to create >1000 data
block extents.
Yes, you are right about needing precise calculation to generate
that 1000 data block. My knowledge on file system is limited at present.
Does it makes sense to put those tests which run with 512 bytes
assumption to not_run state, instead of fail for 4096 block size
devices, as a temporary fix ? or should I just simply report them so
that persons with fs expertise fix it ?

> >  _scratch_mount >> "$seqres.full" 2>&1
> >  
> >  metadump_file="$TEST_DIR/meta-$seq"
> > diff --git a/tests/xfs/516 b/tests/xfs/516
> > index 9e1b9931..b9d4f0c9 100755
> > --- a/tests/xfs/516
> > +++ b/tests/xfs/516
> > @@ -84,7 +84,8 @@ test_su_opts()
> >  	local mounted=0
> >  
> >  	echo "Format with 256k stripe unit; 4x stripe width" >> $seqres.full
> > -	_scratch_mkfs -b size=1k -d su=256k,sw=4 >> $seqres.full 2>&1
> > +	physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
> > +	_scratch_mkfs -b size=${physical:-1k} -d su=256k,sw=4 >> $seqres.full 2>&1

> This is a test of sunit/swidth.  Do you need to scale those up as well?
>

Agreed, they should be scaled.
> --D
> 
> >  
> >  	__test_mount_opts "$@"
> >  }
> > 
> > base-commit: 2ea74ba4e70b546279896e2a733c8c7f4b206193
> > -- 
> > 2.25.1
> > 
>

--
Nitesh Shetty

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]