From f311fa36dc49c1e65460c439e4480f68ae161375 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 01:55:39 +0200 Subject: [PATCH] The setuid walk is paced before each file too, so a large directory is not read blind (review of issue 433) A file's own details (lstat) may be read from the disk as a directory is, so a directory of tens of thousands of files was read without a look and the header's promise that no burst is longer than PaceLook did not hold. The walk now asks the pacer before each file as well; the 250 ms gate makes that a clock read per file. A test with one directory of twenty files fails without it. The clock is read once after a run of pauses, and the constants test says that it guards the constants only. --- internal/accounts/setuid_pace.go | 6 ++++-- internal/accounts/setuid_pace_test.go | 22 +++++++++++++++++++++- internal/accounts/setuid_walk.go | 4 ++++ 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/internal/accounts/setuid_pace.go b/internal/accounts/setuid_pace.go index a6273ea..9cb23d2 100644 --- a/internal/accounts/setuid_pace.go +++ b/internal/accounts/setuid_pace.go @@ -1,6 +1,7 @@ package accounts -// The walk for setuid programs yields to a busy disk (novox/hq issue 433). The walk is put in the idle I/O class +// The walk for setuid programs yields to a busy disk (novox/hq issue 433). It asks before each directory and each +// file it reads, so no burst between two looks is longer than PaceLook, however large a directory. The walk is put in the idle I/O class // (idle_linux.go), but only a disk scheduler that schedules by class honours that (BFQ does). The control-node's // disks run mq-deadline, which does not, and there the walk read at full priority: on 2026-10-11 it kept two // spinning disks saturated for sixteen minutes, the bus whose store shares those disks stalled for half a minute @@ -105,7 +106,8 @@ func (pc *pacer) yield(ctx context.Context) error { break } } - pc.readSince, pc.lastLook = pc.now(), pc.now() + resumed := pc.now() + pc.readSince, pc.lastLook = resumed, resumed return nil } diff --git a/internal/accounts/setuid_pace_test.go b/internal/accounts/setuid_pace_test.go index dd7c0a0..5d117ef 100644 --- a/internal/accounts/setuid_pace_test.go +++ b/internal/accounts/setuid_pace_test.go @@ -138,7 +138,8 @@ func TestTheWalkKeepsATenthOfTheTimeWhateverThePressure(t *testing.T) { // What the floor promises against the ages a search is judged by: the walk that took 16m31s unpaced on the // control-node on 2026-10-11, slowed to its worst, still completes well inside the time the controller stays quiet -// while it waits for a search, and so judges for most of its fresh age. +// while it waits for a search. A guard on the constants only: that the code keeps to the ratio is proven by +// TestTheWalkKeepsATenthOfTheTimeWhateverThePressure. func TestTheSlowestPacedWalkStillJudgesInTime(t *testing.T) { const unpaced = 16*time.Minute + 31*time.Second worst := (1 + PaceRatio) * unpaced @@ -280,3 +281,22 @@ func TestTheIOPressureIsReadAsTheKernelWritesIt(t *testing.T) { } } } + +// One directory of many files is paced as it is read, not only between directories: a file's own details may be +// read from the disk too. +func TestADirectoryOfManyFilesIsPacedAsItIsRead(t *testing.T) { + names := []string{"bin/"} + for i := range 20 { + names = append(names, "bin/f"+strings.Repeat("x", i+1)) + } + top := tree(t, names...) + f := newFake(80, 2) + f.tick = 100 * time.Millisecond + w := mine() + w.pace = f.pacer() + walkAll(t, w, top) + // Two directories alone would be looked at once, 200 ms apart, within one look: no pause at all. + if len(f.slept) == 0 { + t.Fatal("a directory of twenty files was read without a pause under pressure") + } +} diff --git a/internal/accounts/setuid_walk.go b/internal/accounts/setuid_walk.go index 236ddb8..dac285a 100644 --- a/internal/accounts/setuid_walk.go +++ b/internal/accounts/setuid_walk.go @@ -239,6 +239,10 @@ func (w walker) walkMount(ctx context.Context, p *SetuidProgress, mount string, return err } case d.Type().IsRegular(): + // A file's own details may be read from the disk too: a directory of many files is paced as it is read. + if err := w.pace.yield(ctx); err != nil { + return err + } info, err := d.Info() // the entry's own, never through a link if errors.Is(err, fs.ErrNotExist) { return nil