Decline Direct I/O reads on a file handle after a benign verify failure

A Direct I/O read verifies the block checksum over the caller's buffer
after the read completes, to catch the buffer being modified while the
read is in flight.  When an application recycles its O_DIRECT read
buffers across concurrent requests -- QEMU's block layer does this -- a
queued read can overwrite the buffer before the previous read's verify
runs.  The verify then fails even though the data on disk is correct:
ZFS discards the direct read, re-reads the block through the ARC, and
emits an ereport.fs.zfs.dio_verify_rd.  The returned data is correct and
the pool stays healthy, but under concurrent load the stream of failed
verifies and buffered re-reads is a real cost and can stall the
workload (#18610).

Once a file handle hits one of these benign failures -- a DIO read
verify that failed but whose buffered re-read then succeeded, proving
the on-disk data good and the buffer caller-modified -- decline Direct
I/O for reads on that handle for the rest of its life and route them
through the existing uncached buffered path.  An application that uses a
distinct buffer per request never trips the failure and keeps zero-copy
Direct I/O; one that recycles buffers stops paying the verify-failure
and re-read cost after the first occurrence.

zfs_read reports the benign failure outward with a new
UIO_DIO_CKSUM_RETRIED uio_extflg bit, set only when the buffered re-read
returned success so a genuine on-disk error is never flagged and still
self-heals.  The Linux zpl layer records it in a small
file->private_data struct and, on later reads, sets UIO_DIO_DENY so
zfs_setup_direct declines Direct I/O and falls through to the uncached
path.  This also covers direct=always, since the decline is checked
after O_DIRECT is forced on.  The checksum verify and all mirror and
raidz self-heal are untouched; a genuine on-disk error is still
detected and repaired.

This is read-side only -- a Direct I/O write verify failure already
returns EIO to the application.  FreeBSD defines the new bits but does
not yet drive them, as VOP_READ has no open file handle to key the
per-handle state on.

A new ZTS test, functional/direct/dio_read_verify_decline, reads a file
on a single O_DIRECT handle while a second thread rewrites the shared
buffer and checks that the direct read count stays bounded once the
handle declines Direct I/O.  It is Linux only, matching where the bits
are driven.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Michael Heller <[email protected]>
Closes #18610
Closes #18844

include/os/freebsd/spl/sys/uio.h

@@ -42,6 +42,13 @@
  */
 #define	UIO_DIRECT	0x0001	/* Direct I/O requset */
 #define	UIO_UNCACHED	0x0002	/* Caller will not reuse the data */
+/*
+ * See the Linux spl uio.h for how these are used; the common zfs_read path
+ * sets/checks them, but nothing drives them on FreeBSD yet (VOP_READ has no
+ * open file to key the per-handle decline on).
+ */
+#define	UIO_DIO_DENY		0x0004
+#define	UIO_DIO_CKSUM_RETRIED	0x0008
 
 typedef	struct iovec	iovec_t;
 typedef	enum uio_seg	zfs_uio_seg_t;

include/os/linux/spl/sys/uio.h

@@ -41,6 +41,15 @@
  */
 #define	UIO_DIRECT	0x0001 /* Direct I/O request */
 #define	UIO_UNCACHED	0x0002 /* Caller will not reuse the data */
+/*
+ * UIO_DIO_DENY: the zpl caller declines Direct I/O for this request (e.g. a
+ * file handle that already hit a benign DIO read verify failure).
+ * UIO_DIO_CKSUM_RETRIED: set by zfs_read when a DIO read verify failed but the
+ * buffered re-read succeeded -- a recycled O_DIRECT buffer, not an on-disk
+ * error.
+ */
+#define	UIO_DIO_DENY		0x0004
+#define	UIO_DIO_CKSUM_RETRIED	0x0008
 
 #if defined(HAVE_FAULT_IN_IOV_ITER_READABLE)
 #define	iov_iter_fault_in_readable(a, b)	fault_in_iov_iter_readable(a, b)

module/os/linux/zfs/zpl_file.c

@@ -37,6 +37,31 @@
 #include <linux/filelock.h>
 #endif
 
+/*
+ * Per-open-file state, hung off file->private_data.  Allocated lazily the
+ * first time a Direct I/O read on this handle hits a benign checksum verify
+ * failure -- a recycled O_DIRECT buffer whose buffered re-read then succeeded.
+ * Its presence makes zpl_iter_read route subsequent reads through the uncached
+ * buffered path for the remaining lifetime of the handle, which stops the
+ * verify-failure / re-read storm without disabling the verify itself (so
+ * mirror/raidz self-heal for genuine corruption is unaffected).
+ */
+typedef struct zpl_file_data {
+	boolean_t	zfd_dio_read_declined;
+} zpl_file_data_t;
+
+static void
+zpl_dio_read_decline(struct file *filp)
+{
+	if (atomic_load_ptr(&filp->private_data) != NULL)
+		return;
+
+	zpl_file_data_t *zfd = kmem_zalloc(sizeof (*zfd), KM_SLEEP);
+	zfd->zfd_dio_read_declined = B_TRUE;
+	if (atomic_cas_ptr(&filp->private_data, NULL, zfd) != NULL)
+		kmem_free(zfd, sizeof (*zfd));
+}
+
 /*
  * When using fallocate(2) to preallocate space, inflate the requested
  * capacity check by 10% to account for the required metadata blocks.
@@ -81,6 +106,12 @@ zpl_release(struct inode *ip, struct file *filp)
 	crfree(cr);
 	ASSERT3S(error, <=, 0);
 
+	zpl_file_data_t *zfd = filp->private_data;
+	if (zfd != NULL) {
+		filp->private_data = NULL;
+		kmem_free(zfd, sizeof (*zfd));
+	}
+
 	return (error);
 }
 
@@ -233,6 +264,14 @@ zpl_iter_read(struct kiocb *kiocb, struct iov_iter *to)
 	zfs_uio_iov_iter_init(&uio, to, kiocb->ki_pos, count);
 	uio.uio_extflg |= zfs_uio_flags(kiocb);
 
+	/*
+	 * This handle previously declined Direct I/O after a benign read
+	 * verify failure; keep taking the uncached buffered path.
+	 */
+	zpl_file_data_t *zfd = atomic_load_ptr(&filp->private_data);
+	if (zfd != NULL && zfd->zfd_dio_read_declined)
+		uio.uio_extflg |= UIO_DIO_DENY;
+
 	crhold(cr);
 	cookie = spl_fstrans_mark();
 
@@ -242,6 +281,14 @@ zpl_iter_read(struct kiocb *kiocb, struct iov_iter *to)
 	spl_fstrans_unmark(cookie);
 	crfree(cr);
 
+	/*
+	 * A Direct I/O read verify failed benignly (recycled O_DIRECT buffer)
+	 * and the buffered re-read succeeded; decline Direct I/O reads on this
+	 * handle from here on.
+	 */
+	if (uio.uio_extflg & UIO_DIO_CKSUM_RETRIED)
+		zpl_dio_read_decline(filp);
+
 	if (ret < 0)
 		return (ret);
 

module/zfs/zfs_vnops.c

@@ -273,6 +273,17 @@ zfs_setup_direct(struct znode *zp, zfs_uio_t *uio, zfs_uio_rw_t rw,
 		goto out;
 	}
 
+	/*
+	 * The zpl caller declined Direct I/O for this request (a file handle
+	 * that already hit a benign DIO read verify failure from a recycled
+	 * O_DIRECT buffer).  Fall through to uncached buffered I/O.  Placed
+	 * after the ZFS_DIRECT_ALWAYS handling above so direct=always is
+	 * declined too.  The verify itself is untouched, so mirror and raidz
+	 * self-heal for genuine corruption still runs on the buffered path.
+	 */
+	if (uio->uio_extflg & UIO_DIO_DENY)
+		goto out;
+
 	/*
 	 * For short writes the page mapping of Direct I/O makes no sense.
 	 * Direct them through the ARC as uncached I/O.
@@ -516,8 +527,19 @@ zfs_read(struct znode *zp, zfs_uio_t *uio, int ioflag, cred_t *cr)
 out:
 	zfs_rangelock_exit(lr);
 
-	if (dio_checksum_failure == B_TRUE)
+	if (dio_checksum_failure == B_TRUE) {
 		uio->uio_extflg |= UIO_DIRECT;
+		/*
+		 * The DIO read verify failed but the buffered re-read that
+		 * followed succeeded, so the on-disk data is good and the
+		 * caller mutated its own O_DIRECT buffer in flight.  Report
+		 * this outward so the platform layer can decline Direct I/O
+		 * for this file handle from here on.  A real on-disk error
+		 * would have returned nonzero and is not flagged.
+		 */
+		if (error == 0)
+			uio->uio_extflg |= UIO_DIO_CKSUM_RETRIED;
+	}
 
 	/*
 	 * Cleanup for Direct I/O if requested.

tests/runfiles/linux.run

@@ -117,7 +117,7 @@ tests = ['devices_001_pos', 'devices_002_neg', 'devices_003_pos']
 tags = ['functional', 'devices']
 
 [tests/functional/direct:Linux]
-tests = ['dio_loopback_dev', 'dio_write_verify']
+tests = ['dio_loopback_dev', 'dio_read_verify_decline', 'dio_write_verify']
 tags = ['functional', 'direct']
 
 [tests/functional/events:Linux]

tests/zfs-tests/tests/Makefile.am

@@ -1642,6 +1642,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \
 	functional/direct/dio_property.ksh \
 	functional/direct/dio_random.ksh \
 	functional/direct/dio_read_verify.ksh \
+	functional/direct/dio_read_verify_decline.ksh \
 	functional/direct/dio_recordsize.ksh \
 	functional/direct/dio_unaligned_block.ksh \
 	functional/direct/dio_unaligned_filesize.ksh \

tests/zfs-tests/tests/functional/direct/dio_read_verify_decline.ksh

@@ -0,0 +1,117 @@
+#!/bin/ksh -p
+# SPDX-License-Identifier: CDDL-1.0
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# https://opensource.org/license/CDDL-1.0.
+#
+
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/direct/dio.cfg
+. $STF_SUITE/tests/functional/direct/dio.kshlib
+
+#
+# DESCRIPTION:
+#	Verify that a benign Direct I/O read checksum verify failure declines
+#	Direct I/O for the offending file handle.
+#
+#	When an application modifies its O_DIRECT buffer while a read is in
+#	flight the post-read checksum verify fails even though the data on
+#	disk is good; ZFS discards the direct read, re-reads through the ARC
+#	and emits an ereport.fs.zfs.dio_verify_rd.  An application that
+#	recycles read buffers across concurrent requests (QEMU's block layer
+#	does this) trips the failure on every read, paying the verify and
+#	re-read cost repeatedly.  After the first such benign failure on a
+#	file handle ZFS should decline Direct I/O for reads on that handle and
+#	route them through the buffered path, bounding the cost to the first
+#	occurrence while still returning correct data.
+#
+# STRATEGY:
+#	1. Create a pool and a dataset with compression=off.
+#	2. Write a file using Direct I/O.
+#	3. On a single file handle, repeatedly read the file with O_DIRECT
+#	   while another thread continuously modifies the shared buffer
+#	   (manipulate_user_buffer), forcing read checksum verify failures.
+#	4. Confirm at least one dio_verify_rd event was triggered.
+#	5. Confirm the direct read count is bounded to a small number: the
+#	   handle declined Direct I/O after the first benign failure instead
+#	   of issuing a direct read (and verify failure) for every request.
+#	6. Confirm the pool reports no data errors.
+#
+
+verify_runnable "global"
+
+#
+# The per-handle Direct I/O read decline is implemented in the Linux zpl layer
+# (file->private_data); FreeBSD defines the uio bits but does not drive them,
+# so this behaviour is Linux only.
+#
+
+NUMBLOCKS=500
+BS=$((128 * 1024)) # 128k
+
+# With the fix a single direct read hits the benign failure and the handle
+# then declines Direct I/O, so the direct read count stays at a small handful.
+# Without it every one of the NUMBLOCKS reads is a direct read that fails the
+# verify.  Allow a generous margin for the reads issued before the first
+# failure is seen.
+MAX_DIRECT_READS=10
+
+log_assert "A benign Direct I/O read verify failure declines Direct I/O " \
+    "for the file handle."
+log_onexit dio_cleanup
+
+log_must truncate -s $MINVDEVSIZE $DIO_VDEVS
+
+create_pool $TESTPOOL1 $DIO_VDEVS
+log_must eval "zfs create -o recordsize=128k -o compression=off \
+    $TESTPOOL1/$TESTFS1"
+mntpnt=$(get_prop mountpoint $TESTPOOL1/$TESTFS1)
+
+log_must stride_dd -o "$mntpnt/direct-read.iso" -i /dev/urandom \
+    -b $BS -c $NUMBLOCKS -D
+
+log_must zpool sync $TESTPOOL1
+log_must zpool events -c
+
+prev_dio_rd=$(kstat_pool $TESTPOOL1 iostats.direct_read_count)
+
+#
+# A single file handle, sequential O_DIRECT reads, buffer scribbled by a
+# concurrent thread the whole time.
+#
+log_must manipulate_user_buffer -f "$mntpnt/direct-read.iso" \
+    -n $NUMBLOCKS -b $BS -r
+
+curr_dio_rd=$(kstat_pool $TESTPOOL1 iostats.direct_read_count)
+total_dio_rd=$((curr_dio_rd - prev_dio_rd))
+dio_verify=$(get_zed_dio_verify_events $TESTPOOL1 "rd")
+
+log_note "Direct I/O reads: $total_dio_rd, dio_verify_rd events: $dio_verify"
+
+# The benign failure must actually have been triggered, otherwise the bound
+# below is vacuous.
+if [[ $dio_verify -lt 1 ]]; then
+	log_fail "Expected the benign Direct I/O read verify failure to be " \
+	    "triggered, saw $dio_verify dio_verify_rd events"
+fi
+
+# The handle declined Direct I/O after the first benign failure, so the
+# direct read count is bounded well below NUMBLOCKS ($NUMBLOCKS).
+if [[ $total_dio_rd -gt $MAX_DIRECT_READS ]]; then
+	log_fail "Direct I/O was not declined: $total_dio_rd direct reads " \
+	    "(> $MAX_DIRECT_READS); the file handle kept issuing direct reads"
+fi
+
+log_note "Making sure there are no checksum errors with the ZPool"
+log_must check_pool_status $TESTPOOL1 "errors" "No known data errors"
+
+destroy_pool $TESTPOOL1
+
+log_pass "A benign Direct I/O read verify failure declines Direct I/O " \
+    "for the file handle."