From 44c9c1dc4f4594ddc2fe7a879f8bef73b3d3c064 Mon Sep 17 00:00:00 2001 From: Yevgeniy Firsov Date: Mon, 28 Sep 2026 16:50:11 -0700 Subject: [PATCH 1/5] fix: make the slurp gap cache work, and remember when a listing ran off the end Lookups on a directory whose stat cache has expired go to the server unless the root's loaded-range list says the key was already covered by a listing. That list has never held anything: markGapLoaded computed the final slice length one short, so the range it inserted was truncated away every time. With the list always empty, every such lookup issued a ListObjects and a HEAD. This dates to the commit that introduced slurp-on-lookup in 2022 and is the same upstream. Once ranges are kept, a second defect surfaces for the reported workload. A writer that keeps creating names sorting after every key on the server makes each lookup list from that name; the listing returns nothing, and the range was recorded up to the last key returned, or as empty. Nothing after it was ever known, so the next name listed again. Record a listing that ran off the end as open-ended instead, using a sentinel that sorts after any UTF-8 key. markGapLoaded is rewritten to keep the list sorted and disjoint by trimming overlapping ranges to their parts outside the new one, which also stops a narrow re-list inside an open-ended range from discarding what is known past its end. Tests cover insertion, overlap trimming, splitting a containing range, the end-of-listing sentinel, and expiry. On the previous code the first assertion fails: marking (a,m] leaves the list empty. --- core/dir.go | 78 ++++++++++++++++++------------ core/dir_gaps_test.go | 110 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 31 deletions(-) create mode 100644 core/dir_gaps_test.go diff --git a/core/dir.go b/core/dir.go index 1c54ff8..41537d7 100644 --- a/core/dir.go +++ b/core/dir.go @@ -37,6 +37,11 @@ type SlurpGap struct { loadTime time.Time } +// gapEndOfListing is the end of a loaded range that ran off the end of the +// listing: nothing follows its start on the server. A lone 0xFF byte cannot +// occur in a valid UTF-8 key, so it sorts after every key a listing returns. +const gapEndOfListing = "\xff" + type DirInodeData struct { mountPrefix string @@ -330,8 +335,15 @@ func (parent *Inode) listObjectsSlurp(inode *Inode, startAfter string, sealEnd b nextStartAfter = *obj.Key } - // Remember this range as already loaded - parent.dir.markGapLoaded(NilStr(startWith), nextStartAfter) + // Remember this range as already loaded. A listing that ran off the end + // proves nothing follows startWith, so record the range as open-ended: + // bounding it at the last key returned would send every later lookup of a + // name sorting past that key back to the server. + loadedEnd := nextStartAfter + if seal || !resp.IsTruncated { + loadedEnd = gapEndOfListing + } + parent.dir.markGapLoaded(NilStr(startWith), loadedEnd) if lock { parent.mu.Unlock() @@ -340,38 +352,42 @@ func (parent *Inode) listObjectsSlurp(inode *Inode, startAfter string, sealEnd b return } +// markGapLoaded records (start < key <= end] as freshly listed. Gaps stay +// sorted and disjoint: overlapping ranges are cut down to the parts outside the +// new one, keeping their own load time, so a narrow re-list inside a wider +// open-ended range does not discard what is known past its end. func (dir *DirInodeData) markGapLoaded(start, end string) { - pos := 0 - if start != "" { - pos = sort.Search(len(dir.Gaps), func(i int) bool { - return dir.Gaps[i].start >= start - }) - } - for pos > 0 && dir.Gaps[pos-1].end > start { - pos-- - } - endPos := sort.Search(len(dir.Gaps), func(i int) bool { - return dir.Gaps[i].start >= end - }) - if pos < len(dir.Gaps) && dir.Gaps[pos].start < start { - dir.Gaps[pos].end = start - pos++ - } - if endPos > 0 && dir.Gaps[endPos-1].end > end { - dir.Gaps[endPos-1].start = end - endPos-- - } - l := len(dir.Gaps) - (endPos - pos) - if pos == endPos { - dir.Gaps = append(dir.Gaps, nil) + fresh := &SlurpGap{start: start, end: end, loadTime: time.Now()} + out := make([]*SlurpGap, 0, len(dir.Gaps)+2) + inserted := false + for _, g := range dir.Gaps { + if g.end <= start { + out = append(out, g) + continue + } + if g.start >= end { + if !inserted { + out = append(out, fresh) + inserted = true + } + out = append(out, g) + continue + } + if g.start < start { + out = append(out, &SlurpGap{start: g.start, end: start, loadTime: g.loadTime}) + } + if !inserted { + out = append(out, fresh) + inserted = true + } + if g.end > end { + out = append(out, &SlurpGap{start: end, end: g.end, loadTime: g.loadTime}) + } } - copy(dir.Gaps[pos+1:], dir.Gaps[endPos:]) - dir.Gaps[pos] = &SlurpGap{ - start: start, - end: end, - loadTime: time.Now(), + if !inserted { + out = append(out, fresh) } - dir.Gaps = dir.Gaps[0:l] + dir.Gaps = out } // LOCKS_REQUIRED(inode.mu) diff --git a/core/dir_gaps_test.go b/core/dir_gaps_test.go new file mode 100644 index 0000000..e17c7f5 --- /dev/null +++ b/core/dir_gaps_test.go @@ -0,0 +1,110 @@ +// Copyright 2026 Tigris Data, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package core + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func gapRanges(d *DirInodeData) [][2]string { + out := make([][2]string, 0, len(d.Gaps)) + for _, g := range d.Gaps { + out = append(out, [2]string{g.start, g.end}) + } + return out +} + +// The slurp cache was inert: markGapLoaded's final-length arithmetic was one +// short, so the range it inserted was truncated away every time. With Gaps +// always empty, every lookup on an expired directory went back to the server +// for a listing and a HEAD. +func TestMarkGapLoadedKeepsWhatItInserts(t *testing.T) { + old := time.Now().Add(-time.Hour) + d := &DirInodeData{} + + d.markGapLoaded("a", "m") + require.Equal(t, [][2]string{{"a", "m"}}, gapRanges(d)) + require.True(t, d.checkGapLoaded("b", old)) + require.False(t, d.checkGapLoaded("a", old), "start is exclusive: StartAfter never returns it") + require.True(t, d.checkGapLoaded("m", old), "end is inclusive") + require.False(t, d.checkGapLoaded("z", old)) + + d.markGapLoaded("p", "s") + require.Equal(t, [][2]string{{"a", "m"}, {"p", "s"}}, gapRanges(d), "append after") + + d.markGapLoaded("0", "5") + require.Equal(t, [][2]string{{"0", "5"}, {"a", "m"}, {"p", "s"}}, gapRanges(d), "insert before") +} + +func TestMarkGapLoadedOverlaps(t *testing.T) { + d := &DirInodeData{} + d.markGapLoaded("a", "m") + d.markGapLoaded("p", "s") + + d.markGapLoaded("k", "q") + require.Equal(t, [][2]string{{"a", "k"}, {"k", "q"}, {"q", "s"}}, gapRanges(d), + "overlapping neighbours are trimmed to the parts outside the new range") + + d.markGapLoaded("b", "r") + require.Equal(t, [][2]string{{"a", "b"}, {"b", "r"}, {"r", "s"}}, gapRanges(d), + "a new range covering several old ones replaces their interiors") + + d.markGapLoaded("", gapEndOfListing) + require.Equal(t, [][2]string{{"", gapEndOfListing}}, gapRanges(d), "a full listing subsumes everything") +} + +// A narrow re-list inside an open-ended range must not throw away what is +// known past its end, or the next lookup of a later name lists again. +func TestMarkGapLoadedSplitsAContainingRange(t *testing.T) { + old := time.Now().Add(-time.Hour) + d := &DirInodeData{} + d.markGapLoaded("a", gapEndOfListing) + + d.markGapLoaded("f", "h") + require.Equal(t, [][2]string{{"a", "f"}, {"f", "h"}, {"h", gapEndOfListing}}, gapRanges(d)) + require.True(t, d.checkGapLoaded("zzz", old), "still known that nothing follows h") +} + +// This is the reported symptom: an appending writer creates names sorting +// after every key on the server. A listing from such a name returns nothing, +// and must be remembered as "nothing after this", not as an empty range. +func TestGapRecordsEndOfListing(t *testing.T) { + old := time.Now().Add(-time.Hour) + d := &DirInodeData{} + + // What the slurp records when the listing is not truncated. + d.markGapLoaded("dir/file-0100", gapEndOfListing) + + for _, later := range []string{"dir/file-0101", "dir/file-0101/", "dir/file-9999", "zzz"} { + require.True(t, d.checkGapLoaded(later, old), "%q sorts after the last listed key", later) + } + require.False(t, d.checkGapLoaded("dir/file-0099", old), "earlier names were not listed") + + // Keys are UTF-8, so the sentinel must sort after any of them. + require.True(t, d.checkGapLoaded("\U0010FFFF", old)) + require.True(t, d.checkGapLoaded("\xf4\x8f\xbf\xbf", old)) +} + +func TestCheckGapLoadedDropsStaleRanges(t *testing.T) { + d := &DirInodeData{} + d.markGapLoaded("a", "m") + d.Gaps[0].loadTime = time.Now().Add(-2 * time.Minute) + + require.False(t, d.checkGapLoaded("b", time.Now().Add(-time.Minute)), "older than the TTL") + require.Empty(t, d.Gaps, "a stale range is evicted on lookup") +} From 79a5cd3e0106dda9ca42a77fbc11a83f4683932f Mon Sep 17 00:00:00 2001 From: Yevgeniy Firsov Date: Mon, 28 Sep 2026 16:50:11 -0700 Subject: [PATCH 2/5] fix: actually emit loaded ranges in the inode debug dump The dump built a map per range and never appended it, so the gaps field was always empty. Append it, and render the end-of-listing sentinel readably. --- core/handles.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/core/handles.go b/core/handles.go index 2306858..49b5b2b 100644 --- a/core/handles.go +++ b/core/handles.go @@ -1008,8 +1008,13 @@ func (inode *Inode) DumpThis(fn string, withBuffers bool, noLock bool) (children for _, gap := range inode.dir.Gaps { m := make(map[string]interface{}) m["start"] = gap.start - m["end"] = gap.end + if gap.end == gapEndOfListing { + m["end"] = "" + } else { + m["end"] = gap.end + } m["loadTime"] = gap.loadTime + gaps = append(gaps, m) } dirData["gaps"] = gaps } From fc3e70c52a83d2542e8c80e2106ffe8c4110ac28 Mon Sep 17 00:00:00 2001 From: Yevgeniy Firsov Date: Mon, 28 Sep 2026 16:51:28 -0700 Subject: [PATCH 3/5] test: lookups past the last listed key must not list again End-to-end version of the reported symptom: a directory with three files, an expired stat cache, and nine lookups of names sorting after the last key. Every lookup used to cost two ListObjects and a HEAD, because the listing that found nothing past the name was never remembered as covering what follows. Counted through a TestBackend wrapper against a live Tigris bucket. Before the fix the listing count went 3 -> 21 over the nine lookups; after it, the later lookups add no listings and no HEADs. --- core/dir_gaps_e2e_test.go | 77 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 core/dir_gaps_e2e_test.go diff --git a/core/dir_gaps_e2e_test.go b/core/dir_gaps_e2e_test.go new file mode 100644 index 0000000..c16f735 --- /dev/null +++ b/core/dir_gaps_e2e_test.go @@ -0,0 +1,77 @@ +// Copyright 2026 Tigris Data, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package core + +import ( + "fmt" + "sync/atomic" + "syscall" + "time" + + . "gopkg.in/check.v1" +) + +// A writer that keeps creating files whose names sort after every key on the +// server used to make every lookup of the next name list from the server: the +// listing found nothing past the name, and that was never remembered. +func (s *GoofysTest) TestLookupPastLastKeyDoesNotRelist(t *C) { + s.fs.flags.StatCacheTTL = time.Minute + root := s.getRoot(t) + + s.setupBlobs(s.cloud, t, map[string]*string{ + "gapdir/file-0001": nil, + "gapdir/file-0002": nil, + "gapdir/file-0003": nil, + }) + + var lists, heads int64 + s3 := root.fs.getCloud() + cloud := NewTestBackend(&TestBackend{StorageBackend: s3}) + cloud.ListBlobsFunc = func(p *ListBlobsInput) (*ListBlobsOutput, error) { + atomic.AddInt64(&lists, 1) + return s3.ListBlobs(p) + } + cloud.HeadBlobFunc = func(p *HeadBlobInput) (*HeadBlobOutput, error) { + atomic.AddInt64(&heads, 1) + return s3.HeadBlob(p) + } + root.fs.setCloud(cloud) + + // Force the directory to be consulted from the server. + root.mu.Lock() + root.dir.DirTime = time.Time{} + root.mu.Unlock() + dir, err := root.LookUpCached("gapdir") + t.Assert(err, IsNil) + dir.mu.Lock() + dir.dir.DirTime = time.Time{} + dir.mu.Unlock() + + // The first unknown name past the last key may have to list once. + _, err = dir.LookUpCached("file-0100") + t.Assert(err, Equals, syscall.ENOENT) + listsAfterFirst := atomic.LoadInt64(&lists) + headsAfterFirst := atomic.LoadInt64(&heads) + + // Every later name is covered by that listing: nothing follows it. + for i := 101; i < 110; i++ { + _, err = dir.LookUpCached(fmt.Sprintf("file-%04d", i)) + t.Assert(err, Equals, syscall.ENOENT) + } + t.Assert(atomic.LoadInt64(&lists), Equals, listsAfterFirst, + Commentf("later names must not list again; lists went %d -> %d", listsAfterFirst, atomic.LoadInt64(&lists))) + t.Assert(atomic.LoadInt64(&heads), Equals, headsAfterFirst, + Commentf("later names must not HEAD; heads went %d -> %d", headsAfterFirst, atomic.LoadInt64(&heads))) +} From 9c099a158a54a00285e11a933c81dc8521be5e3c Mon Sep 17 00:00:00 2001 From: Yevgeniy Firsov Date: Mon, 28 Sep 2026 17:06:58 -0700 Subject: [PATCH 4/5] fix: an explicit refresh must forget the loaded ranges too With the loaded-range cache now actually holding entries, LookUp serves a name from cache whenever a fresh range covers it. The .invalidate refresh reset the subtree's DirTime and AttrTime but left those ranges alone, so the lookup it provoked returned the very entries it had just expired: after a remote overwrite and a refresh, a file still read its old contents. The four notify-refresh tests caught it. resetDirTimeRec now drops every range the root remembers before resetting times. Over-invalidation is deliberate: a refresh is explicit and rare, and the cost is one listing per lookup until ranges are re-learned. The recursion is split into an inner function so the ranges are dropped once, not per node. --- core/dir.go | 26 +++++++++++++++++++++++++- core/dir_gaps_test.go | 21 +++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/core/dir.go b/core/dir.go index 41537d7..a7af669 100644 --- a/core/dir.go +++ b/core/dir.go @@ -939,6 +939,30 @@ func (dh *DirHandle) CloseDir() error { // Recursively resets the DirTime for child directories. // ACQUIRES_LOCK(inode.mu) func (inode *Inode) resetDirTimeRec() { + // Loaded ranges live on the root and let LookUp serve a name from cache + // without asking the server. An explicit refresh must defeat that too, or + // the entries it just expired are handed straight back. + inode.dropLoadedRanges() + inode.resetDirTimeRecInner() +} + +// dropLoadedRanges forgets every range the root remembers as already listed. +// Over-invalidation is deliberate: a refresh is explicit and rare, and the +// cost is one listing per lookup until ranges are re-learned. +// LOCKS_EXCLUDED(inode.mu) +func (inode *Inode) dropLoadedRanges() { + root := inode + for root.Parent != nil { + root = root.Parent + } + root.mu.Lock() + if root.dir != nil { + root.dir.Gaps = nil + } + root.mu.Unlock() +} + +func (inode *Inode) resetDirTimeRecInner() { inode.mu.Lock() inode.SetAttrTime(time.Time{}) if inode.dir == nil { @@ -954,7 +978,7 @@ func (inode *Inode) resetDirTimeRec() { copy(children, inode.dir.Children) inode.mu.Unlock() for _, child := range children { - child.resetDirTimeRec() + child.resetDirTimeRecInner() } } diff --git a/core/dir_gaps_test.go b/core/dir_gaps_test.go index e17c7f5..1e20513 100644 --- a/core/dir_gaps_test.go +++ b/core/dir_gaps_test.go @@ -19,8 +19,11 @@ import ( "time" "github.com/stretchr/testify/require" + "github.com/tigrisdata/tigrisfs/core/cfg" ) +func cfgDefaultFlagsForTest() *cfg.FlagStorage { return cfg.DefaultFlags() } + func gapRanges(d *DirInodeData) [][2]string { out := make([][2]string, 0, len(d.Gaps)) for _, g := range d.Gaps { @@ -108,3 +111,21 @@ func TestCheckGapLoadedDropsStaleRanges(t *testing.T) { require.False(t, d.checkGapLoaded("b", time.Now().Add(-time.Minute)), "older than the TTL") require.Empty(t, d.Gaps, "a stale range is evicted on lookup") } + +// An explicit refresh must forget the loaded ranges too, or LookUp keeps +// serving the entries the refresh just expired straight from cache. +func TestResetDirTimeDropsLoadedRanges(t *testing.T) { + fs := &Goofys{flags: cfgDefaultFlagsForTest()} + root := NewInode(fs, nil, "") + root.ToDir() + sub := NewInode(fs, root, "sub") + sub.ToDir() + file := NewInode(fs, sub, "file") + root.dir.markGapLoaded("", gapEndOfListing) + require.NotEmpty(t, root.dir.Gaps) + + // resetDirTimeRec calls this first; the rest of it needs a mounted fs, so + // the wiring is covered by the notify-refresh tests over FUSE. + file.dropLoadedRanges() + require.Empty(t, root.dir.Gaps, "refreshing any inode drops the root's ranges") +} From 8b6d9cefa475e74c6675c3ffda0b77e4e009fb56 Mon Sep 17 00:00:00 2001 From: Yevgeniy Firsov Date: Mon, 28 Sep 2026 17:33:34 -0700 Subject: [PATCH 5/5] fix: drop loaded ranges at the invalidation entry points, not inside the recursive reset resetDirTimeRec took the root lock to drop the loaded ranges, but mount calls it while holding the parent's lock. For a mount directly under the root that is the same mutex, which is not reentrant, so the mount hung and left the root locked against every later lookup. For a nested mount it took the root lock while holding a child's, inverting the parent-before-child order and racing a concurrent lookup for a deadlock. Keep the recursive reset lock-free as it was. Drop the ranges at each entry point instead, where no inode lock is held: RefreshInodeCache, ResetForUnmount, and mount before it takes the parent's lock. dropLoadedRanges documents that it takes only the root lock and must be called with none held. Raised in review on #54. --- core/dir.go | 32 +++++++++++++++++--------------- core/goofys.go | 5 +++++ 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/core/dir.go b/core/dir.go index a7af669..79bce7b 100644 --- a/core/dir.go +++ b/core/dir.go @@ -936,20 +936,16 @@ func (dh *DirHandle) CloseDir() error { return nil } -// Recursively resets the DirTime for child directories. -// ACQUIRES_LOCK(inode.mu) -func (inode *Inode) resetDirTimeRec() { - // Loaded ranges live on the root and let LookUp serve a name from cache - // without asking the server. An explicit refresh must defeat that too, or - // the entries it just expired are handed straight back. - inode.dropLoadedRanges() - inode.resetDirTimeRecInner() -} - // dropLoadedRanges forgets every range the root remembers as already listed. -// Over-invalidation is deliberate: a refresh is explicit and rare, and the -// cost is one listing per lookup until ranges are re-learned. -// LOCKS_EXCLUDED(inode.mu) +// Those ranges let LookUp answer a name from cache without asking the server, +// so anything that invalidates cached state must drop them too, or the entries +// it just expired are handed straight back. Over-invalidation is deliberate: a +// refresh is explicit and rare, and the cost is one listing per lookup until +// ranges are re-learned. +// +// Takes only the root lock, so the caller must hold no inode lock at all: the +// locking order is parent before child, and the root is everyone's parent. +// LOCKS_EXCLUDED(any inode.mu) func (inode *Inode) dropLoadedRanges() { root := inode for root.Parent != nil { @@ -962,7 +958,12 @@ func (inode *Inode) dropLoadedRanges() { root.mu.Unlock() } -func (inode *Inode) resetDirTimeRecInner() { +// Recursively resets the DirTime for child directories. Does not touch the +// root's loaded ranges: callers that invalidate cached state drop those +// themselves, at a point where they hold no lock (see dropLoadedRanges), since +// mount calls this with the parent's lock held. +// ACQUIRES_LOCK(inode.mu) +func (inode *Inode) resetDirTimeRec() { inode.mu.Lock() inode.SetAttrTime(time.Time{}) if inode.dir == nil { @@ -978,7 +979,7 @@ func (inode *Inode) resetDirTimeRecInner() { copy(children, inode.dir.Children) inode.mu.Unlock() for _, child := range children { - child.resetDirTimeRecInner() + child.resetDirTimeRec() } } @@ -1007,6 +1008,7 @@ func (inode *Inode) ResetForUnmount() { inode.mu.Unlock() // Reset DirTime for recursively for this node and all its child nodes. // Note: resetDirTimeRec should be called without holding the lock. + inode.dropLoadedRanges() inode.resetDirTimeRec() } diff --git a/core/goofys.go b/core/goofys.go index cad6754..ea83d79 100644 --- a/core/goofys.go +++ b/core/goofys.go @@ -845,6 +845,10 @@ func (fs *Goofys) mount(mp *Inode, b *Mount) { mp = dirInode } + // A mount over an existing directory resets that subtree below, while + // holding mp.mu; the root's loaded ranges must go too, and taking the + // root lock needs no other lock held, so do it here first. + mp.dropLoadedRanges() mp.mu.Lock() defer mp.mu.Unlock() @@ -920,6 +924,7 @@ func (fs *Goofys) RefreshInodeCache(inode *Inode) error { name := inode.Name inodeId := inode.Id inode.mu.Unlock() + inode.dropLoadedRanges() inode.resetDirTimeRec() var mappedErr error var notifications []interface{}