zio_flush: propagate flush errors to the ZIL #16314
Conversation
There was a problem hiding this comment.
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. |
Maybe I missed it, but can you add a link to the follow-up PR? |
|
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? |
@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 |
|
@robn I'm seeing some of the same failures on many of the builders: 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. Given they're all in the same environment, my guess would be something about 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. |
robn
force-pushed
the
zil-flush-fallback
branch
2 times, most recently
from
a9033f6
to
6f08143
Compare
|
@robn we've had some important ZTS fixes come in recently. Would you mind re-basing on master? |
If fsync() (zil_commit()) writes successfully, but then the flush fails, fsync() should not return success, but instead should fall into a full transaction wait. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <[email protected]>
robn
force-pushed
the
zil-flush-fallback
branch
from
6f08143
to
30c3a61
Compare
Since the beginning, ZFS' "flush" operation has always ignored errors[1]. 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[2]. 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()[3]. 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 correct fall back to txg_wait_synced() rather than returning success prematurely. 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: /* * Wait for all the flushes to complete. Not all devices actually * support the DKIOCFLUSHWRITECACHE ioctl, so it's OK if it fails. */ 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 openzfs#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. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <[email protected]>
The first time a device returns ENOTSUP in repsonse to a flush request, we set vdev_nowritecache so we don't issue flushes in the future and instead just pretend the succeeded. However, we still return an error for the initial flush, even though we just decided such errors are meaningless! So, when setting vdev_nowritecache in response to a flush error, also reset the error code to assume success. Along the way, it seems there's no good reason for vdev_disk & vdev_geom to explicitly detect no support for flush and set vdev_nowritecache; just letting the error through to zio_vdev_io_assess() will cause it all to fall out nicely. So remove those checks. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <[email protected]>
robn
force-pushed
the
zil-flush-fallback
branch
from
30c3a61
to
2331d19
Compare
|
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 |
[Sponsors: Klara, Inc., Wasabi Technology, Inc.]
Motivation and Context
Since the beginning, ZFS' "flush" operation has always ignored errors1. 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 succeed2.
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 thusfsync()) will return success rather than the "correct" behaviour of falling back intotxg_wait_synced()3.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 handlerzil_lwb_flush_vdevs_done()already has all the necessary support to properly handle a flush failure and fail the entire zio chain. This causeszil_commit()to correctly fall back totxg_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_debugmodule, 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
Checklist:
Signed-off-by.Footnotes
The ZFS birth commit (illumos/illumos-gate@fa9e4066f0) had support for flushing devices with write caches with the
DKIOCFLUSHWRITECACHEioctl. No errors are checked. The comment inzil_flush_vdevs()from from the time shows the thinking. ↩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. ↩
Somewhat frustratingly,
zil.ccontains 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. ↩