diff --git a/cmd/os-reliable.go b/cmd/os-reliable.go index b333af63c..e4d49bcaf 100644 --- a/cmd/os-reliable.go +++ b/cmd/os-reliable.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "path" + "strings" ) // Wrapper functions to os.RemoveAll, which calls reliableRemoveAll @@ -82,20 +83,63 @@ func mkdirAll(dirPath string, mode os.FileMode, baseDir string) (err error) { return err } - if err = reliableMkdirAll(dirPath, mode, baseDir); err != nil { + return mkdirAllErr(reliableMkdirAll(dirPath, mode, baseDir)) +} + +// mkdirAllErr converts an error of reliableMkdirAll into the error that +// mkdirAll returns. +func mkdirAllErr(err error) error { + switch { + case err == nil: + return nil + case isSysErrNotDir(err): // File path cannot be verified since one of the parents is a file. - if isSysErrNotDir(err) { - return errFileAccessDenied - } else if 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 - } + 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 + default: return osErrToFileErr(err) } +} - return nil +// openFileAll is mkdirAll for the parent of filePath followed by the open +// of filePath with flags, with direct I/O when direct is set. A cleanup of a +// failed write of the same key can remove the directories in between, see +// retriesAgainstCleanup; then it creates them again. An error of the +// directory creation is the error of mkdirAll, an error of the open is the +// error of the OS call. +func openFileAll(filePath string, flags int, direct bool, baseDir string) (*os.File, error) { + parentDir := path.Dir(filePath) + if err := checkPathLength(parentDir); err != nil { + return nil, err + } + retries := retriesAgainstCleanup(filePath, baseDir) + for { + // Create top level directories if they don't exist. + // with mode 0777 mkdir honors system umask. + if err := reliableMkdirAll(parentDir, 0o777, baseDir); err != nil { + if retries > 0 && osIsNotExist(err) { + retries-- + continue + } + return nil, mkdirAllErr(err) + } + var f *os.File + var err error + if direct { + f, err = OpenFileDirectIO(filePath, flags, 0o666) + } else { + f, err = OpenFile(filePath, flags, 0o666) + } + if err != nil && retries > 0 && osIsNotExist(err) && flags&os.O_CREATE != 0 { + retries-- + continue + } + return f, err + } } // Reliably retries os.MkdirAll if for some reason os.MkdirAll returns @@ -175,24 +219,41 @@ func renameAllErr(err error, srcFilePath, dstFilePath string) error { } } -// Reliably retries os.RenameAll if for some reason os.RenameAll returns -// syscall.ENOENT (parent does not exist). -func reliableRename(srcFilePath, dstFilePath, baseDir string) (err error) { - if err = reliableMkdirAll(path.Dir(dstFilePath), 0o777, baseDir); err != nil { - return err - } - - i := 0 - for { - // After a successful parent directory create attempt a renameAll. - if err = Rename(srcFilePath, dstFilePath); err != nil { - // Retry only for the first retryable error. - if osIsNotExist(err) && i == 0 { - i++ - continue - } - } - break - } - return err +// retriesAgainstCleanup returns how often a write to filePath is retried +// when a directory of its path disappears between the directory creation +// and the use of the directory: once per path level below baseDir. +// +// 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 another write of the same key has just +// created. The directory creation or the write then reports "not found" +// although nothing is wrong with the write: 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. +func retriesAgainstCleanup(filePath, baseDir string) int { + return strings.Count(strings.TrimPrefix(path.Clean(filePath), path.Clean(baseDir)), SlashSeparator) +} + +// Reliably retries os.Rename if for some reason the directory creation or +// os.Rename returns syscall.ENOENT (parent does not exist) while the source +// is still in place, see retriesAgainstCleanup. The decision uses the error +// of the attempt itself, since another write can recreate the directories +// in between. +func reliableRename(srcFilePath, dstFilePath, baseDir string) (err error) { + retries := retriesAgainstCleanup(dstFilePath, baseDir) + for { + if err = reliableMkdirAll(path.Dir(dstFilePath), 0o777, baseDir); err == nil { + // After a successful parent directory create attempt a rename. + err = Rename(srcFilePath, dstFilePath) + } + if err == nil || retries == 0 || !osIsNotExist(err) { + return err + } + if _, serr := Lstat(path.Clean(srcFilePath)); serr != nil { + // The source is gone: a late call, which the caller cleans up. + return err + } + retries-- + } } diff --git a/cmd/xl-storage-errors_test.go b/cmd/xl-storage-errors_test.go index 547ab6812..91686a666 100644 --- a/cmd/xl-storage-errors_test.go +++ b/cmd/xl-storage-errors_test.go @@ -21,10 +21,12 @@ import ( "bytes" "context" "errors" + "fmt" "net/http" "os" "path/filepath" "runtime" + "strings" "sync" "sync/atomic" "syscall" @@ -32,6 +34,7 @@ import ( "time" "github.com/minio/madmin-go/v3" + xioutil "github.com/minio/minio/internal/ioutil" ) func TestSysErrors(t *testing.T) { @@ -526,3 +529,153 @@ func TestXLStorageRenameDataRetriesWhileCleanupWalksUp(t *testing.T) { }) } } + +// A failed RenameData that runs late, after its caller gave up, removes its +// empty object directory and the empty parents. When it does so right after +// another write of the same key created the object directory, that write +// must create the directory again and succeed, whichever way it writes its +// metadata file: a fresh xl.meta opened in place (a delete marker on an +// absent key), a first version of a key without xl.meta, or a large xl.meta +// written directly. A cleanup that walks up the whole path, one directory +// per step, is survived as well. UpdateMetadata renames over an existing +// xl.meta, so its directory is never empty and the cleanup cannot remove it. +func TestXLStorageMetadataWriteSurvivesLateCleanup(t *testing.T) { + if runtime.GOOS == globalWindowsOSName { + t.Skip("directory creation is not traced on Windows") + } + const object = "a/b/object" + writers := []struct { + name string + write func(ctx context.Context, s *xlStorage, fi FileInfo) (FileInfo, error) + }{ + {"delete_marker", func(ctx context.Context, s *xlStorage, fi FileInfo) (FileInfo, error) { + // DeleteVersion with forceDelMarker on a key without xl.meta. + fi.Deleted, fi.Fresh = true, true + return fi, s.WriteMetadata(ctx, "", "bucket", object, fi) + }}, + {"first_version", func(ctx context.Context, s *xlStorage, fi FileInfo) (FileInfo, error) { + // Not fresh: the missing xl.meta is read first. + fi.Size, fi.Data = 1, []byte("x") + fi.AddObjectPart(1, "", 1, 1, fi.ModTime, nil, nil) + fi.SetInlineData() + return fi, s.WriteMetadata(ctx, "", "bucket", object, fi) + }}, + {"direct_write", func(ctx context.Context, s *xlStorage, fi FileInfo) (FileInfo, error) { + // Larger than the direct I/O alignment: written through + // writeAllDirect, with O_DIRECT where supported. + fi.Metadata = map[string]string{"x-amz-meta-pad": strings.Repeat("p", 2*xioutil.DirectioAlignSize)} + var xlMeta xlMetaV2 + if err := xlMeta.AddVersion(fi); err != nil { + return fi, err + } + buf, err := xlMeta.AppendTo(nil) + if err != nil { + return fi, err + } + return fi, s.WriteAll(ctx, "bucket", pathJoin(object, xlStorageFormatFile), buf) + }}, + } + for _, wr := range writers { + for _, walkUp := range []bool{false, true} { + name := wr.name + "/late_rename_data" + if walkUp { + name = wr.name + "/walk_up" + } + t.Run(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) + } + objDir := filepath.Join(root, "bucket", filepath.FromSlash(object)) + bDir := filepath.Dir(objDir) + aDir := filepath.Dir(bDir) + fi := newFileInfo(object, 2, 2) + fi.DataDir, fi.VersionID = mustGetUUID(), mustGetUUID() + fi.Erasure.Index = 1 + fi.ModTime = time.Now().UTC() + + // The walk removes object/, then b/, then a/, each right after + // the write's next directory creation reached it, whether it + // created the directory or found it in place. + walk := []string{objDir, bDir, aDir} + var mu sync.Mutex + steps := 0 + var hookErrs []error + var fired atomic.Bool + traceOSCalls(t, func(tr madmin.TraceInfo) { + if tr.FuncName != "os.Mkdir" { + return + } + if walkUp { + mu.Lock() + defer mu.Unlock() + if steps == len(walk) || filepath.Clean(tr.Path) != walk[steps] { + return + } + if err := os.Remove(walk[steps]); err != nil { + hookErrs = append(hookErrs, err) + } + steps++ + return + } + if tr.Error != "" || filepath.Clean(tr.Path) != objDir || !fired.CompareAndSwap(false, true) { + return + } + // The write has just created the object directory. A late + // RenameData of the same key, whose staged data is gone, + // fails now and removes the directory and its empty parents. + late := newFileInfo(object, 2, 2) + late.DataDir, late.VersionID = mustGetUUID(), mustGetUUID() + late.Erasure.Index = 1 + late.ModTime = time.Now().UTC() + late.Size = 1 + late.AddObjectPart(1, "", 1, 1, late.ModTime, nil, nil) + _, err := s.RenameData(context.Background(), minioMetaTmpBucket, mustGetUUID(), late, "bucket", object, RenameOptions{}) + var errs []error + if !errors.Is(err, errFileNotFound) { + errs = append(errs, fmt.Errorf("late RenameData returned %v, want %v", err, errFileNotFound)) + } + if _, serr := os.Stat(objDir); !os.IsNotExist(serr) { + errs = append(errs, fmt.Errorf("late RenameData left %s (%v)", objDir, serr)) + } + mu.Lock() + hookErrs = append(hookErrs, errs...) + steps++ + mu.Unlock() + }) + + fi, err = wr.write(ctx, s, fi) + mu.Lock() + done, errs := steps, hookErrs + mu.Unlock() + for _, herr := range errs { + t.Error(herr) + } + if err != nil { + t.Fatalf("%s failed after a concurrent cleanup: %v", wr.name, err) + } + want := 1 + if walkUp { + want = len(walk) + } + if done != want { + t.Fatalf("the cleanup made %d of %d steps", done, want) + } + got, err := s.ReadVersion(ctx, "", "bucket", object, fi.VersionID, ReadOptions{ReadData: true}) + if err != nil { + t.Fatalf("written version lost: %v", err) + } + if got.VersionID != fi.VersionID || got.Deleted != fi.Deleted || !bytes.Equal(got.Data, fi.Data) { + t.Fatalf("read back version %s deleted=%v data=%q, want %s deleted=%v data=%q", + got.VersionID, got.Deleted, got.Data, fi.VersionID, fi.Deleted, fi.Data) + } + }) + } + } +} diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index 81d9a2cf3..b96937e43 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -2001,13 +2001,8 @@ func (s *xlStorage) openFile(filePath string, mode int, skipParent string) (f *o if skipParent == "" { skipParent = s.drivePath } - // Create top level directories if they don't exist. - // with mode 0777 mkdir honors system umask. - if err = mkdirAll(pathutil.Dir(filePath), 0o777, skipParent); err != nil { - return nil, osErrToFileErr(err) - } - - w, err := OpenFile(filePath, mode, 0o666) + // Create top level directories if they don't exist, then open the file. + w, err := openFileAll(filePath, mode, false, skipParent) if err != nil { // File path cannot be verified since one of the parents is a file. switch { @@ -2158,21 +2153,10 @@ func (s *xlStorage) writeAllDirect(ctx context.Context, filePath string, fileSiz skipParent = s.drivePath } - // Create top level directories if they don't exist. - // with mode 0777 mkdir honors system umask. - parentFilePath := pathutil.Dir(filePath) - if err = mkdirAll(parentFilePath, 0o777, skipParent); err != nil { - return osErrToFileErr(err) - } - odirectEnabled := globalAPIConfig.odirectEnabled() && s.oDirect && fileSize > 0 - var w *os.File - if odirectEnabled { - w, err = OpenFileDirectIO(filePath, flags, 0o666) - } else { - w, err = OpenFile(filePath, flags, 0o666) - } + // Create top level directories if they don't exist, then open the file. + w, err := openFileAll(filePath, flags, odirectEnabled, skipParent) if err != nil { return osErrToFileErr(err) } @@ -2583,40 +2567,6 @@ 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() { @@ -2923,7 +2873,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f if contextCanceled(ctx) { return res, ctx.Err() } - if err = renameDataAll(srcDataPath, dstDataPath, skipParent); err != nil { + if err = renameAll(srcDataPath, dstDataPath, skipParent); err != nil { if legacyPreserved { // Any failed rename calls un-roll previous transaction. s.deleteFile(dstVolumeDir, legacyDataPath, true, false) @@ -2962,7 +2912,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f } // Commit meta-file - if err = renameDataAll(srcFilePath, dstFilePath, skipParent); err != nil { + if err = renameAll(srcFilePath, dstFilePath, skipParent); err != nil { if legacyPreserved { // Any failed rename calls un-roll previous transaction. s.deleteFile(dstVolumeDir, legacyDataPath, true, false)