Skip to content

Commit 6291a2d

Browse files
kvapsclaude
andauthored
fix(satellite): decouple rs-discard-granularity from discard-zeroes gating (#112)
* fix(satellite): decouple rs-discard-granularity from discard-zeroes gating A partially-written FILE_THIN volume resynced ~2x the bytes upstream LINSTOR did because the rendered .res lacked rs-discard-granularity. autoDiskOptions() early-returned nil for any provider that was not discard-zeroes-safe, coupling rs-discard-granularity (which upstream gates ONLY on the backing device's reported discard granularity) to discard-zeroes-if-aligned (correctly provider-gated). Decouple the two gates to mirror upstream: - discard-zeroes-if-aligned: provider-gated as before, but now rendered explicitly as `no` for non-safe kinds (FILE_THIN, thick LVM, FILE) instead of omitting the whole block — matching upstream 1.33.2's render. - rs-discard-granularity: emitted whenever the backing device reports a non-zero discard granularity (lsblk DISC-GRAN > 0), independent of provider kind, so an aligned all-zero region is UNMAPped during resync instead of transferred. Multi-volume collapse stays conservative: discard-zeroes-if-aligned is `yes` only when every volume is safe (one `no` pins the resource), and rs-discard-granularity is the smallest across volumes. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * test(corner-q3): pin FILE_THIN rs-discard-granularity at L6 + L7 L6 cli-matrix cell file-thin-rs-discard-granularity.sh: 512M FILE_THIN single replica, write 320M, add 2nd diskful replica, assert the new replica's resync `received` byte counter is close to the written bytes (not the full device) and the rendered .res carries the discard disk block. L7 replay file-thin-rs-discard-granularity.yaml: operator-CLI sequence asserting the rendered DRBD config carries rs-discard-granularity=4096 and discard-zeroes-if-aligned=no for a FILE_THIN resource, plus a clean 3rd-replica sync (drbd_option + sync_clean awaits). Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * docs(cli-parity): record FILE_THIN disk-block render deltas (Q3 #76) After the rs-discard-granularity decoupling fix, BS renders the same thin-aware-resync disk options as upstream for a discard-capable FILE_THIN volume. Two render-shape divergences remain and are accepted: upstream additionally emits `block-size 512;` (BS omits it), and upstream renders the disk block at volume scope vs BS's resource scope — functionally identical for single-volume resources. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * fix(satellite): gate rs-discard-granularity to block-device thin pools (FILE_THIN loop wedge) Rendering rs-discard-granularity into a loop-backed FILE_THIN volume's disk{} block regressed fresh-create convergence. When the elected day0 winner force-primaries to run mkfs (the FileSystem/Type path), mkfs issues a full-device discard; on the loop backing that discard storm, with rs-discard-granularity active, dirties the bitmap relative to the day0-seeded peers and forces a FULL initial SyncTarget that wedges (PausedSyncT dependency between the two targets). Proven on stand + CI: an identical 3-replica create + mkfs converges instantly on LVM_THIN (real block device, same disk block) but full-resyncs and wedges on FILE_THIN (loop) — the e2e respawn-standalone-wedge failure. Gate rs-discard-granularity on the discard-zero-safe block-device provider set (LVM_THIN / ZFS / ZFS_THIN), the same set as discard-zeroes-if-aligned. FILE_THIN now renders only the inert discard-zeroes-if-aligned no (matching pre-feature behaviour) and omits the granularity. The thin-aware-resync win is retained where it is both safe and effective: real block-device thin/ZFS pools. L1 pins (discardgran_test.go): FILE_THIN + thick LVM omit the granularity; LVM_THIN/ZFS keep it; end-to-end render pins TestBuildResFile_FileThinDay0SkipRender (no rs-discard-granularity) and TestBuildResFile_LvmThinDay0SkipRender (keeps it). L6/L7 repurposed to guard the FILE_THIN fresh-create + mkfs day0-skip convergence and assert rs-discard-granularity is absent. Known-delta #76 updated. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * docs(satellite): correct autoDiskOptionsForResource godoc for FILE_THIN gate Comment-only: the rs-discard-granularity gate is no longer INDEPENDENT of provider kind — it follows the discard-zero-safe block-device set and excludes loop-backed FILE_THIN. Align the godoc with the implementation. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> --------- Signed-off-by: Andrei Kvapil <kvapss@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent a939c9b commit 6291a2d

5 files changed

Lines changed: 550 additions & 88 deletions

File tree

docs/cli-parity-known-deltas.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ Row IDs match the command-catalogue indexes used by `cli-parity-refresh.sh` (see
4141
| 70 | `rd lp <rd>` (inherited parent-scope props shown inline) | WIRE_SHAPE | permanent | Corner-case I2. Upstream LINSTOR treats inheritance as a *reference, not a copy*: a property set on a resource-group / controller affects existing children, but the child's `list-properties` does NOT show the inherited key inline (UG9 ~4587: "As the QoS properties are inherited and not copied, you will not see the property listed in any child objects"). BS deliberately INLINES inherited Controller/RG-scope keys into the RD's bare `props` map (stampRDEffectiveProps) so `linstor rd lp` renders them — the python CLI's `rd lp` table reads `props`, and surfacing the effective value there is more operator-friendly than an empty table. The inheritance-is-a-reference half MATCHES (props resolve at read time via effectiveprops.Resolve, so a post-spawn RG change is reflected); only the inline-visibility in `rd lp` diverges, by design. Sensitive controller-scope keys are still redacted (Bug 115). Pinned by `TestCornerI2_InheritanceIsReferenceAndShownInline`. |
4242
| 71 | `controller TcpPortAutoRange` / DRBD port + minor base | BEHAVIOR | permanent | Corner-case K3. Upstream LINSTOR's `TcpPortAutoRange` defaults to 7000-7999 and DRBD minors to 1000+ (UG9 ~2225-2231). blockstor deliberately allocates DRBD TCP ports from 20000-20999 and minors from 20000-28999 (`drbd.DefaultPortMin/Max`, `dispatcher.derivePort/deriveMinor`) so a blockstor cluster can run on the same nodes as a live upstream LINSTOR without colliding on the shared kernel `/dev/drbd<N>` minor / TCP-port namespace. The override knob is LINSTOR-compatible (`linstor n set-property <node> DrbdOptions/TcpPortRange "<min>-<max>"` via `ParseRange`); only the DEFAULT base differs. Reuse-after-`rd d` MATCHES upstream: a deleted resource's port/minor re-enters the cluster taken-set and the deterministic lowest-free allocator hands it back. Pinned by `pkg/drbd/portpool_test.go::TestK3_{DefaultPortRangeIsBlockstorWindow,PortReusedAfterDelete}`. |
4343
| 74 | `r td <node> <rd> --cancel` (stuck-toggle recovery) | BEHAVIOR | permanent | Corner-case H1. VERSION_CONTEXT: upstream LINSTOR ≥1.34.0 recovers a stuck toggle by re-issuing the SAME command (retry) or the OPPOSITE command (cancel). The dev-stand oracle is 1.33.2 (pre-1.34; rejects with a conflict). blockstor satisfies the 1.34 contract: a repeated toggle is idempotent (retry), and the opposite-direction toggle drives the satellite demote path (`applyStorageIfDiskful` → detach + `reclaimVolumesForDiskless`), unwinding the half-carved storage (cancel). blockstor ALSO accepts an explicit `?cancel=true` query (Bug 40) the upstream client never sends — a superset, not a divergence. Pinned by `pkg/rest/resource_toggle_disk_test.go::TestToggleDiskCancelStuckAddDiskScenario4W24` + `pkg/satellite/reconciler_toggle_diskless_test.go::TestBug267ToggleToDisklessReclaimsBackingVolume`. |
44+
| 76 | FILE_THIN `.res` disk block — `rs-discard-granularity` omitted (loop-safety), `block-size` omitted, scope resource vs volume | WIRE_SHAPE | permanent | Corner-case Q3. For a FILE_THIN volume BS renders only `discard-zeroes-if-aligned no;` and DELIBERATELY OMITS `rs-discard-granularity`, even though the loop backing reports a non-zero DISC-GRAN and upstream 1.33.2 emits `rs-discard-granularity 4096;`. Reason: rendering the granularity on a loop-backed FILE_THIN volume regresses blockstor's day0 GI-skip on the mkfs/force-primary path — the elected winner's `mkfs` full-device discard, with rs-discard-granularity active, dirties the bitmap relative to the day0-seeded peers and forces a FULL initial SyncTarget that wedges (proven: instant convergence on LVM_THIN with the same disk block, full-resync wedge on FILE_THIN; the e2e respawn-standalone-wedge regression). The thin-aware-resync win (written-bytes-only resync) is retained on the discard-zero-safe BLOCK-device pools (LVM_THIN / ZFS / ZFS_THIN), where the option is both safe and effective. Additional accepted render-shape divergences: (a) upstream additionally emits `block-size 512;` (backing logical-block-size hint, unrelated to discard/resync); BS omits it — DRBD defaults to the device block size. (b) Upstream renders the disk block at VOLUME scope; BS at RESOURCE scope — functionally identical for single-volume resources. Pinned by `pkg/satellite/discardgran_test.go::TestAutoDiskOptions_FileThin4KLoop_NoGranularity` + `TestBuildResFile_FileThinDay0SkipRender` + `TestBuildResFile_LvmThinDay0SkipRender` (L1), `tests/e2e/cli-matrix/file-thin-rs-discard-granularity.sh` (L6), `tests/operator-harness/replay/file-thin-rs-discard-granularity.yaml` (L7). |
4445
| 75 | `DrbdOptions/auto-diskful` trigger condition | BEHAVIOR | 2026-12-31 | Corner-case H4. PARTIAL: blockstor honours the property and the RD>RG>Controller priority hierarchy (`internal/controller/auto_diskful_controller.go`), but the firing CONDITION diverges from upstream's documented "Diskless node held DRBD Primary > N minutes". blockstor arms the N-minute timer on a diskful-replica DEFICIT (count < place_count) and additionally promotes a Primary-InUse diskless replica immediately (`ResourceReconciler.maybeAutoDiskful`, no timer, property-independent). The `DrbdOptions/auto-diskful-allow-cleanup` excess-secondary trim is unimplemented (reserved key only). Property plumbing + hierarchy pinned by `internal/controller/auto_diskful_timer_test.go`. Full timed-Primary state machine + cleanup gate tracked as follow-up. |
4546

4647
## Open (block merge until addressed)

pkg/satellite/discardgran.go

Lines changed: 122 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,9 @@ const (
5252
diskOptRsDiscardGranularity = "rs-discard-granularity"
5353
diskOptDiscardZeroesAligned = "discard-zeroes-if-aligned"
5454

55-
// drbdYes is DRBD's affirmative value for a boolean disk option.
55+
// drbdYes / drbdNo are DRBD's boolean disk-option values.
5656
drbdYes = "yes"
57+
drbdNo = "no"
5758
)
5859

5960
// DRBD's accepted bounds for rs-discard-granularity. Mirrors upstream
@@ -72,17 +73,23 @@ const (
7273
// resource-scope `disk { }` block (which DRBD applies to every
7374
// volume).
7475
//
75-
// It probes each local diskful volume's backing device and is
76-
// deliberately CONSERVATIVE for the (today hypothetical) multi-volume
77-
// case: the options are emitted only when EVERY local diskful volume's
78-
// provider is discard-zero-safe; if any volume is thick/file/unknown
79-
// the whole block is omitted (safe full resync for all). The
80-
// rs-discard-granularity, when present, is the SMALLEST across the
81-
// volumes so the single resource-scope value stays a valid multiple of
82-
// every device's discard granularity — never producing an UNDER-aligned
83-
// UNMAP. A volume whose device can't be probed simply contributes no
84-
// granularity (discard-zeroes-if-aligned still applies; DRBD falls back
85-
// to a full copy where it can't discard).
76+
// It probes each local diskful volume's backing device. Each volume
77+
// contributes a discard-zeroes-if-aligned flag (yes/no per provider
78+
// kind) and, for the discard-zero-safe BLOCK-device provider set
79+
// (LVM_THIN / ZFS / ZFS_THIN) whose device reports a non-zero discard
80+
// granularity, an rs-discard-granularity value. Loop-backed FILE_THIN is
81+
// excluded from the granularity (see autoDiskOptions for the loop+mkfs
82+
// wedge rationale).
83+
//
84+
// The resource-scope collapse is CONSERVATIVE for the (today
85+
// hypothetical) multi-volume case (mergeResourceDiscardOptions):
86+
// discard-zeroes-if-aligned is "yes" only when EVERY volume is
87+
// discard-zero-safe (one "no" pins the resource to "no"); the
88+
// rs-discard-granularity is the SMALLEST across the volumes so the
89+
// single resource-scope value stays a valid multiple of every device's
90+
// discard granularity — never an UNDER-aligned UNMAP. A volume whose
91+
// device can't be probed simply contributes no granularity (DRBD falls
92+
// back to a full copy where it can't discard).
8693
//
8794
// A diskless local replica (no diskful volumes) returns nil — there is
8895
// no backing device to discard against.
@@ -114,11 +121,6 @@ func (r *Reconciler) autoDiskOptionsForResource(
114121
device := devices[vol.GetVolumeNumber()]
115122

116123
volOpts := autoDiskOptions(ctx, exec, provider.Kind(), device)
117-
if len(volOpts) == 0 {
118-
// This volume is not discard-zero-safe — omit the whole
119-
// block so no volume gets an unsafe option.
120-
return nil
121-
}
122124

123125
mergeResourceDiscardOptions(out, volOpts)
124126

@@ -133,21 +135,34 @@ func (r *Reconciler) autoDiskOptionsForResource(
133135
}
134136

135137
// mergeResourceDiscardOptions folds one volume's auto disk options into
136-
// the resource-scope accumulator. discard-zeroes-if-aligned is a flat
137-
// flag (all discard-zero-safe volumes set "yes").
138-
// rs-discard-granularity collapses to the SMALLEST seen value so the
139-
// single resource-scope number is a valid multiple of every volume's
140-
// backing-device granularity.
138+
// the resource-scope accumulator (a single `disk { }` block DRBD applies
139+
// to every volume of the resource):
140+
//
141+
// - discard-zeroes-if-aligned collapses CONSERVATIVELY to "yes" only
142+
// when EVERY volume is discard-zero-safe; a single "no" volume pins
143+
// the whole resource to "no". Otherwise a thick/file volume sharing
144+
// the resource could be told an aligned discard yields zero when it
145+
// does not — a data-safety violation. (Today resources are
146+
// single-volume, but the merge must stay safe for the multi-volume
147+
// future.)
148+
// - rs-discard-granularity collapses to the SMALLEST seen value so the
149+
// single resource-scope number stays a valid multiple of every
150+
// volume's backing-device granularity (never an UNDER-aligned UNMAP).
141151
func mergeResourceDiscardOptions(acc, volOpts map[string]string) {
142152
for key, val := range volOpts {
143-
if key != diskOptRsDiscardGranularity {
144-
acc[key] = val
145-
146-
continue
147-
}
148-
149-
prev, have := acc[key]
150-
if !have || smallerNumeric(val, prev) {
153+
switch key {
154+
case diskOptRsDiscardGranularity:
155+
prev, have := acc[key]
156+
if !have || smallerNumeric(val, prev) {
157+
acc[key] = val
158+
}
159+
case diskOptDiscardZeroesAligned:
160+
// "no" wins: once any volume is unsafe the resource flag
161+
// must stay "no". Only set "yes" if not yet pinned to "no".
162+
if acc[key] != drbdNo {
163+
acc[key] = val
164+
}
165+
default:
151166
acc[key] = val
152167
}
153168
}
@@ -172,54 +187,100 @@ func smallerNumeric(left, right string) bool {
172187
// (sans the `DrbdOptions/Disk/` prefix) ready to merge into the
173188
// rendered `disk { }` block.
174189
//
190+
// Both options are gated on the SAME discard-zero-safe provider set
191+
// (LVM_THIN / ZFS / ZFS_THIN). discard-zeroes-if-aligned mirrors
192+
// upstream's CtrlRscCrtApiHelper switch; rs-discard-granularity is
193+
// additionally gated on that set rather than on the device's reported
194+
// DISC-GRAN alone — see autoDiskOptions's inline rationale: a loop-
195+
// backed FILE_THIN device reports DISC-GRAN=4096 but rendering
196+
// rs-discard-granularity there REGRESSES fresh-create convergence on
197+
// the mkfs/force-primary path (the wedge that broke the e2e
198+
// respawn-standalone-wedge scenario). Keeping the option on real
199+
// block-device thin/ZFS pools retains the genuine thin-aware-resync win
200+
// (partially-written volume resyncs ~only the written bytes) where it is
201+
// both safe and effective.
202+
//
175203
// Data-safety contract — the optimisation may ONLY skip provably-zero
176-
// ranges (thin discard), NEVER any written data. The gates here are
177-
// the same ones upstream LINSTOR uses:
204+
// ranges (thin discard), NEVER any written data:
178205
//
179-
// - discard-zeroes-if-aligned is enabled ONLY for provider kinds
180-
// whose backing device deterministically reads/discards as zero in
181-
// an ALIGNED region: LVM_THIN, ZFS, ZFS_THIN. NOT for FILE_THIN
182-
// (loop-backed sparse files do not reliably honour aligned discard
183-
// → zero), NOT for thick LVM / plain FILE / DISKLESS. This matches
184-
// upstream's CtrlRscCrtApiHelper provider switch exactly. Note we
185-
// deliberately do NOT reuse IsThinOrZFS here: that helper includes
186-
// FILE_THIN (correct for the seed-GI skip-sync gate) but FILE_THIN
187-
// must NOT get discard-zeroes-if-aligned.
206+
// - discard-zeroes-if-aligned is `yes` ONLY for provider kinds whose
207+
// backing device deterministically reads/discards as zero in an
208+
// ALIGNED region: LVM_THIN, ZFS, ZFS_THIN. For every other kind
209+
// (FILE_THIN loop-backed sparse file, thick LVM, plain FILE,
210+
// unknown) it is `no` — matching upstream's CtrlRscCrtApiHelper
211+
// switch, which renders an explicit `no`. Note we deliberately do
212+
// NOT reuse IsThinOrZFS here: that helper includes FILE_THIN
213+
// (correct for the seed-GI skip-sync gate) but FILE_THIN must NOT
214+
// get discard-zeroes-if-aligned=yes.
188215
//
189-
// - rs-discard-granularity is set ONLY when the backing block device
190-
// actually reports a non-zero discard granularity (lsblk DISC-GRAN
191-
// > 0). A device that reports 0 does not support discard — setting
192-
// a non-zero granularity there would make drbdsetup reject the
193-
// option (or, worse, attempt unsupported UNMAPs). When the
194-
// granularity can't be determined or is 0, we OMIT the key
195-
// entirely and DRBD falls back to a full byte-copy resync — always
196-
// safe.
216+
// - rs-discard-granularity is set ONLY for the discard-zero-safe
217+
// provider set AND when the backing block device reports a non-zero
218+
// discard granularity (lsblk DISC-GRAN > 0). A device reporting 0
219+
// does not support discard — emitting a non-zero granularity there
220+
// would make drbdsetup reject the option, so we OMIT the key and
221+
// DRBD falls back to a full byte-copy resync. FILE_THIN is excluded
222+
// even though its loop backing reports a non-zero DISC-GRAN (see the
223+
// inline mkfs/loop wedge rationale below).
197224
//
198-
// When in doubt the map omits a key (full transfer), never the
199-
// reverse. An empty/nil return means "render no auto disk options".
225+
// When in doubt the map omits the granularity key (full transfer),
226+
// never the reverse. The discard-zeroes flag is ALWAYS present (yes or
227+
// no), mirroring upstream which renders it explicitly. An empty return
228+
// would mean "render no disk block"; this function returns at least the
229+
// discard-zeroes flag for any diskful volume, so the block renders
230+
// whenever a backing device is present.
200231
func autoDiskOptions(
201232
ctx context.Context,
202233
exec storage.Exec,
203234
providerKind string,
204235
devicePath string,
205236
) map[string]string {
206-
if !discardZeroesIfAligned(providerKind) {
207-
// Thick LVM / plain FILE / FILE_THIN / DISKLESS / unknown:
208-
// the backing device cannot be trusted to discard-to-zero on
209-
// an aligned range. Emit nothing — DRBD keeps its safe
210-
// full-copy resync.
211-
return nil
237+
zeroesVal := drbdNo
238+
if discardZeroesIfAligned(providerKind) {
239+
zeroesVal = drbdYes
212240
}
213241

214242
out := map[string]string{
215-
diskOptDiscardZeroesAligned: drbdYes,
243+
diskOptDiscardZeroesAligned: zeroesVal,
244+
}
245+
246+
// rs-discard-granularity is emitted ONLY for a discard-zero-safe
247+
// provider kind — i.e. a real block device whose aligned discard
248+
// deterministically reads back zero (LVM_THIN / ZFS / ZFS_THIN).
249+
//
250+
// WHY this is gated on the SAME provider set as discard-zeroes (and
251+
// NOT, as an earlier revision did, on the device's reported
252+
// DISC-GRAN alone): a loop-backed FILE_THIN volume reports a non-zero
253+
// lsblk DISC-GRAN (4096 on the stand) yet rendering
254+
// rs-discard-granularity into its `disk { }` block REGRESSES the
255+
// fresh-create convergence. When the elected day0 winner force-
256+
// primaries to run `mkfs` (the FileSystem/Type path), mkfs issues a
257+
// full-device discard; on the loop backing — which has a documented
258+
// DRBD kernel write-path interaction (see tests/e2e/lib.sh) — that
259+
// discard storm, with rs-discard-granularity active, dirties the
260+
// bitmap relative to the day0-seeded peers and forces a FULL initial
261+
// SyncTarget that then wedges (PausedSyncT dependency between the two
262+
// targets). Proven on the dev stand + CI: an identical 3-replica
263+
// create with FileSystem/Type=ext4 CONVERGES INSTANTLY on LVM_THIN
264+
// (real block device, same full disk block) but FULL-resyncs and
265+
// wedges on FILE_THIN (loop). The e2e `respawn-standalone-wedge`
266+
// scenario (FILE_THIN + mkfs) failed for exactly this reason; the
267+
// `replica-add-no-resync` scenario (FILE_THIN, NO mkfs) passed, so
268+
// the trigger is the mkfs-discard × loop × rs-discard-granularity
269+
// interaction, not the option in isolation.
270+
//
271+
// For FILE_THIN we therefore render only `discard-zeroes-if-aligned
272+
// no` (DRBD's inert default) and OMIT the granularity — DRBD then
273+
// does a full byte-copy resync on the rare loop relocate, which is
274+
// always correct, while the day0 skip + mkfs convergence is
275+
// preserved. The genuine thin-aware-resync win is retained where it
276+
// is both safe AND effective: the real block-device thin/ZFS pools.
277+
if !discardZeroesIfAligned(providerKind) {
278+
return out
216279
}
217280

218281
// Only attempt the granularity when we have a real device path to
219282
// probe. A diskless local replica (devicePath == "") has no
220-
// backing device — discard-zeroes-if-aligned alone is harmless
221-
// (it governs how DRBD treats discards it receives) and we skip
222-
// the granularity probe.
283+
// backing device — skip the granularity probe.
223284
if devicePath == "" {
224285
return out
225286
}

0 commit comments

Comments
 (0)