diff mbox series

fstests: btrfs: test setting compression via xattr on nodatacow files

Message ID 20220418075430.484158-1-cccheng@synology.com (mailing list archive)
State New, archived
Headers show
Series fstests: btrfs: test setting compression via xattr on nodatacow files | expand

Commit Message

Chung-Chiang Cheng April 18, 2022, 7:54 a.m. UTC 
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

Comments

Nikolay Borisov April 19, 2022, 12:47 p.m. UTC | #1 
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
Nikolay Borisov April 19, 2022, 1:01 p.m. UTC | #2 
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.
Filipe Manana April 19, 2022, 2:52 p.m. UTC | #3 
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
>
Chung-Chiang Cheng April 22, 2022, 11:11 a.m. UTC | #4 
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.
Chung-Chiang Cheng April 22, 2022, 11:30 a.m. UTC | #5 
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.
Filipe Manana April 22, 2022, 2:31 p.m. UTC | #6 
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.
diff mbox series

Patch

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