All of lore.kernel.org
 help / color / mirror / Atom feed
* [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-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-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-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.