From 73025824a3c61d2928b536c86f2e11de52ef624e Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 5 Nov 2026 02:39:26 +0800 Subject: [PATCH] fix(storage): also remove the empty object directory when the xl.meta commit fails The xl.meta rename at the end of RenameData creates the object directory the same way the data rename does, and its failure branch left that directory behind for writes without a data directory, such as inline and zero-byte objects. Remove it and its empty parents there too, with the same trailing-slash call that PR #286 adds to the data branch for issue #285. Making the cleanup safe against concurrent writes needs two more changes: - deleteFile now walks up with a trailing slash, so each parent step removes only an empty directory. Between two steps of a walk, another walk can remove the rest of the chain and a writer can commit an object whose xl.meta sits at the path the first walk removes next, for example the object "parent" under the key "parent/xl.meta/child". The walk used to delete that file. Ordinary deletes could already do this; the new cleanup made it reachable with failed writes. - The two renames of RenameData retry when the directory creation or the rename reports "not found" while the source is still in place, up to once per path level. A late, failed RenameData can now remove the directory another write of the same key has just created, and that write used to fail on the drive although its data was intact. The retry decides on the error of the attempt itself, because another write can recreate the directories in between. renameAll's error mapping moves unchanged into renameAllErr so that the retry can see the raw error. The stagedDataGoneDisks test helper now swaps the drives behind the set's disk list under the lock that the list takes, instead of replacing the getDisks callback that background tasks read unlocked. Tests: - TestXLStorageRenameDataCommitMetaMissingRemovesEmptyDirs: zero-byte and inline commit failures leave no empty directories, a sibling and a moved data directory stay. - TestXLStorageWalkUpKeepsCommittedObject: a committed object at a parent path survives the walk, through DeleteVersion and both cleanup branches. - TestRenameDataSurvivesConcurrentEmptyDirCleanup: PutObject (large and inline) and CompleteMultipartUpload succeed when a late failed RenameData of the same key removes their fresh directory on all 16 drives. - TestXLStorageRenameDataRetriesWhileCleanupWalksUp: a write survives a cleanup that removes each level of its path in turn, also when a nested-key write recreates the directories in between. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Feng Ruohang --- cmd/erasure-object_test.go | 145 ++++++++++++-- cmd/os-reliable.go | 55 +++--- cmd/xl-storage-errors_test.go | 347 ++++++++++++++++++++++++++++++++++ cmd/xl-storage.go | 50 ++++- 4 files changed, 557 insertions(+), 40 deletions(-) diff --git a/cmd/erasure-object_test.go b/cmd/erasure-object_test.go index c397b1709..bb0f896e2 100644 --- a/cmd/erasure-object_test.go +++ b/cmd/erasure-object_test.go @@ -28,10 +28,15 @@ import ( "os" "path/filepath" "runtime" + "slices" "strconv" + "sync" + "sync/atomic" "testing" + "time" "github.com/dustin/go-humanize" + "github.com/minio/madmin-go/v3" "github.com/minio/minio/internal/config/storageclass" ) @@ -1318,20 +1323,20 @@ func (d stagedDataGoneDisk) RenameData(ctx context.Context, srcVolume, srcPath s // of a write right before RenameData, as when the caller has already given up // and removed it. It returns a function that restores the drives. func stagedDataGoneDisks(z *erasureServerPools) func() { - xl := z.serverPools[0].sets[0] - getDisks := xl.getDisks - disks := getDisks() - wrapped := make([]StorageAPI, len(disks)) + // Swap the drives behind the set's getDisks under the mutex that + // getDisks takes, so that background readers do not race the swap. + sets := z.serverPools[0] + sets.erasureDisksMu.Lock() + disks := sets.erasureDisks[0] + original := slices.Clone(disks) for i, disk := range disks { - wrapped[i] = stagedDataGoneDisk{StorageAPI: disk} + disks[i] = stagedDataGoneDisk{StorageAPI: disk} } - z.serverPools[0].erasureDisksMu.Lock() - xl.getDisks = func() []StorageAPI { return wrapped } - z.serverPools[0].erasureDisksMu.Unlock() + sets.erasureDisksMu.Unlock() return func() { - z.serverPools[0].erasureDisksMu.Lock() - xl.getDisks = getDisks - z.serverPools[0].erasureDisksMu.Unlock() + sets.erasureDisksMu.Lock() + copy(sets.erasureDisks[0], original) + sets.erasureDisksMu.Unlock() } } @@ -1428,3 +1433,121 @@ func TestRenameDataStagedDataGoneKeepsExistingObject(t *testing.T) { t.Error("existing object content changed") } } + +// A failed RenameData that runs late removes its object directory while it is +// empty. When it does so on a drive between the directory creation and the +// rename of another write of the same key, that write must create the +// directory again and succeed instead of failing on the drive. +func TestRenameDataSurvivesConcurrentEmptyDirCleanup(t *testing.T) { + if runtime.GOOS == globalWindowsOSName { + t.Skip("directory creation is not traced on Windows") + } + for _, tc := range []struct { + name string + size int + multipart bool + }{ + {"put", 8 << 20, false}, + {"inline_put", 1 << 10, false}, + {"multipart", 8 << 20, true}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := t.Context() + obj, dirs, err := prepareErasure16(ctx) + if err != nil { + t.Fatal(err) + } + defer obj.Shutdown(context.Background()) + defer removeRoots(dirs) + z := obj.(*erasureServerPools) + const bucket, object = "bucket", "day=1/b=9/object" + if err := z.MakeBucket(ctx, bucket, MakeBucketOptions{}); err != nil { + t.Fatal(err) + } + body := bytes.Repeat([]byte{'a'}, tc.size) + var uploadID, etag string + if tc.multipart { + res, err := z.NewMultipartUpload(ctx, bucket, object, ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + pi, err := z.PutObjectPart(ctx, bucket, object, res.UploadID, 1, mustGetPutObjReader(t, bytes.NewReader(body), int64(len(body)), "", ""), ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + uploadID, etag = res.UploadID, pi.ETag + } + + drives := make(map[string]*xlStorage) + for _, disk := range z.serverPools[0].sets[0].getDisks() { + s := disk.(*xlStorageDiskIDCheck).storage + drives[filepath.Join(s.drivePath, bucket, filepath.FromSlash(object))] = s + } + var mu sync.Mutex + hit := make(map[string]bool) + var hits atomic.Int32 + var lateErrs []error + traceOSCalls(t, func(tr madmin.TraceInfo) { + if tr.FuncName != "os.Mkdir" || tr.Error != "" { + return + } + s := drives[tr.Path] + if s == nil { + return + } + mu.Lock() + first := !hit[tr.Path] + hit[tr.Path] = true + mu.Unlock() + if !first { + return + } + // The write has just created the object directory on this + // drive. A late RenameData of the same key, whose staged data + // is gone, fails now and removes the empty directory. + fi := newFileInfo(object, 8, 8) + fi.DataDir, fi.VersionID = mustGetUUID(), mustGetUUID() + fi.Erasure.Index = 1 + fi.ModTime = time.Now().UTC() + fi.Size = 1 + fi.AddObjectPart(1, "", 1, 1, fi.ModTime, nil, nil) + _, err := s.RenameData(context.Background(), minioMetaTmpBucket, mustGetUUID(), fi, bucket, object, RenameOptions{}) + if _, serr := os.Stat(tr.Path); !os.IsNotExist(serr) { + err = fmt.Errorf("late RenameData left %s (%v, rename error %v)", tr.Path, serr, err) + } else if errors.Is(err, errFileNotFound) { + err = nil + } + mu.Lock() + if err != nil { + lateErrs = append(lateErrs, err) + } + mu.Unlock() + hits.Add(1) + }) + + if tc.multipart { + _, err = z.CompleteMultipartUpload(ctx, bucket, object, uploadID, []CompletePart{{PartNumber: 1, ETag: etag}}, ObjectOptions{}) + } else { + _, err = z.PutObject(ctx, bucket, object, mustGetPutObjReader(t, bytes.NewReader(body), int64(len(body)), "", ""), ObjectOptions{}) + } + if n := hits.Load(); n != int32(len(dirs)) { + t.Fatalf("late cleanup ran on %d drives, want %d", n, len(dirs)) + } + mu.Lock() + for _, lateErr := range lateErrs { + t.Error(lateErr) + } + mu.Unlock() + if err != nil { + t.Fatalf("write failed after a concurrent cleanup: %v", err) + } + var buf bytes.Buffer + if err := GetObject(ctx, z, bucket, object, 0, int64(len(body)), &buf, "", ObjectOptions{}); err != nil { + t.Fatal(err) + } + if !bytes.Equal(buf.Bytes(), body) { + t.Fatal("object content differs") + } + }) + } +} diff --git a/cmd/os-reliable.go b/cmd/os-reliable.go index 3561cbd33..b333af63c 100644 --- a/cmd/os-reliable.go +++ b/cmd/os-reliable.go @@ -142,32 +142,37 @@ func renameAll(srcFilePath, dstFilePath, baseDir string) (err error) { return err } - if err = reliableRename(srcFilePath, dstFilePath, baseDir); err != nil { - switch { - case isSysErrNotDir(err) && !osIsNotExist(err): - // Windows can have both isSysErrNotDir(err) and osIsNotExist(err) returning - // true if the source file path contains an non-existent directory. In that case, - // we want to return errFileNotFound instead, which will honored in subsequent - // switch cases - return errFileAccessDenied - case isSysErrPathNotFound(err): - // This is a special case should be handled only for - // windows, because windows API does not return "not a - // directory" error message. Handle this specifically here. - return errFileAccessDenied - case isSysErrCrossDevice(err): - return fmt.Errorf("%w (%s)->(%s)", errCrossDeviceLink, srcFilePath, dstFilePath) - case osIsNotExist(err): - return errFileNotFound - case osIsExist(err): - // This is returned only when destination is a directory and we - // are attempting a rename from file to directory. - return errIsNotRegular - default: - return err - } + return renameAllErr(reliableRename(srcFilePath, dstFilePath, baseDir), srcFilePath, dstFilePath) +} + +// renameAllErr converts an error of reliableRename into the error that +// renameAll returns. +func renameAllErr(err error, srcFilePath, dstFilePath string) error { + switch { + case err == nil: + return nil + case isSysErrNotDir(err) && !osIsNotExist(err): + // Windows can have both isSysErrNotDir(err) and osIsNotExist(err) returning + // true if the source file path contains an non-existent directory. In that case, + // we want to return errFileNotFound instead, which will honored in subsequent + // switch cases + return errFileAccessDenied + case isSysErrPathNotFound(err): + // This is a special case should be handled only for + // windows, because windows API does not return "not a + // directory" error message. Handle this specifically here. + return errFileAccessDenied + case isSysErrCrossDevice(err): + return fmt.Errorf("%w (%s)->(%s)", errCrossDeviceLink, srcFilePath, dstFilePath) + case osIsNotExist(err): + return errFileNotFound + case osIsExist(err): + // This is returned only when destination is a directory and we + // are attempting a rename from file to directory. + return errIsNotRegular + default: + return err } - return nil } // Reliably retries os.RenameAll if for some reason os.RenameAll returns diff --git a/cmd/xl-storage-errors_test.go b/cmd/xl-storage-errors_test.go index b9d12fc4d..547ab6812 100644 --- a/cmd/xl-storage-errors_test.go +++ b/cmd/xl-storage-errors_test.go @@ -18,6 +18,7 @@ package cmd import ( + "bytes" "context" "errors" "net/http" @@ -25,9 +26,12 @@ import ( "path/filepath" "runtime" "sync" + "sync/atomic" "syscall" "testing" "time" + + "github.com/minio/madmin-go/v3" ) func TestSysErrors(t *testing.T) { @@ -179,3 +183,346 @@ func TestXLStorageRenameDataCommitMetaMissing(t *testing.T) { }) } } + +// A failed commit must not leave behind the object directory that renameAll() +// created for it when nothing was moved into it: once the other objects under +// its parent are gone, it keeps the parent and every ancestor listed as an +// empty prefix. A data directory that was already moved keeps the directory. +func TestXLStorageRenameDataCommitMetaMissingRemovesEmptyDirs(t *testing.T) { + const object = "day=1/b=9/object" + for _, tc := range []struct { + name string + data string // "" (zero bytes), "inline" or "dir" + sibling bool // another object under day=1/b=9/ + }{ + {"empty_object", "", false}, + {"inline_object_with_sibling", "inline", true}, + {"data_directory", "dir", false}, + } { + t.Run(tc.name, func(t *testing.T) { + disk, root, err := newXLStorageTestSetup(t) + if err != nil { + t.Fatal(err) + } + defer disk.Close() + s := disk.storage + if err := s.MakeVol(t.Context(), "bucket"); err != nil { + t.Fatal(err) + } + bucketDir := filepath.Join(root, "bucket") + objDir := filepath.Join(bucketDir, filepath.FromSlash(object)) + siblingMeta := filepath.Join(bucketDir, "day=1", "b=9", "sibling", xlStorageFormatFile) + if tc.sibling { + if err := os.MkdirAll(filepath.Dir(siblingMeta), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(siblingMeta, []byte("sibling"), 0o644); err != nil { + t.Fatal(err) + } + } + fi := newFileInfo(object, 2, 2) + fi.DataDir, fi.VersionID = mustGetUUID(), mustGetUUID() + fi.Erasure.Index = 1 + fi.ModTime = time.Now().UTC() + fi.Fresh = true + ctx := &missingCommitMetaContext{ + Context: t.Context(), + meta: filepath.Join(root, minioMetaTmpBucket, "staged", xlStorageFormatFile), + } + switch tc.data { + case "inline": + fi.Size = 1 + fi.Data = []byte("a") + case "dir": + fi.Size = 1 + fi.AddObjectPart(1, "", 1, 1, fi.ModTime, nil, nil) + part := filepath.Join(root, minioMetaTmpBucket, "staged", fi.DataDir, "part.1") + if err := os.MkdirAll(filepath.Dir(part), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(part, []byte("a"), 0o644); err != nil { + t.Fatal(err) + } + ctx.dataDest = filepath.Join(objDir, fi.DataDir) + } + _, err = s.RenameData(ctx, minioMetaTmpBucket, "staged", fi, "bucket", object, RenameOptions{}) + if !ctx.removed { + t.Fatalf("metadata removal did not execute: removal error = %v, RenameData error = %v", ctx.removeErr, err) + } + if !errors.Is(err, errCommitMetaMissing) { + t.Fatalf("expected commit metadata missing, got %v", err) + } + + if tc.data == "dir" { + // The moved data directory is not deleted, so the object + // directory stays and holds only it. + entries, err := os.ReadDir(objDir) + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 || entries[0].Name() != fi.DataDir { + t.Fatalf("object directory holds %v, want only data directory %s", entries, fi.DataDir) + } + if _, err := os.Stat(filepath.Join(objDir, fi.DataDir, "part.1")); err != nil { + t.Fatalf("moved data must be kept: %v", err) + } + return + } + if _, err := os.Stat(objDir); !os.IsNotExist(err) { + t.Fatalf("empty object directory left after a failed commit (%v)", err) + } + if tc.sibling { + if _, err := os.Stat(siblingMeta); err != nil { + t.Fatalf("sibling object must be kept: %v", err) + } + } else if _, err := os.Stat(filepath.Join(bucketDir, "day=1")); !os.IsNotExist(err) { + t.Fatalf("empty parent day=1 left after a failed commit (%v)", err) + } + if _, err := os.Stat(bucketDir); err != nil { + t.Fatalf("bucket directory must be kept: %v", err) + } + }) + } +} + +// traceOSCalls calls hook synchronously after every instrumented OS call +// until the test ends, so that a test can run other storage calls at an +// exact point inside a storage call without changing production code. +func traceOSCalls(t *testing.T, hook func(madmin.TraceInfo)) { + t.Helper() + before := globalTrace.Subscribers() + done := make(chan struct{}) + if err := globalTrace.Subscribe(madmin.TraceOS, make(chan madmin.TraceInfo, 1), done, func(tr madmin.TraceInfo) bool { + hook(tr) + return false + }); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + close(done) + // Wait for the unsubscribe, which takes the pubsub write lock, so + // that it cannot block a nested trace of a later test. + for globalTrace.Subscribers() > before { + time.Sleep(time.Millisecond) + } + }) +} + +// The walk up from a removed path removes only empty directories. Another +// walk, for example the cleanup of a second failed write of the same key, +// can remove the rest of the chain while this one is between two steps, and +// a writer can then commit an object whose xl.meta is the path this walk +// removes next. That object must stay. +func TestXLStorageWalkUpKeepsCommittedObject(t *testing.T) { + const nested, parent = "parent/xl.meta/child", "parent" + for _, tc := range []string{"delete_version", "data_rename", "commit"} { + t.Run(tc, func(t *testing.T) { + disk, root, err := newXLStorageTestSetup(t) + if err != nil { + t.Fatal(err) + } + defer disk.Close() + s := disk.storage + ctx := t.Context() + if err := s.MakeVol(ctx, "bucket"); err != nil { + t.Fatal(err) + } + newFI := func(object string) FileInfo { + fi := newFileInfo(object, 2, 2) + fi.DataDir, fi.VersionID = mustGetUUID(), mustGetUUID() + fi.Erasure.Index = 1 + fi.ModTime = time.Now().UTC() + fi.Fresh = true + return fi + } + inline := func(object string) FileInfo { + fi := newFI(object) + fi.Size, fi.Data = 1, []byte("x") + fi.AddObjectPart(1, "", 1, 1, fi.ModTime, nil, nil) + fi.SetInlineData() + return fi + } + childDir := filepath.Join(root, "bucket", filepath.FromSlash(nested)) + metaDir := filepath.Dir(childDir) + parentDir := filepath.Dir(metaDir) + + var nestedFI FileInfo + if tc == "delete_version" { + nestedFI = inline(nested) + if _, err := s.RenameData(ctx, minioMetaTmpBucket, "nested", nestedFI, "bucket", nested, RenameOptions{}); err != nil { + t.Fatal(err) + } + } + parentFI := inline(parent) + var fired atomic.Bool + var hookErr error + traceOSCalls(t, func(tr madmin.TraceInfo) { + if tr.FuncName != "os.Remove" || tr.Error != "" || filepath.Clean(tr.Path) != childDir || !fired.CompareAndSwap(false, true) { + return + } + // This walk has removed child/. Another one removes the rest + // of the chain, then a writer commits the object parent, + // whose metadata file is parent/xl.meta. + for _, dir := range []string{metaDir, parentDir} { + if err := os.Remove(dir); err != nil { + hookErr = err + return + } + } + if _, err := s.RenameData(context.Background(), minioMetaTmpBucket, "writer", parentFI, "bucket", parent, RenameOptions{}); err != nil { + hookErr = err + } + }) + + switch tc { + case "delete_version": + err = s.DeleteVersion(ctx, "bucket", nested, nestedFI, false, DeleteOptions{}) + case "data_rename": + // The staged data directory is gone. + fi := newFI(nested) + fi.Size = 1 + fi.AddObjectPart(1, "", 1, 1, fi.ModTime, nil, nil) + _, err = s.RenameData(ctx, minioMetaTmpBucket, "late", fi, "bucket", nested, RenameOptions{}) + case "commit": + mctx := &missingCommitMetaContext{ + Context: ctx, + meta: filepath.Join(root, minioMetaTmpBucket, "late", xlStorageFormatFile), + } + _, err = s.RenameData(mctx, minioMetaTmpBucket, "late", newFI(nested), "bucket", nested, RenameOptions{}) + } + if tc == "delete_version" && err != nil { + t.Fatal(err) + } + if tc != "delete_version" && err == nil { + t.Fatal("RenameData succeeded without its staged data") + } + if !fired.Load() { + t.Fatalf("%s did not remove the empty directory %s", tc, nested) + } + if hookErr != nil { + t.Fatal(hookErr) + } + got, err := s.ReadVersion(ctx, "", "bucket", parent, parentFI.VersionID, ReadOptions{ReadData: true}) + if err != nil { + t.Fatalf("committed object %s lost: %v", parent, err) + } + if !bytes.Equal(got.Data, parentFI.Data) { + t.Fatalf("committed object %s has data %q, want %q", parent, got.Data, parentFI.Data) + } + }) + } +} + +// A cleanup that walks up removes each empty directory of a write's +// destination path once. When it removes them one by one while the write +// creates them, so that every directory the write has just made or found is +// gone before it uses it, the write must keep retrying until the walk has +// passed and then succeed. That holds also when another write recreates the +// directories after a failed attempt, which must not hide the failure. +func TestXLStorageRenameDataRetriesWhileCleanupWalksUp(t *testing.T) { + if runtime.GOOS == globalWindowsOSName { + t.Skip("directory creation is not traced on Windows") + } + const object = "a/b/object" + for _, tc := range []struct { + name string + inline, nested bool + }{ + {"data_dir", false, false}, + {"inline", true, false}, + {"data_dir_nested_writer", false, true}, + } { + inline := tc.inline + t.Run(tc.name, func(t *testing.T) { + disk, root, err := newXLStorageTestSetup(t) + if err != nil { + t.Fatal(err) + } + defer disk.Close() + s := disk.storage + ctx := t.Context() + if err := s.MakeVol(ctx, "bucket"); err != nil { + t.Fatal(err) + } + fi := newFileInfo(object, 2, 2) + fi.DataDir, fi.VersionID = mustGetUUID(), mustGetUUID() + fi.Erasure.Index = 1 + fi.ModTime = time.Now().UTC() + fi.Fresh = true + fi.Size = 1 + fi.AddObjectPart(1, "", 1, 1, fi.ModTime, nil, nil) + if inline { + fi.Data = []byte("x") + fi.SetInlineData() + } else { + part := filepath.Join(root, minioMetaTmpBucket, "staged", fi.DataDir, "part.1") + if err := os.MkdirAll(filepath.Dir(part), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(part, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + } + objDir := filepath.Join(root, "bucket", filepath.FromSlash(object)) + bDir := filepath.Dir(objDir) + aDir := filepath.Dir(bDir) + // The walk removes object/, then b/, then a/. Each step happens + // right after the write's next directory creation reached the + // directory, whether it created it or found it in place. + remove := func(dir string) func() error { return func() error { return os.Remove(dir) } } + steps := []struct { + after string + do func() error + }{ + {objDir, remove(objDir)}, + {bDir, remove(bDir)}, + {aDir, remove(aDir)}, + } + if tc.nested { + // The next creation of b/ fails. A write of the key + // a/b/object/child then creates the directories again. + steps = append(steps, struct { + after string + do func() error + }{bDir, func() error { return os.MkdirAll(filepath.Join(objDir, "child"), 0o755) }}) + } + var mu sync.Mutex + step := 0 + var hookErr error + traceOSCalls(t, func(tr madmin.TraceInfo) { + if tr.FuncName != "os.Mkdir" { + return + } + mu.Lock() + defer mu.Unlock() + if step == len(steps) || filepath.Clean(tr.Path) != steps[step].after { + return + } + if err := steps[step].do(); err != nil && hookErr == nil { + hookErr = err + } + step++ + }) + _, err = s.RenameData(ctx, minioMetaTmpBucket, "staged", fi, "bucket", object, RenameOptions{}) + mu.Lock() + done := step + mu.Unlock() + if hookErr != nil { + t.Fatal(hookErr) + } + if done != len(steps) { + t.Fatalf("the walk made %d of %d steps", done, len(steps)) + } + if err != nil { + t.Fatalf("write failed while a cleanup walked up its path: %v", err) + } + got, err := s.ReadVersion(ctx, "", "bucket", object, fi.VersionID, ReadOptions{ReadData: true}) + if err != nil { + t.Fatal(err) + } + if got.Size != fi.Size { + t.Fatalf("committed size %d, want %d", got.Size, fi.Size) + } + }) + } +} diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index 0cd36806d..725a79d0a 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -2508,8 +2508,11 @@ func (s *xlStorage) deleteFile(basePath, deletePath string, recursive, immediate } // Delete parent directory obviously not recursively. Errors for - // parent directories shouldn't trickle down. - s.deleteFile(basePath, pathutil.Dir(pathutil.Clean(deletePath)), false, false) + // parent directories shouldn't trickle down. The trailing slash + // removes only an empty directory: a concurrent writer can have + // replaced the parent with a file, for example the xl.meta of an + // object whose name is that parent path, and it must stay. + s.deleteFile(basePath, pathutil.Dir(pathutil.Clean(deletePath))+SlashSeparator, false, false) return nil } @@ -2577,6 +2580,40 @@ func skipAccessChecks(volume string) (ok bool) { return strings.HasPrefix(volume, minioMetaBucket) } +// renameDataAll is renameAll for the two renames of RenameData. A failed +// RenameData, typically one that runs after its caller gave up, removes its +// object directory and the parents that are empty, so it can remove the +// directories that renameAll is creating for this rename. The directory +// creation or the rename then reports "not found" although the source is +// still in place: retry, which creates the directories again. A cleanup +// removes each directory of the path at most once and each failed attempt +// needs at least one such removal, so one retry per path level is enough +// against any single cleanup. The decision uses the error of the attempt +// itself, since another write can recreate the directories in between. +func renameDataAll(srcPath, dstPath, skipParent string) error { + if srcPath == "" || dstPath == "" { + return errInvalidArgument + } + if err := checkPathLength(srcPath); err != nil { + return err + } + if err := checkPathLength(dstPath); err != nil { + return err + } + retries := strings.Count(strings.TrimPrefix(pathutil.Clean(dstPath), pathutil.Clean(skipParent)), SlashSeparator) + for { + err := reliableRename(srcPath, dstPath, skipParent) + if err == nil || retries == 0 || !osIsNotExist(err) { + return renameAllErr(err, srcPath, dstPath) + } + if _, serr := Lstat(pathutil.Clean(srcPath)); serr != nil { + // The source is gone: a late call, which the caller cleans up. + return renameAllErr(err, srcPath, dstPath) + } + retries-- + } +} + // RenameData - rename source path to destination path atomically, metadata and data directory. func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, fi FileInfo, dstVolume, dstPath string, opts RenameOptions) (res RenameDataResp, err error) { defer func() { @@ -2883,7 +2920,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f if contextCanceled(ctx) { return res, ctx.Err() } - if err = renameAll(srcDataPath, dstDataPath, skipParent); err != nil { + if err = renameDataAll(srcDataPath, dstDataPath, skipParent); err != nil { if legacyPreserved { // Any failed rename calls un-roll previous transaction. s.deleteFile(dstVolumeDir, legacyDataPath, true, false) @@ -2922,7 +2959,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f } // Commit meta-file - if err = renameAll(srcFilePath, dstFilePath, skipParent); err != nil { + if err = renameDataAll(srcFilePath, dstFilePath, skipParent); err != nil { if legacyPreserved { // Any failed rename calls un-roll previous transaction. s.deleteFile(dstVolumeDir, legacyDataPath, true, false) @@ -2930,6 +2967,11 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f // if its a partial rename() do not attempt to delete recursively. // this can be healed since all parts are available. s.deleteFile(dstVolumeDir, dstDataPath, false, false) + // As with the data rename above, renameAll() may have created the + // object directory; drop it and its parents while empty. A moved + // data directory keeps it, since the trailing slash removes only an + // empty directory. + s.deleteFile(dstVolumeDir, pathutil.Dir(dstFilePath)+SlashSeparator, false, false) err = osErrToFileErr(err) if errors.Is(err, errFileNotFound) { // The staged xl.meta disappeared between staging and commit.