Repository navigation
SuspendLayerUtils: only suspend listed sub-root layers - #541
Open
bernardgut wants to merge 1 commit into
Open
bernardgut wants to merge 1 commit into
bernardgut wants to merge 1 commit into
Conversation
setShouldSuspendStateRec() set shouldSuspendIo on every layer object it visited and recursed into every child, so every layer of a resource was flagged (listed children even twice) and the LAYERS_TO_SUSPEND_SUB_ROOT filter (WRITECACHE, CACHE, DRBD) never took effect. This has been the case since SuspendLayerUtils was introduced in 7047c32 / v1.33.0; before, only the root layer was flagged. The satellite only suspends a flagged layer if its volumes exist. Since d5b4701 / v1.34.0 the LUKS layer reports its volumes as existing, so snapshots and clones also make the satellite run "dmsetup suspend" on the dm-crypt device of a LUKS layer below DRBD, on every node whose own DRBD suspend succeeded. The suspended LUKS device behind the resume hang fixed in e10406c has the same cause. With DRBD >= 9.3, where an admin suspend-io freezes the mounted filesystem, this stalls snapshots whenever the Primary replicates to a diskful LUKS replica on another node (two diskful replicas, or a diskless Primary): the freeze writes on the Primary need the protocol C ack of that peer, and once the peer has suspended its dm-crypt device below DRBD, drbdsetup suspend-io on the Primary blocks until ko-count ejects the peer or the dm-crypt device is resumed. Flag the topmost layer unconditionally, as documented, and lower layers only if their kind is in the given set, while still descending through unlisted layers. Resume still clears every layer, now exactly once. Fixes: LINBIT#540 Authored-By: Bernard Gütermann <bernard.gutermann@sekops.ch>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #540 (see also #524).
Problem
On DRBD ≥ 9.3.0, snapshots and clones stall when the Primary replicates to a diskful LUKS replica on
another node and a filesystem is mounted. That covers DRBD,LUKS,STORAGE with two diskful replicas, and
a diskless Primary with diskful LUKS replicas.
drbdadm suspend-iofails with "did not terminate within 5 seconds" (exit 20).Failed to suspend IO … on layer DrbdLayeronly when DRBD's ko-count dropsthe peer, about 42 s with the defaults.
Production evidence is in #540; the test-cluster reproduction is in the follow-up comment there.
Root cause
The dead filter.
SuspendLayerUtils.setShouldSuspendStateRec(
controller/src/main/java/com/linbit/linstor/layer/utils/SuspendLayerUtils.java:70-86, identical onmaster and v1.35.2) sets
shouldSuspendIoon every layer it visits (line 77) and recurses into everychild (line 84).
LAYERS_TO_SUSPEND_SUB_ROOTcheck at lines 80-83 therefore never takes effect: every layer isflagged, and listed children twice.
Why it started to matter. Until d5b4701 (v1.34.0) the extra flags did nothing.
exists() == false, soSuspendManagerskipped them (SuspendManager.java:205).STORAGE, NVME and BCACHE do not support suspend.
LuksLayersetexists(true)(LuksLayer.java:405).SuspendManager.java:105-121) runsdmsetup suspendon the dm-cryptdevice below DRBD on every node whose own
drbdadm suspend-iosucceeded.How the stall happens. DRBD ≥ 9.3.0
suspend-iocallsbdev_freeze()first, then setssusp_user, then waits for the device to drain (drbd_nl.c:6608-6651@ drbd-9.3.4; LINBIT/drbd@27ca01bf67).Together:
Under protocol C each completes only after the peer's
P_WRITE_ACK.request it suspends its dm-crypt device.
The Primary's
drbdsetupstays inbdev_freezeuntil ko-count or an external resume.drbdsetupholds the inherited pipes, soExtCmd.syncProcess()keeps the Primary's DeviceManager blocked.LINSTOR side ends the wait.
Change
What changes. Flag the topmost layer unconditionally, as documented, and lower layers only if they
are in the given set. The recursion still descends through unlisted layers. Resume still clears every
layer (
ALL_LAYERS), now exactly once.Scope of the change. Only the controller changes; the satellite and the wire format are untouched.
This was validated with stock 1.35.2 satellites and a 1.35.2 controller built from the same change.
Difference from the #540 sketch. The sketch used
getParent() == null. HeresetShouldSuspendStateRecflags the root, andsetShouldSuspendStateOfChildrenRecwalks the childrenand keeps the existing per-child filter. The behaviour is the same, and it does not depend on parent
pointers.
Why snapshots stay consistent
The controller takes the storage snapshot only after every node has answered the suspend update
(including its
SuspendManagerrun), and only after all DRBD volumes are UpToDate(
CtrlSnapshotCrtApiCallHandler.java:536-547,744-768).On the Primary. A
suspend-iothat drbdadm reports as successful has completedbdev_freeze()andthe
io_drained()wait:local_cnt == 0andap_pending_cnt == 0for every peer(
drbd_nl.c:6588-6651). Any drbdadm failure aborts the snapshot.ap_pending_cntuntil the peer'sP_WRITE_ACKarrives.P_WRITE_ACKonly after the write completed on its backing device (e_end_block_tail,drbd_receiver.c:3161-3186@ drbd-9.3.4).crypt_endio→crypt_dec_pending→bio_endio(base_bio),drivers/md/dm-crypt.c:1819-1848, 1867-1896@ v6.18).So once the Primary's suspend has succeeded, every write the Primary issued is in each diskful
replica's STORAGE volume, below dm-crypt, which is what gets snapshotted.
On a peer.
susp_userdoes not block replication. It only gates local application IO(
may_inc_ap_bio,drbd_int.h:3200-3205), andreceive_Datahas no suspend check(
drbd_receiver.c:4121-4329). What blocked the acks was suspending the dm-crypt device below DRBD.Why this is enough.
LAYERS_TO_SUSPEND_SUB_ROOTexists for.snapshot
norecoverydepend on.SuspendManagerwould also avoid the stall, but it is a larger change thanrestoring the documented filter.
that a stock controller left suspended. DRBD-rooted stacks simply no longer suspend LUKS.
Behaviour per stack
WritecacheLayer.java:165-186)dmsetup suspend,CacheLayer.java:180-250)isSuspendIoSupported()is false for NVME, BCACHE, STORAGE)I also checked this mechanically, running stock and patched
SuspendLayerUtilsover all 216 linearstacks that
LayerUtils.isLayerKindStackAllowedaccepts. Results:isSuspendIoSupported(), and for non-root layers a root that was suspended in the same run.suspended.
CtrlRscDfnApiCallHandler.java:1125on master),so they change the same way.
Tests
New
src/test/java/com/linbit/linstor/layer/utils/SuspendLayerUtilsTest.javaruns the real recursionon mocked layer trees. All stacks are allowed by
LayerUtils. Flagged layers are checked withtimes(1), the rest withnever().All 7 fail against master's
SuspendLayerUtils. The WRITECACHE, CACHE and resume tests fail onlybecause stock sets listed lower layers twice.
Local runs on master 87450b1 with JDK 21:
SuspendLayerUtilsTestRscDfnCloneApiTest/SnapshotApiTest/SnapshotRestoreApiTest/SnapshotRollbackApiTest/LayerResourceIdDbDriverTest:controller:checkstyleMain-Werrorcompile of:controllerValidation on a test cluster
Same setup as the reproduction on #540:
"This PR" means stock 1.35.2 satellites with a 1.35.2 controller built from the same change on v1.35.2
(the
SuspendLayerUtilsdiff is identical).synclayer_resource_suspended=truerowsLinstor-Crypt-*seen suspended (0.2 s sampling)norecoverymount, marker checksumFailed to suspend IOreports / ko-count disconnects / ha-controller evictionsRebase and backport
Written on v1.35.2, which we run, then rebased onto master.
We'd appreciate it in a 1.35.x patch release.
[Unreleased]→### Fixedlist. That hunk needs adjusting for a 1.35.x backport.Out of scope (possible follow-ups)
Unbounded pipe-EOF wait.
ExtCmd.syncProcess()waits for EOF on the child's pipes without a boundafter the child exits (
ExtCmd.java:178-180,OutputReceiver.java:210-226). Heredrbdsetupheldthem for about 37 s after drbdadm had exited.
suspend-ioholds the resource'sadm_mutex, so aresume-ioissued meanwhile can give up after drbdadm's 5 s.suspended:userwould also be needed.
No suspend-phase timeout. The suspend phase of snapshot creation has none; only the take-snapshot
step does (
CtrlSnapshotCrtApiCallHandler.java:795-799).Freeze longer than 5 s. drbdadm's 5 s
cmd-timeout-shortstill boundssuspend-io, so a freezethat must write back a lot of dirty data can fail at about 5–10 s, with no dm-crypt involved. This
predates the change.
vgshang (LVM). The LVM VG existence check runsvgswith an empty VG set, so it gets no--configand noignore_suspended_devices=1(LvmUtils.checkVgExistsBoolImpl→getVgsInfo(emptySet),LvmCommands.java:89-93). b0a45cf covered onlypvdisplayandvgscan.resume.
vgssurvived SIGKILL; most likely it was blocked reading the suspended dm-crypt device, but nostack was captured.
DRBD,CACHE,STORAGE. CACHE below DRBD is still
dmsetup suspended on every node whose DRBD suspendsucceeded, so with DRBD ≥ 9.3 it may hit the same ordering problem. Not tested and not changed here.
Note that this PR was created with the help of Claude-Opus-5.5 and might contain some hallucinations/inconsistancies as I am not that familiar with the source code. Take everything it says on "root cause" onwards with a grain of salt. Thanks