zio_flush: propagate flush errors to the ZIL

[robn]

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

Since the beginning, ZFS' "flush" operation has always ignored errors¹. Write errors are captured and dealt with, but if a write succeeds but the subsequent flush fails, the operation as a whole will appear to succeed².

In the end-of-transaction uberblock+label write+flush ceremony, it's very difficult for this situation to occur. Since all devices are written to, typically the first write will succeed, the first flush will fail unobserved, but then the second write will fail, and the entire transaction is aborted. It's difficult to imagine a real-world scenario where all the writes in that sequence could succeed even as the flushes are failing (understanding that the OS is still seeing hardware problems and taking devices offline).

In the ZIL however, it's another story. Since only the write response is checked, if that write succeeds but the flush then fails, the ZIL will believe that it succeeds, and zil_commit() (and thus fsync()) will return success rather than the "correct" behaviour of falling back into txg_wait_synced()³.

Description

This commit fixes this by adding a simple flag to zio_flush() to indicate whether or not the caller wants to receive flush errors. This flag is enabled for ZIL calls. The existing zio chaining inside the ZIL and the flush handler zil_lwb_flush_vdevs_done() already has all the necessary support to properly handle a flush failure and fail the entire zio chain. This causes zil_commit() to correctly fall back to txg_wait_synced() rather than returning success prematurely.

How Has This Been Tested?

A test case is included. Without the propagate change, it fails. With it, success. Note that the test requires the scsi_debug module, which most Github CI does not have available. It also requires the regression fix in #16258 to succeed, though this is not required for the fix proper, just the test.

Full ZTS run has successfully run to completion with this change in place.

This change has been in production at the customer site where it was noticed for several months. The triggering condition (partial backplane failure, pool suspension and power cycle by operator) no longer loses writes that fsync() has succeeded for.

Notes

Performance regression

I believe this change to be correct, but it throws up another latent issue in OpenZFS. zio_flush() is called on a top-level vdev. This creates one child flush operation per leaf device, all linked to the parent. As normal with zios, if any of the children fail, the parent fails. If a leaf device is faulted or degraded, then the entire flush operation fails, and the ZIL always falls back to a full transaction wait. This is still the correct behaviour, but severely degrades performance even if the vdev has full redundancy.

I have a followup PR #16375 resolves this issue, but it's not trivial so I wanted it reviewed separately. This PR is still complete and reviewable as-is, but I suggest not merging it until they can both be merged together.

Other flush operations

I've made no changes to the other flush calls - those will still not propagate errors. Each call needs analysis to understand the possibility and impact of a successful write with a subsequent flush failure, and what the right behaviour would be in this case.

As noted, the end-of-transaction write+flush is vanishingly unlikely to present a problem in practice.

Further reading

I presented this work at AsiaBSDCon 2024. Paper, slides and other notes available at: https://despairlabs.com/presentations/openzfs-fsync/

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

Footnotes

1. The ZFS birth commit (illumos/illumos-gate@fa9e4066f0) had support for flushing devices with write caches with the DKIOCFLUSHWRITECACHE ioctl. No errors are checked. The comment in zil_flush_vdevs() from from the time shows the thinking. ↩

2. It's not entirely clear from the code history why this was acceptable for devices that do have write caches. Our best guess is that this was an oversight: between the combination of hardware, pool topology and application behaviour required to hit this, it basically didn't come up. ↩

3. Somewhat frustratingly, zil.c contains comments describing this exact behaviour, and further discussion in #12443 (September 2021). It appears that those involved saw the potential, but were looking at a different problem and so didn't have the context to recognise it for what it was. ↩

[amotin]

I still don't see the latest update addressing my comment from chat, so I'll leave it here to not forget: Make sure this will not propagate ENOTSUP errors. They should happen only once (or few times if done in parallel) before the flushes are disabled for later calls, but even that/those first time(s) we do not want them to be considered an error.

[robn]

I still don't see the latest update addressing my comment from chat, so I'll leave it here to not forget: Make sure this will not propagate ENOTSUP errors. They should happen only once (or few times if done in parallel) before the flushes are disabled for later calls, but even that/those first time(s) we do not want them to be considered an error.

@amotin thank you for the reminder! I had written the patch, but was having a lot of trouble testing it due to #14872, and then I went down that rabbit hole, and another and another, and forgot everything.

I've pushed the patch on this PR just to have it out there. I have fully self-reviewed it and have not heavily tested it yet, so it may not be right yet. I will try very hard to get it on the test rig today.

[tonyhutter]

I have a followup PR coming shortly (within next 48hrs) that resolves this issue, but it's not trivial so I wanted it reviewed separately. This PR is still complete and reviewable as-is, but I suggest not merging it until they can both be merged together.

Maybe I missed it, but can you add a link to the follow-up PR?

[tonyhutter]

Other than a few very minor things I don't see any issues. Can you rebase on master to pull in the latest ZTS fixes?

[robn]

Maybe I missed it, but can you add a link to the follow-up PR?

@tonyhutter sorry, not posted yet. During final testing it kept running into #14872, and because both appeared ZIL-adjacent I wasn't willing to just ignore it, but also zvol work is not quite in my budget, so it took a while to get through it. As it happens, I do now understand #14872 (and related), finished a possible fix last night and am doing the final ZTS runs now, so hopefully I can post that later today, and then get back to this.

Hi, I'm selling these fine leather yak-hair jackets.

[tonyhutter]

@robn I'm seeing some of the same failures on many of the builders:

    FAIL mmp/mmp_active_import (expected PASS)
    FAIL mmp/mmp_on_zdb (expected PASS)
    FAIL pool_checkpoint/checkpoint_zdb (expected PASS)
    FAIL pool_checkpoint/checkpoint_zhack_feat (expected PASS)
    FAIL removal/removal_condense_export (expected PASS)

Can you take a look and let us know they're related to this PR or not?

[robn]

Can you take a look and let us know they're related to this PR or not?

@tonyhutter All these tests, I think, relate to or use pool discovery in some way, and something isn't able to find the pool properly. checkpoint_zdb shows it most concisely:

04:29:20.37 Dataset testpool_CHECKPOINTED_UNIVERSE/testfs1 [ZPL], ID 140, cr_txg 10, 25K, 9 objects
04:29:20.37 SUCCESS: eval zdb -k testpool | grep "Dataset testpool_CHECKPOINTED_UNIVERSE/testfs1"
04:29:20.40 SUCCESS: zpool export testpool
04:29:20.43 zdb: can't open 'testpool': Invalid argument
04:29:20.43 
04:29:20.43 ZFS_DBGMSG(zdb) START:
04:29:20.43 ZFS_DBGMSG(zdb) END
04:29:20.43 ERROR: eval zdb -e testpool | grep "Checkpointed uberblock found" exited 1

Given they're all in the same environment, my guess would be something about zpool.cache not being written or preserved properly, or something about block devices not being visible or enumerated or something.

Those tests are passing fine here, and I can't really imagine any way that this PR could affect them in a way that wouldn't show up elsewhere.

I'll have a think about better output from zdb in these kind of cases (it frustrates me in real world use), but yeah, don't think its part of this.

[tonyhutter]

@robn we've had some important ZTS fixes come in recently. Would you mind re-basing on master?

[robn]

Rebased to master.

I believe its correct, but as noted, results in an unnecessary ZIL performance regression when the pool is degraded but still operational. #16375 was my intended fix for this, and I think is good for what it is. However, I've been doing more work on flush response in spa_sync() and it needs a whole different mechanism (for reasons that I won't go into here). So I want to wait until that is fleshed out, then I can revisit both PRs to see if they can be re-expressed in terms of whatever I come up with, or if we will need both mechanisms.