[PATCH v3 0/2] generic: test i_blocks for truncated largefiles

[PATCH v3 0/2] generic: test i_blocks for truncated largefiles

[Pavel Reichl]

Please see next revision of the patch set.
Please do not hesitate with proposing better name than
create_sizedfile() or better place for its definition.
Thank you.

*Time taken before falloc optimization to run on my system*

* fs: ext2
generic/694 8s ...  7s
generic/698 9s ...  10s
Passed all 2 tests
* fs: xfs
generic/694 7s ...  11s
generic/698 10s ...  7s
Passed all 2 tests
* fs: btrfs
generic/694 11s ...  6s
generic/698 7s ...  7s
Passed all 2 tests
* fs: exfat
generic/694 6s ...  8s
generic/698 7s ...  9s
Passed all 2 tests

*With falloc optimization in place*

* fs: ext2
generic/694 8s ...  7s
generic/698 9s ...  7s
Passed all 2 tests
* fs: xfs
generic/694 7s ...  0s
generic/698 7s ...  0s
Passed all 2 tests
* fs: btrfs
generic/694 0s ...  0s
generic/698 0s ...  1s
Passed all 2 tests
* fs: exfat
generic/694 0s ...  8s
generic/698 1s ...  9s
Passed all 2 tests
[preichl@f32 xfstests-dev]$ 


V3
* introduced create_sizedfile() helper
* amended g/694 and g/698 to utilize this helper

Pavel Reichl (2):
  common: new helper to alloacate fixed size files
  generic: test i_blocks for truncated large files

 common/rc             | 13 ++++++++++++
 tests/generic/694     |  2 +-
 tests/generic/698     | 47 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/698.out |  2 ++
 4 files changed, 63 insertions(+), 1 deletion(-)
 create mode 100755 tests/generic/698
 create mode 100644 tests/generic/698.out

-- 
2.37.3

[PATCH v3 1/2] common: new helper to alloacate fixed size files

[Pavel Reichl]

Helper that creates files of specified size using falloc if supported,
otherwise pwrite is used.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 common/rc         | 13 +++++++++++++
 tests/generic/694 |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index a25cbcd0..77866582 100644
--- a/common/rc
+++ b/common/rc
@@ -4925,6 +4925,19 @@ hexdump()
 	_fail "Use _hexdump(), please!"
 }
 
+# Helper to write a file containing specified number of bytes using
+# falloc if supported, otherwise use pwrite
+_create_sizedfile()
+{
+	length=$1
+	file=$2
+
+	$XFS_IO_PROG -F -fc "falloc 0 $length" $file 2>&1 | grep -q "Operation not supported"
+	if [ $? -eq 0 ]; then
+		$XFS_IO_PROG -F -fc "pwrite -W 0 $length" $file >/dev/null
+	fi
+}
+
 init_rc
 
 ################################################################################
diff --git a/tests/generic/694 b/tests/generic/694
index dfd988df..64c3dd9a 100755
--- a/tests/generic/694
+++ b/tests/generic/694
@@ -30,7 +30,7 @@ junk_dir=$TEST_DIR/$seq
 junk_file=$junk_dir/junk
 mkdir -p $junk_dir
 
-$XFS_IO_PROG -f -c "pwrite -W 0 4G" $junk_file > /dev/null
+_create_sizedfile 4G $junk_file
 
 iblocks=`stat -c '%b' $junk_file`
 
-- 
2.37.3

[PATCH v3 2/2] generic: test i_blocks for truncated large files

[Pavel Reichl]

This is a regression test for an incorrect computation of i_blocks for
truncated files larger than 4 GiB. Bug was filed for exFAT.

Test is based on reproducer provied by Christophe Vu-Brugier as part
of kernel patch-fix submission.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 tests/generic/698     | 47 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/698.out |  2 ++
 2 files changed, 49 insertions(+)
 create mode 100755 tests/generic/698
 create mode 100644 tests/generic/698.out

diff --git a/tests/generic/698 b/tests/generic/698
new file mode 100755
index 00000000..bb21957f
--- /dev/null
+++ b/tests/generic/698
@@ -0,0 +1,47 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022  Red Hat Inc. All Rights Reserved.
+#
+# FS QA Test 698
+#
+# Verify that i_blocks for truncated files larger than 4 GiB have correct
+# values.
+#
+# This test verifies the problem fixed in kernel with commit
+# 92fba084b79e exfat: fix i_blocks for files truncated over 4 GiB
+#
+. ./common/preamble
+. ./common/filter
+
+_begin_fstest auto
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.* $junk_dir
+}
+
+_supported_fs generic
+_fixed_by_kernel_commit 92fba084b79e \
+	"exfat: fix i_blocks for files truncated over 4 GiB"
+
+_require_test
+_require_fs_space $TEST_DIR $((5 * 1024 * 1024)) #kB
+
+junk_dir=$TEST_DIR/$seq
+junk_file=$junk_dir/junk
+mkdir -p $junk_dir
+
+_create_sizedfile 5G $junk_file
+truncate -s 4G $junk_file
+
+block_size=`stat -c '%B' $junk_file`
+iblocks_after_truncate=`stat -c '%b' $junk_file`
+iblocks_expected=$((4 * 1024 * 1024 * 1024 / $block_size))
+
+_within_tolerance "Number of allocated blocks after truncate" $iblocks_after_truncate $iblocks_expected 1% -v
+
+status=0
+
+exit
diff --git a/tests/generic/698.out b/tests/generic/698.out
new file mode 100644
index 00000000..cbb02d37
--- /dev/null
+++ b/tests/generic/698.out
@@ -0,0 +1,2 @@
+QA output created by 698
+Number of allocated blocks after truncate is in range
-- 
2.37.3

Re: [PATCH v3 1/2] common: new helper to alloacate fixed size files

[Zorro Lang]

On Tue, Sep 20, 2022 at 09:35:13AM +0200, Pavel Reichl wrote:
> Helper that creates files of specified size using falloc if supported,
> otherwise pwrite is used.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  common/rc         | 13 +++++++++++++
>  tests/generic/694 |  2 +-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index a25cbcd0..77866582 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4925,6 +4925,19 @@ hexdump()
>  	_fail "Use _hexdump(), please!"
>  }
>  
> +# Helper to write a file containing specified number of bytes using
> +# falloc if supported, otherwise use pwrite
> +_create_sizedfile()
> +{
> +	length=$1
> +	file=$2
> +
> +	$XFS_IO_PROG -F -fc "falloc 0 $length" $file 2>&1 | grep -q "Operation not supported"

Do we need to make sure if the $file really get $length space, due to if might
fail but not due to "Operation not supported", e.g. ENOSPC or others.

> +	if [ $? -eq 0 ]; then
> +		$XFS_IO_PROG -F -fc "pwrite -W 0 $length" $file >/dev/null
> +	fi

Another question I'm thinking is do we need "-t" to make sure the $file isn't
bigger than what we want. And do we need to truncate the file to $length, to
make sure fs doesn't allocate more space?

Thanks,
Zorro

> +}
> +
>  init_rc
>  
>  ################################################################################
> diff --git a/tests/generic/694 b/tests/generic/694
> index dfd988df..64c3dd9a 100755
> --- a/tests/generic/694
> +++ b/tests/generic/694
> @@ -30,7 +30,7 @@ junk_dir=$TEST_DIR/$seq
>  junk_file=$junk_dir/junk
>  mkdir -p $junk_dir
>  
> -$XFS_IO_PROG -f -c "pwrite -W 0 4G" $junk_file > /dev/null
> +_create_sizedfile 4G $junk_file
>  
>  iblocks=`stat -c '%b' $junk_file`
>  
> -- 
> 2.37.3
> 

Re: [PATCH v3 1/2] common: new helper to alloacate fixed size files

[Zorro Lang]

On Tue, Sep 20, 2022 at 09:35:13AM +0200, Pavel Reichl wrote:
> Helper that creates files of specified size using falloc if supported,
> otherwise pwrite is used.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  common/rc         | 13 +++++++++++++
>  tests/generic/694 |  2 +-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index a25cbcd0..77866582 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4925,6 +4925,19 @@ hexdump()
>  	_fail "Use _hexdump(), please!"
>  }
>  
> +# Helper to write a file containing specified number of bytes using
> +# falloc if supported, otherwise use pwrite
> +_create_sizedfile()
> +{
> +	length=$1
> +	file=$2
> +
> +	$XFS_IO_PROG -F -fc "falloc 0 $length" $file 2>&1 | grep -q "Operation not supported"
> +	if [ $? -eq 0 ]; then
> +		$XFS_IO_PROG -F -fc "pwrite -W 0 $length" $file >/dev/null
> +	fi
> +}

I think about it more, above code might ignore a failed falloc, if it's not failed
by "Operation not supported" but really fails, this function won't print or return
any valid things.

So how about we write it like below (for reference only):

# Try to create a file which inode->i_blocks = $length (maybe a little bigger
# than expect)
_create_file_sized()
{
	local length=$1
	local file=$2
	local tmp=`mktemp -u`
	local ret=0

	$XFS_IO_PROG -ft -c "falloc 0 $length" $file >$tmp.out 2>&1
	ret=$?
	if (grep -Eq "Operation not supported|command .* not found" $tmp.out);then
		# fallocate isn't supported, fallback to general buffer write
		$XFS_IO_PROG -ft -c "pwrite 0 $length" $file >$tmp.out 2>&1
		ret=$?
	fi
	[ $ret -ne 0 ] && cat $tmp.out
	rm -f $tmp.out
	return $ret
}

Even though, I think this function might still not good, feel free to tell me if
anyone has better idea/suggestion about how to get a file with specified
inode->i_blocks.

Thanks,
Zorro

> +
>  init_rc
>  
>  ################################################################################
> diff --git a/tests/generic/694 b/tests/generic/694
> index dfd988df..64c3dd9a 100755
> --- a/tests/generic/694
> +++ b/tests/generic/694
> @@ -30,7 +30,7 @@ junk_dir=$TEST_DIR/$seq
>  junk_file=$junk_dir/junk
>  mkdir -p $junk_dir
>  
> -$XFS_IO_PROG -f -c "pwrite -W 0 4G" $junk_file > /dev/null
> +_create_sizedfile 4G $junk_file
>  
>  iblocks=`stat -c '%b' $junk_file`
>  
> -- 
> 2.37.3
> 

Re: [PATCH v3 1/2] common: new helper to alloacate fixed size files

[Darrick J. Wong]

On Wed, Sep 21, 2022 at 10:48:57AM +0800, Zorro Lang wrote:
> On Tue, Sep 20, 2022 at 09:35:13AM +0200, Pavel Reichl wrote:
> > Helper that creates files of specified size using falloc if supported,
> > otherwise pwrite is used.
> > 
> > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > ---
> >  common/rc         | 13 +++++++++++++
> >  tests/generic/694 |  2 +-
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index a25cbcd0..77866582 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -4925,6 +4925,19 @@ hexdump()
> >  	_fail "Use _hexdump(), please!"
> >  }
> >  
> > +# Helper to write a file containing specified number of bytes using
> > +# falloc if supported, otherwise use pwrite
> > +_create_sizedfile()
> > +{
> > +	length=$1
> > +	file=$2
> > +
> > +	$XFS_IO_PROG -F -fc "falloc 0 $length" $file 2>&1 | grep -q "Operation not supported"
> > +	if [ $? -eq 0 ]; then
> > +		$XFS_IO_PROG -F -fc "pwrite -W 0 $length" $file >/dev/null
> > +	fi
> > +}
> 
> I think about it more, above code might ignore a failed falloc, if it's not failed
> by "Operation not supported" but really fails, this function won't print or return
> any valid things.
> 
> So how about we write it like below (for reference only):
> 
> # Try to create a file which inode->i_blocks = $length (maybe a little bigger
> # than expect)
> _create_file_sized()
> {
> 	local length=$1
> 	local file=$2
> 	local tmp=`mktemp -u`
> 	local ret=0
> 
> 	$XFS_IO_PROG -ft -c "falloc 0 $length" $file >$tmp.out 2>&1
> 	ret=$?
> 	if (grep -Eq "Operation not supported|command .* not found" $tmp.out);then
> 		# fallocate isn't supported, fallback to general buffer write
> 		$XFS_IO_PROG -ft -c "pwrite 0 $length" $file >$tmp.out 2>&1
> 		ret=$?
> 	fi
> 	[ $ret -ne 0 ] && cat $tmp.out
> 	rm -f $tmp.out
> 	return $ret
> }
> 
> Even though, I think this function might still not good, feel free to tell me if
> anyone has better idea/suggestion about how to get a file with specified
> inode->i_blocks.

Looks reasonable enough to /me...

--D

> Thanks,
> Zorro
> 
> > +
> >  init_rc
> >  
> >  ################################################################################
> > diff --git a/tests/generic/694 b/tests/generic/694
> > index dfd988df..64c3dd9a 100755
> > --- a/tests/generic/694
> > +++ b/tests/generic/694
> > @@ -30,7 +30,7 @@ junk_dir=$TEST_DIR/$seq
> >  junk_file=$junk_dir/junk
> >  mkdir -p $junk_dir
> >  
> > -$XFS_IO_PROG -f -c "pwrite -W 0 4G" $junk_file > /dev/null
> > +_create_sizedfile 4G $junk_file
> >  
> >  iblocks=`stat -c '%b' $junk_file`
> >  
> > -- 
> > 2.37.3
> > 
> 

Re: [PATCH v3 1/2] common: new helper to alloacate fixed size files

[Zorro Lang]

On Tue, Sep 20, 2022 at 07:53:52PM -0700, Darrick J. Wong wrote:
> On Wed, Sep 21, 2022 at 10:48:57AM +0800, Zorro Lang wrote:
> > On Tue, Sep 20, 2022 at 09:35:13AM +0200, Pavel Reichl wrote:
> > > Helper that creates files of specified size using falloc if supported,
> > > otherwise pwrite is used.
> > > 
> > > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > > ---
> > >  common/rc         | 13 +++++++++++++
> > >  tests/generic/694 |  2 +-
> > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index a25cbcd0..77866582 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -4925,6 +4925,19 @@ hexdump()
> > >  	_fail "Use _hexdump(), please!"
> > >  }
> > >  
> > > +# Helper to write a file containing specified number of bytes using
> > > +# falloc if supported, otherwise use pwrite
> > > +_create_sizedfile()
> > > +{
> > > +	length=$1
> > > +	file=$2
> > > +
> > > +	$XFS_IO_PROG -F -fc "falloc 0 $length" $file 2>&1 | grep -q "Operation not supported"
> > > +	if [ $? -eq 0 ]; then
> > > +		$XFS_IO_PROG -F -fc "pwrite -W 0 $length" $file >/dev/null
> > > +	fi
> > > +}
> > 
> > I think about it more, above code might ignore a failed falloc, if it's not failed
> > by "Operation not supported" but really fails, this function won't print or return
> > any valid things.
> > 
> > So how about we write it like below (for reference only):
> > 
> > # Try to create a file which inode->i_blocks = $length (maybe a little bigger
> > # than expect)
> > _create_file_sized()
> > {
> > 	local length=$1
> > 	local file=$2
> > 	local tmp=`mktemp -u`
> > 	local ret=0
> > 
> > 	$XFS_IO_PROG -ft -c "falloc 0 $length" $file >$tmp.out 2>&1
> > 	ret=$?
> > 	if (grep -Eq "Operation not supported|command .* not found" $tmp.out);then
> > 		# fallocate isn't supported, fallback to general buffer write
> > 		$XFS_IO_PROG -ft -c "pwrite 0 $length" $file >$tmp.out 2>&1
> > 		ret=$?
> > 	fi
> > 	[ $ret -ne 0 ] && cat $tmp.out
> > 	rm -f $tmp.out
> > 	return $ret
> > }
> > 
> > Even though, I think this function might still not good, feel free to tell me if
> > anyone has better idea/suggestion about how to get a file with specified
> > inode->i_blocks.
> 
> Looks reasonable enough to /me...

Thanks for your review, Darrick.

Hi Pavel, the [PATCH 2/2] looks good to me. I'll merge this patchset after you
making this change.

Thanks,
Zorro

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > +
> > >  init_rc
> > >  
> > >  ################################################################################
> > > diff --git a/tests/generic/694 b/tests/generic/694
> > > index dfd988df..64c3dd9a 100755
> > > --- a/tests/generic/694
> > > +++ b/tests/generic/694
> > > @@ -30,7 +30,7 @@ junk_dir=$TEST_DIR/$seq
> > >  junk_file=$junk_dir/junk
> > >  mkdir -p $junk_dir
> > >  
> > > -$XFS_IO_PROG -f -c "pwrite -W 0 4G" $junk_file > /dev/null
> > > +_create_sizedfile 4G $junk_file
> > >  
> > >  iblocks=`stat -c '%b' $junk_file`
> > >  
> > > -- 
> > > 2.37.3
> > > 
> > 
>