[PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Qu Wenruo]

Normally, a bull fallocate call on a fully written and synced file
should not add an extent.

But not all filesystem follows the correct behavior.

Btrfs has a bug to always truncate the last page if the fallocate start
offset is smaller than inode size.

So add this test case to detect such malfunction.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
v2:
  Add author info...
  Fix some comment typo
---
 tests/generic/110     | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/110.out |  3 ++
 tests/generic/group   |  1 +
 3 files changed, 82 insertions(+)
 create mode 100755 tests/generic/110
 create mode 100644 tests/generic/110.out

diff --git a/tests/generic/110 b/tests/generic/110
new file mode 100755
index 0000000..b2b140c
--- /dev/null
+++ b/tests/generic/110
@@ -0,0 +1,78 @@
+#! /bin/bash
+# FS QA Test 110
+#
+# Test if fallocate will create uneeded extra tailing extent
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/defrag
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+_supported_fs generic
+_supported_os IRIX Linux
+_require_scratch
+_need_to_be_root
+
+# Use 64K file size to match any sectorsize
+# And with a unaligned tailing range to ensure it will be at least 2 pages
+filesize=$(( 64 * 1024 + 1024 ))
+
+_scratch_mkfs > /dev/null 2>&1
+_scratch_mount
+$XFS_IO_PROG -f -c "pwrite 0 $filesize" $SCRATCH_MNT/foo | _filter_xfs_io
+sync
+orig_extent_nr=`_extent_count $SCRATCH_MNT/foo`
+
+# As all space are allocated and even written to disk, this falloc
+# should do nothing with extent modification.
+$XFS_IO_PROG -f -c "falloc 0 $filesize" $SCRATCH_MNT/foo
+sync
+new_extent_nr=`_extent_count $SCRATCH_MNT/foo`
+
+echo "orig: $orig_extent_nr, new: $new_extent_nr" >> $seqres.full
+
+if [ "x$orig_extent_nr" != "x$new_extent_nr" ]; then
+	echo "number of extents mis-match after bull fallocate"
+fi
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/110.out b/tests/generic/110.out
new file mode 100644
index 0000000..64049da
--- /dev/null
+++ b/tests/generic/110.out
@@ -0,0 +1,3 @@
+QA output created by 110
+wrote 66560/66560 bytes at offset 0
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
diff --git a/tests/generic/group b/tests/generic/group
index 4ae256f..428f3e3 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -112,6 +112,7 @@
 107 auto quick metadata
 108 auto quick rw
 109 auto metadata dir
+110 auto quick prealloc
 112 rw aio auto quick
 113 rw aio auto quick
 117 attr auto quick
-- 
1.8.3.1

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Eryu Guan]

On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
> Normally, a bull fallocate call on a fully written and synced file
> should not add an extent.
> 
> But not all filesystem follows the correct behavior.
> 
> Btrfs has a bug to always truncate the last page if the fallocate start
> offset is smaller than inode size.
> 
> So add this test case to detect such malfunction.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
> v2:
>   Add author info...
>   Fix some comment typo

(I was going to point these out then saw a v2 :)

> ---
>  tests/generic/110     | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/110.out |  3 ++
>  tests/generic/group   |  1 +
>  3 files changed, 82 insertions(+)
>  create mode 100755 tests/generic/110
>  create mode 100644 tests/generic/110.out
> 
> diff --git a/tests/generic/110 b/tests/generic/110
> new file mode 100755
> index 0000000..b2b140c
> --- /dev/null
> +++ b/tests/generic/110
> @@ -0,0 +1,78 @@
> +#! /bin/bash
> +# FS QA Test 110
> +#
> +# Test if fallocate will create uneeded extra tailing extent
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/defrag
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +_supported_fs generic
> +_supported_os IRIX Linux
> +_require_scratch
> +_need_to_be_root

need a '_require_xfs_io_command "falloc"' here, so test _notrun
correctly on ext2/3/NFS/CIFS that don't support fallocate.

> +
> +# Use 64K file size to match any sectorsize
> +# And with a unaligned tailing range to ensure it will be at least 2 pages
> +filesize=$(( 64 * 1024 + 1024 ))
> +
> +_scratch_mkfs > /dev/null 2>&1
> +_scratch_mount
> +$XFS_IO_PROG -f -c "pwrite 0 $filesize" $SCRATCH_MNT/foo | _filter_xfs_io
> +sync
> +orig_extent_nr=`_extent_count $SCRATCH_MNT/foo`
> +
> +# As all space are allocated and even written to disk, this falloc
> +# should do nothing with extent modification.
> +$XFS_IO_PROG -f -c "falloc 0 $filesize" $SCRATCH_MNT/foo
> +sync
> +new_extent_nr=`_extent_count $SCRATCH_MNT/foo`
> +
> +echo "orig: $orig_extent_nr, new: $new_extent_nr" >> $seqres.full
> +
> +if [ "x$orig_extent_nr" != "x$new_extent_nr" ]; then
> +	echo "number of extents mis-match after bull fallocate"

print out the $orig_extent_nr and $new_extent_nr in this failure case? I
think it's useful to see the difference just from the output diff, don't
have to check the full file.

Thanks,
Eryu

> +fi
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/110.out b/tests/generic/110.out
> new file mode 100644
> index 0000000..64049da
> --- /dev/null
> +++ b/tests/generic/110.out
> @@ -0,0 +1,3 @@
> +QA output created by 110
> +wrote 66560/66560 bytes at offset 0
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> diff --git a/tests/generic/group b/tests/generic/group
> index 4ae256f..428f3e3 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -112,6 +112,7 @@
>  107 auto quick metadata
>  108 auto quick rw
>  109 auto metadata dir
> +110 auto quick prealloc
>  112 rw aio auto quick
>  113 rw aio auto quick
>  117 attr auto quick
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Qu Wenruo]

在 2015年09月29日 17:55, Eryu Guan 写道:
> On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
>> Normally, a bull fallocate call on a fully written and synced file
>> should not add an extent.
>>
>> But not all filesystem follows the correct behavior.
>>
>> Btrfs has a bug to always truncate the last page if the fallocate start
>> offset is smaller than inode size.
>>
>> So add this test case to detect such malfunction.
>>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>> v2:
>>    Add author info...
>>    Fix some comment typo
>
> (I was going to point these out then saw a v2 :)
>
>> ---
>>   tests/generic/110     | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/generic/110.out |  3 ++
>>   tests/generic/group   |  1 +
>>   3 files changed, 82 insertions(+)
>>   create mode 100755 tests/generic/110
>>   create mode 100644 tests/generic/110.out
>>
>> diff --git a/tests/generic/110 b/tests/generic/110
>> new file mode 100755
>> index 0000000..b2b140c
>> --- /dev/null
>> +++ b/tests/generic/110
>> @@ -0,0 +1,78 @@
>> +#! /bin/bash
>> +# FS QA Test 110
>> +#
>> +# Test if fallocate will create uneeded extra tailing extent
>> +#
>> +#-----------------------------------------------------------------------
>> +# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> +#-----------------------------------------------------------------------
>> +#
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1	# failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +	cd /
>> +	rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +. ./common/defrag
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +# real QA test starts here
>> +
>> +_supported_fs generic
>> +_supported_os IRIX Linux
>> +_require_scratch
>> +_need_to_be_root
>
> need a '_require_xfs_io_command "falloc"' here, so test _notrun
> correctly on ext2/3/NFS/CIFS that don't support fallocate.

Will add it.

>
>> +
>> +# Use 64K file size to match any sectorsize
>> +# And with a unaligned tailing range to ensure it will be at least 2 pages
>> +filesize=$(( 64 * 1024 + 1024 ))
>> +
>> +_scratch_mkfs > /dev/null 2>&1
>> +_scratch_mount
>> +$XFS_IO_PROG -f -c "pwrite 0 $filesize" $SCRATCH_MNT/foo | _filter_xfs_io
>> +sync
>> +orig_extent_nr=`_extent_count $SCRATCH_MNT/foo`
>> +
>> +# As all space are allocated and even written to disk, this falloc
>> +# should do nothing with extent modification.
>> +$XFS_IO_PROG -f -c "falloc 0 $filesize" $SCRATCH_MNT/foo
>> +sync
>> +new_extent_nr=`_extent_count $SCRATCH_MNT/foo`
>> +
>> +echo "orig: $orig_extent_nr, new: $new_extent_nr" >> $seqres.full
>> +
>> +if [ "x$orig_extent_nr" != "x$new_extent_nr" ]; then
>> +	echo "number of extents mis-match after bull fallocate"
>
> print out the $orig_extent_nr and $new_extent_nr in this failure case? I
> think it's useful to see the difference just from the output diff, don't
> have to check the full file.

The problem is, we can't ensure orig/new_extent_nr always be a constant 
value(1 for btrfs case).

Maybe there is some file system which can only allocate extent up to 
64K, then the orig_extent_nr will be 2 and fails on that filesystem.

So I follow the method used in btrfs/010 to do it.

Thanks,
Qu
>
> Thanks,
> Eryu
>
>> +fi
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/generic/110.out b/tests/generic/110.out
>> new file mode 100644
>> index 0000000..64049da
>> --- /dev/null
>> +++ b/tests/generic/110.out
>> @@ -0,0 +1,3 @@
>> +QA output created by 110
>> +wrote 66560/66560 bytes at offset 0
>> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> diff --git a/tests/generic/group b/tests/generic/group
>> index 4ae256f..428f3e3 100644
>> --- a/tests/generic/group
>> +++ b/tests/generic/group
>> @@ -112,6 +112,7 @@
>>   107 auto quick metadata
>>   108 auto quick rw
>>   109 auto metadata dir
>> +110 auto quick prealloc
>>   112 rw aio auto quick
>>   113 rw aio auto quick
>>   117 attr auto quick
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Eryu Guan]

On Tue, Sep 29, 2015 at 06:16:11PM +0800, Qu Wenruo wrote:
>
<snip>
> >>+
> >>+if [ "x$orig_extent_nr" != "x$new_extent_nr" ]; then
> >>+	echo "number of extents mis-match after bull fallocate"
> >
> >print out the $orig_extent_nr and $new_extent_nr in this failure case? I
> >think it's useful to see the difference just from the output diff, don't
> >have to check the full file.
> 
> The problem is, we can't ensure orig/new_extent_nr always be a constant
> value(1 for btrfs case).

Sorry, I might be unclear, I mean print the extent number in the error
path, e.g.

echo "number of extents mis-match after null fallocate"
echo "old: $orig_extent_nr, new: $new_extent_nr"

not matching the number with golden output.

Thanks,
Eryu

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Qu Wenruo]

在 2015年09月29日 18:33, Eryu Guan 写道:
> On Tue, Sep 29, 2015 at 06:16:11PM +0800, Qu Wenruo wrote:
>>
> <snip>
>>>> +
>>>> +if [ "x$orig_extent_nr" != "x$new_extent_nr" ]; then
>>>> +	echo "number of extents mis-match after bull fallocate"
>>>
>>> print out the $orig_extent_nr and $new_extent_nr in this failure case? I
>>> think it's useful to see the difference just from the output diff, don't
>>> have to check the full file.
>>
>> The problem is, we can't ensure orig/new_extent_nr always be a constant
>> value(1 for btrfs case).
>
> Sorry, I might be unclear, I mean print the extent number in the error
> path, e.g.
>
> echo "number of extents mis-match after null fallocate"
> echo "old: $orig_extent_nr, new: $new_extent_nr"
>
> not matching the number with golden output.
>
> Thanks,
> Eryu

That sounds pretty nice.
Will change it in next version.

Thanks,
Qu
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Hugo Mills]

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

On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
> Normally, a bull fallocate call on a fully written and synced file
> should not add an extent.

   What's a "bull" fallocate call? Is it a typo, or simply something
I'm not familiar with?

   Hugo.

> But not all filesystem follows the correct behavior.
> 
> Btrfs has a bug to always truncate the last page if the fallocate start
> offset is smaller than inode size.
> 
> So add this test case to detect such malfunction.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
> v2:
>   Add author info...
>   Fix some comment typo
> ---
>  tests/generic/110     | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/110.out |  3 ++
>  tests/generic/group   |  1 +
>  3 files changed, 82 insertions(+)
>  create mode 100755 tests/generic/110
>  create mode 100644 tests/generic/110.out
> 
> diff --git a/tests/generic/110 b/tests/generic/110
> new file mode 100755
> index 0000000..b2b140c
> --- /dev/null
> +++ b/tests/generic/110
> @@ -0,0 +1,78 @@
> +#! /bin/bash
> +# FS QA Test 110
> +#
> +# Test if fallocate will create uneeded extra tailing extent
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/defrag
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +_supported_fs generic
> +_supported_os IRIX Linux
> +_require_scratch
> +_need_to_be_root
> +
> +# Use 64K file size to match any sectorsize
> +# And with a unaligned tailing range to ensure it will be at least 2 pages
> +filesize=$(( 64 * 1024 + 1024 ))
> +
> +_scratch_mkfs > /dev/null 2>&1
> +_scratch_mount
> +$XFS_IO_PROG -f -c "pwrite 0 $filesize" $SCRATCH_MNT/foo | _filter_xfs_io
> +sync
> +orig_extent_nr=`_extent_count $SCRATCH_MNT/foo`
> +
> +# As all space are allocated and even written to disk, this falloc
> +# should do nothing with extent modification.
> +$XFS_IO_PROG -f -c "falloc 0 $filesize" $SCRATCH_MNT/foo
> +sync
> +new_extent_nr=`_extent_count $SCRATCH_MNT/foo`
> +
> +echo "orig: $orig_extent_nr, new: $new_extent_nr" >> $seqres.full
> +
> +if [ "x$orig_extent_nr" != "x$new_extent_nr" ]; then
> +	echo "number of extents mis-match after bull fallocate"
> +fi
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/110.out b/tests/generic/110.out
> new file mode 100644
> index 0000000..64049da
> --- /dev/null
> +++ b/tests/generic/110.out
> @@ -0,0 +1,3 @@
> +QA output created by 110
> +wrote 66560/66560 bytes at offset 0
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> diff --git a/tests/generic/group b/tests/generic/group
> index 4ae256f..428f3e3 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -112,6 +112,7 @@
>  107 auto quick metadata
>  108 auto quick rw
>  109 auto metadata dir
> +110 auto quick prealloc
>  112 rw aio auto quick
>  113 rw aio auto quick
>  117 attr auto quick

-- 
Hugo Mills             | Mixing mathematics and alcohol is dangerous. Don't
hugo@... carfax.org.uk | drink and derive.
http://carfax.org.uk/  |
PGP: E2AB1DE4          |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Qu Wenruo]

在 2015年09月29日 18:00, Hugo Mills 写道:
> On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
>> Normally, a bull fallocate call on a fully written and synced file
>> should not add an extent.
>
>     What's a "bull" fallocate call? Is it a typo, or simply something
> I'm not familiar with?
>
>     Hugo.

Oh, it should be null.
But null still seems not appropriate here.

I mean a fallocate call which doesn't really allocate any space...

Any good ideas?

Thanks,
Qu
>
>> But not all filesystem follows the correct behavior.
>>
>> Btrfs has a bug to always truncate the last page if the fallocate start
>> offset is smaller than inode size.
>>
>> So add this test case to detect such malfunction.
>>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>> v2:
>>    Add author info...
>>    Fix some comment typo
>> ---
>>   tests/generic/110     | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/generic/110.out |  3 ++
>>   tests/generic/group   |  1 +
>>   3 files changed, 82 insertions(+)
>>   create mode 100755 tests/generic/110
>>   create mode 100644 tests/generic/110.out
>>
>> diff --git a/tests/generic/110 b/tests/generic/110
>> new file mode 100755
>> index 0000000..b2b140c
>> --- /dev/null
>> +++ b/tests/generic/110
>> @@ -0,0 +1,78 @@
>> +#! /bin/bash
>> +# FS QA Test 110
>> +#
>> +# Test if fallocate will create uneeded extra tailing extent
>> +#
>> +#-----------------------------------------------------------------------
>> +# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> +#-----------------------------------------------------------------------
>> +#
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1	# failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +	cd /
>> +	rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +. ./common/defrag
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +# real QA test starts here
>> +
>> +_supported_fs generic
>> +_supported_os IRIX Linux
>> +_require_scratch
>> +_need_to_be_root
>> +
>> +# Use 64K file size to match any sectorsize
>> +# And with a unaligned tailing range to ensure it will be at least 2 pages
>> +filesize=$(( 64 * 1024 + 1024 ))
>> +
>> +_scratch_mkfs > /dev/null 2>&1
>> +_scratch_mount
>> +$XFS_IO_PROG -f -c "pwrite 0 $filesize" $SCRATCH_MNT/foo | _filter_xfs_io
>> +sync
>> +orig_extent_nr=`_extent_count $SCRATCH_MNT/foo`
>> +
>> +# As all space are allocated and even written to disk, this falloc
>> +# should do nothing with extent modification.
>> +$XFS_IO_PROG -f -c "falloc 0 $filesize" $SCRATCH_MNT/foo
>> +sync
>> +new_extent_nr=`_extent_count $SCRATCH_MNT/foo`
>> +
>> +echo "orig: $orig_extent_nr, new: $new_extent_nr" >> $seqres.full
>> +
>> +if [ "x$orig_extent_nr" != "x$new_extent_nr" ]; then
>> +	echo "number of extents mis-match after bull fallocate"
>> +fi
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/generic/110.out b/tests/generic/110.out
>> new file mode 100644
>> index 0000000..64049da
>> --- /dev/null
>> +++ b/tests/generic/110.out
>> @@ -0,0 +1,3 @@
>> +QA output created by 110
>> +wrote 66560/66560 bytes at offset 0
>> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> diff --git a/tests/generic/group b/tests/generic/group
>> index 4ae256f..428f3e3 100644
>> --- a/tests/generic/group
>> +++ b/tests/generic/group
>> @@ -112,6 +112,7 @@
>>   107 auto quick metadata
>>   108 auto quick rw
>>   109 auto metadata dir
>> +110 auto quick prealloc
>>   112 rw aio auto quick
>>   113 rw aio auto quick
>>   117 attr auto quick
>

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Filipe Manana]

On Tue, Sep 29, 2015 at 11:13 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
> 在 2015年09月29日 18:00, Hugo Mills 写道:
>>
>> On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
>>>
>>> Normally, a bull fallocate call on a fully written and synced file
>>> should not add an extent.
>>
>>
>>     What's a "bull" fallocate call? Is it a typo, or simply something
>> I'm not familiar with?
>>
>>     Hugo.
>
>
> Oh, it should be null.
> But null still seems not appropriate here.
>
> I mean a fallocate call which doesn't really allocate any space...
>
> Any good ideas?

"Test if fallocate will create uneeded extra tailing extent"

change it to:

"Test that calling fallocate against a range which is already
allocated does not create new file extents".

No need to categorize such case for fallocate imho (but I'm not a
native English speaker either).

>
> Thanks,
> Qu
>
>>
>>> But not all filesystem follows the correct behavior.
>>>
>>> Btrfs has a bug to always truncate the last page if the fallocate start
>>> offset is smaller than inode size.
>>>
>>> So add this test case to detect such malfunction.
>>>
>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>> ---
>>> v2:
>>>    Add author info...
>>>    Fix some comment typo
>>> ---
>>>   tests/generic/110     | 78
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   tests/generic/110.out |  3 ++
>>>   tests/generic/group   |  1 +
>>>   3 files changed, 82 insertions(+)
>>>   create mode 100755 tests/generic/110
>>>   create mode 100644 tests/generic/110.out
>>>
>>> diff --git a/tests/generic/110 b/tests/generic/110
>>> new file mode 100755
>>> index 0000000..b2b140c
>>> --- /dev/null
>>> +++ b/tests/generic/110
>>> @@ -0,0 +1,78 @@
>>> +#! /bin/bash
>>> +# FS QA Test 110
>>> +#
>>> +# Test if fallocate will create uneeded extra tailing extent
>>> +#
>>> +#-----------------------------------------------------------------------
>>> +# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
>>> +#
>>> +# This program is free software; you can redistribute it and/or
>>> +# modify it under the terms of the GNU General Public License as
>>> +# published by the Free Software Foundation.
>>> +#
>>> +# This program is distributed in the hope that it would be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program; if not, write the Free Software Foundation,
>>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>>> +#-----------------------------------------------------------------------
>>> +#
>>> +
>>> +seq=`basename $0`
>>> +seqres=$RESULT_DIR/$seq
>>> +echo "QA output created by $seq"
>>> +
>>> +here=`pwd`
>>> +tmp=/tmp/$$
>>> +status=1       # failure is the default!
>>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>>> +
>>> +_cleanup()
>>> +{
>>> +       cd /
>>> +       rm -f $tmp.*
>>> +}
>>> +
>>> +# get standard environment, filters and checks
>>> +. ./common/rc
>>> +. ./common/filter
>>> +. ./common/defrag
>>> +
>>> +# remove previous $seqres.full before test
>>> +rm -f $seqres.full
>>> +
>>> +# real QA test starts here
>>> +
>>> +_supported_fs generic
>>> +_supported_os IRIX Linux
>>> +_require_scratch
>>> +_need_to_be_root
>>> +
>>> +# Use 64K file size to match any sectorsize
>>> +# And with a unaligned tailing range to ensure it will be at least 2
>>> pages
>>> +filesize=$(( 64 * 1024 + 1024 ))
>>> +
>>> +_scratch_mkfs > /dev/null 2>&1
>>> +_scratch_mount
>>> +$XFS_IO_PROG -f -c "pwrite 0 $filesize" $SCRATCH_MNT/foo |
>>> _filter_xfs_io
>>> +sync
>>> +orig_extent_nr=`_extent_count $SCRATCH_MNT/foo`
>>> +
>>> +# As all space are allocated and even written to disk, this falloc
>>> +# should do nothing with extent modification.
>>> +$XFS_IO_PROG -f -c "falloc 0 $filesize" $SCRATCH_MNT/foo
>>> +sync
>>> +new_extent_nr=`_extent_count $SCRATCH_MNT/foo`
>>> +
>>> +echo "orig: $orig_extent_nr, new: $new_extent_nr" >> $seqres.full
>>> +
>>> +if [ "x$orig_extent_nr" != "x$new_extent_nr" ]; then
>>> +       echo "number of extents mis-match after bull fallocate"
>>> +fi
>>> +
>>> +# success, all done
>>> +status=0
>>> +exit
>>> diff --git a/tests/generic/110.out b/tests/generic/110.out
>>> new file mode 100644
>>> index 0000000..64049da
>>> --- /dev/null
>>> +++ b/tests/generic/110.out
>>> @@ -0,0 +1,3 @@
>>> +QA output created by 110
>>> +wrote 66560/66560 bytes at offset 0
>>> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>> diff --git a/tests/generic/group b/tests/generic/group
>>> index 4ae256f..428f3e3 100644
>>> --- a/tests/generic/group
>>> +++ b/tests/generic/group
>>> @@ -112,6 +112,7 @@
>>>   107 auto quick metadata
>>>   108 auto quick rw
>>>   109 auto metadata dir
>>> +110 auto quick prealloc
>>>   112 rw aio auto quick
>>>   113 rw aio auto quick
>>>   117 attr auto quick
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Qu Wenruo]

在 2015年09月29日 18:24, Filipe Manana 写道:
> On Tue, Sep 29, 2015 at 11:13 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>> 在 2015年09月29日 18:00, Hugo Mills 写道:
>>>
>>> On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
>>>>
>>>> Normally, a bull fallocate call on a fully written and synced file
>>>> should not add an extent.
>>>
>>>
>>>      What's a "bull" fallocate call? Is it a typo, or simply something
>>> I'm not familiar with?
>>>
>>>      Hugo.
>>
>>
>> Oh, it should be null.
>> But null still seems not appropriate here.
>>
>> I mean a fallocate call which doesn't really allocate any space...
>>
>> Any good ideas?
>
> "Test if fallocate will create uneeded extra tailing extent"
>
> change it to:
>
> "Test that calling fallocate against a range which is already
> allocated does not create new file extents".
>
> No need to categorize such case for fallocate imho (but I'm not a
> native English speaker either).

Thank you, Filipe and Huge,

Both gives quite good expression, I'll pick one of them.

Thanks,
Qu
>
>>
>> Thanks,
>> Qu
>>
>>>
>>>> But not all filesystem follows the correct behavior.
>>>>
>>>> Btrfs has a bug to always truncate the last page if the fallocate start
>>>> offset is smaller than inode size.
>>>>
>>>> So add this test case to detect such malfunction.
>>>>
>>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>>> ---
>>>> v2:
>>>>     Add author info...
>>>>     Fix some comment typo
>>>> ---
>>>>    tests/generic/110     | 78
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    tests/generic/110.out |  3 ++
>>>>    tests/generic/group   |  1 +
>>>>    3 files changed, 82 insertions(+)
>>>>    create mode 100755 tests/generic/110
>>>>    create mode 100644 tests/generic/110.out
>>>>
>>>> diff --git a/tests/generic/110 b/tests/generic/110
>>>> new file mode 100755
>>>> index 0000000..b2b140c
>>>> --- /dev/null
>>>> +++ b/tests/generic/110
>>>> @@ -0,0 +1,78 @@
>>>> +#! /bin/bash
>>>> +# FS QA Test 110
>>>> +#
>>>> +# Test if fallocate will create uneeded extra tailing extent
>>>> +#
>>>> +#-----------------------------------------------------------------------
>>>> +# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
>>>> +#
>>>> +# This program is free software; you can redistribute it and/or
>>>> +# modify it under the terms of the GNU General Public License as
>>>> +# published by the Free Software Foundation.
>>>> +#
>>>> +# This program is distributed in the hope that it would be useful,
>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> +# GNU General Public License for more details.
>>>> +#
>>>> +# You should have received a copy of the GNU General Public License
>>>> +# along with this program; if not, write the Free Software Foundation,
>>>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>>>> +#-----------------------------------------------------------------------
>>>> +#
>>>> +
>>>> +seq=`basename $0`
>>>> +seqres=$RESULT_DIR/$seq
>>>> +echo "QA output created by $seq"
>>>> +
>>>> +here=`pwd`
>>>> +tmp=/tmp/$$
>>>> +status=1       # failure is the default!
>>>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>>>> +
>>>> +_cleanup()
>>>> +{
>>>> +       cd /
>>>> +       rm -f $tmp.*
>>>> +}
>>>> +
>>>> +# get standard environment, filters and checks
>>>> +. ./common/rc
>>>> +. ./common/filter
>>>> +. ./common/defrag
>>>> +
>>>> +# remove previous $seqres.full before test
>>>> +rm -f $seqres.full
>>>> +
>>>> +# real QA test starts here
>>>> +
>>>> +_supported_fs generic
>>>> +_supported_os IRIX Linux
>>>> +_require_scratch
>>>> +_need_to_be_root
>>>> +
>>>> +# Use 64K file size to match any sectorsize
>>>> +# And with a unaligned tailing range to ensure it will be at least 2
>>>> pages
>>>> +filesize=$(( 64 * 1024 + 1024 ))
>>>> +
>>>> +_scratch_mkfs > /dev/null 2>&1
>>>> +_scratch_mount
>>>> +$XFS_IO_PROG -f -c "pwrite 0 $filesize" $SCRATCH_MNT/foo |
>>>> _filter_xfs_io
>>>> +sync
>>>> +orig_extent_nr=`_extent_count $SCRATCH_MNT/foo`
>>>> +
>>>> +# As all space are allocated and even written to disk, this falloc
>>>> +# should do nothing with extent modification.
>>>> +$XFS_IO_PROG -f -c "falloc 0 $filesize" $SCRATCH_MNT/foo
>>>> +sync
>>>> +new_extent_nr=`_extent_count $SCRATCH_MNT/foo`
>>>> +
>>>> +echo "orig: $orig_extent_nr, new: $new_extent_nr" >> $seqres.full
>>>> +
>>>> +if [ "x$orig_extent_nr" != "x$new_extent_nr" ]; then
>>>> +       echo "number of extents mis-match after bull fallocate"
>>>> +fi
>>>> +
>>>> +# success, all done
>>>> +status=0
>>>> +exit
>>>> diff --git a/tests/generic/110.out b/tests/generic/110.out
>>>> new file mode 100644
>>>> index 0000000..64049da
>>>> --- /dev/null
>>>> +++ b/tests/generic/110.out
>>>> @@ -0,0 +1,3 @@
>>>> +QA output created by 110
>>>> +wrote 66560/66560 bytes at offset 0
>>>> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>> diff --git a/tests/generic/group b/tests/generic/group
>>>> index 4ae256f..428f3e3 100644
>>>> --- a/tests/generic/group
>>>> +++ b/tests/generic/group
>>>> @@ -112,6 +112,7 @@
>>>>    107 auto quick metadata
>>>>    108 auto quick rw
>>>>    109 auto metadata dir
>>>> +110 auto quick prealloc
>>>>    112 rw aio auto quick
>>>>    113 rw aio auto quick
>>>>    117 attr auto quick
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Duncan]

Qu Wenruo posted on Tue, 29 Sep 2015 18:48:37 +0800 as excerpted:

> Both gives quite good expression, I'll pick one of them.

... And for the one-line title, /bull/bad/ should do it. =:^)

People wanting details about bad /how/ can look at the fuller description 
or source.

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Hugo Mills]

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

On Tue, Sep 29, 2015 at 06:13:37PM +0800, Qu Wenruo wrote:
> 
> 
> 在 2015年09月29日 18:00, Hugo Mills 写道:
> >On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
> >>Normally, a bull fallocate call on a fully written and synced file
> >>should not add an extent.
> >
> >    What's a "bull" fallocate call? Is it a typo, or simply something
> >I'm not familiar with?
> >
> >    Hugo.
> 
> Oh, it should be null.
> But null still seems not appropriate here.
> 
> I mean a fallocate call which doesn't really allocate any space...
> 
> Any good ideas?

   Not in a single word. Maybe one of the following:

Normally, an fallocate call which only touches existing extents should
not add any more extents on a fully written and synced file.

Normally, an fallocate call which does not allocate outside existing
extents should not add any more extents on a fully written and synced
file.

   I think I slightly prefer the second option.

   Hugo

> Thanks,
> Qu
> >
> >>But not all filesystem follows the correct behavior.
> >>
> >>Btrfs has a bug to always truncate the last page if the fallocate start
> >>offset is smaller than inode size.
> >>
> >>So add this test case to detect such malfunction.
> >>
> >>Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> >>---
> >>v2:
> >>   Add author info...
> >>   Fix some comment typo
> >>---
> >>  tests/generic/110     | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/generic/110.out |  3 ++
> >>  tests/generic/group   |  1 +
> >>  3 files changed, 82 insertions(+)
> >>  create mode 100755 tests/generic/110
> >>  create mode 100644 tests/generic/110.out
> >>
> >>diff --git a/tests/generic/110 b/tests/generic/110
> >>new file mode 100755
> >>index 0000000..b2b140c
> >>--- /dev/null
> >>+++ b/tests/generic/110
> >>@@ -0,0 +1,78 @@
> >>+#! /bin/bash
> >>+# FS QA Test 110
> >>+#
> >>+# Test if fallocate will create uneeded extra tailing extent
> >>+#
> >>+#-----------------------------------------------------------------------
> >>+# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
> >>+#
> >>+# This program is free software; you can redistribute it and/or
> >>+# modify it under the terms of the GNU General Public License as
> >>+# published by the Free Software Foundation.
> >>+#
> >>+# This program is distributed in the hope that it would be useful,
> >>+# but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>+# GNU General Public License for more details.
> >>+#
> >>+# You should have received a copy of the GNU General Public License
> >>+# along with this program; if not, write the Free Software Foundation,
> >>+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> >>+#-----------------------------------------------------------------------
> >>+#
> >>+
> >>+seq=`basename $0`
> >>+seqres=$RESULT_DIR/$seq
> >>+echo "QA output created by $seq"
> >>+
> >>+here=`pwd`
> >>+tmp=/tmp/$$
> >>+status=1	# failure is the default!
> >>+trap "_cleanup; exit \$status" 0 1 2 3 15
> >>+
> >>+_cleanup()
> >>+{
> >>+	cd /
> >>+	rm -f $tmp.*
> >>+}
> >>+
> >>+# get standard environment, filters and checks
> >>+. ./common/rc
> >>+. ./common/filter
> >>+. ./common/defrag
> >>+
> >>+# remove previous $seqres.full before test
> >>+rm -f $seqres.full
> >>+
> >>+# real QA test starts here
> >>+
> >>+_supported_fs generic
> >>+_supported_os IRIX Linux
> >>+_require_scratch
> >>+_need_to_be_root
> >>+
> >>+# Use 64K file size to match any sectorsize
> >>+# And with a unaligned tailing range to ensure it will be at least 2 pages
> >>+filesize=$(( 64 * 1024 + 1024 ))
> >>+
> >>+_scratch_mkfs > /dev/null 2>&1
> >>+_scratch_mount
> >>+$XFS_IO_PROG -f -c "pwrite 0 $filesize" $SCRATCH_MNT/foo | _filter_xfs_io
> >>+sync
> >>+orig_extent_nr=`_extent_count $SCRATCH_MNT/foo`
> >>+
> >>+# As all space are allocated and even written to disk, this falloc
> >>+# should do nothing with extent modification.
> >>+$XFS_IO_PROG -f -c "falloc 0 $filesize" $SCRATCH_MNT/foo
> >>+sync
> >>+new_extent_nr=`_extent_count $SCRATCH_MNT/foo`
> >>+
> >>+echo "orig: $orig_extent_nr, new: $new_extent_nr" >> $seqres.full
> >>+
> >>+if [ "x$orig_extent_nr" != "x$new_extent_nr" ]; then
> >>+	echo "number of extents mis-match after bull fallocate"
> >>+fi
> >>+
> >>+# success, all done
> >>+status=0
> >>+exit
> >>diff --git a/tests/generic/110.out b/tests/generic/110.out
> >>new file mode 100644
> >>index 0000000..64049da
> >>--- /dev/null
> >>+++ b/tests/generic/110.out
> >>@@ -0,0 +1,3 @@
> >>+QA output created by 110
> >>+wrote 66560/66560 bytes at offset 0
> >>+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >>diff --git a/tests/generic/group b/tests/generic/group
> >>index 4ae256f..428f3e3 100644
> >>--- a/tests/generic/group
> >>+++ b/tests/generic/group
> >>@@ -112,6 +112,7 @@
> >>  107 auto quick metadata
> >>  108 auto quick rw
> >>  109 auto metadata dir
> >>+110 auto quick prealloc
> >>  112 rw aio auto quick
> >>  113 rw aio auto quick
> >>  117 attr auto quick
> >

-- 
Hugo Mills             | For months now, we have been making triumphant
hugo@... carfax.org.uk | retreats before a demoralised enemy who is advancing
http://carfax.org.uk/  | in utter disorder.
PGP: E2AB1DE4          |                              Eric Frank Russell, Wasp

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Dave Chinner]

On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
> Normally, a bull fallocate call on a fully written and synced file
> should not add an extent.

Why not? Filesystems can do whatever they want with extents during
a fallocate call. e.g. if the blocks are shared, then fallocate
might break the block sharing so future overwrites don't get
ENOSPC. This is a requirement set down by posix_fallocate(3)

"After a successful call to posix_fallocate(), subsequent writes to
bytes in the specified range are guaranteed not  to fail because of
lack of disk space."

Hence if you've got a file with shared blocks, a "full fallocate"
must change the extent layout to break the sharing. As such, the
premise of this test is wrong.

That's not to say that btrfs has a bug:

> Btrfs has a bug to always truncate the last page if the fallocate start
> offset is smaller than inode size.

But it' not clear that this behaviour is actually a bug if it's not
changing the file data.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Qu Wenruo]

Dave Chinner wrote on 2015/09/30 07:51 +1000:
> On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
>> Normally, a bull fallocate call on a fully written and synced file
>> should not add an extent.
>
> Why not? Filesystems can do whatever they want with extents during
> a fallocate call. e.g. if the blocks are shared, then fallocate
> might break the block sharing so future overwrites don't get
> ENOSPC. This is a requirement set down by posix_fallocate(3)
>
> "After a successful call to posix_fallocate(), subsequent writes to
> bytes in the specified range are guaranteed not  to fail because of
> lack of disk space."
>
> Hence if you've got a file with shared blocks, a "full fallocate"
> must change the extent layout to break the sharing. As such, the
> premise of this test is wrong.

First, btrfs never meets the posix_fallocate requirement by its COW nature.

Btrfs fallocate can only ensure at most next write will not cause ENOSPC.
Only when btrfs fallocate a new prealloc extent, next write into it may 
use the space if they are not shared between different snapshots.

Under most case, btrfs fallocate follows the behavior of other non-COW 
filesystems.
Which means, btrfs won't alloc new extent if there is existing extent, 
not matter if it's shared or not.

As a result, fallocate in btrfs only works in a limited use-case, and 
can easily break posix requirement.
Like the following case without snapshots:
1)Fallocate 0~50M
2)Write 0~50M		<- Will not return ENOSPC
3)Write 0~25M		<- COW happens, allocate another 25M,
			   may cause ENOSPC.
Or even easier with snapshot:
1)Fallocate 0~50M in subvol A
2)Snapshot subvol A into snap B
3)Write 0~25M in subvol A *OR* snap B <- COW happens, may cause ENOSPC

As in step 3), fallocated 50M is shared, so write will be forced COW.

So I'd prefer to make btrfs follows the behavior of other non-COW 
filesystems, as posix standard doesn't fit well here.
>
> That's not to say that btrfs has a bug:
>
>> Btrfs has a bug to always truncate the last page if the fallocate start
>> offset is smaller than inode size.
>
> But it' not clear that this behaviour is actually a bug if it's not
> changing the file data.
File data is not changed, as btrfs just COW the last tailing page, as 
reset the last already 0 part.

Like the follow ascii arts:

0)Before
0	4K	8K	12K	16K
|///////////////////////////|000|
|<----------Extent A----------->|
The file is 14K size, on disk(to be accurate, btrfs logical address 
space) it takes 16K, with last 2K padding 0.

And all that 16K is in extent A.

1)Fallocate 0~14K
In fact, all space in range 0~14K is allocated, so there is no need to 
reallocate any space.

2)But in btrfs
Result will be:
0	4K	8K	12K	16K
|///////////////////////////|000|
|<------Extent A------->|<--B-->|

Btrfs has a wrong judgment, which will always re-padding the last page.
Causing a new extent, extent B to be created.
Even the contents is the same with original last page.

It's OK not to consider it as a bug, at least data is not corrupted.
But IMO the btrfs behavior is not needed and need optimization.
So kernel patch is submitted to btrfs ml:
https://patchwork.kernel.org/patch/7284431/

And if fstests is not the proper place, any idea where such "test case" 
should belong?

Thanks,
Qu

>
> Cheers,
>
> Dave.
>

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Tsutomu Itoh]

On 2015/09/30 10:05, Qu Wenruo wrote:
> Dave Chinner wrote on 2015/09/30 07:51 +1000:
>> On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
>>> Normally, a bull fallocate call on a fully written and synced file
>>> should not add an extent.
>>
>> Why not? Filesystems can do whatever they want with extents during
>> a fallocate call. e.g. if the blocks are shared, then fallocate
>> might break the block sharing so future overwrites don't get
>> ENOSPC. This is a requirement set down by posix_fallocate(3)
>>
>> "After a successful call to posix_fallocate(), subsequent writes to
>> bytes in the specified range are guaranteed not  to fail because of
>> lack of disk space."
>>
>> Hence if you've got a file with shared blocks, a "full fallocate"
>> must change the extent layout to break the sharing. As such, the
>> premise of this test is wrong.
>
> First, btrfs never meets the posix_fallocate requirement by its COW nature.
>
> Btrfs fallocate can only ensure at most next write will not cause ENOSPC.
> Only when btrfs fallocate a new prealloc extent, next write into it may use the space if they are not shared between different snapshots.
>
> Under most case, btrfs fallocate follows the behavior of other non-COW filesystems.
> Which means, btrfs won't alloc new extent if there is existing extent, not matter if it's shared or not.
>
> As a result, fallocate in btrfs only works in a limited use-case, and can easily break posix requirement.
> Like the following case without snapshots:
> 1)Fallocate 0~50M
> 2)Write 0~50M        <- Will not return ENOSPC
> 3)Write 0~25M        <- COW happens, allocate another 25M,
>                 may cause ENOSPC.
> Or even easier with snapshot:
> 1)Fallocate 0~50M in subvol A
> 2)Snapshot subvol A into snap B
> 3)Write 0~25M in subvol A *OR* snap B <- COW happens, may cause ENOSPC
>
> As in step 3), fallocated 50M is shared, so write will be forced COW.
>
> So I'd prefer to make btrfs follows the behavior of other non-COW filesystems, as posix standard doesn't fit well here.
>>
>> That's not to say that btrfs has a bug:
>>
>>> Btrfs has a bug to always truncate the last page if the fallocate start
>>> offset is smaller than inode size.
>>
>> But it' not clear that this behaviour is actually a bug if it's not
>> changing the file data.
> File data is not changed, as btrfs just COW the last tailing page, as reset the last already 0 part.
>
> Like the follow ascii arts:
>
> 0)Before
> 0    4K    8K    12K    16K
> |///////////////////////////|000|
> |<----------Extent A----------->|
> The file is 14K size, on disk(to be accurate, btrfs logical address space) it takes 16K, with last 2K padding 0.
>
> And all that 16K is in extent A.
>
> 1)Fallocate 0~14K
> In fact, all space in range 0~14K is allocated, so there is no need to reallocate any space.
>
> 2)But in btrfs
> Result will be:
> 0    4K    8K    12K    16K
> |///////////////////////////|000|
> |<------Extent A------->|<--B-->|
>
> Btrfs has a wrong judgment, which will always re-padding the last page.
> Causing a new extent, extent B to be created.
> Even the contents is the same with original last page.
>
> It's OK not to consider it as a bug, at least data is not corrupted.
> But IMO the btrfs behavior is not needed and need optimization.

> So kernel patch is submitted to btrfs ml:
> https://patchwork.kernel.org/patch/7284431/

Is this https://patchwork.kernel.org/patch/7283461/ ?  Right?

Thanks,
Tsutomu

>
> And if fstests is not the proper place, any idea where such "test case" should belong?
>
> Thanks,
> Qu
>
>>
>> Cheers,
>>
>> Dave.
>>
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Qu Wenruo]

Tsutomu Itoh wrote on 2015/09/30 10:45 +0900:
> On 2015/09/30 10:05, Qu Wenruo wrote:
>> Dave Chinner wrote on 2015/09/30 07:51 +1000:
>>> On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
>>>> Normally, a bull fallocate call on a fully written and synced file
>>>> should not add an extent.
>>>
>>> Why not? Filesystems can do whatever they want with extents during
>>> a fallocate call. e.g. if the blocks are shared, then fallocate
>>> might break the block sharing so future overwrites don't get
>>> ENOSPC. This is a requirement set down by posix_fallocate(3)
>>>
>>> "After a successful call to posix_fallocate(), subsequent writes to
>>> bytes in the specified range are guaranteed not  to fail because of
>>> lack of disk space."
>>>
>>> Hence if you've got a file with shared blocks, a "full fallocate"
>>> must change the extent layout to break the sharing. As such, the
>>> premise of this test is wrong.
>>
>> First, btrfs never meets the posix_fallocate requirement by its COW
>> nature.
>>
>> Btrfs fallocate can only ensure at most next write will not cause ENOSPC.
>> Only when btrfs fallocate a new prealloc extent, next write into it
>> may use the space if they are not shared between different snapshots.
>>
>> Under most case, btrfs fallocate follows the behavior of other non-COW
>> filesystems.
>> Which means, btrfs won't alloc new extent if there is existing extent,
>> not matter if it's shared or not.
>>
>> As a result, fallocate in btrfs only works in a limited use-case, and
>> can easily break posix requirement.
>> Like the following case without snapshots:
>> 1)Fallocate 0~50M
>> 2)Write 0~50M        <- Will not return ENOSPC
>> 3)Write 0~25M        <- COW happens, allocate another 25M,
>>                 may cause ENOSPC.
>> Or even easier with snapshot:
>> 1)Fallocate 0~50M in subvol A
>> 2)Snapshot subvol A into snap B
>> 3)Write 0~25M in subvol A *OR* snap B <- COW happens, may cause ENOSPC
>>
>> As in step 3), fallocated 50M is shared, so write will be forced COW.
>>
>> So I'd prefer to make btrfs follows the behavior of other non-COW
>> filesystems, as posix standard doesn't fit well here.
>>>
>>> That's not to say that btrfs has a bug:
>>>
>>>> Btrfs has a bug to always truncate the last page if the fallocate start
>>>> offset is smaller than inode size.
>>>
>>> But it' not clear that this behaviour is actually a bug if it's not
>>> changing the file data.
>> File data is not changed, as btrfs just COW the last tailing page, as
>> reset the last already 0 part.
>>
>> Like the follow ascii arts:
>>
>> 0)Before
>> 0    4K    8K    12K    16K
>> |///////////////////////////|000|
>> |<----------Extent A----------->|
>> The file is 14K size, on disk(to be accurate, btrfs logical address
>> space) it takes 16K, with last 2K padding 0.
>>
>> And all that 16K is in extent A.
>>
>> 1)Fallocate 0~14K
>> In fact, all space in range 0~14K is allocated, so there is no need to
>> reallocate any space.
>>
>> 2)But in btrfs
>> Result will be:
>> 0    4K    8K    12K    16K
>> |///////////////////////////|000|
>> |<------Extent A------->|<--B-->|
>>
>> Btrfs has a wrong judgment, which will always re-padding the last page.
>> Causing a new extent, extent B to be created.
>> Even the contents is the same with original last page.
>>
>> It's OK not to consider it as a bug, at least data is not corrupted.
>> But IMO the btrfs behavior is not needed and need optimization.
>
>> So kernel patch is submitted to btrfs ml:
>> https://patchwork.kernel.org/patch/7284431/
>
> Is this https://patchwork.kernel.org/patch/7283461/ ?  Right?
>
> Thanks,
> Tsutomu

Oh, my fault, 728461 is the right patch...

Thanks for pointing this out,
Qu
>
>>
>> And if fstests is not the proper place, any idea where such "test
>> case" should belong?
>>
>> Thanks,
>> Qu
>>
>>>
>>> Cheers,
>>>
>>> Dave.
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Dave Chinner]

On Wed, Sep 30, 2015 at 09:05:15AM +0800, Qu Wenruo wrote:
> 
> 
> Dave Chinner wrote on 2015/09/30 07:51 +1000:
> >On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
> >>Normally, a bull fallocate call on a fully written and synced file
> >>should not add an extent.
> >
> >Why not? Filesystems can do whatever they want with extents during
> >a fallocate call. e.g. if the blocks are shared, then fallocate
> >might break the block sharing so future overwrites don't get
> >ENOSPC. This is a requirement set down by posix_fallocate(3)
> >
> >"After a successful call to posix_fallocate(), subsequent writes to
> >bytes in the specified range are guaranteed not  to fail because of
> >lack of disk space."
> >
> >Hence if you've got a file with shared blocks, a "full fallocate"
> >must change the extent layout to break the sharing. As such, the
> >premise of this test is wrong.
> 
> First, btrfs never meets the posix_fallocate requirement by its COW nature.
> 
> Btrfs fallocate can only ensure at most next write will not cause ENOSPC.

Which, it can be successfully argued, meets the basic requirement of
posix_fallocate().  i.e. "subsequent writes" is "one or more" future
writes.

But in trying to explain how COW works you've completely missed the
point I was making about a fundamental principle that COW is based
on - overwrite requires allocation. Hence fallocate must be allowed
modify the underlying layout of a file, even if the file is already
full of allocated blocks and data.

This isn't just btrfs - any filesystem that does dedupe or reflink
or snapshots or compression or any other sort of operation that can
cause extent reallocation on overwrite *may* change the file layout
during a fallocate call to guarantee the next write succeeds.

> It's OK not to consider it as a bug, at least data is not corrupted.
> But IMO the btrfs behavior is not needed and need optimization.
> So kernel patch is submitted to btrfs ml:
> https://patchwork.kernel.org/patch/7284431/

That's a link to the fstests patch, not a btrfs kernel patch. :/

> And if fstests is not the proper place, any idea where such "test
> case" should belong?

You still haven't understood what I said. If you want to test that
btrfs does not truncate extents beyond EOF when fallocate is called,
then it's a btrfs test. Yes, You can do whatever you want with
btrfs, but you've proposed a generic test that applies a constraint
to a generic operation that has no such constraint defined in it's
API. If you want to constrain fallocate behaviour like this, then
take it to linux-fsdevel and get everyone to agree on the
constraint, and then I'll take it as a generic test...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Qu Wenruo]

Dave Chinner wrote on 2015/09/30 14:20 +1000:
> On Wed, Sep 30, 2015 at 09:05:15AM +0800, Qu Wenruo wrote:
>>
>>
>> Dave Chinner wrote on 2015/09/30 07:51 +1000:
>>> On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
>>>> Normally, a bull fallocate call on a fully written and synced file
>>>> should not add an extent.
>>>
>>> Why not? Filesystems can do whatever they want with extents during
>>> a fallocate call. e.g. if the blocks are shared, then fallocate
>>> might break the block sharing so future overwrites don't get
>>> ENOSPC. This is a requirement set down by posix_fallocate(3)
>>>
>>> "After a successful call to posix_fallocate(), subsequent writes to
>>> bytes in the specified range are guaranteed not  to fail because of
>>> lack of disk space."
>>>
>>> Hence if you've got a file with shared blocks, a "full fallocate"
>>> must change the extent layout to break the sharing. As such, the
>>> premise of this test is wrong.
>>
>> First, btrfs never meets the posix_fallocate requirement by its COW nature.
>>
>> Btrfs fallocate can only ensure at most next write will not cause ENOSPC.
>
> Which, it can be successfully argued, meets the basic requirement of
> posix_fallocate().  i.e. "subsequent writes" is "one or more" future
> writes.
>
> But in trying to explain how COW works you've completely missed the
> point I was making about a fundamental principle that COW is based
> on - overwrite requires allocation. Hence fallocate must be allowed
> modify the underlying layout of a file, even if the file is already
> full of allocated blocks and data.
>
> This isn't just btrfs - any filesystem that does dedupe or reflink
> or snapshots or compression or any other sort of operation that can
> cause extent reallocation on overwrite *may* change the file layout
> during a fallocate call to guarantee the next write succeeds.
>
>> It's OK not to consider it as a bug, at least data is not corrupted.
>> But IMO the btrfs behavior is not needed and need optimization.
>> So kernel patch is submitted to btrfs ml:
>> https://patchwork.kernel.org/patch/7284431/
>
> That's a link to the fstests patch, not a btrfs kernel patch. :/

Sorry, the real patch is https://patchwork.kernel.org/patch/7283461/

>
>> And if fstests is not the proper place, any idea where such "test
>> case" should belong?
>
> You still haven't understood what I said. If you want to test that
> btrfs does not truncate extents beyond EOF when fallocate is called,
> then it's a btrfs test. Yes, You can do whatever you want with
> btrfs, but you've proposed a generic test that applies a constraint
> to a generic operation that has no such constraint defined in it's
> API. If you want to constrain fallocate behaviour like this, then
> take it to linux-fsdevel and get everyone to agree on the
> constraint, and then I'll take it as a generic test...

Now, I think I understand the point.
The test case is not for generic, but for Btrfs only.

The reason is:
Fallocate should be able to change extent layout
Even all extent is allocated, for its purpose.

So the assumption for the test case is not valid, and how a filesystem 
handle the last page truncate should follow its own standard.

This is convincing now.

I'll change it to btrfs test.

BTW, at least xfs doesn't truncate beyond EOF, will xfs also need such test?

Thanks,
Qu

>
> Cheers,
>
> Dave.
>

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Qu Wenruo]

Hi Dave, I updated the patch and moved it to btrfs.

But I still has some question about the fallocate behavior.

Just as the new btrfs test case, I changed the fallocate range, not to 
cover the last part, to make the problem more obvious:

Btrfs will truncate beyond EOF even that's *not covered* by the 
fallocate range.

It's OK for a fs to modify the extent layout during fallocate, but is it 
OK to modify extent layout completely *out* of the fallocate range?

Thanks,
Qu

在 2015年09月30日 05:51, Dave Chinner 写道:
> On Tue, Sep 29, 2015 at 05:34:24PM +0800, Qu Wenruo wrote:
>> Normally, a bull fallocate call on a fully written and synced file
>> should not add an extent.
>
> Why not? Filesystems can do whatever they want with extents during
> a fallocate call. e.g. if the blocks are shared, then fallocate
> might break the block sharing so future overwrites don't get
> ENOSPC. This is a requirement set down by posix_fallocate(3)
>
> "After a successful call to posix_fallocate(), subsequent writes to
> bytes in the specified range are guaranteed not  to fail because of
> lack of disk space."
>
> Hence if you've got a file with shared blocks, a "full fallocate"
> must change the extent layout to break the sharing. As such, the
> premise of this test is wrong.
>
> That's not to say that btrfs has a bug:
>
>> Btrfs has a bug to always truncate the last page if the fallocate start
>> offset is smaller than inode size.
>
> But it' not clear that this behaviour is actually a bug if it's not
> changing the file data.
>
> Cheers,
>
> Dave.
>

Re: [PATCH v2] fstests: generic: Check if a bull fallocate will change extent number

[Dave Chinner]

On Fri, Oct 02, 2015 at 04:35:53PM +0800, Qu Wenruo wrote:
> Hi Dave, I updated the patch and moved it to btrfs.
> 
> But I still has some question about the fallocate behavior.
> 
> Just as the new btrfs test case, I changed the fallocate range, not
> to cover the last part, to make the problem more obvious:
> 
> Btrfs will truncate beyond EOF even that's *not covered* by the
> fallocate range.
> 
> It's OK for a fs to modify the extent layout during fallocate, but
> is it OK to modify extent layout completely *out* of the fallocate
> range?

It's beyond EOF, so the filesystem can make whatever changes to
allocated blocks in that space whenever it likes because userspace
cannot access it.

In XFS, we don't remove unwritten extents beyond EOF if they were
placed there by fallocate except via explicit truncate() or
fallocate() operations (e.g. hole punch) from userspace that
manipulate that extent range beyond EOF.

What other filesytems do with blocks beyond EOF in any given
operation is up to the filesystem, really. If btrfs wants to
truncate away all extents beyond EOF when punching a hole that spans
EOF, then you can do that. It might not be what the user expects,
but blocks beyond EOF don't fall under posix_fallocate() because
it explicitly states:

"If the size of the file is less than offset+len, then the file is
increased to this size; otherwise the file size is left unchanged."

which means it can't allocate blocks beyond EOF....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com