* Re: [PATCH] generic/211: verify if the filesystem being tested supports in place writes
2025-07-23 12:04 [PATCH] generic/211: verify if the filesystem being tested supports in place writes fdmanana
@ 2025-07-23 14:49 ` Darrick J. Wong
2025-07-24 7:17 ` Christoph Hellwig
2025-07-24 12:05 ` [PATCH v2] " fdmanana
2 siblings, 0 replies; 5+ messages in thread
From: Darrick J. Wong @ 2025-07-23 14:49 UTC (permalink / raw)
To: fdmanana; +Cc: fstests, linux-btrfs, Filipe Manana
On Wed, Jul 23, 2025 at 01:04:06PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> The test currently assumes the filesystem can do in place writes (no
> Copy-On-Write, no allocation of new extents) when overwriting a file.
> While that is the case for most filesystems in most configurations, there
> are exceptions such as zoned xfs where overwriting results in allocating
> new extents for the new data.
>
> So make the test check that in place writes are supported and skip the
> test if they are not supported.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
This is a smarter sollution than the fugly hack that I came up with:
# Don't run on mounted filesystems that only support out of place writes
if [ "$FSTYP" = "xfs" ]; then
_require_no_xfs_always_cow
_require_xfs_scratch_non_zoned
fi
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
> ---
> common/rc | 59 +++++++++++++++++++++++++++++++++++++++++++++++
> tests/generic/211 | 1 +
> 2 files changed, 60 insertions(+)
>
> diff --git a/common/rc b/common/rc
> index 96578d15..52aade10 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -5873,6 +5873,65 @@ _require_program() {
> _have_program "$1" || _notrun "$tag required"
> }
>
> +# Test that a filesystem can do writes to a file in place (without allocating
> +# new extents, without Copy-On-Write semantics).
> +_require_inplace_writes()
> +{
> + _require_xfs_io_command "fiemap"
> +
> + local target=$1
> + local test_file="${target}/test_inplace_writes"
> + local fiemap_before
> + local fiemap_after
> +
> + if [ -z "$target" ]; then
> + _fail "Usage: _require_inplace_writes <filesystem path>"
> + fi
> +
> + rm -f "$test_file"
> + touch "$test_file"
> +
> + # Set the file to NOCOW mode on btrfs, which must be done while the file
> + # is empty, otherwise it fails.
> + if [ "$FSTYP" == "btrfs" ]; then
> + _require_chattr C
> + $CHATTR_PROG +C "$test_file"
> + fi
> +
> + $XFS_IO_PROG -f -c "pwrite 0 128K" -c "fsync" "$test_file" &> /dev/null
> + if [ $? -ne 0 ]; then
> + rm -f "$test_file"
> + _fail "_require_inplace_writes failed to write to test file"
> + fi
> +
> + # Grab fiemap output before overwriting.
> + fiemap_before=$($XFS_IO_PROG -c "fiemap" "$test_file")
> + if [ $? -ne 0 ]; then
> + rm -f "$test_file"
> + _fail "_require_inplace_writes first fiemap call failed"
> + fi
> +
> + $XFS_IO_PROG -c "pwrite 0 128K" -c "fsync" "$test_file" &> /dev/null
> + if [ $? -ne 0 ]; then
> + rm -f "$test_file"
> + _fail "_require_inplace_writes failed to overwrite test file"
> + fi
> +
> + fiemap_after=$($XFS_IO_PROG -c "fiemap" "$test_file")
> + if [ $? -ne 0 ]; then
> + rm -f "$test_file"
> + _fail "_require_inplace_writes second fiemap call failed"
> + fi
> +
> + rm -f "$test_file"
> +
> + # If the filesystem supports inplace writes, then the extent mapping is
> + # the same before and after overwriting.
> + if [ "${fiemap_after}" != "${fiemap_before}" ]; then
> + _notrun "inplace writes not supported"
> + fi
> +}
> +
> ################################################################################
> # make sure this script returns success
> /bin/true
> diff --git a/tests/generic/211 b/tests/generic/211
> index e87d1e01..ed426db2 100755
> --- a/tests/generic/211
> +++ b/tests/generic/211
> @@ -27,6 +27,7 @@ fs_size=$(_small_fs_size_mb 512)
> _try_scratch_mkfs_sized $((fs_size * 1024 * 1024)) >>$seqres.full 2>&1 || \
> _fail "mkfs failed"
> _scratch_mount
> +_require_inplace_writes "${SCRATCH_MNT}"
>
> touch $SCRATCH_MNT/foobar
>
> --
> 2.47.2
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] generic/211: verify if the filesystem being tested supports in place writes
2025-07-23 12:04 [PATCH] generic/211: verify if the filesystem being tested supports in place writes fdmanana
2025-07-23 14:49 ` Darrick J. Wong
@ 2025-07-24 7:17 ` Christoph Hellwig
2025-07-24 12:05 ` [PATCH v2] " fdmanana
2 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2025-07-24 7:17 UTC (permalink / raw)
To: fdmanana; +Cc: fstests, linux-btrfs, Filipe Manana
On Wed, Jul 23, 2025 at 01:04:06PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> The test currently assumes the filesystem can do in place writes (no
> Copy-On-Write, no allocation of new extents) when overwriting a file.
> While that is the case for most filesystems in most configurations, there
> are exceptions such as zoned xfs where overwriting results in allocating
> new extents for the new data.
>
> So make the test check that in place writes are supported and skip the
> test if they are not supported.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
> common/rc | 59 +++++++++++++++++++++++++++++++++++++++++++++++
> tests/generic/211 | 1 +
> 2 files changed, 60 insertions(+)
>
> diff --git a/common/rc b/common/rc
> index 96578d15..52aade10 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -5873,6 +5873,65 @@ _require_program() {
> _have_program "$1" || _notrun "$tag required"
> }
>
> +# Test that a filesystem can do writes to a file in place (without allocating
> +# new extents, without Copy-On-Write semantics).
> +_require_inplace_writes()
> +{
> + _require_xfs_io_command "fiemap"
> +
> + local target=$1
> + local test_file="${target}/test_inplace_writes"
> + local fiemap_before
> + local fiemap_after
> +
> + if [ -z "$target" ]; then
> + _fail "Usage: _require_inplace_writes <filesystem path>"
> + fi
> +
> + rm -f "$test_file"
> + touch "$test_file"
> +
> + # Set the file to NOCOW mode on btrfs, which must be done while the file
> + # is empty, otherwise it fails.
> + if [ "$FSTYP" == "btrfs" ]; then
> + _require_chattr C
> + $CHATTR_PROG +C "$test_file"
> + fi
Can you factor this into a _force_inplace helper instead of spreading
file systems specific in random helpers (I know we have a few of those,
but we need to get rid of that to make things maintainable..)
> + # If the filesystem supports inplace writes, then the extent mapping is
> + # the same before and after overwriting.
> + if [ "${fiemap_after}" != "${fiemap_before}" ]; then
> + _notrun "inplace writes not supported"
I think in-place would be the more usual spelling instead of inplace.
Otherwise this looks great, thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2] generic/211: verify if the filesystem being tested supports in place writes
2025-07-23 12:04 [PATCH] generic/211: verify if the filesystem being tested supports in place writes fdmanana
2025-07-23 14:49 ` Darrick J. Wong
2025-07-24 7:17 ` Christoph Hellwig
@ 2025-07-24 12:05 ` fdmanana
2025-07-29 7:46 ` Christoph Hellwig
2 siblings, 1 reply; 5+ messages in thread
From: fdmanana @ 2025-07-24 12:05 UTC (permalink / raw)
To: fstests; +Cc: linux-btrfs, Filipe Manana, Darrick J. Wong
From: Filipe Manana <fdmanana@suse.com>
The test currently assumes the filesystem can do in place writes (no
Copy-On-Write, no allocation of new extents) when overwriting a file.
While that is the case for most filesystems in most configurations, there
are exceptions such as zoned xfs where overwriting results in allocating
new extents for the new data.
So make the test check that in place writes are supported and skip the
test if they are not supported.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
V2: Move chattr +C logic into a helper.
Change "inplace" to "in-place" in a message.
common/rc | 71 +++++++++++++++++++++++++++++++++++++++++++++++
tests/generic/211 | 9 ++----
2 files changed, 73 insertions(+), 7 deletions(-)
diff --git a/common/rc b/common/rc
index 96578d15..b9292a78 100644
--- a/common/rc
+++ b/common/rc
@@ -5873,6 +5873,77 @@ _require_program() {
_have_program "$1" || _notrun "$tag required"
}
+_force_inplace_writes()
+{
+ local file=$1
+
+ if [ -z "$file" ]; then
+ _fail "Usage: _force_inplace_writes <file path>"
+ fi
+
+ case "$FSTYP" in
+ "btrfs")
+ # Set the file to NOCOW mode on btrfs, which must be done while
+ # the file is empty, otherwise it fails with -EINVAL.
+ _require_chattr C
+ $CHATTR_PROG +C "$file"
+ ;;
+ esac
+}
+
+# Test that a filesystem can do writes to a file in place (without allocating
+# new extents, without Copy-On-Write semantics).
+_require_inplace_writes()
+{
+ _require_xfs_io_command "fiemap"
+
+ local target=$1
+ local test_file="${target}/test_inplace_writes"
+ local fiemap_before
+ local fiemap_after
+
+ if [ -z "$target" ]; then
+ _fail "Usage: _require_inplace_writes <filesystem path>"
+ fi
+
+ rm -f "$test_file"
+ touch "$test_file"
+ _force_inplace_writes "$test_file"
+
+ $XFS_IO_PROG -c "pwrite 0 128K" -c "fsync" "$test_file" &> /dev/null
+ if [ $? -ne 0 ]; then
+ rm -f "$test_file"
+ _fail "_require_inplace_writes failed to write to test file"
+ fi
+
+ # Grab fiemap output before overwriting.
+ fiemap_before=$($XFS_IO_PROG -c "fiemap" "$test_file")
+ if [ $? -ne 0 ]; then
+ rm -f "$test_file"
+ _fail "_require_inplace_writes first fiemap call failed"
+ fi
+
+ $XFS_IO_PROG -c "pwrite 0 128K" -c "fsync" "$test_file" &> /dev/null
+ if [ $? -ne 0 ]; then
+ rm -f "$test_file"
+ _fail "_require_inplace_writes failed to overwrite test file"
+ fi
+
+ fiemap_after=$($XFS_IO_PROG -c "fiemap" "$test_file")
+ if [ $? -ne 0 ]; then
+ rm -f "$test_file"
+ _fail "_require_inplace_writes second fiemap call failed"
+ fi
+
+ rm -f "$test_file"
+
+ # If the filesystem supports inplace writes, then the extent mapping is
+ # the same before and after overwriting.
+ if [ "${fiemap_after}" != "${fiemap_before}" ]; then
+ _notrun "in-place writes not supported"
+ fi
+}
+
################################################################################
# make sure this script returns success
/bin/true
diff --git a/tests/generic/211 b/tests/generic/211
index e87d1e01..6eda1608 100755
--- a/tests/generic/211
+++ b/tests/generic/211
@@ -27,15 +27,10 @@ fs_size=$(_small_fs_size_mb 512)
_try_scratch_mkfs_sized $((fs_size * 1024 * 1024)) >>$seqres.full 2>&1 || \
_fail "mkfs failed"
_scratch_mount
+_require_inplace_writes $SCRATCH_MNT
touch $SCRATCH_MNT/foobar
-
-# Set the file to NOCOW mode on btrfs, which must be done while the file is
-# empty, otherwise it fails.
-if [ $FSTYP == "btrfs" ]; then
- _require_chattr C
- $CHATTR_PROG +C $SCRATCH_MNT/foobar
-fi
+_force_inplace_writes $SCRATCH_MNT/foobar
# Add initial data to the file we will later overwrite with mmap.
$XFS_IO_PROG -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foobar | _filter_xfs_io
--
2.47.2
^ permalink raw reply related [flat|nested] 5+ messages in thread