* [PATCH] fstests: btrfs: test setting compression via xattr on nodatacow files @ 2022-04-18 7:54 Chung-Chiang Cheng 2022-04-19 12:47 ` Nikolay Borisov 2022-04-19 14:52 ` Filipe Manana 0 siblings, 2 replies; 7+ messages in thread From: Chung-Chiang Cheng @ 2022-04-18 7:54 UTC (permalink / raw) To: fstests, linux-btrfs, nborisov, fdmanana, dsterba Cc: shepjeng, kernel, Chung-Chiang Cheng Compression and nodatacow are mutually exclusive. Besides ioctl, there is another way to setting compression via xattrs, and shouldn't produce invalid combinations. Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> --- tests/btrfs/264 | 76 +++++++++++++++++++++++++++++++++++++++++++++ tests/btrfs/264.out | 15 +++++++++ 2 files changed, 91 insertions(+) create mode 100755 tests/btrfs/264 create mode 100644 tests/btrfs/264.out diff --git a/tests/btrfs/264 b/tests/btrfs/264 new file mode 100755 index 000000000000..42bfcd4f93a0 --- /dev/null +++ b/tests/btrfs/264 @@ -0,0 +1,76 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2022 Synology Inc. All Rights Reserved. +# +# FS QA Test No. 264 +# +# Compression and nodatacow are mutually exclusive. Besides ioctl, there +# is another way to setting compression via xattrs, and shouldn't produce +# invalid combinations. +# +# To prevent mix any compression-related options with nodatacow, FS_NOCOMP_FL +# is also rejected by ioctl as well as FS_COMPR_FL on nodatacow files. To +# align with it, no and none are also unacceptable in this test. +# +# The regression is fixed by a patch with the following subject: +# btrfs: do not allow compression on nodatacow files +# +. ./common/preamble +_begin_fstest auto quick compress attr + +# Import common functions. +. ./common/filter +. ./common/attr + +# real QA test starts here + +_supported_fs btrfs +_require_scratch +_require_chattr C +_require_chattr c + +_scratch_mkfs >>$seqres.full 2>&1 +_scratch_mount + +# +# DATACOW +# +test_file="$SCRATCH_MNT/foo" +touch "$test_file" +$CHATTR_PROG -C "$test_file" +$LSATTR_PROG -l "$test_file" | _filter_scratch + +$SETFATTR_PROG -n "btrfs.compression" -v zlib "$test_file" |& _filter_scratch +$LSATTR_PROG -l "$test_file" | _filter_scratch +$SETFATTR_PROG -n "btrfs.compression" -v no "$test_file" |& _filter_scratch +$LSATTR_PROG -l "$test_file" | _filter_scratch +$SETFATTR_PROG -n "btrfs.compression" -v lzo "$test_file" |& _filter_scratch +$LSATTR_PROG -l "$test_file" | _filter_scratch +$SETFATTR_PROG -n "btrfs.compression" -v none "$test_file" |& _filter_scratch +$LSATTR_PROG -l "$test_file" | _filter_scratch +$SETFATTR_PROG -n "btrfs.compression" -v zstd "$test_file" |& _filter_scratch +$LSATTR_PROG -l "$test_file" | _filter_scratch + + +# +# NODATACOW +# +test_file="$SCRATCH_MNT/bar" +touch "$test_file" +$CHATTR_PROG +C "$test_file" +$LSATTR_PROG -l "$test_file" | _filter_scratch + +# all valid compression type are not allowed on nodatacow files +$SETFATTR_PROG -n "btrfs.compression" -v zlib "$test_file" |& _filter_scratch +$SETFATTR_PROG -n "btrfs.compression" -v lzo "$test_file" |& _filter_scratch +$SETFATTR_PROG -n "btrfs.compression" -v zstd "$test_file" |& _filter_scratch +$LSATTR_PROG -l "$test_file" | _filter_scratch + +# no/none are also not allowed on nodatacow files +$SETFATTR_PROG -n "btrfs.compression" -v no "$test_file" |& _filter_scratch +$SETFATTR_PROG -n "btrfs.compression" -v none "$test_file" |& _filter_scratch +$LSATTR_PROG -l "$test_file" | _filter_scratch + +# success, all done +status=0 +exit diff --git a/tests/btrfs/264.out b/tests/btrfs/264.out new file mode 100644 index 000000000000..82c551411411 --- /dev/null +++ b/tests/btrfs/264.out @@ -0,0 +1,15 @@ +QA output created by 264 +SCRATCH_MNT/foo --- +SCRATCH_MNT/foo Compression_Requested +SCRATCH_MNT/foo --- +SCRATCH_MNT/foo Compression_Requested +SCRATCH_MNT/foo --- +SCRATCH_MNT/foo Compression_Requested +SCRATCH_MNT/bar No_COW +setfattr: SCRATCH_MNT/bar: Invalid argument +setfattr: SCRATCH_MNT/bar: Invalid argument +setfattr: SCRATCH_MNT/bar: Invalid argument +SCRATCH_MNT/bar No_COW +setfattr: SCRATCH_MNT/bar: Invalid argument +setfattr: SCRATCH_MNT/bar: Invalid argument +SCRATCH_MNT/bar No_COW -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] fstests: btrfs: test setting compression via xattr on nodatacow files 2022-04-18 7:54 [PATCH] fstests: btrfs: test setting compression via xattr on nodatacow files Chung-Chiang Cheng @ 2022-04-19 12:47 ` Nikolay Borisov 2022-04-19 13:01 ` Nikolay Borisov 2022-04-22 11:30 ` Chung-Chiang Cheng 2022-04-19 14:52 ` Filipe Manana 1 sibling, 2 replies; 7+ messages in thread From: Nikolay Borisov @ 2022-04-19 12:47 UTC (permalink / raw) To: Chung-Chiang Cheng, fstests, linux-btrfs, fdmanana, dsterba Cc: shepjeng, kernel On 18.04.22 г. 10:54 ч., Chung-Chiang Cheng wrote: > Compression and nodatacow are mutually exclusive. Besides ioctl, there > is another way to setting compression via xattrs, and shouldn't produce > invalid combinations. > > Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> > --- > tests/btrfs/264 | 76 +++++++++++++++++++++++++++++++++++++++++++++ > tests/btrfs/264.out | 15 +++++++++ > 2 files changed, 91 insertions(+) > create mode 100755 tests/btrfs/264 > create mode 100644 tests/btrfs/264.out > > diff --git a/tests/btrfs/264 b/tests/btrfs/264 > new file mode 100755 > index 000000000000..42bfcd4f93a0 > --- /dev/null > +++ b/tests/btrfs/264 > @@ -0,0 +1,76 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (C) 2022 Synology Inc. All Rights Reserved. > +# > +# FS QA Test No. 264 > +# > +# Compression and nodatacow are mutually exclusive. Besides ioctl, there > +# is another way to setting compression via xattrs, and shouldn't produce > +# invalid combinations. > +# > +# To prevent mix any compression-related options with nodatacow, FS_NOCOMP_FL > +# is also rejected by ioctl as well as FS_COMPR_FL on nodatacow files. To > +# align with it, no and none are also unacceptable in this test. > +# > +# The regression is fixed by a patch with the following subject: > +# btrfs: do not allow compression on nodatacow files > +# > +. ./common/preamble > +_begin_fstest auto quick compress attr > + > +# Import common functions. > +. ./common/filter > +. ./common/attr > + > +# real QA test starts here > + > +_supported_fs btrfs > +_require_scratch > +_require_chattr C > +_require_chattr c > + > +_scratch_mkfs >>$seqres.full 2>&1 > +_scratch_mount > + > +# > +# DATACOW > +# > +test_file="$SCRATCH_MNT/foo" > +touch "$test_file" > +$CHATTR_PROG -C "$test_file" > +$LSATTR_PROG -l "$test_file" | _filter_scratch > + > +$SETFATTR_PROG -n "btrfs.compression" -v zlib "$test_file" |& _filter_scratch In my testing setfattr doesn't produce any output, so why the _filter_scratch ? > +$LSATTR_PROG -l "$test_file" | _filter_scratch This (as all other lsattr lines) fail for me because of difference in output - in your golden file there is a single space char between the filename and its state whilst lsattr produces a number of spaces. So in addition to filter_scratch this needs _filter_spaces as well : +$LSATTR_PROG -l "$test_file" | _filter_scratch | _filter_spaces > +$SETFATTR_PROG -n "btrfs.compression" -v no "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v lzo "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v none "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v zstd "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > + > + > +# > +# NODATACOW > +# > +test_file="$SCRATCH_MNT/bar" > +touch "$test_file" > +$CHATTR_PROG +C "$test_file" > +$LSATTR_PROG -l "$test_file" | _filter_scratch > + > +# all valid compression type are not allowed on nodatacow files > +$SETFATTR_PROG -n "btrfs.compression" -v zlib "$test_file" |& _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v lzo "$test_file" |& _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v zstd "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > + > +# no/none are also not allowed on nodatacow files > +$SETFATTR_PROG -n "btrfs.compression" -v no "$test_file" |& _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v none "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > + > +# success, all done > +status=0 > +exit > diff --git a/tests/btrfs/264.out b/tests/btrfs/264.out > new file mode 100644 > index 000000000000..82c551411411 > --- /dev/null > +++ b/tests/btrfs/264.out > @@ -0,0 +1,15 @@ > +QA output created by 264 > +SCRATCH_MNT/foo --- > +SCRATCH_MNT/foo Compression_Requested > +SCRATCH_MNT/foo --- > +SCRATCH_MNT/foo Compression_Requested > +SCRATCH_MNT/foo --- > +SCRATCH_MNT/foo Compression_Requested > +SCRATCH_MNT/bar No_COW > +setfattr: SCRATCH_MNT/bar: Invalid argument > +setfattr: SCRATCH_MNT/bar: Invalid argument > +setfattr: SCRATCH_MNT/bar: Invalid argument > +SCRATCH_MNT/bar No_COW > +setfattr: SCRATCH_MNT/bar: Invalid argument > +setfattr: SCRATCH_MNT/bar: Invalid argument > +SCRATCH_MNT/bar No_COW ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fstests: btrfs: test setting compression via xattr on nodatacow files 2022-04-19 12:47 ` Nikolay Borisov @ 2022-04-19 13:01 ` Nikolay Borisov 2022-04-22 11:30 ` Chung-Chiang Cheng 1 sibling, 0 replies; 7+ messages in thread From: Nikolay Borisov @ 2022-04-19 13:01 UTC (permalink / raw) To: Chung-Chiang Cheng, fstests, linux-btrfs, fdmanana, dsterba Cc: shepjeng, kernel On 19.04.22 г. 15:47 ч., Nikolay Borisov wrote: > > > On 18.04.22 г. 10:54 ч., Chung-Chiang Cheng wrote: >> Compression and nodatacow are mutually exclusive. Besides ioctl, there >> is another way to setting compression via xattrs, and shouldn't produce >> invalid combinations. >> >> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> >> --- >> tests/btrfs/264 | 76 +++++++++++++++++++++++++++++++++++++++++++++ >> tests/btrfs/264.out | 15 +++++++++ >> 2 files changed, 91 insertions(+) >> create mode 100755 tests/btrfs/264 >> create mode 100644 tests/btrfs/264.out >> >> diff --git a/tests/btrfs/264 b/tests/btrfs/264 >> new file mode 100755 >> index 000000000000..42bfcd4f93a0 >> --- /dev/null >> +++ b/tests/btrfs/264 >> @@ -0,0 +1,76 @@ >> +#! /bin/bash >> +# SPDX-License-Identifier: GPL-2.0 >> +# Copyright (C) 2022 Synology Inc. All Rights Reserved. >> +# >> +# FS QA Test No. 264 >> +# >> +# Compression and nodatacow are mutually exclusive. Besides ioctl, there >> +# is another way to setting compression via xattrs, and shouldn't >> produce >> +# invalid combinations. >> +# >> +# To prevent mix any compression-related options with nodatacow, >> FS_NOCOMP_FL >> +# is also rejected by ioctl as well as FS_COMPR_FL on nodatacow >> files. To >> +# align with it, no and none are also unacceptable in this test. >> +# >> +# The regression is fixed by a patch with the following subject: >> +# btrfs: do not allow compression on nodatacow files >> +# >> +. ./common/preamble >> +_begin_fstest auto quick compress attr >> + >> +# Import common functions. >> +. ./common/filter >> +. ./common/attr >> + >> +# real QA test starts here >> + >> +_supported_fs btrfs >> +_require_scratch >> +_require_chattr C >> +_require_chattr c >> + >> +_scratch_mkfs >>$seqres.full 2>&1 >> +_scratch_mount >> + >> +# >> +# DATACOW >> +# >> +test_file="$SCRATCH_MNT/foo" >> +touch "$test_file" >> +$CHATTR_PROG -C "$test_file" >> +$LSATTR_PROG -l "$test_file" | _filter_scratch >> + >> +$SETFATTR_PROG -n "btrfs.compression" -v zlib "$test_file" |& >> _filter_scratch > > In my testing setfattr doesn't produce any output, so why the > _filter_scratch ? nit: Ah right, that applies to those setfattr calls that are expected to fail when doing the nodatacow ops. So perhaps you could only do |& on the setfattr calls which are expected to fail. <snip> SO the main thing which remains to fix is calling the _filter_spaces to ensure the test succeeds. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fstests: btrfs: test setting compression via xattr on nodatacow files 2022-04-19 12:47 ` Nikolay Borisov 2022-04-19 13:01 ` Nikolay Borisov @ 2022-04-22 11:30 ` Chung-Chiang Cheng 1 sibling, 0 replies; 7+ messages in thread From: Chung-Chiang Cheng @ 2022-04-22 11:30 UTC (permalink / raw) To: Nikolay Borisov Cc: Chung-Chiang Cheng, fstests, linux-btrfs, Filipe Manana, David Sterba, kernel Hi Nikolay, > This (as all other lsattr lines) fail for me because of difference in > output - in your golden file there is a single space char between the > filename and its state whilst lsattr produces a number of spaces. So in > addition to filter_scratch this needs _filter_spaces as well : > > +$LSATTR_PROG -l "$test_file" | _filter_scratch | _filter_spaces > Thanks for your comment. I did not notice the space issue before. From the output of Filipe's test, there is also only one space between filename and its state. This is the same as my test results. I have no idea which procedure these space were eliminated, but I think it is reasonable to add a space filter here. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fstests: btrfs: test setting compression via xattr on nodatacow files 2022-04-18 7:54 [PATCH] fstests: btrfs: test setting compression via xattr on nodatacow files Chung-Chiang Cheng 2022-04-19 12:47 ` Nikolay Borisov @ 2022-04-19 14:52 ` Filipe Manana 2022-04-22 11:11 ` Chung-Chiang Cheng 1 sibling, 1 reply; 7+ messages in thread From: Filipe Manana @ 2022-04-19 14:52 UTC (permalink / raw) To: Chung-Chiang Cheng Cc: fstests, linux-btrfs, nborisov, dsterba, shepjeng, kernel On Mon, Apr 18, 2022 at 03:54:30PM +0800, Chung-Chiang Cheng wrote: > Compression and nodatacow are mutually exclusive. Besides ioctl, there > is another way to setting compression via xattrs, and shouldn't produce > invalid combinations. Hi Chung-Chiang, Thanks for taking the time to write and submit the test. Some inlined comments below. > > Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> > --- > tests/btrfs/264 | 76 +++++++++++++++++++++++++++++++++++++++++++++ > tests/btrfs/264.out | 15 +++++++++ > 2 files changed, 91 insertions(+) > create mode 100755 tests/btrfs/264 > create mode 100644 tests/btrfs/264.out > > diff --git a/tests/btrfs/264 b/tests/btrfs/264 > new file mode 100755 > index 000000000000..42bfcd4f93a0 > --- /dev/null > +++ b/tests/btrfs/264 > @@ -0,0 +1,76 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (C) 2022 Synology Inc. All Rights Reserved. > +# > +# FS QA Test No. 264 > +# > +# Compression and nodatacow are mutually exclusive. Besides ioctl, there > +# is another way to setting compression via xattrs, and shouldn't produce > +# invalid combinations. > +# > +# To prevent mix any compression-related options with nodatacow, FS_NOCOMP_FL > +# is also rejected by ioctl as well as FS_COMPR_FL on nodatacow files. To > +# align with it, no and none are also unacceptable in this test. > +# > +# The regression is fixed by a patch with the following subject: > +# btrfs: do not allow compression on nodatacow files > +# > +. ./common/preamble > +_begin_fstest auto quick compress attr > + > +# Import common functions. > +. ./common/filter > +. ./common/attr > + > +# real QA test starts here > + > +_supported_fs btrfs > +_require_scratch > +_require_chattr C > +_require_chattr c This require, for chattr c, is not needed, since the test never calls chattr with +c or -c. It also misses a call to: _require_attrs Due to the calls to setfattr and lsattr. > + > +_scratch_mkfs >>$seqres.full 2>&1 > +_scratch_mount > + > +# > +# DATACOW > +# > +test_file="$SCRATCH_MNT/foo" > +touch "$test_file" > +$CHATTR_PROG -C "$test_file" > +$LSATTR_PROG -l "$test_file" | _filter_scratch > + > +$SETFATTR_PROG -n "btrfs.compression" -v zlib "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v no "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v lzo "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v none "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v zstd "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > + > + > +# > +# NODATACOW > +# > +test_file="$SCRATCH_MNT/bar" > +touch "$test_file" > +$CHATTR_PROG +C "$test_file" > +$LSATTR_PROG -l "$test_file" | _filter_scratch > + > +# all valid compression type are not allowed on nodatacow files > +$SETFATTR_PROG -n "btrfs.compression" -v zlib "$test_file" |& _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v lzo "$test_file" |& _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v zstd "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > + > +# no/none are also not allowed on nodatacow files > +$SETFATTR_PROG -n "btrfs.compression" -v no "$test_file" |& _filter_scratch > +$SETFATTR_PROG -n "btrfs.compression" -v none "$test_file" |& _filter_scratch > +$LSATTR_PROG -l "$test_file" | _filter_scratch > + > +# success, all done > +status=0 > +exit > diff --git a/tests/btrfs/264.out b/tests/btrfs/264.out > new file mode 100644 > index 000000000000..82c551411411 > --- /dev/null > +++ b/tests/btrfs/264.out > @@ -0,0 +1,15 @@ > +QA output created by 264 > +SCRATCH_MNT/foo --- > +SCRATCH_MNT/foo Compression_Requested > +SCRATCH_MNT/foo --- > +SCRATCH_MNT/foo Compression_Requested > +SCRATCH_MNT/foo --- > +SCRATCH_MNT/foo Compression_Requested > +SCRATCH_MNT/bar No_COW > +setfattr: SCRATCH_MNT/bar: Invalid argument > +setfattr: SCRATCH_MNT/bar: Invalid argument > +setfattr: SCRATCH_MNT/bar: Invalid argument > +SCRATCH_MNT/bar No_COW > +setfattr: SCRATCH_MNT/bar: Invalid argument > +setfattr: SCRATCH_MNT/bar: Invalid argument > +SCRATCH_MNT/bar No_COW This is all fine, and I appreciate that you had special care to make sure the test works even if one runs the test with "-o compress" or "-o nodatacow" set in MOUNT_OPTIONS. However the test is based on some pre-5.14 kernel, presumably one of the Synology forks, and because of that it fails even after applying the btrfs fix on a recent kernel: root 15:43:45 /home/fdmanana/git/hub/xfstests (master)> ./check btrfs/264 FSTYP -- btrfs PLATFORM -- Linux/x86_64 debian9 5.18.0-rc2-btrfs-next-115 #1 SMP PREEMPT_DYNAMIC Wed Apr 13 09:17:09 WEST 2022 MKFS_OPTIONS -- /dev/sdc MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1 btrfs/264 - output mismatch (see /home/fdmanana/git/hub/xfstests/results//btrfs/264.out.bad) --- tests/btrfs/264.out 2022-04-19 14:49:03.845696283 +0100 +++ /home/fdmanana/git/hub/xfstests/results//btrfs/264.out.bad 2022-04-19 15:43:50.413816742 +0100 @@ -1,9 +1,9 @@ QA output created by 264 SCRATCH_MNT/foo --- SCRATCH_MNT/foo Compression_Requested -SCRATCH_MNT/foo --- +SCRATCH_MNT/foo Dont_Compress SCRATCH_MNT/foo Compression_Requested -SCRATCH_MNT/foo --- ... (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/264.out /home/fdmanana/git/hub/xfstests/results//btrfs/264.out.bad' to see the entire diff) Ran: btrfs/264 Failures: btrfs/264 Failed 1 of 1 tests root 15:43:50 /home/fdmanana/git/hub/xfstests (master)> diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/264.out /home/fdmanana/git/hub/xfstests/results//btrfs/264.out.bad --- /home/fdmanana/git/hub/xfstests/tests/btrfs/264.out 2022-04-19 14:49:03.845696283 +0100 +++ /home/fdmanana/git/hub/xfstests/results//btrfs/264.out.bad 2022-04-19 15:43:50.413816742 +0100 @@ -1,9 +1,9 @@ QA output created by 264 SCRATCH_MNT/foo --- SCRATCH_MNT/foo Compression_Requested -SCRATCH_MNT/foo --- +SCRATCH_MNT/foo Dont_Compress SCRATCH_MNT/foo Compression_Requested -SCRATCH_MNT/foo --- +SCRATCH_MNT/foo Dont_Compress SCRATCH_MNT/foo Compression_Requested SCRATCH_MNT/bar No_COW setfattr: SCRATCH_MNT/bar: Invalid argument root 15:43:52 /home/fdmanana/git/hub/xfstests (master)> The reason is that the test is expecting that setting "no" for the compression property removes the compression flag only, but does not set the NOCOMPRESS flag. After 5.14, setting 'no' or 'none' clears the COMPRESS bit and sets the NOCOMPRESS bit. The change happened in this commit: commit 5548c8c6f55bf0097075b3720e14857e3272429f Author: David Sterba <dsterba@suse.com> Date: Mon Jun 14 18:10:04 2021 +0200 btrfs: props: change how empty value is interpreted So the test needs to be updated and tested on a recent kernel. Other than that, it looks fine to me. Thanks. > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fstests: btrfs: test setting compression via xattr on nodatacow files 2022-04-19 14:52 ` Filipe Manana @ 2022-04-22 11:11 ` Chung-Chiang Cheng 2022-04-22 14:31 ` Filipe Manana 0 siblings, 1 reply; 7+ messages in thread From: Chung-Chiang Cheng @ 2022-04-22 11:11 UTC (permalink / raw) To: Filipe Manana Cc: Chung-Chiang Cheng, fstests, linux-btrfs, nborisov, David Sterba, kernel Hi Filipe, > > +_require_scratch > > +_require_chattr C > > +_require_chattr c > > This require, for chattr c, is not needed, since the test never calls > chattr with +c or -c. > > It also misses a call to: > > _require_attrs > > Due to the calls to setfattr and lsattr. Thanks for your notification. I'll fix these issues. > root 15:43:50 /home/fdmanana/git/hub/xfstests (master)> diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/264.out /home/fdmanana/git/hub/xfstests/results//btrfs/264.out.bad > --- /home/fdmanana/git/hub/xfstests/tests/btrfs/264.out 2022-04-19 14:49:03.845696283 +0100 > +++ /home/fdmanana/git/hub/xfstests/results//btrfs/264.out.bad 2022-04-19 15:43:50.413816742 +0100 > @@ -1,9 +1,9 @@ > QA output created by 264 > SCRATCH_MNT/foo --- > SCRATCH_MNT/foo Compression_Requested > -SCRATCH_MNT/foo --- > +SCRATCH_MNT/foo Dont_Compress > SCRATCH_MNT/foo Compression_Requested > -SCRATCH_MNT/foo --- > +SCRATCH_MNT/foo Dont_Compress > SCRATCH_MNT/foo Compression_Requested > SCRATCH_MNT/bar No_COW > setfattr: SCRATCH_MNT/bar: Invalid argument > root 15:43:52 /home/fdmanana/git/hub/xfstests (master)> > > So the test needs to be updated and tested on a recent kernel. > Other than that, it looks fine to me. I can see why my output is different from yours. I tested this item with the latest upstream kernel, but my `chattr` comes from e2fsprogs-1.45.5, which does not yet support Dont_Compress. This test relies on a recent chattr, but `_require_attrs` does not check its version. In any case, I will send a v2 patch based on the latest chattr. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fstests: btrfs: test setting compression via xattr on nodatacow files 2022-04-22 11:11 ` Chung-Chiang Cheng @ 2022-04-22 14:31 ` Filipe Manana 0 siblings, 0 replies; 7+ messages in thread From: Filipe Manana @ 2022-04-22 14:31 UTC (permalink / raw) To: Chung-Chiang Cheng Cc: Chung-Chiang Cheng, fstests, linux-btrfs, nborisov, David Sterba, kernel On Fri, Apr 22, 2022 at 07:11:42PM +0800, Chung-Chiang Cheng wrote: > Hi Filipe, > > > > +_require_scratch > > > +_require_chattr C > > > +_require_chattr c > > > > This require, for chattr c, is not needed, since the test never calls > > chattr with +c or -c. > > > > It also misses a call to: > > > > _require_attrs > > > > Due to the calls to setfattr and lsattr. > > Thanks for your notification. I'll fix these issues. > > > > root 15:43:50 /home/fdmanana/git/hub/xfstests (master)> diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/264.out /home/fdmanana/git/hub/xfstests/results//btrfs/264.out.bad > > --- /home/fdmanana/git/hub/xfstests/tests/btrfs/264.out 2022-04-19 14:49:03.845696283 +0100 > > +++ /home/fdmanana/git/hub/xfstests/results//btrfs/264.out.bad 2022-04-19 15:43:50.413816742 +0100 > > @@ -1,9 +1,9 @@ > > QA output created by 264 > > SCRATCH_MNT/foo --- > > SCRATCH_MNT/foo Compression_Requested > > -SCRATCH_MNT/foo --- > > +SCRATCH_MNT/foo Dont_Compress > > SCRATCH_MNT/foo Compression_Requested > > -SCRATCH_MNT/foo --- > > +SCRATCH_MNT/foo Dont_Compress > > SCRATCH_MNT/foo Compression_Requested > > SCRATCH_MNT/bar No_COW > > setfattr: SCRATCH_MNT/bar: Invalid argument > > root 15:43:52 /home/fdmanana/git/hub/xfstests (master)> > > > > So the test needs to be updated and tested on a recent kernel. > > Other than that, it looks fine to me. > > I can see why my output is different from yours. I tested this item with > the latest upstream kernel, but my `chattr` comes from e2fsprogs-1.45.5, > which does not yet support Dont_Compress. This test relies on a recent > chattr, but `_require_attrs` does not check its version. In any case, I > will send a v2 patch based on the latest chattr. Ah, that's interesting. Indeed, I tested on a box with e2fsprogs 1.46.5. Switching to 1.45.5, the test passes after the btrfs fix is applied. Looking at the e2fsprogs git history the behaviour changed in 1.46.2: commit 1f4a5aba59f39a33a84152b5ae3ec0a5657b12a1 Author: Lennart Poettering <lennart@poettering.net> Date: Thu Feb 25 12:33:35 2021 -0500 chattr/lsattr: expose FS_NOCOMP_FL (aka EXT2_NOCOMPR_FL) Before that, lsattr never printed anything if the 'no compress' bit was set. So generally test cases for fstests are written such that the test passes with any version of a particular tool, or it requires a minimum version to run (and is skipped on older versions). We do the former approach using filters on the output of a tool, converting the output from an older version to match the output of a new version. Though in this case it would be tricky, because we really want to distinguish between having the 'no compress' bit set versus not having it set. And setting the value 'no' or 'none' for the btrfs.compression xattr, means setting the 'no compress' bit, therefore we want to verify it's set. Making the test require e2fsprogs 1.46.2+, is tricky: lsattr has no '--version' flag to query the version, the only way would be to use distro specific commands to check the e2fsprogs version (e.g. "apt-cache policy e2fsprogs" on debian for example). The only way I can think of now is to add a require helper that tests the output of lsattr after setting 'no compress' - if it ouputs 'Dont_Compress', then it's 1.46.2+, otherwise something older. That's complex, so a simple way would be to make the test fail if compression is still set after the setfattr, instead of relying on the exact output of lsattr. Something like this: $SETFATTR_PROG -n btrfs.compression -v no "$test_file" |& _filter_scratch $LSATTR_PROG -l "$test_file" | grep -q 'Compression_Requested' [ $? -eq 0 ] && echo "Compression is still set in the file" With a comment somewhere before mentioning the behaviour change in e2fsprogs 1.46.2, and removing the lsattr output from the golden output file (264.out). This way the test serves its purpose, to test we can not set both compress and nodatacow, and runs with any version of e2fsprogs. 1.46.2 is about 1 year old, and most enterprise distros are very likely using an older version - we want the test to be able to run on them. Thanks. > > Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-04-22 14:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-18 7:54 [PATCH] fstests: btrfs: test setting compression via xattr on nodatacow files Chung-Chiang Cheng 2022-04-19 12:47 ` Nikolay Borisov 2022-04-19 13:01 ` Nikolay Borisov 2022-04-22 11:30 ` Chung-Chiang Cheng 2022-04-19 14:52 ` Filipe Manana 2022-04-22 11:11 ` Chung-Chiang Cheng 2022-04-22 14:31 ` Filipe Manana
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.