fstests: test btrfs fsync after hole punching with no-holes mode
diff mbox

Message ID 20180326225921.10096-1-fdmanana@kernel.org
State New
Headers show

Commit Message

Filipe Manana March 26, 2018, 10:59 p.m. UTC
From: Filipe Manana <fdmanana@suse.com>

Test that when we have the no-holes mode enabled and a specific metadata
layout, if we punch a hole and fsync the file, at replay time the whole
hole was preserved.

This issue is fixed by the following btrfs patch for the linux kernel:

  "Btrfs: fix fsync after hole punching when using no-holes feature"

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 tests/btrfs/159     | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/159.out |   5 +++
 tests/btrfs/group   |   1 +
 3 files changed, 106 insertions(+)
 create mode 100755 tests/btrfs/159
 create mode 100644 tests/btrfs/159.out

Comments

Eryu Guan March 28, 2018, 2:17 a.m. UTC | #1
On Mon, Mar 26, 2018 at 11:59:21PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Test that when we have the no-holes mode enabled and a specific metadata
> layout, if we punch a hole and fsync the file, at replay time the whole
> hole was preserved.
> 
> This issue is fixed by the following btrfs patch for the linux kernel:
> 
>   "Btrfs: fix fsync after hole punching when using no-holes feature"

I'd expect a test failure with 4.16-rc6 kernel, as the mentioned fix
above is not there. But test always passes for me. Did I miss anything?
btrfs-progs version is btrfs-progs-4.11.1-3.fc27.

> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  tests/btrfs/159     | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/159.out |   5 +++
>  tests/btrfs/group   |   1 +
>  3 files changed, 106 insertions(+)
>  create mode 100755 tests/btrfs/159
>  create mode 100644 tests/btrfs/159.out
> 
> diff --git a/tests/btrfs/159 b/tests/btrfs/159
> new file mode 100755
> index 00000000..6083975a
> --- /dev/null
> +++ b/tests/btrfs/159
> @@ -0,0 +1,100 @@
> +#! /bin/bash
> +# FSQA Test No. 159
> +#
> +# Test that when we have the no-holes mode enabled and a specific metadata
> +# layout, if we punch a hole and fsync the file, at replay time the whole
> +# hole was preserved.
> +#
> +#-----------------------------------------------------------------------
> +#
> +# Copyright (C) 2018 SUSE Linux Products GmbH. All Rights Reserved.
> +# Author: Filipe Manana <fdmanana@suse.com>
> +#
> +# 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"
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	_cleanup_flakey
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/dmflakey
> +
> +# real QA test starts here
> +_supported_fs generic
                 ^^^^^^^ btrfs
> +_supported_os Linux
> +_require_scratch
> +_require_dm_target flakey
> +_require_xfs_io_command "fpunch"
> +
> +rm -f $seqres.full
> +
> +# We create the filesystem with a node size of 64Kb because we need to create a
> +# specific metadata layout in order to trigger the bug we are testing. At the
> +# moment the node size can not be smaller then the system's page size, so given
> +# that the largest possible page size is 64Kb and by default the node size is
> +# set to the system's page size value, we explictly create a filesystem with a
> +# 64Kb node size.
> +_scratch_mkfs -O no-holes -n $((64 * 1024)) >>$seqres.full 2>&1
> +_require_metadata_journaling $SCRATCH_DEV
> +_init_flakey
> +_mount_flakey
> +
> +# Create our test file with 831 extents of 256Kb each. Before each extent, there

I think it's 832 extents in total? As the index starts with 0.

Otherwise looks good to me.

Thanks,
Eryu

> +# is a 256Kb hole (except for the first extent, which starts at offset 0). This
> +# creates two leafs in the filesystem tree, with the extent at offset 216530944
> +# being the last item in the first leaf and the extent at offset 217055232 being
> +# the first item in the second leaf.
> +for ((i = 0; i <= 831; i++)); do
> +	offset=$((i * 2 * 256 * 1024))
> +	$XFS_IO_PROG -f -c "pwrite -S 0xab -b 256K $offset 256K" \
> +		$SCRATCH_MNT/foobar >/dev/null
> +done
> +
> +# Now persist everything done so far.
> +sync
> +
> +# Now punch a hole that covers part of the extent at offset 216530944.
> +$XFS_IO_PROG -c "fpunch $((216530944 + 128 * 1024 - 4000)) 256K" \
> +	     -c "fsync" \
> +	     $SCRATCH_MNT/foobar
> +
> +echo "File digest before power failure:"
> +md5sum $SCRATCH_MNT/foobar | _filter_scratch
> +
> +# Simulate a power failure and mount the filesystem to check that replaying the
> +# fsync log/journal succeeds and our test file has the expected content.
> +_flakey_drop_and_remount
> +
> +echo "File digest after power failure and log replay:"
> +md5sum $SCRATCH_MNT/foobar | _filter_scratch
> +
> +_unmount_flakey
> +_cleanup_flakey
> +
> +status=0
> +exit
> diff --git a/tests/btrfs/159.out b/tests/btrfs/159.out
> new file mode 100644
> index 00000000..3317e516
> --- /dev/null
> +++ b/tests/btrfs/159.out
> @@ -0,0 +1,5 @@
> +QA output created by 159
> +File digest before power failure:
> +c5c0a13588a639529c979c57c336f441  SCRATCH_MNT/foobar
> +File digest after power failure and log replay:
> +c5c0a13588a639529c979c57c336f441  SCRATCH_MNT/foobar
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index 8007e07e..ba766f6b 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -161,3 +161,4 @@
>  156 auto quick trim
>  157 auto quick raid
>  158 auto quick raid scrub
> +159 auto quick
> -- 
> 2.11.0
> 
> --
> 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
Filipe Manana March 28, 2018, 8:48 a.m. UTC | #2
On Wed, Mar 28, 2018 at 3:17 AM, Eryu Guan <guaneryu@gmail.com> wrote:
> On Mon, Mar 26, 2018 at 11:59:21PM +0100, fdmanana@kernel.org wrote:
>> From: Filipe Manana <fdmanana@suse.com>
>>
>> Test that when we have the no-holes mode enabled and a specific metadata
>> layout, if we punch a hole and fsync the file, at replay time the whole
>> hole was preserved.
>>
>> This issue is fixed by the following btrfs patch for the linux kernel:
>>
>>   "Btrfs: fix fsync after hole punching when using no-holes feature"
>
> I'd expect a test failure with 4.16-rc6 kernel, as the mentioned fix
> above is not there. But test always passes for me. Did I miss anything?
> btrfs-progs version is btrfs-progs-4.11.1-3.fc27.

It should fail on any kernel, with any btrfs-progs version (which
should be irrelevant).
Somehow on your system we are not getting the specific metadata layout
needed to trigger the issue.

Can you apply the following patch on top of the test and provide the
result 159.full file?

https://friendpaste.com/6xAuLeN4xl1AGjO9Qc5I8L

So that I can see what metadata layout you are getting.
Thanks!

>
>>
>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>> ---
>>  tests/btrfs/159     | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/btrfs/159.out |   5 +++
>>  tests/btrfs/group   |   1 +
>>  3 files changed, 106 insertions(+)
>>  create mode 100755 tests/btrfs/159
>>  create mode 100644 tests/btrfs/159.out
>>
>> diff --git a/tests/btrfs/159 b/tests/btrfs/159
>> new file mode 100755
>> index 00000000..6083975a
>> --- /dev/null
>> +++ b/tests/btrfs/159
>> @@ -0,0 +1,100 @@
>> +#! /bin/bash
>> +# FSQA Test No. 159
>> +#
>> +# Test that when we have the no-holes mode enabled and a specific metadata
>> +# layout, if we punch a hole and fsync the file, at replay time the whole
>> +# hole was preserved.
>> +#
>> +#-----------------------------------------------------------------------
>> +#
>> +# Copyright (C) 2018 SUSE Linux Products GmbH. All Rights Reserved.
>> +# Author: Filipe Manana <fdmanana@suse.com>
>> +#
>> +# 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"
>> +tmp=/tmp/$$
>> +status=1     # failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +     _cleanup_flakey
>> +     cd /
>> +     rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +. ./common/dmflakey
>> +
>> +# real QA test starts here
>> +_supported_fs generic
>                  ^^^^^^^ btrfs
>> +_supported_os Linux
>> +_require_scratch
>> +_require_dm_target flakey
>> +_require_xfs_io_command "fpunch"
>> +
>> +rm -f $seqres.full
>> +
>> +# We create the filesystem with a node size of 64Kb because we need to create a
>> +# specific metadata layout in order to trigger the bug we are testing. At the
>> +# moment the node size can not be smaller then the system's page size, so given
>> +# that the largest possible page size is 64Kb and by default the node size is
>> +# set to the system's page size value, we explictly create a filesystem with a
>> +# 64Kb node size.
>> +_scratch_mkfs -O no-holes -n $((64 * 1024)) >>$seqres.full 2>&1
>> +_require_metadata_journaling $SCRATCH_DEV
>> +_init_flakey
>> +_mount_flakey
>> +
>> +# Create our test file with 831 extents of 256Kb each. Before each extent, there
>
> I think it's 832 extents in total? As the index starts with 0.
>
> Otherwise looks good to me.
>
> Thanks,
> Eryu
>
>> +# is a 256Kb hole (except for the first extent, which starts at offset 0). This
>> +# creates two leafs in the filesystem tree, with the extent at offset 216530944
>> +# being the last item in the first leaf and the extent at offset 217055232 being
>> +# the first item in the second leaf.
>> +for ((i = 0; i <= 831; i++)); do
>> +     offset=$((i * 2 * 256 * 1024))
>> +     $XFS_IO_PROG -f -c "pwrite -S 0xab -b 256K $offset 256K" \
>> +             $SCRATCH_MNT/foobar >/dev/null
>> +done
>> +
>> +# Now persist everything done so far.
>> +sync
>> +
>> +# Now punch a hole that covers part of the extent at offset 216530944.
>> +$XFS_IO_PROG -c "fpunch $((216530944 + 128 * 1024 - 4000)) 256K" \
>> +          -c "fsync" \
>> +          $SCRATCH_MNT/foobar
>> +
>> +echo "File digest before power failure:"
>> +md5sum $SCRATCH_MNT/foobar | _filter_scratch
>> +
>> +# Simulate a power failure and mount the filesystem to check that replaying the
>> +# fsync log/journal succeeds and our test file has the expected content.
>> +_flakey_drop_and_remount
>> +
>> +echo "File digest after power failure and log replay:"
>> +md5sum $SCRATCH_MNT/foobar | _filter_scratch
>> +
>> +_unmount_flakey
>> +_cleanup_flakey
>> +
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/159.out b/tests/btrfs/159.out
>> new file mode 100644
>> index 00000000..3317e516
>> --- /dev/null
>> +++ b/tests/btrfs/159.out
>> @@ -0,0 +1,5 @@
>> +QA output created by 159
>> +File digest before power failure:
>> +c5c0a13588a639529c979c57c336f441  SCRATCH_MNT/foobar
>> +File digest after power failure and log replay:
>> +c5c0a13588a639529c979c57c336f441  SCRATCH_MNT/foobar
>> diff --git a/tests/btrfs/group b/tests/btrfs/group
>> index 8007e07e..ba766f6b 100644
>> --- a/tests/btrfs/group
>> +++ b/tests/btrfs/group
>> @@ -161,3 +161,4 @@
>>  156 auto quick trim
>>  157 auto quick raid
>>  158 auto quick raid scrub
>> +159 auto quick
>> --
>> 2.11.0
>>
>> --
>> 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
Eryu Guan March 28, 2018, 10:33 a.m. UTC | #3
On Wed, Mar 28, 2018 at 09:48:17AM +0100, Filipe Manana wrote:
> On Wed, Mar 28, 2018 at 3:17 AM, Eryu Guan <guaneryu@gmail.com> wrote:
> > On Mon, Mar 26, 2018 at 11:59:21PM +0100, fdmanana@kernel.org wrote:
> >> From: Filipe Manana <fdmanana@suse.com>
> >>
> >> Test that when we have the no-holes mode enabled and a specific metadata
> >> layout, if we punch a hole and fsync the file, at replay time the whole
> >> hole was preserved.
> >>
> >> This issue is fixed by the following btrfs patch for the linux kernel:
> >>
> >>   "Btrfs: fix fsync after hole punching when using no-holes feature"
> >
> > I'd expect a test failure with 4.16-rc6 kernel, as the mentioned fix
> > above is not there. But test always passes for me. Did I miss anything?
> > btrfs-progs version is btrfs-progs-4.11.1-3.fc27.
> 
> It should fail on any kernel, with any btrfs-progs version (which
> should be irrelevant).
> Somehow on your system we are not getting the specific metadata layout
> needed to trigger the issue.
> 
> Can you apply the following patch on top of the test and provide the
> result 159.full file?
> 
> https://friendpaste.com/6xAuLeN4xl1AGjO9Qc5I8L
> 
> So that I can see what metadata layout you are getting.
> Thanks!

Sure, please see attachment.

Thanks,
Eryu
Filipe Manana March 29, 2018, 1:45 p.m. UTC | #4
On Wed, Mar 28, 2018 at 11:33 AM, Eryu Guan <guaneryu@gmail.com> wrote:
> On Wed, Mar 28, 2018 at 09:48:17AM +0100, Filipe Manana wrote:
>> On Wed, Mar 28, 2018 at 3:17 AM, Eryu Guan <guaneryu@gmail.com> wrote:
>> > On Mon, Mar 26, 2018 at 11:59:21PM +0100, fdmanana@kernel.org wrote:
>> >> From: Filipe Manana <fdmanana@suse.com>
>> >>
>> >> Test that when we have the no-holes mode enabled and a specific metadata
>> >> layout, if we punch a hole and fsync the file, at replay time the whole
>> >> hole was preserved.
>> >>
>> >> This issue is fixed by the following btrfs patch for the linux kernel:
>> >>
>> >>   "Btrfs: fix fsync after hole punching when using no-holes feature"
>> >
>> > I'd expect a test failure with 4.16-rc6 kernel, as the mentioned fix
>> > above is not there. But test always passes for me. Did I miss anything?
>> > btrfs-progs version is btrfs-progs-4.11.1-3.fc27.
>>
>> It should fail on any kernel, with any btrfs-progs version (which
>> should be irrelevant).
>> Somehow on your system we are not getting the specific metadata layout
>> needed to trigger the issue.
>>
>> Can you apply the following patch on top of the test and provide the
>> result 159.full file?
>>
>> https://friendpaste.com/6xAuLeN4xl1AGjO9Qc5I8L
>>
>> So that I can see what metadata layout you are getting.
>> Thanks!
>
> Sure, please see attachment.

Thanks!
So you indeed get a different metadata layout, and that is because you
have SELinux enabled which causes some xattr to be added to the test
file (foobar):

        item 6 key (257 XATTR_ITEM 3817753667) itemoff 64932 itemsize 83
                location key (0 UNKNOWN.0 0) type XATTR
                transid 7 data_len 37 name_len 16
                name: security.selinux
                data unconfined_u:object_r:unlabeled_t:s0

I can make the test work with and without selinux enabled (by punching
holes on a few extents that are candidates to be at leaf boundaries).
Is it worth it? (I assume most people run the tests without selinux)

thanks

>
> Thanks,
> Eryu
--
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
Jayashree Mohan March 29, 2018, 6:55 p.m. UTC | #5
Hi Filipe,

I tried running the xfstest above on kernel 4.15.0 and I haven't been
able to hit the bug. The xfstest passes clean for me. I compared the
btrfs-debug-tree in my case with the one attached by Eryu, and I see I
do not have any xattr as he does. However, for every run of the
xfstest, the extent tree info that I get from btrfs-debug-tree has
varying number of items in the first leaf node. (I have attached two
dump files for your reference.)

I also tried changing the offset at which fpunch is performed to match
the offset of the last extent in the first leaf of the extent tree -
however the md5 before and after crash was the same.

Could you give more details on this?

https://friendpaste.com/1wVAz7hG0U5SgYtZavbJhL
https://friendpaste.com/1wVAz7hG0U5SgYtZavxALg

Thanks,
Jayashree Mohan



On Thu, Mar 29, 2018 at 8:45 AM, Filipe Manana <fdmanana@kernel.org> wrote:
> On Wed, Mar 28, 2018 at 11:33 AM, Eryu Guan <guaneryu@gmail.com> wrote:
>> On Wed, Mar 28, 2018 at 09:48:17AM +0100, Filipe Manana wrote:
>>> On Wed, Mar 28, 2018 at 3:17 AM, Eryu Guan <guaneryu@gmail.com> wrote:
>>> > On Mon, Mar 26, 2018 at 11:59:21PM +0100, fdmanana@kernel.org wrote:
>>> >> From: Filipe Manana <fdmanana@suse.com>
>>> >>
>>> >> Test that when we have the no-holes mode enabled and a specific metadata
>>> >> layout, if we punch a hole and fsync the file, at replay time the whole
>>> >> hole was preserved.
>>> >>
>>> >> This issue is fixed by the following btrfs patch for the linux kernel:
>>> >>
>>> >>   "Btrfs: fix fsync after hole punching when using no-holes feature"
>>> >
>>> > I'd expect a test failure with 4.16-rc6 kernel, as the mentioned fix
>>> > above is not there. But test always passes for me. Did I miss anything?
>>> > btrfs-progs version is btrfs-progs-4.11.1-3.fc27.
>>>
>>> It should fail on any kernel, with any btrfs-progs version (which
>>> should be irrelevant).
>>> Somehow on your system we are not getting the specific metadata layout
>>> needed to trigger the issue.
>>>
>>> Can you apply the following patch on top of the test and provide the
>>> result 159.full file?
>>>
>>> https://friendpaste.com/6xAuLeN4xl1AGjO9Qc5I8L
>>>
>>> So that I can see what metadata layout you are getting.
>>> Thanks!
>>
>> Sure, please see attachment.
>
> Thanks!
> So you indeed get a different metadata layout, and that is because you
> have SELinux enabled which causes some xattr to be added to the test
> file (foobar):
>
>         item 6 key (257 XATTR_ITEM 3817753667) itemoff 64932 itemsize 83
>                 location key (0 UNKNOWN.0 0) type XATTR
>                 transid 7 data_len 37 name_len 16
>                 name: security.selinux
>                 data unconfined_u:object_r:unlabeled_t:s0
>
> I can make the test work with and without selinux enabled (by punching
> holes on a few extents that are candidates to be at leaf boundaries).
> Is it worth it? (I assume most people run the tests without selinux)
>
> thanks
>
>>
>> Thanks,
>> Eryu
> --
> 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
--
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
Eryu Guan March 30, 2018, 12:58 a.m. UTC | #6
On Thu, Mar 29, 2018 at 02:45:26PM +0100, Filipe Manana wrote:
> On Wed, Mar 28, 2018 at 11:33 AM, Eryu Guan <guaneryu@gmail.com> wrote:
> > On Wed, Mar 28, 2018 at 09:48:17AM +0100, Filipe Manana wrote:
> >> On Wed, Mar 28, 2018 at 3:17 AM, Eryu Guan <guaneryu@gmail.com> wrote:
> >> > On Mon, Mar 26, 2018 at 11:59:21PM +0100, fdmanana@kernel.org wrote:
> >> >> From: Filipe Manana <fdmanana@suse.com>
> >> >>
> >> >> Test that when we have the no-holes mode enabled and a specific metadata
> >> >> layout, if we punch a hole and fsync the file, at replay time the whole
> >> >> hole was preserved.
> >> >>
> >> >> This issue is fixed by the following btrfs patch for the linux kernel:
> >> >>
> >> >>   "Btrfs: fix fsync after hole punching when using no-holes feature"
> >> >
> >> > I'd expect a test failure with 4.16-rc6 kernel, as the mentioned fix
> >> > above is not there. But test always passes for me. Did I miss anything?
> >> > btrfs-progs version is btrfs-progs-4.11.1-3.fc27.
> >>
> >> It should fail on any kernel, with any btrfs-progs version (which
> >> should be irrelevant).
> >> Somehow on your system we are not getting the specific metadata layout
> >> needed to trigger the issue.
> >>
> >> Can you apply the following patch on top of the test and provide the
> >> result 159.full file?
> >>
> >> https://friendpaste.com/6xAuLeN4xl1AGjO9Qc5I8L
> >>
> >> So that I can see what metadata layout you are getting.
> >> Thanks!
> >
> > Sure, please see attachment.
> 
> Thanks!
> So you indeed get a different metadata layout, and that is because you
> have SELinux enabled which causes some xattr to be added to the test
> file (foobar):
> 
>         item 6 key (257 XATTR_ITEM 3817753667) itemoff 64932 itemsize 83
>                 location key (0 UNKNOWN.0 0) type XATTR
>                 transid 7 data_len 37 name_len 16
>                 name: security.selinux
>                 data unconfined_u:object_r:unlabeled_t:s0
> 
> I can make the test work with and without selinux enabled (by punching
> holes on a few extents that are candidates to be at leaf boundaries).
> Is it worth it? (I assume most people run the tests without selinux)

Yes, please make it work with selinux if possible (but if that requires
too much complexity, we can give it a second thought).

I'm not sure about others, but I run fstests with selinux almost all the
time, because Fedora/RHEL distros have selinux on by default :) so are
all other people using Fedora/RHEL/Centos as test hosts, I guess.

Thanks,
Eryu
--
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
Filipe Manana April 2, 2018, 2:24 p.m. UTC | #7
On Thu, Mar 29, 2018 at 7:55 PM, Jayashree Mohan
<jayashree2912@gmail.com> wrote:
> Hi Filipe,
>
> I tried running the xfstest above on kernel 4.15.0 and I haven't been
> able to hit the bug. The xfstest passes clean for me. I compared the
> btrfs-debug-tree in my case with the one attached by Eryu, and I see I
> do not have any xattr as he does. However, for every run of the
> xfstest, the extent tree info that I get from btrfs-debug-tree has
> varying number of items in the first leaf node. (I have attached two
> dump files for your reference.)
>
> I also tried changing the offset at which fpunch is performed to match
> the offset of the last extent in the first leaf of the extent tree -
> however the md5 before and after crash was the same.
>
> Could you give more details on this?

You are getting extents smaller then 256Kb, because writeback is being
triggered by the kernel (likely due to memory pressure).
The second version of the test uses direct IO instead to avoid that.
Thanks.

>
> https://friendpaste.com/1wVAz7hG0U5SgYtZavbJhL
> https://friendpaste.com/1wVAz7hG0U5SgYtZavxALg
>
> Thanks,
> Jayashree Mohan
>
>
>
> On Thu, Mar 29, 2018 at 8:45 AM, Filipe Manana <fdmanana@kernel.org> wrote:
>> On Wed, Mar 28, 2018 at 11:33 AM, Eryu Guan <guaneryu@gmail.com> wrote:
>>> On Wed, Mar 28, 2018 at 09:48:17AM +0100, Filipe Manana wrote:
>>>> On Wed, Mar 28, 2018 at 3:17 AM, Eryu Guan <guaneryu@gmail.com> wrote:
>>>> > On Mon, Mar 26, 2018 at 11:59:21PM +0100, fdmanana@kernel.org wrote:
>>>> >> From: Filipe Manana <fdmanana@suse.com>
>>>> >>
>>>> >> Test that when we have the no-holes mode enabled and a specific metadata
>>>> >> layout, if we punch a hole and fsync the file, at replay time the whole
>>>> >> hole was preserved.
>>>> >>
>>>> >> This issue is fixed by the following btrfs patch for the linux kernel:
>>>> >>
>>>> >>   "Btrfs: fix fsync after hole punching when using no-holes feature"
>>>> >
>>>> > I'd expect a test failure with 4.16-rc6 kernel, as the mentioned fix
>>>> > above is not there. But test always passes for me. Did I miss anything?
>>>> > btrfs-progs version is btrfs-progs-4.11.1-3.fc27.
>>>>
>>>> It should fail on any kernel, with any btrfs-progs version (which
>>>> should be irrelevant).
>>>> Somehow on your system we are not getting the specific metadata layout
>>>> needed to trigger the issue.
>>>>
>>>> Can you apply the following patch on top of the test and provide the
>>>> result 159.full file?
>>>>
>>>> https://friendpaste.com/6xAuLeN4xl1AGjO9Qc5I8L
>>>>
>>>> So that I can see what metadata layout you are getting.
>>>> Thanks!
>>>
>>> Sure, please see attachment.
>>
>> Thanks!
>> So you indeed get a different metadata layout, and that is because you
>> have SELinux enabled which causes some xattr to be added to the test
>> file (foobar):
>>
>>         item 6 key (257 XATTR_ITEM 3817753667) itemoff 64932 itemsize 83
>>                 location key (0 UNKNOWN.0 0) type XATTR
>>                 transid 7 data_len 37 name_len 16
>>                 name: security.selinux
>>                 data unconfined_u:object_r:unlabeled_t:s0
>>
>> I can make the test work with and without selinux enabled (by punching
>> holes on a few extents that are candidates to be at leaf boundaries).
>> Is it worth it? (I assume most people run the tests without selinux)
>>
>> thanks
>>
>>>
>>> Thanks,
>>> Eryu
>> --
>> 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
--
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
Jayashree Mohan April 2, 2018, 4:14 p.m. UTC | #8
On Mon, Apr 2, 2018 at 9:24 AM, Filipe Manana <fdmanana@kernel.org> wrote:
> On Thu, Mar 29, 2018 at 7:55 PM, Jayashree Mohan
> <jayashree2912@gmail.com> wrote:
>> Hi Filipe,
>>
>> I tried running the xfstest above on kernel 4.15.0 and I haven't been
>> able to hit the bug. The xfstest passes clean for me. I compared the
>> btrfs-debug-tree in my case with the one attached by Eryu, and I see I
>> do not have any xattr as he does. However, for every run of the
>> xfstest, the extent tree info that I get from btrfs-debug-tree has
>> varying number of items in the first leaf node. (I have attached two
>> dump files for your reference.)
>>
>> I also tried changing the offset at which fpunch is performed to match
>> the offset of the last extent in the first leaf of the extent tree -
>> however the md5 before and after crash was the same.
>>
>> Could you give more details on this?
>
> You are getting extents smaller then 256Kb, because writeback is being
> triggered by the kernel (likely due to memory pressure).
> The second version of the test uses direct IO instead to avoid that.
> Thanks.

Thanks for the clarification. I am able to reproduce the bug with the
new version of the test that uses direct writes.
--
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

Patch
diff mbox

diff --git a/tests/btrfs/159 b/tests/btrfs/159
new file mode 100755
index 00000000..6083975a
--- /dev/null
+++ b/tests/btrfs/159
@@ -0,0 +1,100 @@ 
+#! /bin/bash
+# FSQA Test No. 159
+#
+# Test that when we have the no-holes mode enabled and a specific metadata
+# layout, if we punch a hole and fsync the file, at replay time the whole
+# hole was preserved.
+#
+#-----------------------------------------------------------------------
+#
+# Copyright (C) 2018 SUSE Linux Products GmbH. All Rights Reserved.
+# Author: Filipe Manana <fdmanana@suse.com>
+#
+# 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"
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	_cleanup_flakey
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/dmflakey
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+_require_dm_target flakey
+_require_xfs_io_command "fpunch"
+
+rm -f $seqres.full
+
+# We create the filesystem with a node size of 64Kb because we need to create a
+# specific metadata layout in order to trigger the bug we are testing. At the
+# moment the node size can not be smaller then the system's page size, so given
+# that the largest possible page size is 64Kb and by default the node size is
+# set to the system's page size value, we explictly create a filesystem with a
+# 64Kb node size.
+_scratch_mkfs -O no-holes -n $((64 * 1024)) >>$seqres.full 2>&1
+_require_metadata_journaling $SCRATCH_DEV
+_init_flakey
+_mount_flakey
+
+# Create our test file with 831 extents of 256Kb each. Before each extent, there
+# is a 256Kb hole (except for the first extent, which starts at offset 0). This
+# creates two leafs in the filesystem tree, with the extent at offset 216530944
+# being the last item in the first leaf and the extent at offset 217055232 being
+# the first item in the second leaf.
+for ((i = 0; i <= 831; i++)); do
+	offset=$((i * 2 * 256 * 1024))
+	$XFS_IO_PROG -f -c "pwrite -S 0xab -b 256K $offset 256K" \
+		$SCRATCH_MNT/foobar >/dev/null
+done
+
+# Now persist everything done so far.
+sync
+
+# Now punch a hole that covers part of the extent at offset 216530944.
+$XFS_IO_PROG -c "fpunch $((216530944 + 128 * 1024 - 4000)) 256K" \
+	     -c "fsync" \
+	     $SCRATCH_MNT/foobar
+
+echo "File digest before power failure:"
+md5sum $SCRATCH_MNT/foobar | _filter_scratch
+
+# Simulate a power failure and mount the filesystem to check that replaying the
+# fsync log/journal succeeds and our test file has the expected content.
+_flakey_drop_and_remount
+
+echo "File digest after power failure and log replay:"
+md5sum $SCRATCH_MNT/foobar | _filter_scratch
+
+_unmount_flakey
+_cleanup_flakey
+
+status=0
+exit
diff --git a/tests/btrfs/159.out b/tests/btrfs/159.out
new file mode 100644
index 00000000..3317e516
--- /dev/null
+++ b/tests/btrfs/159.out
@@ -0,0 +1,5 @@ 
+QA output created by 159
+File digest before power failure:
+c5c0a13588a639529c979c57c336f441  SCRATCH_MNT/foobar
+File digest after power failure and log replay:
+c5c0a13588a639529c979c57c336f441  SCRATCH_MNT/foobar
diff --git a/tests/btrfs/group b/tests/btrfs/group
index 8007e07e..ba766f6b 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -161,3 +161,4 @@ 
 156 auto quick trim
 157 auto quick raid
 158 auto quick raid scrub
+159 auto quick