Add zfs_vdev_direct_read_verify tunable

[mkhllr]

Motivation and Context

Applications that issue concurrent O_DIRECT reads and recycle their I/O
buffers across in-flight requests (e.g. QEMU's block layer) trip the Direct I/O
read checksum verify: the buffer is overwritten by another request before the
post-read verify of the previous one runs. This is not data corruption —
the on-disk block is correct and ZFS returns the right data via a buffered
re-read — but it produces a flood of spurious dio_verify_rd zevents, and the
verify-and-refallback storm can stall the workload under load. See #18610 for a
reproduction and analysis.

Unlike the write case, read verification provides no data-integrity guarantee
(the on-disk block is already correct); it only detects the caller mutating its
own buffer mid-flight. As discussed in #18610, @behlendorf agreed that a knob
modeled on zfs_vdev_direct_write_verify was the right approach.

Description

Add a zfs_vdev_direct_read_verify module parameter, mirroring
zfs_vdev_direct_write_verify, so Direct I/O read verification can be disabled
for workloads that legitimately recycle buffers. The default of 1 preserves
the current behavior.

The DIO read verify is performed per top-level vdev type, so the tunable is
honored in each path: zio_checksum_verify() (disk/file), raidz_checksum_verify()
(raidz and draid), and the indirect (device removal) path. Mirror vdevs inherit
the behavior from their children.

How Has This Been Tested?

New ZTS test dio_read_verify_tunable exercises both settings (0 and 1) across
the stripe, mirror, raidz, and draid vdev types, asserting that =0 produces no
dio_verify_rd events while =1 reports them, with the pool error-free in both
cases. Passing locally; cstyle clean.

Types of changes

• New feature (non-breaking change which adds functionality)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All commit messages are properly formatted and contain Signed-off-by.

Closes #18610

[behlendorf]

We can tighten up this check. I'd suggest pushing it in to zio_checksum_error() and restricting it to failed Direct IO reads using ABDs constructed from user pages. Everything else we still should report checksum errors for. Something like this (untested):

diff --git a/module/zfs/zio_checksum.c b/module/zfs/zio_checksum.c
index 1d0646a611..96e2395864 100644
--- a/module/zfs/zio_checksum.c
+++ b/module/zfs/zio_checksum.c
@@ -585,6 +585,11 @@ zio_checksum_error(zio_t *zio, zio_bad_cksum_t *info)
                        info->zbc_injected = 1;
        }
 
+       if (error == ECKSUM && (zio->io_flags & ZIO_FLAG_DIO_READ) &&
+           abd_is_from_pages(data) && zfs_vdev_direct_read_verify == 0) {
+               error = 0;
+       }
+
        return (error);
 }
 

[behlendorf]

cstyle: 80 columns limit.

[behlendorf]

Should be added alphabetically, so let's put this zfs_vdev_direct_read_verify before zfs_vdev_direct_write_verify.

[mkhllr]

Thanks for the review — all three addressed:

• Reworked the check as you suggested: moved it into zio_checksum_error() and gated on abd_is_from_pages(data), so it now covers every vdev type in one place and only suppresses the error for Direct I/O reads served from user pages — genuine checksum errors (and any non-user-page read) are still reported. Dropped the per-vdev gating in zio_checksum_verify() / raidz_checksum_verify() / the indirect path.
• Reordered the man4/zfs.4 entry alphabetically (before zfs_vdev_direct_write_verify).
• Fixed the common.run line over 80 columns.

The dio_read_verify_tunable ZTS test still passes with the new approach across stripe, mirror, raidz, and draid (both =0 → no dio_verify_rd and =1 → reported). Force-pushed.

[amotin]

I feel it is too much. As I understand, you are effectively ignoring all ZFS checksums on reads if the I/O is Direct. It cover not only the case of buffer corruption by application, but also valid checksum errors from disk. I was going to say that it allow RAIDZ to do recovery writes, but in fact it may not even notice that something is wrong to issue recovery, but instead return garbage to the application.

[mkhllr]

Thanks @amotin — fair, and I don't want to trade away real corruption detection on Direct reads. You're right that at zio_checksum_error() the suppression can't distinguish an application recycling its buffer from a genuine bad block: with the knob off, ZIO_POST_DIO_CHKSUM_ERR is never set, the buffered re-read in dmu_direct.c never fires, and RAIDZ/mirror never get the chance to self-heal.

Two ways forward, your call:

1. Withdraw this PR. Rate limit Direct I/O verify zevents #18795 already rate-limits the dio_verify_rd/dio_verify_wr event storm — the concrete Spurious dio_verify_rd on concurrent DIO reads of lz4-compressed records (read-path only, non-corrupting) #18610 symptom — with the integrity path fully intact. If the event flood was the real problem, that covers it, and this knob may not be worth carrying.
2. Rework to preserve integrity. Keep the buffered re-read (so genuine errors are still detected and repaired) and have the knob affect only the reporting of the user-page verify failures, not error handling. I'll be upfront that this overlaps Rate limit Direct I/O verify zevents #18795 fairly heavily, so it may add little on top of it.

My inclination is (1), since #18795 addresses the actual pain without the downside you flagged — but I'm happy to do (2) if you'd rather keep an explicit knob. Which do you prefer?

[amotin]

Yeah, I'd close this unless you find something much better. Tunable to ignore errors sounds wrong. I generally don't like tunables.

[mkhllr]

Makes sense — closing this. Thanks for the reviews, @amotin and @behlendorf. #18795 handles the event storm (the concrete #18610 symptom) without the integrity tradeoff, so I'll let that carry the fix.

[behlendorf]

Yeah, agreed this is just too heavy handed. I'm glad we were at least able to mitigate the symptoms with #18795.