From b0bdcd02c902a5567904128f9ff0722608c215dd Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 5 Nov 2026 09:39:24 +0100 Subject: [PATCH] nfs-server: the look below a share runs beside the pass, so the status is written every pass (issue 1971) A walk of minutes inside the pass held the status back until the module was found unhealthy and the assigner held the version. Claude-Session: https://claude.ai/code/session_01JG73KJNrH5EgFXMKKYSrig --- modules/nfs-server/cmd/nfs-server/machine.go | 6 +- modules/nfs-server/cmd/nfs-server/nfs_test.go | 2 +- modules/nfs-server/cmd/nfs-server/process.go | 3 +- .../nfs-server/cmd/nfs-server/review_test.go | 51 ++++++++++++ modules/nfs-server/cmd/nfs-server/shares.go | 81 ++++++++++++++----- 5 files changed, 119 insertions(+), 24 deletions(-) diff --git a/modules/nfs-server/cmd/nfs-server/machine.go b/modules/nfs-server/cmd/nfs-server/machine.go index fdd948bf4..6025426bb 100644 --- a/modules/nfs-server/cmd/nfs-server/machine.go +++ b/modules/nfs-server/cmd/nfs-server/machine.go @@ -105,8 +105,10 @@ type Machine struct { // ownerOf reads a file's owner from its inode; nil is the inode as the kernel says it. Tests set it // so the folders they make stand for a non-root owner, whoever runs them. ownerOf func(info os.FileInfo) (uid, gid int) - // clean is when a look below each real path last found no backup repository (LookKept). - clean map[string]time.Time + // Beside runs a look below a share beside the pass; nil is a goroutine. Tests run it in place. + Beside func(func()) + // looks is the last look below each real path (LookKept). + looks map[string]*lookBelow cleanMu sync.Mutex // walk is the running pass's context, which ends the look below a share when the process stops. walk context.Context diff --git a/modules/nfs-server/cmd/nfs-server/nfs_test.go b/modules/nfs-server/cmd/nfs-server/nfs_test.go index dbc45970f..b6d7424fd 100644 --- a/modules/nfs-server/cmd/nfs-server/nfs_test.go +++ b/modules/nfs-server/cmd/nfs-server/nfs_test.go @@ -32,7 +32,7 @@ func newFake(t *testing.T) *fake { } func (f *fake) machine() *Machine { - return &Machine{Root: f.root, UID: f.uid, owners: f.owners, ownerOf: asTheLibrarysOwner, Now: func() time.Time { return f.now }, Run: func(_ context.Context, name string, args ...string) Ran { + return &Machine{Root: f.root, UID: f.uid, owners: f.owners, ownerOf: asTheLibrarysOwner, Beside: func(f func()) { f() }, Now: func() time.Time { return f.now }, Run: func(_ context.Context, name string, args ...string) Ran { line := strings.Join(append([]string{name}, args...), " ") f.ran = append(f.ran, line) if (line == "exportfs -ra" || line == "sudo -n exportfs -ra") && f.onExportfs != nil { diff --git a/modules/nfs-server/cmd/nfs-server/process.go b/modules/nfs-server/cmd/nfs-server/process.go index c6c6172ed..39d33ce90 100644 --- a/modules/nfs-server/cmd/nfs-server/process.go +++ b/modules/nfs-server/cmd/nfs-server/process.go @@ -61,9 +61,8 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status { if _, note := m.Backup(); note != "" { s.Warnings = append(s.Warnings, note) } - m.setWalk(ctx) + m.setWalk(ctx) // stays set: a look beside the pass ends when the process does exports, refused, said := m.Judge(c, roster, wants) - m.setWalk(nil) if ctx.Err() != nil { // Stopping: write nothing and reload nothing, so Stopped starts at once. s.Problems = append(s.Problems, "the pass was ended by the process stopping; nothing was written") diff --git a/modules/nfs-server/cmd/nfs-server/review_test.go b/modules/nfs-server/cmd/nfs-server/review_test.go index 86128e7a9..e987bf463 100644 --- a/modules/nfs-server/cmd/nfs-server/review_test.go +++ b/modules/nfs-server/cmd/nfs-server/review_test.go @@ -432,3 +432,54 @@ func TestAHangingCommandStillLetsStoppedRemoveTheFileInTime(t *testing.T) { t.Errorf("the export file stays after an unassignment whose commands hung") } } + +// Issue 1971: the look below a share runs beside the pass. A pass whose walk has not finished still writes +// its status, refuses the share until a look has finished, keeps believing a clean look while it is renewed, +// and a walk ended by the process stopping leaves no result. +func TestTheLookBelowRunsBesideThePassAndNeverHoldsItsStatus(t *testing.T) { + f := newFake(t) + f.mountinfo("/") + f.dir("/srv/clean/films/one") + f.file(ConfigFile, grantsConfig("clean=/srv/clean", "clean=ws:rw")) + f.roster("ws 10.0.0.3") + f.wants(map[string][]Want{"ws": {{Share: "clean", Access: "rw"}}}) + m := f.machine() + var walks []func() + m.Beside = func(w func()) { walks = append(walks, w) } // the walk is still running + now := time.Date(2026, 11, 5, 5, 0, 0, 0, time.UTC) + m.Now = func() time.Time { return now } + + s := m.Pass(context.Background(), nil) + if !s.At.Equal(now) || f.read(StatusFile) == "" || len(walks) != 1 { + t.Fatalf("the pass waited for the walk: at %v, walks %d", s.At, len(walks)) + } + if !strings.Contains(refusedWhy(s, "clean"), "not been looked through yet") { + t.Errorf("a share whose look is unfinished is exported: %+v", s.Refused) + } + m.Pass(context.Background(), &s) + if len(walks) != 1 { + t.Errorf("a second walk started while one runs: %d", len(walks)) + } + walks[0]() + if s = m.Pass(context.Background(), &s); refusedWhy(s, "clean") != "" { + t.Fatalf("the finished look was not used: %+v", s.Refused) + } + + now = now.Add(LookKept) + s = m.Pass(context.Background(), &s) + if len(walks) != 2 || refusedWhy(s, "clean") != "" { + t.Errorf("the renewal must run beside the pass and the clean look stand: walks %d, %+v", len(walks), s.Refused) + } + ctx, cancel := context.WithCancel(context.Background()) + m.Pass(ctx, &s) + cancel() + f.restic("/srv/clean/films/one/backup") + walks[1]() + if s = m.Pass(context.Background(), &s); refusedWhy(s, "clean") != "" { + t.Errorf("a walk ended by the stop left a result: %+v", s.Refused) + } + walks[len(walks)-1]() + if s = m.Pass(context.Background(), &s); !strings.Contains(refusedWhy(s, "clean"), "/srv/clean/films/one/backup") { + t.Errorf("the repository was not found by the next look: %+v", s.Refused) + } +} diff --git a/modules/nfs-server/cmd/nfs-server/shares.go b/modules/nfs-server/cmd/nfs-server/shares.go index 073ee0cbc..ca81ddf9e 100644 --- a/modules/nfs-server/cmd/nfs-server/shares.go +++ b/modules/nfs-server/cmd/nfs-server/shares.go @@ -244,33 +244,72 @@ const ( ) // LookKept is how long a look below a share that found no backup repository is believed. The look walks up -// to WalkLimit folders, which on a large library is too much for every 30 s pass; a repository put under a +// to WalkLimit folders, which on a large library takes minutes on a cold disk; a repository put under a // share is found within this long. A look that found one, or was cut short, is never kept: the share stays // refused and is looked at again on the next pass. const LookKept = time.Hour -// repositoryBelow finds a backup repository within WalkDepth levels below dir. complete is false when -// the look stopped at its bound before it saw everything. -func (m *Machine) repositoryBelow(dir string) (found string, complete bool) { - now := time.Now() - if m.Now != nil { - now = m.Now() +// lookBelow is the last look below one folder, and whether another runs now. +type lookBelow struct { + found string + complete bool + at time.Time + done bool // a look has finished; until then there is no result + running bool +} + +// repositoryBelow answers the last look below dir for a backup repository, and starts the next one beside +// the caller when there is none, or it was not clean, or LookKept has passed. The pass therefore never waits +// for the walk (issue 1971: a walk of minutes held the status's time back until the module was found +// unhealthy). pending is true while no look has finished yet; the share is refused until then, and while a +// kept clean look is being renewed the old result stands. complete is false when the look stopped at its +// bound or with the process. +func (m *Machine) repositoryBelow(dir string) (found string, complete, pending bool) { + m.cleanMu.Lock() + r := m.looks[dir] + if r == nil { + if m.looks == nil { + m.looks = map[string]*lookBelow{} + } + r = &lookBelow{} + m.looks[dir] = r + } + fresh := r.done && r.found == "" && r.complete && m.clock().Sub(r.at) < LookKept + start := !fresh && !r.running + if start { + r.running = true + } + m.cleanMu.Unlock() + if start { + walk := func() { + f, c := m.walkBelow(dir) + m.cleanMu.Lock() + defer m.cleanMu.Unlock() + r.running = false + if w := m.walking(); w != nil && w.Err() != nil { + return // ended by the process stopping: no result, the old one stands + } + r.found, r.complete, r.at, r.done = f, c, m.clock(), true + } + if m.Beside != nil { + m.Beside(walk) + } else { + go walk() + } } m.cleanMu.Lock() defer m.cleanMu.Unlock() - if at, kept := m.clean[dir]; kept && now.Sub(at) < LookKept { - return "", true + if !r.done { + return "", false, true } - found, complete = m.walkBelow(dir) - if found == "" && complete { - if m.clean == nil { - m.clean = map[string]time.Time{} - } - m.clean[dir] = now - } else { - delete(m.clean, dir) + return r.found, r.complete, false +} + +func (m *Machine) clock() time.Time { + if m.Now != nil { + return m.Now() } - return found, complete + return time.Now() } // walkBelow is the look itself, breadth first, bounded by WalkDepth and WalkLimit. @@ -411,11 +450,15 @@ func (m *Machine) Judge(c Config, roster map[string]string, wants map[string][]W no(fmt.Sprintf("The share %s's folder %s%s is inside the backup repository %s, and backups are never exported.", s.Name, real, via, inside)) continue } - holds, complete := m.repositoryBelow(real) + holds, complete, pending := m.repositoryBelow(real) if holds != "" { no(fmt.Sprintf("The share %s's folder %s%s holds a backup repository (%s), and backups are never exported: share the folders beside it instead.", s.Name, real, via, holds)) continue } + if pending { + no(fmt.Sprintf("The share %s's folder %s%s has not been looked through yet for a backup repository (the look runs beside the pass and takes a while on a large folder), so it is not exported yet.", s.Name, real, via)) + continue + } if !complete { no(fmt.Sprintf("The share %s's folder %s%s holds more folders than the module looks through (%d levels, at most %d folders), so it could not make sure no backup repository is under it: share smaller folders.", s.Name, real, via, WalkDepth, m.limit())) continue