Re: [PATCH 1/1] xfs: add test for ENOSR/ENOSPC on DIO into delalloc extents

[Brian Foster <bfoster@redhat.com>]

On Fri, Oct 16, 2026 at 11:16:25AM +0200, Lukas Herbolt wrote:
>   Test if a DIO extending write into a speculative delalloc region on a
>   fragmented XFS filesystem does not trigger a dirty transaction cancel
>   (-ENOSR) or return a spurious -ENOSPC.
> 
>   With allocsize=128MB, a buffered write creates a large delalloc extent.
>   A subsequent DIO write at i_size lands in the speculative prealloc
>   region. On a fragmented filesystem, xfs_bmapi_write converts the
>   delalloc starting from offset 0 but the allocation is too short to
>   reach the DIO offset, causing either -ENOSR (dirty transaction cancel
>   and filesystem shutdown) or -ENOSPC (wrong error from xfs_bmapi_write).
> 
>   Assisted by LLM.
> 
>   Fixed by kernel commits:
>     2a9b99d45be0 xfs: pass the actual offset and len to xfs_bmapi_allocate
>     21255afdd729 xfs: do not allocate the entire delalloc extent
>     6773da870ab8 xfs: fix error returns from xfs_bmapi_write
> 
> Co-developed-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Lukas Herbolt <lherbolt@redhat.com>
> ---
>  .gitignore                 |   2 +
>  src/Makefile               |   3 +-
>  src/buffer_direct_io.c     | 154 ++++++++++++++++++++++++++++++++
>  src/fragment_fs.c          | 101 +++++++++++++++++++++
>  src/mix_buffer_direct_io.c | 175 +++++++++++++++++++++++++++++++++++++
>  tests/xfs/842              |  63 +++++++++++++
>  tests/xfs/842.out          |   2 +
>  7 files changed, 499 insertions(+), 1 deletion(-)
>  create mode 100644 src/buffer_direct_io.c
>  create mode 100644 src/fragment_fs.c
>  create mode 100644 src/mix_buffer_direct_io.c
>  create mode 100755 tests/xfs/842
>  create mode 100644 tests/xfs/842.out
> 
...

Just a few higher level comments..

> diff --git a/src/buffer_direct_io.c b/src/buffer_direct_io.c
> new file mode 100644
> index 000000000000..01b19994a2e2
> --- /dev/null
> +++ b/src/buffer_direct_io.c
...
> diff --git a/src/fragment_fs.c b/src/fragment_fs.c
> new file mode 100644
> index 000000000000..cee4edef73ff
> --- /dev/null
> +++ b/src/fragment_fs.c
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2026 Eric Sandeen, Lukas Herbolt
> + */
> +
> +/*
> + * Fragment filesystem by creating fixed-size files and then deleting
> + * every n-th one.
> + *
> + * Usage:
> + *	fragment_fs <directory> <prefix> <file size> <delete every n-th file>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <sys/statvfs.h>
> +
> +static int fragment_fs(const char *dir, const char *prefix,
> +		       off_t size, int delete_nth)
> +{
> +	char path[4096];
> +	int i, fd;
> +	int created = 0;
> +
> +	fprintf(stderr, "Fragmenting filesystem (filling to ENOSPC)...\n");
> +
> +	for (i = 0; ; i++) {
> +		snprintf(path, sizeof(path), "%s/%s_%06d", dir, prefix, i);
> +		fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
> +		if (fd < 0) {
> +			if (errno == ENOSPC)
> +				break;
> +			perror("open frag file");
> +			return -1;
> +		}
> +		if (fallocate(fd, 0, 0, size) < 0) {
> +			close(fd);
> +			unlink(path);
> +			if (errno == ENOSPC)
> +				break;
> +			perror("fallocate frag file");
> +			return -1;
> +		}
> +		close(fd);
> +		created++;
> +	}
> +	fprintf(stderr, "Created %d files\n", created);
> +
> +	fprintf(stderr, "Deleting every %d-th file...\n", delete_nth);
> +	for (i = 0; i < created; i += delete_nth) {
> +		snprintf(path, sizeof(path), "%s/%s_%06d", dir, prefix, i);
> +		unlink(path);
> +	}
> +	sync();
> +
> +	{
> +		struct statvfs st;
> +		if (statvfs(dir, &st) == 0) {
> +			unsigned long free_mb = (st.f_bfree * st.f_bsize)
> +						/ (1024 * 1024);
> +			fprintf(stderr,
> +				"%d files remain, ~%lu MB free\n",
> +				created - created / delete_nth, free_mb);
> +		}
> +	}
> +
> +	return 0;
> +}
> +

Any chance we can reuse existing code in fstests for this sort of free
space fragmentation? For example, see the punch-alternating tool and/or
something like generic/186 (if useful, maybe that code can be lifted
into a common helper).

...
> diff --git a/src/mix_buffer_direct_io.c b/src/mix_buffer_direct_io.c
> new file mode 100644
> index 000000000000..0af227a7c9f0
> --- /dev/null
> +++ b/src/mix_buffer_direct_io.c
> diff --git a/tests/xfs/842 b/tests/xfs/842
...

It looks like we have two mix I/O helper programs here, where this one
appears unused..?

Also both appear to have some minor whitespace damage in the form of
trailing spaces at the end of some lines, so it might be worth fixing
that up as well.

> new file mode 100755
> index 000000000000..b2b2f7c493e7
> --- /dev/null
> +++ b/tests/xfs/842
> @@ -0,0 +1,63 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2026 Lukas Herbolt.  All Rights Reserved.
> +#
> +# FS QA Test 842
> +#
> +# Regression test for XFS -ENOSR dirty transaction cancel during DIO
> +# write into a speculative delalloc region on a fragmented filesystem.
> +# With allocsize=128MB, a buffered write creates a large delalloc extent.
> +# A subsequent DIO extending write at i_size triggers xfs_bmapi_write to
> +# convert the delalloc from offset 0; on a fragmented FS the allocation
> +# is too short to reach the DIO offset, returning -ENOSR and cancelling
> +# a dirty transaction, which shuts down the filesystem.
> +#
> +. ./common/preamble
> +_begin_fstest auto enospc
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -r -f $tmp.*
> +}
> +
> +_require_test

Looks like we don't use the test dev in this test.

> +_require_scratch
> +_require_test_program "fragment_fs"
> +_require_test_program "buffer_direct_io"
> +
> +_scratch_mkfs_sized $((1<<30)) > "$seqres.full" 2>&1
> +_scratch_mount -o allocsize=131072k >> "$seqres.full" 2>&1
> +
> +src/fragment_fs $SCRATCH_MNT frag 4096 2 >> "$seqres.full" 2>&1
> +src/buffer_direct_io $SCRATCH_MNT >> "$seqres.full" 2>&1
> +RET=$?
> +
> +_dmesg_since_test_start | grep -q "detected at xfs_trans_cancel"
> +DMESG_CHECK=$?
> +
> +if [ $RET -eq 5 -a $DMESG_CHECK -eq 0 ]; then
> +	_fixed_by_kernel_commit 2a9b99d45be0 \
> +		"xfs: pass the actual offset and len to xfs_bmapi_allocate"
> +	_fixed_by_kernel_commit 21255afdd729 \
> +		"xfs: do not allocate the entire delalloc extent"
> +	_fail "ENOSR dirty transaction cancel detected (ret=$RET)"
> +
> +elif [ $RET -eq 28 ]; then
> +	_fixed_by_kernel_commit 2a9b99d45be0 \
> +		"xfs: pass the actual offset and len to xfs_bmapi_allocate"
> +	_fixed_by_kernel_commit 21255afdd729 \
> +		"xfs: do not allocate the entire delalloc extent"
> +	_fixed_by_kernel_commit 6773da870ab8 \
> +		"xfs: fix error returns from xfs_bmapi_write"
> +	_fail "ENOSPC returned from xfs_bmapi_write (ret=$RET)"
> +
> +elif [ $RET -eq 0 ]; then
> +	echo "Silence is golden"
> +else
> +	_fail "Unknown error: src/buffer_direct_io error code $RET"
> +fi

A brief comment above this might be helpful. I'm also curious if it can
be simplified a little. For example, I'm not sure we need to explicitly
_fail when we can rely on golden output and/or the dmesg checking that
happens by default. Also it looks like the commit hints are duplicate
except for the -ENOSPC specific one. So perhaps we could do something
like the following?

echo "Silence is golden"

# words about the error codes, i.e. EIO vs ENOSPC?
if [ $RET -neq 0 ]; then
	_fixed_by_kernel_commit <X>
	_fixed_by_kernel_commit <Y>
	[ $RET -eq 28 ] && _fixed_by_kernel_commit <Z>
	echo "... error code $RET"
fi

Brian

> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/842.out b/tests/xfs/842.out
> new file mode 100644
> index 000000000000..643d05289a33
> --- /dev/null
> +++ b/tests/xfs/842.out
> @@ -0,0 +1,2 @@
> +QA output created by 842
> +Silence is golden
> -- 
> 2.56.0
> 
>