Skip to content

Do not mark a disk as spun down when the spindown command fails - #140

Open
shyney7 wants to merge 1 commit into
adelolmo:masterfrom
shyney7:do-not-mark-failed-spindown-as-spun-down
Open

shyney7 wants to merge 1 commit into
adelolmo:masterfrom
shyney7:do-not-mark-failed-spindown-as-spun-down

Conversation

@shyney7

@shyney7 shyney7 commented Jul 26, 2026

Copy link
Copy Markdown

Problem

spindownDisk's error is printed and then discarded, and SpunDown is set to true either way (hdidle.go:156-161 on master):

if err := spindownDisk(device, ds.CommandType, ds.PowerCondition, config.Defaults.Debug); err != nil {
    fmt.Println(err.Error())
}
previousSnapshots[dsi].LastSpunDownAt = now
previousSnapshots[dsi].SpinDownAt = now
previousSnapshots[dsi].SpunDown = true

The spindown branch is gated on !ds.SpunDown, and only observed disk activity clears that flag (hdidle.go:176). So when the command fails on an idle disk, the disk is never retried: it keeps spinning while hd-idle considers it parked. logSpinup only writes to the logfile on the spin-up transition, which never comes, so the logfile stays empty too.

The observable result is one error line at the moment of failure and then permanent silence, with the disk spinning 24/7. That is the same symptom described in #131 ("first spindown works, later ones do not"), reachable without any state-tracking regression.

This is a different root cause from #113, where the disk wakes without registering activity in /proc/diskstats so the wake is never observed. That one needs a way to learn the disk's real state. This one does not: the error is already in hand at line 156 and is simply dropped.

Fix

Keep SpunDown false when the command fails, so the next idle period tries again. LastSpunDownAt is still recorded on failure, which rate-limits the retry to one per idle period instead of one per poll, so a disk that always rejects the command does not spam the log every poolInterval.

updateState now takes the spindown function as a parameter, following the existing diskHolderGetterFunc pattern in diskstats/snapshot.go, so the failure path is testable without touching hardware.

Tests

hdidle_test.go (new, table-driven per AGENTS.md):

  • TestSpindownResultDeterminesSpunDownState: success sets SpunDown and SpinDownAt; failure leaves both unset.
  • TestFailedSpindownIsRetriedOncePerIdlePeriod: after a failure there is no retry within the idle period and exactly one retry after it.

go test ./... -race -cover passes, go vet ./... is clean, and gofmt -l is clean for both changed files. Note gofmt -l already reports diskstats/snapshot.go on unmodified master (a missing space after a comma at line 143); left untouched as it is unrelated.

Hardware verification

Real Seagate ST10000NM017B (10 TB SATA), Linux 6.18, both binaries built from this branch's base. Ground truth was a 64 MB iflag=direct read at a fresh offset, which reliably separates a spinning disk from a stopped one: about 0.28 s versus about 9.5 s including spin-up.

Failure path, forced with -c ata on a transport that rejects opcode 0x85, -i 20, over 75 s (about 37 polls):

master this branch
spindown attempts 1 3
failures reported 1 3
state after spunDown=true spunDown=false

Three attempts over 75 s at -i 20 is one per idle period, as intended.

Success path, -c scsi, two full cycles: spindown, then spinup detected on the waking read, then spindown again, with the 64 MB probe measuring 9.51 s and 9.68 s at each stop and the logfile recording both running:/stopped: pairs. No behavior change versus master on this path.

One note on the setup, in case it is useful: the probe reads had to go through a partition (/dev/sde2) rather than the whole disk (/dev/sde), because master computes per-disk activity from the sum of the partition counters, so whole-disk I/O does not register. v1.22 behaves differently here (it carries 675d30f, which master does not). Not related to this change, just why the test reads target a partition.

The error returned by spindownDisk was printed and then discarded, and
SpunDown was set to true regardless. Since the spindown branch is gated on
!ds.SpunDown and only real disk activity clears that flag, an idle disk
whose spindown failed was never retried: the disk kept spinning while
hd-idle reported it as parked.

Keep SpunDown false when the command fails so the next idle period tries
again. LastSpunDownAt is still recorded on failure, which rate-limits the
retry to one per idle period rather than one per poll.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant