diff --git a/core/backend_s3.go b/core/backend_s3.go index fa768c9..6e81bad 100644 --- a/core/backend_s3.go +++ b/core/backend_s3.go @@ -335,7 +335,7 @@ func (s *S3Backend) detectBucketLocationByHEAD() (err error, isAws bool) { switch resp.StatusCode { case 200: // note that this only happen if the bucket is in us-east-1 - if len(s.config.Profile) == 0 && os.Getenv("AWS_ACCESS_KEY_ID") == "" { + if len(s.config.Profile) == 0 && os.Getenv("AWS_ACCESS_KEY_ID") == "" && !s.config.UseIAM { s.awsConfig.Credentials = credentials.AnonymousCredentials s3Log.Info().Msg("anonymous bucket detected") } @@ -933,12 +933,24 @@ func (s *S3Backend) copyObjectMultipart(size int64, from string, to string, mpuI params.ACL = &s.config.ACL } - resp, err := s.CreateMultipartUpload(params) - if err != nil { - return "", err + resp, createErr := s.CreateMultipartUpload(params) + if createErr != nil { + return "", createErr } mpuId = *resp.UploadId + // This helper owns uploads it creates; callers cannot resume them because + // their IDs are not returned on failure. Leave caller-supplied uploads alone. + defer func() { + if err != nil { + _, abortErr := s.AbortMultipartUpload(&s3.AbortMultipartUploadInput{ + Bucket: &s.bucket, Key: &to, UploadId: &mpuId, + }) + if abortErr != nil { + s3Log.Warnf("Abort multipart copy %s: %v", to, abortErr) + } + } + }() } partSizes := s.defaultCopyPartSizes(size) @@ -975,6 +987,10 @@ func (s *S3Backend) copyObjectMultipart(size int64, from string, to string, mpuI } func (s *S3Backend) CopyBlob(param *CopyBlobInput) (*CopyBlobOutput, error) { + return s.copyBlob(param, false) +} + +func (s *S3Backend) copyBlob(param *CopyBlobInput, forceMultipart bool) (*CopyBlobOutput, error) { metadataDirective := s3.MetadataDirectiveCopy if param.Metadata != nil { metadataDirective = s3.MetadataDirectiveReplace @@ -982,12 +998,12 @@ func (s *S3Backend) CopyBlob(param *CopyBlobInput) (*CopyBlobOutput, error) { from := s.bucket + "/" + param.Source - // Copy into the same object is used to just update metadata - // and should be very quick regardless of parameters - if param.Source != param.Destination { + // Prefer the optimized metadata-only self-copy when the backend supports it. + // Some S3 implementations enforce the single-copy size limit on self-copies too. + if forceMultipart || param.Source != param.Destination { // FIXME Remove additional HEAD query - if param.Size == nil || param.ETag == nil || (*param.Size > s.config.MultipartCopyThreshold && + if param.Size == nil || param.ETag == nil || ((forceMultipart || *param.Size > s.config.MultipartCopyThreshold) && (param.Metadata == nil || param.StorageClass == nil)) { params := &HeadBlobInput{Key: param.Source} @@ -1008,7 +1024,7 @@ func (s *S3Backend) CopyBlob(param *CopyBlobInput) (*CopyBlobOutput, error) { param.StorageClass = s.selectStorageClass(param.Size) } - if !s.gcs && *param.Size > s.config.MultipartCopyThreshold { + if !s.gcs && (forceMultipart || *param.Size > s.config.MultipartCopyThreshold) { reqId, err := s.copyObjectMultipart(int64(*param.Size), from, param.Destination, "", param.ETag, param.Metadata, param.StorageClass) if err != nil { return nil, err @@ -1056,6 +1072,13 @@ func (s *S3Backend) CopyBlob(param *CopyBlobInput) (*CopyBlobOutput, error) { req.Config.HTTPClient.Timeout = 15 * time.Minute err := req.Send() if err != nil { + // Some S3 implementations enforce the single-copy size limit even on a + // self-copy, which is only used to rewrite metadata. Fall back to a + // multipart copy rather than failing the metadata update. + var awsErr awserr.Error + if !s.gcs && param.Source == param.Destination && errors.As(err, &awsErr) && awsErr.Code() == "EntityTooLarge" { + return s.copyBlob(param, true) + } s3Log.Warn().Interface("params", params).Err(err).Msg("CopyObject failed") return nil, err } diff --git a/core/backend_s3_copy_test.go b/core/backend_s3_copy_test.go new file mode 100644 index 0000000..fe11be5 --- /dev/null +++ b/core/backend_s3_copy_test.go @@ -0,0 +1,159 @@ +package core + +import ( + "fmt" + "net/http" + "net/http/httptest" + "reflect" + "sync" + "testing" + + "github.com/tigrisdata/tigrisfs/core/cfg" +) + +func TestS3CopyBlobMetadataSelfCopy(t *testing.T) { + const size = uint64(100 * 1024 * 1024) + for _, tc := range []struct { + name string + copyError string + failPart bool + failCommit bool + withoutHint bool + wantErr bool + wantMPU bool + }{ + {name: "supported self-copy remains a single request"}, + {name: "oversized self-copy uses multipart", copyError: "EntityTooLarge", wantMPU: true}, + {name: "missing hints preserve source metadata", copyError: "EntityTooLarge", withoutHint: true, wantMPU: true}, + {name: "access denial is not retried as multipart", copyError: "AccessDenied", wantErr: true}, + {name: "part failure aborts owned upload", copyError: "EntityTooLarge", failPart: true, wantMPU: true, wantErr: true}, + {name: "completion failure aborts owned upload", copyError: "EntityTooLarge", failCommit: true, wantMPU: true, wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + var mu sync.Mutex + var copies, heads, starts, parts, completes, aborts int + var pending map[string]string + stored := map[string]string{"mtime": "100", "custom": "retained"} + wanted := map[string]string{"mtime": "200", "custom": "retained"} + if tc.withoutHint { + wanted = map[string]string{"mtime": "100", "custom": "retained"} + } + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + defer mu.Unlock() + failure := func(status int, code string) { + w.WriteHeader(status) + fmt.Fprintf(w, "%sfixture", code) + } + if r.URL.Path != "/testbucket/object.iso" { + t.Errorf("unexpected object path: %s", r.URL.Path) + failure(http.StatusBadRequest, "InvalidRequest") + return + } + query := r.URL.Query() + switch { + case r.Method == http.MethodHead: + heads++ + w.Header().Set("Content-Length", fmt.Sprint(size)) + w.Header().Set("ETag", `"original"`) + w.Header().Set("Last-Modified", "Mon, 01 Jan 2024 00:00:00 GMT") + w.Header().Set("X-Amz-Storage-Class", "STANDARD_IA") + w.Header().Set("X-Amz-Meta-Mtime", "100") + w.Header().Set("X-Amz-Meta-Custom", "retained") + case r.Method == http.MethodPost && query.Has("uploads"): + starts++ + pending = map[string]string{"mtime": r.Header.Get("X-Amz-Meta-Mtime"), "custom": r.Header.Get("X-Amz-Meta-Custom")} + if r.Header.Get("X-Amz-Storage-Class") != "STANDARD_IA" { + t.Error("multipart copy lost source storage class") + } + fmt.Fprint(w, "upload") + case r.Method == http.MethodPut && query.Get("uploadId") == "upload": + parts++ + if r.Header.Get("X-Amz-Copy-Source") != "testbucket/object.iso" || r.Header.Get("X-Amz-Copy-Source-If-Match") != `"original"` { + t.Error("multipart copy did not pin the original source/ETag") + } + if tc.failPart { + failure(http.StatusPreconditionFailed, "PreconditionFailed") + return + } + fmt.Fprint(w, `"part"`) + case r.Method == http.MethodPost && query.Get("uploadId") == "upload": + completes++ + if tc.failCommit { + failure(http.StatusForbidden, "AccessDenied") + return + } + stored = pending + fmt.Fprint(w, `"copied"`) + case r.Method == http.MethodDelete && query.Get("uploadId") == "upload": + aborts++ + w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodPut && len(query) == 0: + copies++ + if tc.copyError != "" { + status := http.StatusBadRequest + if tc.copyError == "AccessDenied" { + status = http.StatusForbidden + } + failure(status, tc.copyError) + return + } + stored = map[string]string{"mtime": r.Header.Get("X-Amz-Meta-Mtime"), "custom": r.Header.Get("X-Amz-Meta-Custom")} + fmt.Fprint(w, `"copied"`) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL) + failure(http.StatusBadRequest, "InvalidRequest") + } + })) + defer server.Close() + s, err := NewS3("testbucket", &cfg.FlagStorage{Endpoint: server.URL, MaxParallelParts: 2}, (&cfg.S3Config{ + Region: "us-east-1", AccessKey: "test", SecretKey: "test", + // The service's single-copy limit can be below the configured threshold. + MultipartCopyThreshold: 256 * 1024 * 1024, + }).Init()) + if err != nil { + t.Fatal(err) + } + in := &CopyBlobInput{Source: "object.iso", Destination: "object.iso"} + if !tc.withoutHint { + in.Size = PUInt64(size) + in.ETag = PString(`"original"`) + in.Metadata = map[string]*string{"mtime": PString("200"), "custom": PString("retained")} + } + _, err = s.CopyBlob(in) + if (err != nil) != tc.wantErr { + t.Fatalf("CopyBlob error = %v, want error %v", err, tc.wantErr) + } + mu.Lock() + defer mu.Unlock() + if copies != 1 { + t.Errorf("single-copy attempts = %d, want 1", copies) + } + if tc.wantMPU { + if heads != 1 || starts != 1 || parts == 0 { + t.Errorf("invalid multipart flow: heads=%d starts=%d parts=%d", heads, starts, parts) + } + wantAborts, wantCompletes := 0, 1 + if tc.wantErr { + wantAborts = 1 + } + if tc.failPart { + wantCompletes = 0 + } + if aborts != wantAborts || completes != wantCompletes { + t.Errorf("aborts/completes = %d/%d, want %d/%d", aborts, completes, wantAborts, wantCompletes) + } + } else { + if heads != 0 || starts != 0 || parts != 0 || completes != 0 || aborts != 0 { + t.Errorf("unexpected multipart activity: heads=%d starts=%d parts=%d completes=%d aborts=%d", heads, starts, parts, completes, aborts) + } + } + if tc.wantErr { + wanted = map[string]string{"mtime": "100", "custom": "retained"} + } + if !reflect.DeepEqual(stored, wanted) { + t.Errorf("stored metadata = %v, want %v", stored, wanted) + } + }) + } +} diff --git a/core/cfg/flags.go b/core/cfg/flags.go index 71dd48a..48dbd3f 100644 --- a/core/cfg/flags.go +++ b/core/cfg/flags.go @@ -277,7 +277,7 @@ MISC OPTIONS: cli.BoolFlag{ Name: "no-detect", - Usage: "Turn off bucket location and signature algorithm autodetection on start", + Usage: "Turn off autodetection of anonymous access, bucket location and signature algorithm on start", }, cli.BoolFlag{ @@ -621,8 +621,8 @@ MISC OPTIONS: cli.IntFlag{ Name: "read-retry-attempts", - Value: 0, - Usage: "Maximum read retry attempts (0 means unlimited)", + Value: 10, + Usage: "Maximum read retry attempts (minimum: 1)", }, cli.IntFlag{ @@ -874,6 +874,11 @@ func PopulateFlags(c *cli.Context) (ret *FlagStorage) { singlePart = 5 } + readRetryAttempts := c.Int("read-retry-attempts") + if readRetryAttempts < 1 { + panic("--read-retry-attempts must be at least 1") + } + flags := &FlagStorage{ // File system MountOptions: c.StringSlice("o"), @@ -901,7 +906,7 @@ func PopulateFlags(c *cli.Context) (ret *FlagStorage) { ReadRetryInterval: c.Duration("read-retry-interval"), ReadRetryMultiplier: c.Float64("read-retry-mul"), ReadRetryMax: c.Duration("read-retry-max-interval"), - ReadRetryAttempts: c.Int("read-retry-attempts"), + ReadRetryAttempts: readRetryAttempts, ReadAheadKB: uint64(c.Int("read-ahead")), SmallReadCount: uint64(c.Int("small-read-count")), SmallReadCutoffKB: uint64(c.Int("small-read-cutoff")), @@ -1130,6 +1135,7 @@ func DefaultFlags() *FlagStorage { StatCacheTTL: 30 * time.Second, HTTPTimeout: 30 * time.Second, RetryInterval: 30 * time.Second, + ReadRetryAttempts: 10, MaxDiskCacheFD: 512, RefreshFilename: ".invalidate", FlushFilename: ".fsyncdir", 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/file.go b/core/file.go index a2cf28d..9ca28b6 100644 --- a/core/file.go +++ b/core/file.go @@ -75,6 +75,18 @@ func (fs *Goofys) partNum(offset uint64) uint64 { )) } +func (fs *Goofys) numParts(size uint64) uint64 { + part := fs.partNum(size) + if part == fs.maxParts() { + return part + } + partOffset, _ := fs.partRange(part) + if partOffset < size { + part++ + } + return part +} + func (fs *Goofys) partRange(num uint64) (offset uint64, size uint64) { n := uint64(0) start := uint64(0) @@ -88,6 +100,13 @@ func (fs *Goofys) partRange(num uint64) (offset uint64, size uint64) { panic(fmt.Sprintf("Part number too large: %v", num)) } +func (fs *Goofys) maxParts() (parts uint64) { + for _, s := range fs.flags.PartSizes { + parts += s.PartCount + } + return parts +} + func (fs *Goofys) getMaxFileSize() (size uint64) { for _, s := range fs.flags.PartSizes { size += s.PartSize * s.PartCount @@ -1756,11 +1775,7 @@ func (inode *Inode) completeMultipart() { return } finalSize := inode.Attributes.Size - numParts := inode.fs.partNum(finalSize) - numPartOffset, _ := inode.fs.partRange(numParts) - if numPartOffset < finalSize { - numParts++ - } + numParts := inode.fs.numParts(finalSize) err := inode.copyUnmodifiedParts(numParts) if !(inode.CacheState == ST_CREATED || inode.CacheState == ST_MODIFIED) { // State changed, abort this flush (even if we get ENOENT) @@ -1860,11 +1875,18 @@ func (inode *Inode) SyncFile() (err error) { inode.forceFlush = true inode.mu.Unlock() inode.TryFlush(MAX_FLUSH_PRIORITY) - inode.fs.flusherMu.Lock() - if inode.fs.flushPending == 0 { - inode.fs.flusherCond.Wait() + inode.mu.Lock() + if inode.CacheState > ST_DEAD && inode.flushError == nil { + // Register for the wakeup while flush completion cannot change inode state. + inode.fs.flusherMu.Lock() + inode.mu.Unlock() + if inode.fs.flushPending == 0 { + inode.fs.flusherCond.Wait() + } + inode.fs.flusherMu.Unlock() + } else { + inode.mu.Unlock() } - inode.fs.flusherMu.Unlock() } inode.logFuse("Done SyncFile", err) return diff --git a/core/file_sync_test.go b/core/file_sync_test.go new file mode 100644 index 0000000..b14dd95 --- /dev/null +++ b/core/file_sync_test.go @@ -0,0 +1,88 @@ +// Copyright 2026 Yandex LLC +// +// 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 ( + "runtime" + "sync" + "testing" + "time" +) + +func TestSyncFileDoesNotMissFlushCompletion(t *testing.T) { + oldMaxProcs := runtime.GOMAXPROCS(1) + defer runtime.GOMAXPROCS(oldMaxProcs) + + fs := &Goofys{} + fs.flusherCond = sync.NewCond(&fs.flusherMu) + // A deleted inode with an active flush makes TryFlush a controlled no-op. + inode := &Inode{ + fs: fs, + CacheState: ST_DELETED, + IsFlushing: 1, + } + + fs.flusherMu.Lock() + syncDone := make(chan error, 1) + go func() { + syncDone <- inode.SyncFile() + }() + runtime.Gosched() + + // Keep SyncFile from registering its wait until flush completion changes the inode. + stateChanged := make(chan struct{}) + allowWakeup := make(chan struct{}) + go func() { + inode.mu.Lock() + inode.CacheState = ST_CACHED + inode.mu.Unlock() + close(stateChanged) + <-allowWakeup + fs.WakeupFlusher() + }() + runtime.Gosched() + + stateChangedBeforeWait := false + select { + case <-stateChanged: + stateChangedBeforeWait = true + default: + } + // Model the flusher consuming the completion wakeup before SyncFile can observe it. + fs.flusherCond.Broadcast() + fs.flusherMu.Unlock() + if !stateChangedBeforeWait { + close(allowWakeup) + } + + select { + case err := <-syncDone: + if stateChangedBeforeWait { + close(allowWakeup) + } + if err != nil { + t.Fatalf("SyncFile returned an error: %v", err) + } + case <-time.After(time.Second): + if stateChangedBeforeWait { + close(allowWakeup) + } + select { + case <-syncDone: + case <-time.After(time.Second): + } + t.Fatal("SyncFile missed the completed flush and kept waiting") + } +} 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/goofys_fuse.go b/core/goofys_fuse.go index 4f7386d..4dfab7e 100644 --- a/core/goofys_fuse.go +++ b/core/goofys_fuse.go @@ -401,7 +401,15 @@ func (fs *GoofysFuse) ReadDir( if n == 0 { break } - e.Ref() + // readdirPlus will not increase nlookup for . and .., which + // ReadDir yields at internal offsets 0 and 1; the position is + // still this entry's, since dh.Next advances it below. Comparing + // inode identity instead would race with a rename changing + // dh.inode.Parent between ReadDir returning it and the check, + // leaving the .. reference this is meant to avoid. + if dh.lastInternalOffset > 1 { + e.Ref() + } } else { e.mu.Lock() dirent = makeDirEntry(e, dh.lastExternalOffset) 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 } diff --git a/core/part_size_test.go b/core/part_size_test.go new file mode 100644 index 0000000..7959115 --- /dev/null +++ b/core/part_size_test.go @@ -0,0 +1,104 @@ +package core + +import ( + "testing" + + "github.com/tigrisdata/tigrisfs/core/cfg" +) + +func TestPartRangeTierBoundaries(t *testing.T) { + flags := cfg.DefaultFlags() + fs := &Goofys{flags: flags} + + const MiB = uint64(1024 * 1024) + + checks := []struct { + part uint64 + wantOffset uint64 + wantSize uint64 + }{ + {part: 999, wantOffset: 4995 * MiB, wantSize: 5 * MiB}, + {part: 1000, wantOffset: 5000 * MiB, wantSize: 25 * MiB}, + {part: 1001, wantOffset: 5025 * MiB, wantSize: 25 * MiB}, + {part: 1999, wantOffset: 29975 * MiB, wantSize: 25 * MiB}, + {part: 2000, wantOffset: 30000 * MiB, wantSize: 125 * MiB}, + } + + for _, check := range checks { + offset, size := fs.partRange(check.part) + if offset != check.wantOffset || size != check.wantSize { + t.Fatalf("partRange(%d) = (%d, %d), want (%d, %d)", + check.part, offset, size, check.wantOffset, check.wantSize) + } + } +} + +func TestNumPartsOnPartBoundaries(t *testing.T) { + flags := cfg.DefaultFlags() + fs := &Goofys{flags: flags} + + const MiB = uint64(1024 * 1024) + + checks := []struct { + size uint64 + want uint64 + }{ + {size: 0, want: 0}, + {size: 1, want: 1}, + {size: 5 * MiB, want: 1}, + {size: 5*MiB + 1, want: 2}, + {size: 5000 * MiB, want: 1000}, + {size: 5000*MiB + 1, want: 1001}, + {size: 5025 * MiB, want: 1001}, + {size: 30000 * MiB, want: 2000}, + {size: fs.getMaxFileSize(), want: 10000}, + } + + for _, check := range checks { + parts := fs.numParts(check.size) + if parts != check.want { + t.Fatalf("numParts(%d) = %d, want %d", check.size, parts, check.want) + } + } +} + +func TestNumPartsWithTenThousandFiveMiBParts(t *testing.T) { + const MiB = uint64(1024 * 1024) + + flags := cfg.DefaultFlags() + flags.PartSizes = []cfg.PartSizeConfig{ + {PartSize: 5 * MiB, PartCount: 10000}, + } + fs := &Goofys{flags: flags} + + const fileSize = 50000 * MiB + + if got := fs.partNum(fileSize); got != 10000 { + t.Fatalf("partNum(%d) = %d, want 10000", fileSize, got) + } + if got := fs.numParts(fileSize); got != 10000 { + t.Fatalf("numParts(%d) = %d, want 10000", fileSize, got) + } + + offset, size := fs.partRange(9999) + if offset != 49995*MiB || size != 5*MiB { + t.Fatalf("partRange(9999) = (%d, %d), want (%d, %d)", + offset, size, uint64(49995)*MiB, uint64(5)*MiB) + } + + assertPanics(t, func() { + fs.partRange(10000) + }) +} + +func assertPanics(t *testing.T, f func()) { + t.Helper() + + defer func() { + if recover() == nil { + t.Fatalf("function did not panic") + } + }() + + f() +}