diff --git a/core/dir.go b/core/dir.go index 1c54ff8..79bce7b 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) @@ -920,7 +936,32 @@ func (dh *DirHandle) CloseDir() error { return nil } -// Recursively resets the DirTime for child directories. +// dropLoadedRanges forgets every range the root remembers as already listed. +// 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 { + root = root.Parent + } + root.mu.Lock() + if root.dir != nil { + root.dir.Gaps = nil + } + root.mu.Unlock() +} + +// 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() @@ -967,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/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))) +} diff --git a/core/dir_gaps_test.go b/core/dir_gaps_test.go new file mode 100644 index 0000000..1e20513 --- /dev/null +++ b/core/dir_gaps_test.go @@ -0,0 +1,131 @@ +// 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" + "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 { + 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") +} + +// 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") +} 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{} 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 }