All of lore.kernel.org
 help / color / mirror / Atom feed
From: Filipe Manana <fdmanana@kernel.org>
To: Chung-Chiang Cheng <cccheng@synology.com>
Cc: fstests@vger.kernel.org, linux-btrfs@vger.kernel.org,
	nborisov@suse.com, dsterba@suse.com, shepjeng@gmail.com,
	kernel@cccheng.net
Subject: Re: [PATCH] fstests: btrfs: test setting compression via xattr on nodatacow files
Date: Tue, 19 Apr 2022 15:52:33 +0100	[thread overview]
Message-ID: <Yl7MsVxpaYWfIEZH@debian9.Home> (raw)
In-Reply-To: <20220418075430.484158-1-cccheng@synology.com>

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
> 

  parent reply	other threads:[~2022-04-19 14:52 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2022-04-22 11:11   ` Chung-Chiang Cheng
2022-04-22 14:31     ` Filipe Manana

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Yl7MsVxpaYWfIEZH@debian9.Home \
    --to=fdmanana@kernel.org \
    --cc=cccheng@synology.com \
    --cc=dsterba@suse.com \
    --cc=fstests@vger.kernel.org \
    --cc=kernel@cccheng.net \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=nborisov@suse.com \
    --cc=shepjeng@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.