From 8b8be2695f76ce5b77d99a4a167b29376764adb6 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Wed, 13 Sep 2023 08:14:36 -0700 Subject: [PATCH] optimize mkdir calls to avoid base-dir `Mkdir` attempts (#18021) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently we have IOPs of these patterns ``` [OS] os.Mkdir play.min.io:9000 /disk1 2.718µs [OS] os.Mkdir play.min.io:9000 /disk1/data 2.406µs [OS] os.Mkdir play.min.io:9000 /disk1/data/.minio.sys 4.068µs [OS] os.Mkdir play.min.io:9000 /disk1/data/.minio.sys/tmp 2.843µs [OS] os.Mkdir play.min.io:9000 /disk1/data/.minio.sys/tmp/d89c8ceb-f8d1-4cc6-b483-280f87c4719f 20.152µs ``` It can be seen that we can save quite Nx levels such as if your drive is mounted at `/disk1/minio` you can simply skip sending an `Mkdir /disk1/` and `Mkdir /disk1/minio`. Since they are expected to exist already, this PR adds a way for us to ignore all paths upto the mount or a directory which ever has been provided to MinIO setup. --- cmd/disk-cache-backend.go | 8 ++++---- cmd/format-erasure.go | 2 +- cmd/metacache-server-pool.go | 2 +- cmd/os-instrumented.go | 4 ++-- cmd/os-reliable.go | 16 ++++++++-------- cmd/os-reliable_test.go | 20 +++++++++---------- cmd/os_other.go | 3 ++- cmd/os_unix.go | 11 +++++++++-- cmd/os_windows.go | 3 ++- cmd/prepare-storage.go | 4 ++-- cmd/xl-storage.go | 37 ++++++++++++++++++------------------ 11 files changed, 60 insertions(+), 50 deletions(-) diff --git a/cmd/disk-cache-backend.go b/cmd/disk-cache-backend.go index b5f4cebce..9813ded53 100644 --- a/cmd/disk-cache-backend.go +++ b/cmd/disk-cache-backend.go @@ -604,7 +604,7 @@ func (c *diskCache) SaveMetadata(ctx context.Context, bucket, object string, met // move part saved in writeback directory and cache.json atomically if finalizeWB { wbdir := getCacheWriteBackSHADir(c.dir, bucket, object) - if err = renameAll(pathJoin(wbdir, cacheDataFile), pathJoin(cachedPath, cacheDataFile)); err != nil { + if err = renameAll(pathJoin(wbdir, cacheDataFile), pathJoin(cachedPath, cacheDataFile), c.dir); err != nil { return err } removeAll(wbdir) // cleanup writeback/shadir @@ -1273,7 +1273,7 @@ func (c *diskCache) NewMultipartUpload(ctx context.Context, bucket, object, uID cachePath := getMultipartCacheSHADir(c.dir, bucket, object) uploadIDDir := path.Join(cachePath, uploadID) - if err := mkdirAll(uploadIDDir, 0o777); err != nil { + if err := mkdirAll(uploadIDDir, 0o777, c.dir); err != nil { return uploadID, err } metaPath := pathJoin(uploadIDDir, cacheMetaJSONFile) @@ -1570,9 +1570,9 @@ func (c *diskCache) CompleteMultipartUpload(ctx context.Context, bucket, object, jsonSave(f, uploadMeta) for _, pi := range uploadedParts { part := fmt.Sprintf("part.%d", pi.PartNumber) - renameAll(pathJoin(uploadIDDir, part), pathJoin(cachePath, part)) + renameAll(pathJoin(uploadIDDir, part), pathJoin(cachePath, part), c.dir) } - renameAll(pathJoin(uploadIDDir, cacheMetaJSONFile), pathJoin(cachePath, cacheMetaJSONFile)) + renameAll(pathJoin(uploadIDDir, cacheMetaJSONFile), pathJoin(cachePath, cacheMetaJSONFile), c.dir) removeAll(uploadIDDir) // clean up any unused parts in the uploadIDDir return uploadMeta.ToObjectInfo(), nil } diff --git a/cmd/format-erasure.go b/cmd/format-erasure.go index 86e62a488..31384f99c 100644 --- a/cmd/format-erasure.go +++ b/cmd/format-erasure.go @@ -275,7 +275,7 @@ func formatErasureMigrateV2ToV3(data []byte, export, version string) ([]byte, er tmpOld := pathJoin(export, minioMetaTmpDeletedBucket, mustGetUUID()) if err := renameAll(pathJoin(export, minioMetaMultipartBucket), - tmpOld); err != nil && err != errFileNotFound { + tmpOld, export); err != nil && err != errFileNotFound { logger.LogIf(GlobalContext, fmt.Errorf("unable to rename (%s -> %s) %w, drive may be faulty please investigate", pathJoin(export, minioMetaMultipartBucket), tmpOld, diff --git a/cmd/metacache-server-pool.go b/cmd/metacache-server-pool.go index 4c972b5f5..61ef5cc76 100644 --- a/cmd/metacache-server-pool.go +++ b/cmd/metacache-server-pool.go @@ -38,7 +38,7 @@ func renameAllBucketMetacache(epPath string) error { if typ == os.ModeDir { tmpMetacacheOld := pathutil.Join(epPath, minioMetaTmpDeletedBucket, mustGetUUID()) if err := renameAll(pathJoin(epPath, minioMetaBucket, metacachePrefixForID(name, slashSeparator)), - tmpMetacacheOld); err != nil && err != errFileNotFound { + tmpMetacacheOld, epPath); err != nil && err != errFileNotFound { return fmt.Errorf("unable to rename (%s -> %s) %w", pathJoin(epPath, minioMetaBucket+metacachePrefixForID(minioMetaBucket, slashSeparator)), tmpMetacacheOld, diff --git a/cmd/os-instrumented.go b/cmd/os-instrumented.go index adeb7ca99..c3d03a774 100644 --- a/cmd/os-instrumented.go +++ b/cmd/os-instrumented.go @@ -129,9 +129,9 @@ func Mkdir(dirPath string, mode os.FileMode) (err error) { } // MkdirAll captures time taken to call os.MkdirAll -func MkdirAll(dirPath string, mode os.FileMode) (err error) { +func MkdirAll(dirPath string, mode os.FileMode, baseDir string) (err error) { defer updateOSMetrics(osMetricMkdirAll, dirPath)(err) - return osMkdirAll(dirPath, mode) + return osMkdirAll(dirPath, mode, baseDir) } // Rename captures time taken to call os.Rename diff --git a/cmd/os-reliable.go b/cmd/os-reliable.go index 6de5a4aba..9ab77939a 100644 --- a/cmd/os-reliable.go +++ b/cmd/os-reliable.go @@ -73,7 +73,7 @@ func reliableRemoveAll(dirPath string) (err error) { // Wrapper functions to os.MkdirAll, which calls reliableMkdirAll // this is to ensure that if there is a racy parent directory // delete in between we can simply retry the operation. -func mkdirAll(dirPath string, mode os.FileMode) (err error) { +func mkdirAll(dirPath string, mode os.FileMode, baseDir string) (err error) { if dirPath == "" { return errInvalidArgument } @@ -82,7 +82,7 @@ func mkdirAll(dirPath string, mode os.FileMode) (err error) { return err } - if err = reliableMkdirAll(dirPath, mode); err != nil { + if err = reliableMkdirAll(dirPath, mode, baseDir); err != nil { // File path cannot be verified since one of the parents is a file. if isSysErrNotDir(err) { return errFileAccessDenied @@ -100,11 +100,11 @@ func mkdirAll(dirPath string, mode os.FileMode) (err error) { // Reliably retries os.MkdirAll if for some reason os.MkdirAll returns // syscall.ENOENT (parent does not exist). -func reliableMkdirAll(dirPath string, mode os.FileMode) (err error) { +func reliableMkdirAll(dirPath string, mode os.FileMode, baseDir string) (err error) { i := 0 for { // Creates all the parent directories, with mode 0777 mkdir honors system umask. - if err = osMkdirAll(dirPath, mode); err != nil { + if err = osMkdirAll(dirPath, mode, baseDir); err != nil { // Retry only for the first retryable error. if osIsNotExist(err) && i == 0 { i++ @@ -120,7 +120,7 @@ func reliableMkdirAll(dirPath string, mode os.FileMode) (err error) { // and reliableRenameAll. This is to ensure that if there is a // racy parent directory delete in between we can simply retry // the operation. -func renameAll(srcFilePath, dstFilePath string) (err error) { +func renameAll(srcFilePath, dstFilePath, baseDir string) (err error) { if srcFilePath == "" || dstFilePath == "" { return errInvalidArgument } @@ -132,7 +132,7 @@ func renameAll(srcFilePath, dstFilePath string) (err error) { return err } - if err = reliableRename(srcFilePath, dstFilePath); err != nil { + if err = reliableRename(srcFilePath, dstFilePath, baseDir); err != nil { switch { case isSysErrNotDir(err) && !osIsNotExist(err): // Windows can have both isSysErrNotDir(err) and osIsNotExist(err) returning @@ -162,8 +162,8 @@ func renameAll(srcFilePath, dstFilePath string) (err error) { // Reliably retries os.RenameAll if for some reason os.RenameAll returns // syscall.ENOENT (parent does not exist). -func reliableRename(srcFilePath, dstFilePath string) (err error) { - if err = reliableMkdirAll(path.Dir(dstFilePath), 0o777); err != nil { +func reliableRename(srcFilePath, dstFilePath, baseDir string) (err error) { + if err = reliableMkdirAll(path.Dir(dstFilePath), 0o777, baseDir); err != nil { return err } diff --git a/cmd/os-reliable_test.go b/cmd/os-reliable_test.go index 14a890b9c..7fd295881 100644 --- a/cmd/os-reliable_test.go +++ b/cmd/os-reliable_test.go @@ -29,15 +29,15 @@ func TestOSMkdirAll(t *testing.T) { t.Fatalf("Unable to create xlStorage test setup, %s", err) } - if err = mkdirAll("", 0o777); err != errInvalidArgument { + if err = mkdirAll("", 0o777, ""); err != errInvalidArgument { t.Fatal("Unexpected error", err) } - if err = mkdirAll(pathJoin(path, "my-obj-del-0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001"), 0o777); err != errFileNameTooLong { + if err = mkdirAll(pathJoin(path, "my-obj-del-0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001"), 0o777, ""); err != errFileNameTooLong { t.Fatal("Unexpected error", err) } - if err = mkdirAll(pathJoin(path, "success-vol", "success-object"), 0o777); err != nil { + if err = mkdirAll(pathJoin(path, "success-vol", "success-object"), 0o777, ""); err != nil { t.Fatal("Unexpected error", err) } } @@ -50,25 +50,25 @@ func TestOSRenameAll(t *testing.T) { t.Fatalf("Unable to create xlStorage test setup, %s", err) } - if err = mkdirAll(pathJoin(path, "testvolume1"), 0o777); err != nil { + if err = mkdirAll(pathJoin(path, "testvolume1"), 0o777, ""); err != nil { t.Fatal(err) } - if err = renameAll("", "foo"); err != errInvalidArgument { + if err = renameAll("", "foo", ""); err != errInvalidArgument { t.Fatal(err) } - if err = renameAll("foo", ""); err != errInvalidArgument { + if err = renameAll("foo", "", ""); err != errInvalidArgument { t.Fatal(err) } - if err = renameAll(pathJoin(path, "testvolume1"), pathJoin(path, "testvolume2")); err != nil { + if err = renameAll(pathJoin(path, "testvolume1"), pathJoin(path, "testvolume2"), ""); err != nil { t.Fatal(err) } - if err = renameAll(pathJoin(path, "testvolume1"), pathJoin(path, "testvolume2")); err != errFileNotFound { + if err = renameAll(pathJoin(path, "testvolume1"), pathJoin(path, "testvolume2"), ""); err != errFileNotFound { t.Fatal(err) } - if err = renameAll(pathJoin(path, "my-obj-del-0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001"), pathJoin(path, "testvolume2")); err != errFileNameTooLong { + if err = renameAll(pathJoin(path, "my-obj-del-0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001"), pathJoin(path, "testvolume2"), ""); err != errFileNameTooLong { t.Fatal("Unexpected error", err) } - if err = renameAll(pathJoin(path, "testvolume1"), pathJoin(path, "my-obj-del-0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001")); err != errFileNameTooLong { + if err = renameAll(pathJoin(path, "testvolume1"), pathJoin(path, "my-obj-del-0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001"), ""); err != errFileNameTooLong { t.Fatal("Unexpected error", err) } } diff --git a/cmd/os_other.go b/cmd/os_other.go index 15129bfb8..340ca1b98 100644 --- a/cmd/os_other.go +++ b/cmd/os_other.go @@ -31,7 +31,8 @@ func access(name string) error { return err } -func osMkdirAll(dirPath string, perm os.FileMode) error { +func osMkdirAll(dirPath string, perm os.FileMode, _ string) error { + // baseDir is not honored in plan9 and solaris platforms. return os.MkdirAll(dirPath, perm) } diff --git a/cmd/os_unix.go b/cmd/os_unix.go index 95368d239..4aa5cf855 100644 --- a/cmd/os_unix.go +++ b/cmd/os_unix.go @@ -24,6 +24,7 @@ import ( "bytes" "fmt" "os" + "strings" "sync" "syscall" "unsafe" @@ -47,7 +48,13 @@ func access(name string) error { // directories that MkdirAll creates. // If path is already a directory, MkdirAll does nothing // and returns nil. -func osMkdirAll(dirPath string, perm os.FileMode) error { +func osMkdirAll(dirPath string, perm os.FileMode, baseDir string) error { + if baseDir != "" { + if strings.HasPrefix(baseDir, dirPath) { + return nil + } + } + // Slow path: make sure parent exists and then call Mkdir for path. i := len(dirPath) for i > 0 && os.IsPathSeparator(dirPath[i-1]) { // Skip trailing path separator. @@ -61,7 +68,7 @@ func osMkdirAll(dirPath string, perm os.FileMode) error { if j > 1 { // Create parent. - if err := osMkdirAll(dirPath[:j-1], perm); err != nil { + if err := osMkdirAll(dirPath[:j-1], perm, baseDir); err != nil { return err } } diff --git a/cmd/os_windows.go b/cmd/os_windows.go index f017768bd..98c797c88 100644 --- a/cmd/os_windows.go +++ b/cmd/os_windows.go @@ -31,7 +31,8 @@ func access(name string) error { return err } -func osMkdirAll(dirPath string, perm os.FileMode) error { +func osMkdirAll(dirPath string, perm os.FileMode, _ string) error { + // baseDir is not honored in windows platform return os.MkdirAll(dirPath, perm) } diff --git a/cmd/prepare-storage.go b/cmd/prepare-storage.go index cf7e7544b..a80887408 100644 --- a/cmd/prepare-storage.go +++ b/cmd/prepare-storage.go @@ -85,14 +85,14 @@ func bgFormatErasureCleanupTmp(diskPath string) { tmpID := mustGetUUID() tmpOld := pathJoin(diskPath, minioMetaTmpBucket+"-old", tmpID) if err := renameAll(pathJoin(diskPath, minioMetaTmpBucket), - tmpOld); err != nil && !errors.Is(err, errFileNotFound) { + tmpOld, diskPath); err != nil && !errors.Is(err, errFileNotFound) { logger.LogIf(GlobalContext, fmt.Errorf("unable to rename (%s -> %s) %w, drive may be faulty please investigate", pathJoin(diskPath, minioMetaTmpBucket), tmpOld, osErrToFileErr(err))) } - if err := mkdirAll(pathJoin(diskPath, minioMetaTmpDeletedBucket), 0o777); err != nil { + if err := mkdirAll(pathJoin(diskPath, minioMetaTmpDeletedBucket), 0o777, diskPath); err != nil { logger.LogIf(GlobalContext, fmt.Errorf("unable to create (%s) %w, drive may be faulty please investigate", pathJoin(diskPath, minioMetaTmpBucket), err)) diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index da9c4d804..b9314dc52 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -180,7 +180,7 @@ func getValidPath(path string) (string, error) { } if osIsNotExist(err) { // Disk not found create it. - if err = mkdirAll(path, 0o777); err != nil { + if err = mkdirAll(path, 0o777, ""); err != nil { return path, err } } @@ -397,6 +397,11 @@ func (s *xlStorage) checkODirectDiskSupport() error { // Check if backend is writable and supports O_DIRECT uuid := mustGetUUID() filePath := pathJoin(s.drivePath, minioMetaTmpDeletedBucket, ".writable-check-"+uuid+".tmp") + + // Create top level directories if they don't exist. + // with mode 0o777 mkdir honors system umask. + mkdirAll(pathutil.Dir(filePath), 0o777, s.drivePath) // don't need to fail here + w, err := s.openFileDirect(filePath, os.O_CREATE|os.O_WRONLY|os.O_EXCL) if err != nil { return err @@ -822,7 +827,7 @@ func (s *xlStorage) MakeVol(ctx context.Context, volume string) error { // Volume does not exist we proceed to create. if osIsNotExist(err) { // Make a volume entry, with mode 0777 mkdir honors system umask. - err = mkdirAll(volumeDir, 0o777) + err = mkdirAll(volumeDir, 0o777, s.drivePath) } if osIsPermission(err) { return errDiskAccessDenied @@ -1089,16 +1094,16 @@ func (s *xlStorage) moveToTrash(filePath string, recursive, force bool) error { pathUUID := mustGetUUID() targetPath := pathutil.Join(s.drivePath, minioMetaTmpDeletedBucket, pathUUID) - var renameFn func(source, target string) error if recursive { - renameFn = renameAll + if err := renameAll(filePath, targetPath, s.drivePath); err != nil { + return err + } } else { - renameFn = Rename + if err := Rename(filePath, targetPath); err != nil { + return err + } } - if err := renameFn(filePath, targetPath); err != nil { - return err - } // immediately purge the target if force { removeAll(targetPath) @@ -1717,10 +1722,6 @@ func (s *xlStorage) ReadFile(ctx context.Context, volume string, path string, of } func (s *xlStorage) openFileDirect(path string, mode int) (f *os.File, err error) { - // Create top level directories if they don't exist. - // with mode 0o777 mkdir honors system umask. - mkdirAll(pathutil.Dir(path), 0o777) // don't need to fail here - w, err := OpenFileDirectIO(path, mode, 0o666) if err != nil { switch { @@ -1747,7 +1748,7 @@ func (s *xlStorage) openFileSync(filePath string, mode int) (f *os.File, err err func (s *xlStorage) openFile(filePath string, mode int) (f *os.File, err error) { // Create top level directories if they don't exist. // with mode 0777 mkdir honors system umask. - if err = mkdirAll(pathutil.Dir(filePath), 0o777); err != nil { + if err = mkdirAll(pathutil.Dir(filePath), 0o777, s.drivePath); err != nil { return nil, osErrToFileErr(err) } @@ -1925,7 +1926,7 @@ func (s *xlStorage) writeAllDirect(ctx context.Context, filePath string, fileSiz // 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); err != nil { + if err = mkdirAll(parentFilePath, 0o777, s.drivePath); err != nil { return osErrToFileErr(err) } @@ -2388,7 +2389,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f } // legacy data dir means its old content, honor system umask. - if err = mkdirAll(legacyDataPath, 0o777); err != nil { + if err = mkdirAll(legacyDataPath, 0o777, s.drivePath); err != nil { // any failed mkdir-calls delete them. s.deleteFile(dstVolumeDir, legacyDataPath, true, false) return 0, osErrToFileErr(err) @@ -2505,7 +2506,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f // on a versioned bucket. s.moveToTrash(legacyDataPath, true, false) } - if err = renameAll(srcDataPath, dstDataPath); err != nil { + if err = renameAll(srcDataPath, dstDataPath, s.drivePath); err != nil { if legacyPreserved { // Any failed rename calls un-roll previous transaction. s.deleteFile(dstVolumeDir, legacyDataPath, true, false) @@ -2516,7 +2517,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f } // Commit meta-file - if err = renameAll(srcFilePath, dstFilePath); err != nil { + if err = renameAll(srcFilePath, dstFilePath, s.drivePath); err != nil { if legacyPreserved { // Any failed rename calls un-roll previous transaction. s.deleteFile(dstVolumeDir, legacyDataPath, true, false) @@ -2626,7 +2627,7 @@ func (s *xlStorage) RenameFile(ctx context.Context, srcVolume, srcPath, dstVolum } } - if err = renameAll(srcFilePath, dstFilePath); err != nil { + if err = renameAll(srcFilePath, dstFilePath, s.drivePath); err != nil { if isSysErrNotEmpty(err) || isSysErrNotDir(err) { return errFileAccessDenied }