ZIL: Second attempt to reduce scope of zl_issuer_lock. #15122
Conversation
There was a problem hiding this comment.
This looks good to me. The state transition sequence is pretty straightforward to follow and the use of null IOs to maintain that linear-ness is smart. I agree that the issuer lock in particular has a fairly narrow scope.
I have a test bench that I've been using in the past few months to explore the behaviour the ZIL in various "catastrophic" scenarios (eg multiple disks failing). Its been very good at finding locking issues, among other things. I've put this patch through some of the baseline tests which are mostly about hammering 12-wide RAIDz3s with hundreds of concurrent write()s and fsync()s, while doing horrible things to the pool. These tests didn't trigger any locking problems (even when IOs fail, disks fail, pools fail), which granted, is hard to make it happen in 2.1 as well, so I feel confident its at least not worse than 2.1 on that score. Average fsync() latency on these particular workloads is 15-20% improved over 2.1.
Sorry for being vague; I can't really talk about the details of what I'm working on yet. But at least, I feel pretty good that this is doing the right thing, and is quite a bit faster.
|
This pull request fixes consistent, easily reproducible deadlocks I've been experiencing in FreeBSD 14 for the past two months (run this script to reproduce). Please get this merged ASAP. |
|
Thanks @grwilson for finding one more peculiar deadlock scenario. Since this patch allows single zil_lwb_write_issue() to issue several ready LWBs, it may be that after issuing LWB required for the current zcw the thread may block working on following one while holding zcw_lock. The problem arise when the only one available per pool null interrupt taskq is getting blocked by the LWB ZIO completion, waiting for zcw_lock, while it is also needed to drop config lock, wanted by the first zil_lwb_write_issue(). To fix this deadlock I reduced scope of zcw_lock inside zil_commit_waiter_timeout(). It may cost us two more atomics per LWB, but I am going to avoid all this code path together for single-threaded workloads a bit later. |
|
Testing the latest change to make sure it addresses the last deadlock |
| @@ -1041,7 +1061,8 @@ zil_destroy(zilog_t *zilog, boolean_t keep_first) | |||
| while ((lwb = list_remove_head(&zilog->zl_lwb_list)) != NULL) { | |||
| if (lwb->lwb_buf != NULL) | |||
| zio_buf_free(lwb->lwb_buf, lwb->lwb_sz); | |||
| zio_free(zilog->zl_spa, txg, &lwb->lwb_blk); | |||
| if (!BP_IS_HOLE(&lwb->lwb_blk)) | |||
There was a problem hiding this comment.
The original code didn't expect to see a HOLE. Can you add a comment that explains when we would end up having a HOLE on the lwb.
There was a problem hiding this comment.
With the old code every LWB always had a block pointer. New code allows to allocate number of LWBs to fill them in parallel before allocating them block pointers as part of zil_lwb_write_issue(). I am not sure I want to bloat this particular piece of code with this explanation.
module/zfs/zil.c
Outdated
| * first issue to parent IOs before waiting on the lock. | ||
| * The lwb is now ready to be issued, but it can be only if it already | ||
| * got its block pointer allocated or the allocation has failed. | ||
| * Otherwise leave it as-is, relying on some other thread to issue it. |
There was a problem hiding this comment.
So if the lwb_blk is a hole, then who will issue it? It might be good to expand on this comments since this seems like a relevant detail about how holes are issued. I'm assuming this is going to be a thread that is already issuing other lwbs but it would be good to explain that here.
There was a problem hiding this comment.
I've expanded the comment.
The previous patch openzfs#14841 appeared to have significant flaw, causing deadlocks if zl_get_data callback got blocked waiting for TXG sync. I already handled some of such cases in the original patch, but issue openzfs#14982 shown cases that were impossible to solve in that design. This patch fixes the problem by postponing log blocks allocation till the very end, just before the zios issue, leaving nothing blocking after that point to cause deadlocks. Before that point though any sleeps are now allowed, not causing sync thread blockage. This require slightly more complicated lwb state machine to allocate blocks and issue zios in proper order. But with removal of special early issue workarounds the new code is much cleaner now, and should even be more efficient. Since this patch uses null zios between write, I've found that null zios do not wait for logical children ready status in zio_ready(), that makes parent write to proceed prematurely, producing incorrect log blocks. Added ZIO_CHILD_LOGICAL_BIT to zio_wait_for_children() fixes it. Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc.
behlendorf
added
Status: Accepted
The previous patch openzfs#14841 appeared to have significant flaw, causing deadlocks if zl_get_data callback got blocked waiting for TXG sync. I already handled some of such cases in the original patch, but issue openzfs#14982 shown cases that were impossible to solve in that design. This patch fixes the problem by postponing log blocks allocation till the very end, just before the zios issue, leaving nothing blocking after that point to cause deadlocks. Before that point though any sleeps are now allowed, not causing sync thread blockage. This require slightly more complicated lwb state machine to allocate blocks and issue zios in proper order. But with removal of special early issue workarounds the new code is much cleaner now, and should even be more efficient. Since this patch uses null zios between write, I've found that null zios do not wait for logical children ready status in zio_ready(), that makes parent write to proceed prematurely, producing incorrect log blocks. Added ZIO_CHILD_LOGICAL_BIT to zio_wait_for_children() fixes it. Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Reviewed-by: George Wilson <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc. Closes openzfs#15122
The previous patch #14841 appeared to have significant flaw, causing deadlocks if zl_get_data callback got blocked waiting for TXG sync. I already handled some of such cases in the original patch, but issue #14982 shown cases that were impossible to solve in that design. This patch fixes the problem by postponing log blocks allocation till the very end, just before the zios issue, leaving nothing blocking after that point to cause deadlocks. Before that point though any sleeps are now allowed, not causing sync thread blockage. This require slightly more complicated lwb state machine to allocate blocks and issue zios in proper order. But with removal of special early issue workarounds the new code is much cleaner now, and should even be more efficient. Since this patch uses null zios between write, I've found that null zios do not wait for logical children ready status in zio_ready(), that makes parent write to proceed prematurely, producing incorrect log blocks. Added ZIO_CHILD_LOGICAL_BIT to zio_wait_for_children() fixes it. Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Reviewed-by: George Wilson <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc. Closes #15122
The previous patch openzfs#14841 appeared to have significant flaw, causing deadlocks if zl_get_data callback got blocked waiting for TXG sync. I already handled some of such cases in the original patch, but issue openzfs#14982 shown cases that were impossible to solve in that design. This patch fixes the problem by postponing log blocks allocation till the very end, just before the zios issue, leaving nothing blocking after that point to cause deadlocks. Before that point though any sleeps are now allowed, not causing sync thread blockage. This require slightly more complicated lwb state machine to allocate blocks and issue zios in proper order. But with removal of special early issue workarounds the new code is much cleaner now, and should even be more efficient. Since this patch uses null zios between write, I've found that null zios do not wait for logical children ready status in zio_ready(), that makes parent write to proceed prematurely, producing incorrect log blocks. Added ZIO_CHILD_LOGICAL_BIT to zio_wait_for_children() fixes it. Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Reviewed-by: George Wilson <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc. Closes openzfs#15122
The previous patch #14841 appeared to have significant flaw, causing deadlocks if zl_get_data callback got blocked waiting for TXG sync. I already handled some of such cases in the original patch, but issue #14982 shown cases that were impossible to solve in that design.
This patch fixes the problem by postponing log blocks allocation till the very end, just before the zios issue, leaving nothing blocking after that point to cause deadlocks. Before that point though any sleeps are now allowed, not causing sync thread blockage. This require slightly more complicated lwb state machine to allocate blocks and issue zios in proper order. But with removal of special early issue workarounds the new code is much cleaner now, and should even be more efficient.
Since this patch uses null zios between write, I've found that null zios do not wait for logical children ready status in io_ready(), that makes parent write to proceed prematurely, producing incorrect log blocks. Added ZIO_CHILD_LOGICAL_BIT to zio_wait_for_children() fixes it.
How Has This Been Tested?
The patch successfully survives heavily parallel synchronous writes of VMware vMotion over iSCSI.
Types of changes
Checklist:
Signed-off-by.