From 7725425e051b15d8bb3696ce5b5208c8276f50b9 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Sun, 24 Jul 2022 00:43:11 -0700 Subject: [PATCH] fix: fork os.MkdirAll to optimize cases where parent exists (#15379) a/b/c/d/ where `a/b/c/` exists results in additional syscalls such as an Lstat() call to verify if the `a/b/c/` exists and its a directory. We do not need to do this on MinIO since the parent prefixes if exist, we can simply return success without spending additional syscalls. Also this implementation attempts to simply use Access() calls to avoid os.Stat() calls since the latter does memory allocation for things we do not need to use. Access() is simpler since we have a predictable structure on the backend and we know exactly how our path structures are. --- cmd/fs-v1-rwpool.go | 2 + cmd/os-instrumented.go | 9 +++- cmd/os-reliable.go | 3 +- cmd/{os-readdir_other.go => os_other.go} | 4 ++ cmd/{os-readdir_unix.go => os_unix.go} | 49 ++++++++++++++++++++ cmd/{os-readdir_windows.go => os_windows.go} | 4 ++ cmd/osmetric_string.go | 31 +++++++------ cmd/xl-storage.go | 9 +++- 8 files changed, 93 insertions(+), 18 deletions(-) rename cmd/{os-readdir_other.go => os_other.go} (97%) rename cmd/{os-readdir_unix.go => os_unix.go} (85%) rename cmd/{os-readdir_windows.go => os_windows.go} (98%) diff --git a/cmd/fs-v1-rwpool.go b/cmd/fs-v1-rwpool.go index 3ea10ec3a..462f94a9f 100644 --- a/cmd/fs-v1-rwpool.go +++ b/cmd/fs-v1-rwpool.go @@ -187,6 +187,8 @@ func (fsi *fsIOPool) Create(path string) (wlk *lock.LockedFile, err error) { return nil, errFileAccessDenied case isSysErrIsDir(err): return nil, errIsNotRegular + case isSysErrNotDir(err): + return nil, errFileAccessDenied case isSysErrPathNotFound(err): return nil, errFileAccessDenied default: diff --git a/cmd/os-instrumented.go b/cmd/os-instrumented.go index 51aee2306..867a86489 100644 --- a/cmd/os-instrumented.go +++ b/cmd/os-instrumented.go @@ -35,6 +35,7 @@ type osMetric uint8 const ( osMetricRemoveAll osMetric = iota osMetricMkdirAll + osMetricMkdir osMetricRename osMetricOpenFile osMetricOpen @@ -114,10 +115,16 @@ func RemoveAll(dirPath string) error { return os.RemoveAll(dirPath) } +// Mkdir captures time taken to call os.Mkdir +func Mkdir(dirPath string, mode os.FileMode) error { + defer updateOSMetrics(osMetricMkdir, dirPath)() + return os.Mkdir(dirPath, mode) +} + // MkdirAll captures time taken to call os.MkdirAll func MkdirAll(dirPath string, mode os.FileMode) error { defer updateOSMetrics(osMetricMkdirAll, dirPath)() - return os.MkdirAll(dirPath, mode) + return osMkdirAll(dirPath, mode) } // Rename captures time taken to call os.Rename diff --git a/cmd/os-reliable.go b/cmd/os-reliable.go index 4ba92b8f4..6de5a4aba 100644 --- a/cmd/os-reliable.go +++ b/cmd/os-reliable.go @@ -104,7 +104,7 @@ func reliableMkdirAll(dirPath string, mode os.FileMode) (err error) { i := 0 for { // Creates all the parent directories, with mode 0777 mkdir honors system umask. - if err = MkdirAll(dirPath, mode); err != nil { + if err = osMkdirAll(dirPath, mode); err != nil { // Retry only for the first retryable error. if osIsNotExist(err) && i == 0 { i++ @@ -166,6 +166,7 @@ func reliableRename(srcFilePath, dstFilePath string) (err error) { if err = reliableMkdirAll(path.Dir(dstFilePath), 0o777); err != nil { return err } + i := 0 for { // After a successful parent directory create attempt a renameAll. diff --git a/cmd/os-readdir_other.go b/cmd/os_other.go similarity index 97% rename from cmd/os-readdir_other.go rename to cmd/os_other.go index dfbec38a3..9d0bd8891 100644 --- a/cmd/os-readdir_other.go +++ b/cmd/os_other.go @@ -31,6 +31,10 @@ func access(name string) error { return err } +func osMkdirAll(dirPath string, perm os.FileMode) error { + return os.MkdirAll(dirPath, perm) +} + // readDirFn applies the fn() function on each entries at dirPath, doesn't recurse into // the directory itself, if the dirPath doesn't exist this function doesn't return // an error. diff --git a/cmd/os-readdir_unix.go b/cmd/os_unix.go similarity index 85% rename from cmd/os-readdir_unix.go rename to cmd/os_unix.go index 51374ac1d..782702502 100644 --- a/cmd/os-readdir_unix.go +++ b/cmd/os_unix.go @@ -38,6 +38,55 @@ func access(name string) error { return nil } +// Forked from Golang but chooses fast path upon os.Mkdir() +// error to avoid os.Lstat() call. +// +// osMkdirAll creates a directory named path, +// along with any necessary parents, and returns nil, +// or else returns an error. +// The permission bits perm (before umask) are used for all +// directories that MkdirAll creates. +// If path is already a directory, MkdirAll does nothing +// and returns nil. +func osMkdirAll(dirPath string, perm os.FileMode) error { + // Fast path: if we can tell whether path is a directory or file, stop with success or error. + err := Access(dirPath) + if err == nil { + return nil + } + if !osIsNotExist(err) { + return &os.PathError{Op: "mkdir", Path: dirPath, Err: err} + } + + // 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. + i-- + } + + j := i + for j > 0 && !os.IsPathSeparator(dirPath[j-1]) { // Scan backward over element. + j-- + } + + if j > 1 { + // Create parent. + if err = osMkdirAll(dirPath[:j-1], perm); err != nil { + return err + } + } + + // Parent now exists; invoke Mkdir and use its result. + if err = Mkdir(dirPath, perm); err != nil { + if osIsExist(err) { + return nil + } + return err + } + + return nil +} + // The buffer must be at least a block long. // refer https://github.com/golang/go/issues/24015 const blockSize = 8 << 10 // 8192 diff --git a/cmd/os-readdir_windows.go b/cmd/os_windows.go similarity index 98% rename from cmd/os-readdir_windows.go rename to cmd/os_windows.go index 4150a5c8d..0c2e236f9 100644 --- a/cmd/os-readdir_windows.go +++ b/cmd/os_windows.go @@ -31,6 +31,10 @@ func access(name string) error { return err } +func osMkdirAll(dirPath string, perm os.FileMode) error { + return os.MkdirAll(dirPath, perm) +} + // readDirFn applies the fn() function on each entries at dirPath, doesn't recurse into // the directory itself, if the dirPath doesn't exist this function doesn't return // an error. diff --git a/cmd/osmetric_string.go b/cmd/osmetric_string.go index a3273cc31..e684b51cb 100644 --- a/cmd/osmetric_string.go +++ b/cmd/osmetric_string.go @@ -10,24 +10,25 @@ func _() { var x [1]struct{} _ = x[osMetricRemoveAll-0] _ = x[osMetricMkdirAll-1] - _ = x[osMetricRename-2] - _ = x[osMetricOpenFile-3] - _ = x[osMetricOpen-4] - _ = x[osMetricOpenFileDirectIO-5] - _ = x[osMetricLstat-6] - _ = x[osMetricRemove-7] - _ = x[osMetricStat-8] - _ = x[osMetricAccess-9] - _ = x[osMetricCreate-10] - _ = x[osMetricReadDirent-11] - _ = x[osMetricFdatasync-12] - _ = x[osMetricSync-13] - _ = x[osMetricLast-14] + _ = x[osMetricMkdir-2] + _ = x[osMetricRename-3] + _ = x[osMetricOpenFile-4] + _ = x[osMetricOpen-5] + _ = x[osMetricOpenFileDirectIO-6] + _ = x[osMetricLstat-7] + _ = x[osMetricRemove-8] + _ = x[osMetricStat-9] + _ = x[osMetricAccess-10] + _ = x[osMetricCreate-11] + _ = x[osMetricReadDirent-12] + _ = x[osMetricFdatasync-13] + _ = x[osMetricSync-14] + _ = x[osMetricLast-15] } -const _osMetric_name = "RemoveAllMkdirAllRenameOpenFileOpenOpenFileDirectIOLstatRemoveStatAccessCreateReadDirentFdatasyncSyncLast" +const _osMetric_name = "RemoveAllMkdirAllMkdirRenameOpenFileOpenOpenFileDirectIOLstatRemoveStatAccessCreateReadDirentFdatasyncSyncLast" -var _osMetric_index = [...]uint8{0, 9, 17, 23, 31, 35, 51, 56, 62, 66, 72, 78, 88, 97, 101, 105} +var _osMetric_index = [...]uint8{0, 9, 17, 22, 28, 36, 40, 56, 61, 67, 71, 77, 83, 93, 102, 106, 110} func (i osMetric) String() string { if i >= osMetric(len(_osMetric_index)-1) { diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index 119099f5d..4923b44e4 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -1647,6 +1647,8 @@ func (s *xlStorage) openFileSync(filePath string, mode int) (f *os.File, err err return nil, errIsNotRegular case osIsPermission(err): return nil, errFileAccessDenied + case isSysErrNotDir(err): + return nil, errFileAccessDenied case isSysErrIO(err): return nil, errFaultyDisk case isSysErrTooManyFiles(err): @@ -2487,8 +2489,10 @@ func (s *xlStorage) RenameFile(ctx context.Context, srcVolume, srcPath, dstVolum return errFileAccessDenied } if err = Remove(dstFilePath); err != nil { - if isSysErrNotEmpty(err) { + if isSysErrNotEmpty(err) || isSysErrNotDir(err) { return errFileAccessDenied + } else if isSysErrIO(err) { + return errFaultyDisk } return err } @@ -2496,6 +2500,9 @@ func (s *xlStorage) RenameFile(ctx context.Context, srcVolume, srcPath, dstVolum } if err = renameAll(srcFilePath, dstFilePath); err != nil { + if isSysErrNotEmpty(err) || isSysErrNotDir(err) { + return errFileAccessDenied + } return osErrToFileErr(err) }