[PATCH 0/9] 64K blocksize related fixes

[PATCH 0/9] 64K blocksize related fixes

[Ritesh Harjani]

Below are the list of fixes mostly centered around 64K blocksize (on PPC64)
and with ext4 filesystem. Tested this with both 64K & 4K blocksize on Power
with (ext4/ext3/ext2/xfs/btrfs).

Ritesh Harjani (9):
  ext4/003: Fix this test on 64K platform for dax config
  ext4/027: Correct the right code of block and inode bitmap
  ext4/306: Add -b blocksize parameter too to avoid failure with DAX config
  ext4/022: exclude this test for dax config on 64KB pagesize platform
  generic/031: Fix the test case for 64k blocksize config
  gitignore: Add 031.out file to .gitignore
  generic/620: Remove -b blocksize option for ext4
  common/attr: Cleanup end of line whitespaces issues
  common/attr: Reduce MAX_ATTRS to leave some overhead for 64K blocksize

 .gitignore                                 |  1 +
 common/attr                                | 20 ++++++------
 tests/ext4/003                             |  3 +-
 tests/ext4/022                             |  7 ++--
 tests/ext4/027                             |  4 +--
 tests/ext4/306                             |  5 ++-
 tests/generic/031                          | 37 ++++++++++++++++++----
 tests/generic/031.out.64k                  | 19 +++++++++++
 tests/generic/{031.out => 031.out.default} |  0
 tests/generic/620                          |  7 ++++
 10 files changed, 80 insertions(+), 23 deletions(-)
 create mode 100644 tests/generic/031.out.64k
 rename tests/generic/{031.out => 031.out.default} (100%)

--
2.31.1

[PATCH 1/9] ext4/003: Fix this test on 64K platform for dax config

[Ritesh Harjani]

mkfs.ext4 by default uses 4K blocksize which doesn't mount when testing
with dax config and the test fails. This patch fixes it.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 tests/ext4/003 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/ext4/003 b/tests/ext4/003
index 00ea9150..1ddb3063 100755
--- a/tests/ext4/003
+++ b/tests/ext4/003
@@ -31,7 +31,8 @@ _require_scratch_ext4_feature "bigalloc"
 
 rm -f $seqres.full
 
-$MKFS_EXT4_PROG -F -O bigalloc -C 65536  -g 256 $SCRATCH_DEV 512m \
+BLOCK_SIZE=$(get_page_size)
+$MKFS_EXT4_PROG -F -b $BLOCK_SIZE -O bigalloc -C 65536  -g 256 $SCRATCH_DEV 512m \
 	>> $seqres.full 2>&1
 _scratch_mount
 
-- 
2.31.1

Re: [PATCH 1/9] ext4/003: Fix this test on 64K platform for dax config

[Theodore Ts'o]

On Mon, Jun 14, 2021 at 11:58:05AM +0530, Ritesh Harjani wrote:
> mkfs.ext4 by default uses 4K blocksize which doesn't mount when testing
> with dax config and the test fails. This patch fixes it.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  tests/ext4/003 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/ext4/003 b/tests/ext4/003
> index 00ea9150..1ddb3063 100755
> --- a/tests/ext4/003
> +++ b/tests/ext4/003
> @@ -31,7 +31,8 @@ _require_scratch_ext4_feature "bigalloc"
>  
>  rm -f $seqres.full
>  
> -$MKFS_EXT4_PROG -F -O bigalloc -C 65536  -g 256 $SCRATCH_DEV 512m \
> +BLOCK_SIZE=$(get_page_size)
> +$MKFS_EXT4_PROG -F -b $BLOCK_SIZE -O bigalloc -C 65536  -g 256 $SCRATCH_DEV 512m \
>  	>> $seqres.full 2>&1
>  _scratch_mount

Thanks for the patch!

If the block size is 64k, then the cluster_size == block_size at which
point ext4/003 won't be able to test for the regression its designed
to test.  So we probably need to scale the cluster size and file
system size relative to the block size.

					- Ted

Re: [PATCH 1/9] ext4/003: Fix this test on 64K platform for dax config

[Ritesh Harjani]

On 21/06/30 12:27PM, Theodore Ts'o wrote:
> On Mon, Jun 14, 2021 at 11:58:05AM +0530, Ritesh Harjani wrote:
> > mkfs.ext4 by default uses 4K blocksize which doesn't mount when testing
> > with dax config and the test fails. This patch fixes it.
> >
> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > ---
> >  tests/ext4/003 | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/ext4/003 b/tests/ext4/003
> > index 00ea9150..1ddb3063 100755
> > --- a/tests/ext4/003
> > +++ b/tests/ext4/003
> > @@ -31,7 +31,8 @@ _require_scratch_ext4_feature "bigalloc"
> >
> >  rm -f $seqres.full
> >
> > -$MKFS_EXT4_PROG -F -O bigalloc -C 65536  -g 256 $SCRATCH_DEV 512m \
> > +BLOCK_SIZE=$(get_page_size)
> > +$MKFS_EXT4_PROG -F -b $BLOCK_SIZE -O bigalloc -C 65536  -g 256 $SCRATCH_DEV 512m \
> >  	>> $seqres.full 2>&1
> >  _scratch_mount
>
> Thanks for the patch!

Thanks for the review, sorry about the delay (- Last week was short a week for
me).

>
> If the block size is 64k, then the cluster_size == block_size at which
> point ext4/003 won't be able to test for the regression its designed
> to test.  So we probably need to scale the cluster size and file
> system size relative to the block size.

Yes, thanks for catching it. I think if make below change, i.e. scale cluster
size, we should be good. Since this will make blocks_per_group = 4096 and
clusters_per_group = 256. This is the condition, which I guess the original
kernel patch fixed it for. So, we need not increase the filesystem size.

$MKFS_EXT4_PROG -F -b $BLOCK_SIZE -O bigalloc -C $((BLOCK_SIZE * 16))  -g 256 $SCRATCH_DEV 512m \

-ritesh






>
> 					- Ted

Re: [PATCH 1/9] ext4/003: Fix this test on 64K platform for dax config

[Theodore Ts'o]

On Thu, Jul 08, 2021 at 11:54:45AM +0530, Ritesh Harjani wrote:
> Yes, thanks for catching it. I think if make below change, i.e. scale cluster
> size, we should be good. Since this will make blocks_per_group = 4096 and
> clusters_per_group = 256. This is the condition, which I guess the original
> kernel patch fixed it for. So, we need not increase the filesystem size.
> 
> $MKFS_EXT4_PROG -F -b $BLOCK_SIZE -O bigalloc -C $((BLOCK_SIZE * 16))  -g 256 $SCRATCH_DEV 512m \
>

Agreed, it looks like that should work.

Cheers,

					- Ted

[PATCH 2/9] ext4/027: Correct the right code of block and inode bitmap

[Ritesh Harjani]

Observed occasional failure of this test sometimes say with 64k config
and small device size. Reason is we were grepping for wrong values for
inode and block bitmap.

Correct those values according to [1] to fix this test.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/fsmap.h#n53

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 tests/ext4/027 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/ext4/027 b/tests/ext4/027
index 97c14cf5..83d5a413 100755
--- a/tests/ext4/027
+++ b/tests/ext4/027
@@ -45,11 +45,11 @@ x=$(grep -c 'static fs metadata' $TEST_DIR/fsmap)
 test $x -gt 0 || echo "No fs metadata?"
 
 echo "Check block bitmap" | tee -a $seqres.full
-x=$(grep -c 'special 102:1' $TEST_DIR/fsmap)
+x=$(grep -c 'special 102:3' $TEST_DIR/fsmap)
 test $x -gt 0 || echo "No block bitmaps?"
 
 echo "Check inode bitmap" | tee -a $seqres.full
-x=$(grep -c 'special 102:2' $TEST_DIR/fsmap)
+x=$(grep -c 'special 102:4' $TEST_DIR/fsmap)
 test $x -gt 0 || echo "No inode bitmaps?"
 
 echo "Check inodes" | tee -a $seqres.full
-- 
2.31.1

Re: [PATCH 2/9] ext4/027: Correct the right code of block and inode bitmap

[Darrick J. Wong]

On Mon, Jun 14, 2021 at 11:58:06AM +0530, Ritesh Harjani wrote:
> Observed occasional failure of this test sometimes say with 64k config
> and small device size. Reason is we were grepping for wrong values for
> inode and block bitmap.
> 
> Correct those values according to [1] to fix this test.
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/fsmap.h#n53
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  tests/ext4/027 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/ext4/027 b/tests/ext4/027
> index 97c14cf5..83d5a413 100755
> --- a/tests/ext4/027
> +++ b/tests/ext4/027
> @@ -45,11 +45,11 @@ x=$(grep -c 'static fs metadata' $TEST_DIR/fsmap)
>  test $x -gt 0 || echo "No fs metadata?"
>  
>  echo "Check block bitmap" | tee -a $seqres.full
> -x=$(grep -c 'special 102:1' $TEST_DIR/fsmap)
> +x=$(grep -c 'special 102:3' $TEST_DIR/fsmap)
>  test $x -gt 0 || echo "No block bitmaps?"
>  
>  echo "Check inode bitmap" | tee -a $seqres.full
> -x=$(grep -c 'special 102:2' $TEST_DIR/fsmap)
> +x=$(grep -c 'special 102:4' $TEST_DIR/fsmap)

Maaaaybe I should have added textual descriptions for the ext4 getfsmap
owners.  Sorry... :(

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  test $x -gt 0 || echo "No inode bitmaps?"
>  
>  echo "Check inodes" | tee -a $seqres.full
> -- 
> 2.31.1
> 

[PATCH 3/9] ext4/306: Add -b blocksize parameter too to avoid failure with DAX config

[Ritesh Harjani]

mkfs.ext4 by default uses 4K blocksize. On DAX config with a 64K
pagesize platform (PPC64), this will fail to mount since DAX requires bs
== ps.
Hence add the -b blocksize paramter in ext4/306.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 tests/ext4/306 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/ext4/306 b/tests/ext4/306
index 146fdb39..1d45a9d0 100755
--- a/tests/ext4/306
+++ b/tests/ext4/306
@@ -38,7 +38,10 @@ features="^extents"
 if grep -q 64bit /etc/mke2fs.conf ; then
     features="^extents,^64bit"
 fi
-$MKFS_EXT4_PROG -F -O "$features" $SCRATCH_DEV 512m >> $seqres.full 2>&1
+
+blksz=$(get_page_size)
+
+$MKFS_EXT4_PROG -F -b $blksz -O "$features" $SCRATCH_DEV 512m >> $seqres.full 2>&1
 _scratch_mount
 
 # Create a small non-extent-based file
-- 
2.31.1

Re: [PATCH 3/9] ext4/306: Add -b blocksize parameter too to avoid failure with DAX config

[Theodore Ts'o]

On Mon, Jun 14, 2021 at 11:58:07AM +0530, Ritesh Harjani wrote:
> mkfs.ext4 by default uses 4K blocksize. On DAX config with a 64K
> pagesize platform (PPC64), this will fail to mount since DAX requires bs
> == ps.
> Hence add the -b blocksize paramter in ext4/306.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

Looks good, thanks.

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

					- Ted

[PATCH 4/9] ext4/022: exclude this test for dax config on 64KB pagesize platform

[Ritesh Harjani]

This test case assumes blocksize to be 4KB and hence it fails
to mount with "-o dax" option on a 64kb pagesize platform (e.g. PPC64).
This leads to test case reported as failed with dax config on PPC64.

This patch exclude this test when pagesize is 64KB and for dax config.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 tests/ext4/022 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/ext4/022 b/tests/ext4/022
index 3de7619a..ca58f34e 100755
--- a/tests/ext4/022
+++ b/tests/ext4/022
@@ -41,10 +41,13 @@ _require_dumpe2fs
 _require_command "$DEBUGFS_PROG" debugfs
 _require_attrs
 
-# Use large inodes to have enough space for experimentation
-INODE_SIZE=1024
 # Block size
 BLOCK_SIZE=4096
+if [[ $(get_page_size) -ne $BLOCK_SIZE ]]; then
+       _exclude_scratch_mount_option dax
+fi
+# Use large inodes to have enough space for experimentation
+INODE_SIZE=1024
 # We leave this amount of bytes for xattrs
 XATTR_SPACE=256
 # We grow extra_isize by this much
-- 
2.31.1

Re: [PATCH 4/9] ext4/022: exclude this test for dax config on 64KB pagesize platform

[Theodore Ts'o]

On Mon, Jun 14, 2021 at 11:58:08AM +0530, Ritesh Harjani wrote:
> This test case assumes blocksize to be 4KB and hence it fails
> to mount with "-o dax" option on a 64kb pagesize platform (e.g. PPC64).
> This leads to test case reported as failed with dax config on PPC64.
> 
> This patch exclude this test when pagesize is 64KB and for dax config.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

Looks good, thanks.

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

[PATCH 5/9] generic/031: Fix the test case for 64k blocksize config

[Ritesh Harjani]

This test fails with blocksize 64k since the test assumes 4k blocksize
in fcollapse param. This patch fixes that and also tests for 64k
blocksize.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 tests/generic/031                          | 37 ++++++++++++++++++----
 tests/generic/031.out.64k                  | 19 +++++++++++
 tests/generic/{031.out => 031.out.default} |  0
 3 files changed, 49 insertions(+), 7 deletions(-)
 create mode 100644 tests/generic/031.out.64k
 rename tests/generic/{031.out => 031.out.default} (100%)

diff --git a/tests/generic/031 b/tests/generic/031
index db84031b..40cb23af 100755
--- a/tests/generic/031
+++ b/tests/generic/031
@@ -8,6 +8,7 @@
 # correctly written and aren't left behind causing invalidation or data
 # corruption issues.
 #
+seqfull=$0
 seq=`basename $0`
 seqres=$RESULT_DIR/$seq
 echo "QA output created by $seq"
@@ -39,12 +40,35 @@ testfile=$SCRATCH_MNT/testfile
 _scratch_mkfs > /dev/null 2>&1
 _scratch_mount
 
-$XFS_IO_PROG -f \
-	-c "pwrite 185332 55756" \
-	-c "fcollapse 28672 40960" \
-	-c "pwrite 133228 63394" \
-	-c "fcollapse 0 4096" \
-$testfile | _filter_xfs_io
+# fcollapse need offset and len to be multiple of blocksize for filesystems
+# hence make this test work with 64k blocksize as well.
+blksz=$(_get_block_size $SCRATCH_MNT)
+
+rm -f $seqfull.out
+if [ "$blksz" -eq 65536 ]; then
+	ln -s $seq.out.64k $seqfull.out
+else
+	ln -s $seq.out.default $seqfull.out
+fi
+
+if [[ $blksz -le 4096 ]]; then
+	$XFS_IO_PROG -f \
+		-c "pwrite 185332 55756" \
+		-c "fcollapse 28672 40960" \
+		-c "pwrite 133228 63394" \
+		-c "fcollapse 0 4096" \
+	$testfile | _filter_xfs_io
+elif [[ $blksz -eq 65536 ]]; then
+	fact=$blksz/4096
+	$XFS_IO_PROG -f \
+		-c "pwrite $((185332*fact + 12)) $((55756*fact + 12))" \
+		-c "fcollapse $((28672 * fact)) $((40960 * fact))" \
+		-c "pwrite $((133228 * fact + 12)) $((63394 * fact + 12))" \
+		-c "fcollapse 0 $((4096 * fact))" \
+	$testfile | _filter_xfs_io
+else
+	_notrun "blocksize not supported"
+fi
 
 echo "==== Pre-Remount ==="
 hexdump -C $testfile
@@ -54,4 +78,3 @@ hexdump -C $testfile
 
 status=0
 exit
-
diff --git a/tests/generic/031.out.64k b/tests/generic/031.out.64k
new file mode 100644
index 00000000..7dfcfe41
--- /dev/null
+++ b/tests/generic/031.out.64k
@@ -0,0 +1,19 @@
+QA output created by 031
+wrote 892108/892108 bytes at offset 2965324
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1014316/1014316 bytes at offset 2131660
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+==== Pre-Remount ===
+00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
+*
+001f86c0  00 00 00 00 00 00 00 00  00 00 00 00 cd cd cd cd  |................|
+001f86d0  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  |................|
+*
+002fdc18
+==== Post-Remount ==
+00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
+*
+001f86c0  00 00 00 00 00 00 00 00  00 00 00 00 cd cd cd cd  |................|
+001f86d0  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  |................|
+*
+002fdc18
diff --git a/tests/generic/031.out b/tests/generic/031.out.default
similarity index 100%
rename from tests/generic/031.out
rename to tests/generic/031.out.default
-- 
2.31.1

Re: [PATCH 5/9] generic/031: Fix the test case for 64k blocksize config

[Darrick J. Wong]

On Mon, Jun 14, 2021 at 11:58:09AM +0530, Ritesh Harjani wrote:
> This test fails with blocksize 64k since the test assumes 4k blocksize
> in fcollapse param. This patch fixes that and also tests for 64k
> blocksize.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  tests/generic/031                          | 37 ++++++++++++++++++----
>  tests/generic/031.out.64k                  | 19 +++++++++++
>  tests/generic/{031.out => 031.out.default} |  0
>  3 files changed, 49 insertions(+), 7 deletions(-)
>  create mode 100644 tests/generic/031.out.64k
>  rename tests/generic/{031.out => 031.out.default} (100%)
> 
> diff --git a/tests/generic/031 b/tests/generic/031
> index db84031b..40cb23af 100755
> --- a/tests/generic/031
> +++ b/tests/generic/031
> @@ -8,6 +8,7 @@
>  # correctly written and aren't left behind causing invalidation or data
>  # corruption issues.
>  #
> +seqfull=$0
>  seq=`basename $0`
>  seqres=$RESULT_DIR/$seq
>  echo "QA output created by $seq"
> @@ -39,12 +40,35 @@ testfile=$SCRATCH_MNT/testfile
>  _scratch_mkfs > /dev/null 2>&1
>  _scratch_mount
>  
> -$XFS_IO_PROG -f \
> -	-c "pwrite 185332 55756" \
> -	-c "fcollapse 28672 40960" \
> -	-c "pwrite 133228 63394" \
> -	-c "fcollapse 0 4096" \
> -$testfile | _filter_xfs_io
> +# fcollapse need offset and len to be multiple of blocksize for filesystems
> +# hence make this test work with 64k blocksize as well.
> +blksz=$(_get_block_size $SCRATCH_MNT)
> +
> +rm -f $seqfull.out
> +if [ "$blksz" -eq 65536 ]; then
> +	ln -s $seq.out.64k $seqfull.out
> +else
> +	ln -s $seq.out.default $seqfull.out
> +fi
> +
> +if [[ $blksz -le 4096 ]]; then
> +	$XFS_IO_PROG -f \
> +		-c "pwrite 185332 55756" \
> +		-c "fcollapse 28672 40960" \
> +		-c "pwrite 133228 63394" \
> +		-c "fcollapse 0 4096" \
> +	$testfile | _filter_xfs_io
> +elif [[ $blksz -eq 65536 ]]; then
> +	fact=$blksz/4096

What if the blocksize is 32k?

--D

> +	$XFS_IO_PROG -f \
> +		-c "pwrite $((185332*fact + 12)) $((55756*fact + 12))" \
> +		-c "fcollapse $((28672 * fact)) $((40960 * fact))" \
> +		-c "pwrite $((133228 * fact + 12)) $((63394 * fact + 12))" \
> +		-c "fcollapse 0 $((4096 * fact))" \
> +	$testfile | _filter_xfs_io
> +else
> +	_notrun "blocksize not supported"
> +fi
>  
>  echo "==== Pre-Remount ==="
>  hexdump -C $testfile
> @@ -54,4 +78,3 @@ hexdump -C $testfile
>  
>  status=0
>  exit
> -
> diff --git a/tests/generic/031.out.64k b/tests/generic/031.out.64k
> new file mode 100644
> index 00000000..7dfcfe41
> --- /dev/null
> +++ b/tests/generic/031.out.64k
> @@ -0,0 +1,19 @@
> +QA output created by 031
> +wrote 892108/892108 bytes at offset 2965324
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 1014316/1014316 bytes at offset 2131660
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +==== Pre-Remount ===
> +00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> +*
> +001f86c0  00 00 00 00 00 00 00 00  00 00 00 00 cd cd cd cd  |................|
> +001f86d0  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  |................|
> +*
> +002fdc18
> +==== Post-Remount ==
> +00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> +*
> +001f86c0  00 00 00 00 00 00 00 00  00 00 00 00 cd cd cd cd  |................|
> +001f86d0  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  |................|
> +*
> +002fdc18
> diff --git a/tests/generic/031.out b/tests/generic/031.out.default
> similarity index 100%
> rename from tests/generic/031.out
> rename to tests/generic/031.out.default
> -- 
> 2.31.1
> 

Re: [PATCH 5/9] generic/031: Fix the test case for 64k blocksize config

[Theodore Ts'o]

On Wed, Jun 30, 2021 at 08:50:01AM -0700, Darrick J. Wong wrote:
> > +# fcollapse need offset and len to be multiple of blocksize for filesystems
> > +# hence make this test work with 64k blocksize as well.
> ...
>
> What if the blocksize is 32k?

... or 8k?  or 16k?  (which might be more likely)

How bout if we change the offsets and lengths in the test so they are
all multiples of 64k, and adjusting 31.out appropriately?  That will
allow the test to work for block sizes up to 64k without needing to
having a special case for 031.out.64k.

I don't know of architectures with a page size > 64k (yet), so this
should hold us for a while.

				- Ted

Re: [PATCH 5/9] generic/031: Fix the test case for 64k blocksize config

[Ritesh Harjani]

On 21/06/30 01:18PM, Theodore Ts'o wrote:
> On Wed, Jun 30, 2021 at 08:50:01AM -0700, Darrick J. Wong wrote:
> > > +# fcollapse need offset and len to be multiple of blocksize for filesystems
> > > +# hence make this test work with 64k blocksize as well.
> > ...
> >
> > What if the blocksize is 32k?
>
> ... or 8k?  or 16k?  (which might be more likely)
>
> How bout if we change the offsets and lengths in the test so they are
> all multiples of 64k, and adjusting 31.out appropriately?  That will
> allow the test to work for block sizes up to 64k without needing to
> having a special case for 031.out.64k.
>
> I don't know of architectures with a page size > 64k (yet), so this
> should hold us for a while.
>

yes, so I already had done the changes in such a way that we can adapt to any
blocksize.  I will make the changes such that we take fact=65536/4096 by
default. Then this should cover all the cases for all blocksizes and we don't
have to change 031.out file for different blocksizes.

And the test tries to test non-aligned writes, but since I am adding some
additional unaligned bytes and also I have kept the layout of the writes same
as before, so I think the test should still cover the regression it is meant
for.


fact=65536/4096
    $XFS_IO_PROG -f \
        -c "pwrite $((185332*fact + 12)) $((55756*fact + 12))" \
        -c "fcollapse $((28672 * fact)) $((40960 * fact))" \
        -c "pwrite $((133228 * fact + 12)) $((63394 * fact + 12))" \
        -c "fcollapse 0 $((4096 * fact))" \
    $testfile | _filter_xfs_io

-ritesh

[PATCH 6/9] gitignore: Add 031.out file to .gitignore

[Ritesh Harjani]

Add 031.out file to .gitignore

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index d3194e76..7a3be5e3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -191,6 +191,7 @@ tags
 # Symlinked files
 /tests/generic/035.out
 /tests/generic/050.out
+/tests/generic/031.out
 /tests/xfs/033.out
 /tests/xfs/071.out
 /tests/xfs/096.out
-- 
2.31.1

Re: [PATCH 6/9] gitignore: Add 031.out file to .gitignore

[Theodore Ts'o]

On Mon, Jun 14, 2021 at 11:58:10AM +0530, Ritesh Harjani wrote:
> Add 031.out file to .gitignore
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

[PATCH 7/9] generic/620: Remove -b blocksize option for ext4

[Ritesh Harjani]

ext4 with 64k blocksize fails with below error for this given test which
requires dmhugedisk. Also since dax is not supported for this test, so
make sure to remove -b option, if set by config file for ext4 FSTYP for
the test to then use 4K blocksize by default.

mkfs.ext4: Input/output error while writing out and closing file system

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 tests/generic/620 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/generic/620 b/tests/generic/620
index 60559441..3ccda5e4 100755
--- a/tests/generic/620
+++ b/tests/generic/620
@@ -50,6 +50,13 @@ _require_dmhugedisk
 sectors=$((2*1024*1024*1024*17))
 chunk_size=128
 
+# ext4 with 64k blocksize fails to mkfs with below error.
+# So remove -b option, if set by config file.
+# mkfs.ext4: Input/output error while writing out and closing file system
+if [[ $FSTYP = "ext4" ]]; then
+	MKFS_OPTIONS=$(echo $MKFS_OPTIONS | sed -rn 's/(.*)(-b ?+[0-9]+)(.*)/\1 \3/p')
+fi
+
 _dmhugedisk_init $sectors $chunk_size
 _mkfs_dev $DMHUGEDISK_DEV
 _mount $DMHUGEDISK_DEV $SCRATCH_MNT || _fail "mount failed for $DMHUGEDISK_DEV $SCRATCH_MNT"
-- 
2.31.1

Re: [PATCH 7/9] generic/620: Remove -b blocksize option for ext4

[Theodore Ts'o]

On Mon, Jun 14, 2021 at 11:58:11AM +0530, Ritesh Harjani wrote:
> ext4 with 64k blocksize fails with below error for this given test which
> requires dmhugedisk. Also since dax is not supported for this test, so
> make sure to remove -b option, if set by config file for ext4 FSTYP for
> the test to then use 4K blocksize by default.
> 
> mkfs.ext4: Input/output error while writing out and closing file system
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

Looking at this test, I'm not convinced it actually does the right
thing when the block size is 64k, since the whole point is to test
what happens when the block number > INT_MAX.  So we should be able to
fix the block size to be 1k, which would allow us to use a smaller
dmhugedisk, and then skip this test if dax is enabled.

OTOH, generic/620 runs pretty quicky, so perhaps it's better to do
thie quick fix: hardcode the block size to 4k, and then skip it if dax
&& page_size != 4k.

					- Ted

Re: [PATCH 7/9] generic/620: Remove -b blocksize option for ext4

[Ritesh Harjani]

On 21/06/30 01:07PM, Theodore Ts'o wrote:
> On Mon, Jun 14, 2021 at 11:58:11AM +0530, Ritesh Harjani wrote:
> > ext4 with 64k blocksize fails with below error for this given test which
> > requires dmhugedisk. Also since dax is not supported for this test, so
> > make sure to remove -b option, if set by config file for ext4 FSTYP for
> > the test to then use 4K blocksize by default.
> >
> > mkfs.ext4: Input/output error while writing out and closing file system
> >
> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
>
> Looking at this test, I'm not convinced it actually does the right
> thing when the block size is 64k, since the whole point is to test
> what happens when the block number > INT_MAX.  So we should be able to
> fix the block size to be 1k, which would allow us to use a smaller
> dmhugedisk, and then skip this test if dax is enabled.
>
> OTOH, generic/620 runs pretty quicky, so perhaps it's better to do
> thie quick fix: hardcode the block size to 4k, and then skip it if dax
> && page_size != 4k.

Ok, so it is time to implement _mkfs_dev_blocksized() something like how we have
for _scratch_mkfs_blocksized(). This is since we can have different way of
passing blocksize parameter for mkfs prog for different filesystems.

-ritesh

[PATCH 8/9] common/attr: Cleanup end of line whitespaces issues

[Ritesh Harjani]

This patch clears the end of line whitespace issues in this file.
Mostly since many kernel developers also keep this editor config to clear
any end of line whitespaces on file save.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 common/attr | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/common/attr b/common/attr
index 42ceab92..d3902346 100644
--- a/common/attr
+++ b/common/attr
@@ -59,10 +59,10 @@ _acl_setup_ids()
         j=1
         for(i=1; i<1000000 && j<=3;i++){
           if (! (i in ids)) {
-	     printf "acl%d=%d;", j, i;		 
+	     printf "acl%d=%d;", j, i;
 	     j++
           }
-        }	
+        }
       }'`
 }
 
@@ -101,7 +101,7 @@ _getfacl_filter_id()
 _acl_ls()
 {
     _ls_l -n $* | awk '{ print $1, $3, $4, $NF }' | _acl_filter_id
-} 
+}
 
 # create an ACL with n ACEs in it
 #
@@ -128,7 +128,7 @@ _filter_aces()
 	BEGIN {
 	    FS=":"
 	    while ( getline <tmpfile > 0 ) {
-		idlist[$1] = $3 
+		idlist[$1] = $3
 	    }
 	}
 	/^user/ { if ($2 in idlist) sub($2, idlist[$2]); print; next}
@@ -180,17 +180,17 @@ _require_attrs()
 {
 	local args
 	local nsp
-	
+
 	if [ $# -eq 0 ]; then
 		args="user"
 	else
 	  	args="$*"
 	fi
-	
+
 	[ -n "$ATTR_PROG" ] || _notrun "attr command not found"
 	[ -n "$GETFATTR_PROG" ] || _notrun "getfattr command not found"
 	[ -n "$SETFATTR_PROG" ] || _notrun "setfattr command not found"
-	
+
 	for nsp in $args; do
 		#
 		# Test if chacl is able to write an attribute on the target
@@ -204,14 +204,14 @@ _require_attrs()
 		touch $TEST_DIR/syscalltest
 		$SETFATTR_PROG -n "$nsp.xfstests" -v "attr" $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
 		cat $TEST_DIR/syscalltest.out >> $seqres.full
-		
+
 		if grep -q 'Function not implemented' $TEST_DIR/syscalltest.out; then
 			_notrun "kernel does not support attrs"
 		fi
 		if grep -q 'Operation not supported' $TEST_DIR/syscalltest.out; then
 			_notrun "attr namespace $nsp not supported by this filesystem type: $FSTYP"
 		fi
-		
+
 		rm -f $TEST_DIR/syscalltest.out
 	done
 }
-- 
2.31.1

Re: [PATCH 8/9] common/attr: Cleanup end of line whitespaces issues

[Darrick J. Wong]

On Mon, Jun 14, 2021 at 11:58:12AM +0530, Ritesh Harjani wrote:
> This patch clears the end of line whitespace issues in this file.
> Mostly since many kernel developers also keep this editor config to clear
> any end of line whitespaces on file save.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  common/attr | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/common/attr b/common/attr
> index 42ceab92..d3902346 100644
> --- a/common/attr
> +++ b/common/attr
> @@ -59,10 +59,10 @@ _acl_setup_ids()
>          j=1
>          for(i=1; i<1000000 && j<=3;i++){
>            if (! (i in ids)) {
> -	     printf "acl%d=%d;", j, i;		 
> +	     printf "acl%d=%d;", j, i;
>  	     j++
>            }
> -        }	
> +        }
>        }'`
>  }
>  
> @@ -101,7 +101,7 @@ _getfacl_filter_id()
>  _acl_ls()
>  {
>      _ls_l -n $* | awk '{ print $1, $3, $4, $NF }' | _acl_filter_id
> -} 
> +}
>  
>  # create an ACL with n ACEs in it
>  #
> @@ -128,7 +128,7 @@ _filter_aces()
>  	BEGIN {
>  	    FS=":"
>  	    while ( getline <tmpfile > 0 ) {
> -		idlist[$1] = $3 
> +		idlist[$1] = $3
>  	    }
>  	}
>  	/^user/ { if ($2 in idlist) sub($2, idlist[$2]); print; next}
> @@ -180,17 +180,17 @@ _require_attrs()
>  {
>  	local args
>  	local nsp
> -	
> +
>  	if [ $# -eq 0 ]; then
>  		args="user"
>  	else
>  	  	args="$*"
>  	fi
> -	
> +
>  	[ -n "$ATTR_PROG" ] || _notrun "attr command not found"
>  	[ -n "$GETFATTR_PROG" ] || _notrun "getfattr command not found"
>  	[ -n "$SETFATTR_PROG" ] || _notrun "setfattr command not found"
> -	
> +
>  	for nsp in $args; do
>  		#
>  		# Test if chacl is able to write an attribute on the target
> @@ -204,14 +204,14 @@ _require_attrs()
>  		touch $TEST_DIR/syscalltest
>  		$SETFATTR_PROG -n "$nsp.xfstests" -v "attr" $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
>  		cat $TEST_DIR/syscalltest.out >> $seqres.full
> -		
> +
>  		if grep -q 'Function not implemented' $TEST_DIR/syscalltest.out; then
>  			_notrun "kernel does not support attrs"
>  		fi
>  		if grep -q 'Operation not supported' $TEST_DIR/syscalltest.out; then
>  			_notrun "attr namespace $nsp not supported by this filesystem type: $FSTYP"
>  		fi
> -		
> +
>  		rm -f $TEST_DIR/syscalltest.out
>  	done
>  }
> -- 
> 2.31.1
> 

Re: [PATCH 8/9] common/attr: Cleanup end of line whitespaces issues

[Theodore Ts'o]

On Mon, Jun 14, 2021 at 11:58:12AM +0530, Ritesh Harjani wrote:
> This patch clears the end of line whitespace issues in this file.
> Mostly since many kernel developers also keep this editor config to clear
> any end of line whitespaces on file save.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

[PATCH 9/9] common/attr: Reduce MAX_ATTRS to leave some overhead for 64K blocksize

[Ritesh Harjani]

Test generic/020 fails for ext4 with 64K blocksize. So increase some overhead
value to reduce the MAX_ATTRS so that it can accomodate for 64K blocksize.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 common/attr | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/attr b/common/attr
index d3902346..e8661d80 100644
--- a/common/attr
+++ b/common/attr
@@ -260,7 +260,7 @@ xfs|udf|pvfs2|9p|ceph|nfs)
 	# Assume max ~1 block of attrs
 	BLOCK_SIZE=`_get_block_size $TEST_DIR`
 	# user.attribute_XXX="value.XXX" is about 32 bytes; leave some overhead
-	let MAX_ATTRS=$BLOCK_SIZE/40
+	let MAX_ATTRS=$BLOCK_SIZE/48
 esac
 
 export MAX_ATTRS
-- 
2.31.1

Re: [PATCH 9/9] common/attr: Reduce MAX_ATTRS to leave some overhead for 64K blocksize

[Darrick J. Wong]

On Mon, Jun 14, 2021 at 11:58:13AM +0530, Ritesh Harjani wrote:
> Test generic/020 fails for ext4 with 64K blocksize. So increase some overhead
> value to reduce the MAX_ATTRS so that it can accomodate for 64K blocksize.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  common/attr | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/attr b/common/attr
> index d3902346..e8661d80 100644
> --- a/common/attr
> +++ b/common/attr
> @@ -260,7 +260,7 @@ xfs|udf|pvfs2|9p|ceph|nfs)
>  	# Assume max ~1 block of attrs
>  	BLOCK_SIZE=`_get_block_size $TEST_DIR`
>  	# user.attribute_XXX="value.XXX" is about 32 bytes; leave some overhead
> -	let MAX_ATTRS=$BLOCK_SIZE/40
> +	let MAX_ATTRS=$BLOCK_SIZE/48

50% is quite a lot of overhead; maybe we should special-case this?

--D

>  esac
>  
>  export MAX_ATTRS
> -- 
> 2.31.1
> 

Re: [PATCH 9/9] common/attr: Reduce MAX_ATTRS to leave some overhead for 64K blocksize

[Theodore Ts'o]

On Wed, Jun 30, 2021 at 08:51:50AM -0700, Darrick J. Wong wrote:
> On Mon, Jun 14, 2021 at 11:58:13AM +0530, Ritesh Harjani wrote:
> > Test generic/020 fails for ext4 with 64K blocksize. So increase some overhead
> > value to reduce the MAX_ATTRS so that it can accomodate for 64K blocksize.
> > 
> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > ---
> >  common/attr | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/common/attr b/common/attr
> > index d3902346..e8661d80 100644
> > --- a/common/attr
> > +++ b/common/attr
> > @@ -260,7 +260,7 @@ xfs|udf|pvfs2|9p|ceph|nfs)
> >  	# Assume max ~1 block of attrs
> >  	BLOCK_SIZE=`_get_block_size $TEST_DIR`
> >  	# user.attribute_XXX="value.XXX" is about 32 bytes; leave some overhead
> > -	let MAX_ATTRS=$BLOCK_SIZE/40
> > +	let MAX_ATTRS=$BLOCK_SIZE/48
> 
> 50% is quite a lot of overhead; maybe we should special-case this?

The problem is that 32 bytes is an underestimate when i > 99 for
user.attribute_$i=value_$i.  And with a 4k blocksize, MAX_ATTRS =
4096 / 40 = 102.

The exact calculation for ext4 is:

fixed_block_overhead = 32
fixed_entry_overhead = 16
max_attr = (block_size - fixed_block_overhead) /
	(fixed_entry_overhead + round_up(len(attr_name), 4) +
	 round_up(len(value), 4))

For 4k blocksizes, most of the attributes have an attr_name of
"attribute_NN" which is 8, and "value_NN" which is 12.

But for larger block sizes, we start having extended attributes of the
form "attribute_NNN" or "attribute_NNNN", and "value_NNN" and
"value_NNNN", which causes the round(len(..), 4) to jump up by 4
bytes.  So round_up(len(attr_name, 4)) becomes 12 instead of 8, and
round_up(len(value, 4)) becomes 16 instead of 12.  So:

	max_attrs = (block_size - 32) / (16 + 12 + 16)
or
	max_attrs = (block_size - 32) / 44

instead of:

	max_attrs = (block_size - 32) / (16 + 8 + 12)
or
	max_attrs = (block_size - 32) / 36

So special casing things for block sizes > 4k may very well make
sense.  Perhaps it's even worth it to put in an ext[234] specific,
exalc calculation for MAX_ATTRS in common/attr.

Cheers,

						- Ted

Re: [PATCH 9/9] common/attr: Reduce MAX_ATTRS to leave some overhead for 64K blocksize

[Ritesh Harjani]

On 21/06/30 03:20PM, Theodore Ts'o wrote:
> On Wed, Jun 30, 2021 at 08:51:50AM -0700, Darrick J. Wong wrote:
> > On Mon, Jun 14, 2021 at 11:58:13AM +0530, Ritesh Harjani wrote:
> > > Test generic/020 fails for ext4 with 64K blocksize. So increase some overhead
> > > value to reduce the MAX_ATTRS so that it can accomodate for 64K blocksize.
> > >
> > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > > ---
> > >  common/attr | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/common/attr b/common/attr
> > > index d3902346..e8661d80 100644
> > > --- a/common/attr
> > > +++ b/common/attr
> > > @@ -260,7 +260,7 @@ xfs|udf|pvfs2|9p|ceph|nfs)
> > >  	# Assume max ~1 block of attrs
> > >  	BLOCK_SIZE=`_get_block_size $TEST_DIR`
> > >  	# user.attribute_XXX="value.XXX" is about 32 bytes; leave some overhead
> > > -	let MAX_ATTRS=$BLOCK_SIZE/40
> > > +	let MAX_ATTRS=$BLOCK_SIZE/48
> >
> > 50% is quite a lot of overhead; maybe we should special-case this?
>
> The problem is that 32 bytes is an underestimate when i > 99 for
> user.attribute_$i=value_$i.  And with a 4k blocksize, MAX_ATTRS =
> 4096 / 40 = 102.
>
> The exact calculation for ext4 is:
>
> fixed_block_overhead = 32
> fixed_entry_overhead = 16
> max_attr = (block_size - fixed_block_overhead) /
> 	(fixed_entry_overhead + round_up(len(attr_name), 4) +
> 	 round_up(len(value), 4))
>
> For 4k blocksizes, most of the attributes have an attr_name of
> "attribute_NN" which is 8, and "value_NN" which is 12.
>
> But for larger block sizes, we start having extended attributes of the
> form "attribute_NNN" or "attribute_NNNN", and "value_NNN" and
> "value_NNNN", which causes the round(len(..), 4) to jump up by 4
> bytes.  So round_up(len(attr_name, 4)) becomes 12 instead of 8, and
> round_up(len(value, 4)) becomes 16 instead of 12.  So:
>
> 	max_attrs = (block_size - 32) / (16 + 12 + 16)
> or
> 	max_attrs = (block_size - 32) / 44
>
> instead of:
>
> 	max_attrs = (block_size - 32) / (16 + 8 + 12)
> or
> 	max_attrs = (block_size - 32) / 36

Thanks for the indepth details. Yes, in my testing as well it was coming to be
around ~44%. But to be safe I chose 50%.
I verified from the code as well. We do have 32 bytes of overhead per block for
the the header. And per entry overhead of 16 bytes. The rounding happens for
both name (EXT4_XATTR_LEN) and value (EXT4_XATTR_SIZE) of attr.

Perhaps, it will be helpful if we update above info in ext4 documentation as
well. In that the rounding off is only mentioned for value and not for name
length.

>
> So special casing things for block sizes > 4k may very well make
> sense.  Perhaps it's even worth it to put in an ext[234] specific,
> exalc calculation for MAX_ATTRS in common/attr.

yes, will make these changes for ext[234] specific in common/attr.

-ritesh

Re: [PATCH 0/9] 64K blocksize related fixes

[Ritesh Harjani]

On 21/06/14 11:58AM, Ritesh Harjani wrote:
> Below are the list of fixes mostly centered around 64K blocksize (on PPC64)
> and with ext4 filesystem. Tested this with both 64K & 4K blocksize on Power
> with (ext4/ext3/ext2/xfs/btrfs).

Gentle reminder on this patch series.

-ritesh

>
> Ritesh Harjani (9):
>   ext4/003: Fix this test on 64K platform for dax config
>   ext4/027: Correct the right code of block and inode bitmap
>   ext4/306: Add -b blocksize parameter too to avoid failure with DAX config
>   ext4/022: exclude this test for dax config on 64KB pagesize platform
>   generic/031: Fix the test case for 64k blocksize config
>   gitignore: Add 031.out file to .gitignore
>   generic/620: Remove -b blocksize option for ext4
>   common/attr: Cleanup end of line whitespaces issues
>   common/attr: Reduce MAX_ATTRS to leave some overhead for 64K blocksize
>
>  .gitignore                                 |  1 +
>  common/attr                                | 20 ++++++------
>  tests/ext4/003                             |  3 +-
>  tests/ext4/022                             |  7 ++--
>  tests/ext4/027                             |  4 +--
>  tests/ext4/306                             |  5 ++-
>  tests/generic/031                          | 37 ++++++++++++++++++----
>  tests/generic/031.out.64k                  | 19 +++++++++++
>  tests/generic/{031.out => 031.out.default} |  0
>  tests/generic/620                          |  7 ++++
>  10 files changed, 80 insertions(+), 23 deletions(-)
>  create mode 100644 tests/generic/031.out.64k
>  rename tests/generic/{031.out => 031.out.default} (100%)
>
> --
> 2.31.1
>