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@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxx>
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
>
