From f3c6c5571974ed6fb4ece036b2accb1741f5b612 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Mon, 31 Oct 2016 09:34:44 -0700 Subject: [PATCH] posix: Fix windows performance issues. (#3132) Do not attempt to fetch volume/drive information for each i/o situation. In our case we do this in all calls `posix.go` this in-turn created a terrible situation for windows. This issue does not affect the i/o path on Unix platforms since statvfs calls are in the range of micro seconds on these platforms. This verification is only needed during startup and we let things fail at a later stage on windows. --- cmd/posix-errors.go | 15 ++++- cmd/posix-errors_test.go | 57 +++++++++++++++++++ cmd/posix.go | 117 ++++++++++++++++++++++++--------------- cmd/posix_test.go | 17 ------ pkg/disk/stat_windows.go | 24 ++++---- pkg/disk/type_windows.go | 8 ++- 6 files changed, 160 insertions(+), 78 deletions(-) create mode 100644 cmd/posix-errors_test.go diff --git a/cmd/posix-errors.go b/cmd/posix-errors.go index 337c068ff..74dbea72c 100644 --- a/cmd/posix-errors.go +++ b/cmd/posix-errors.go @@ -42,7 +42,7 @@ func isSysErrIO(err error) bool { return err != nil && err == syscall.EIO } -// Check if the given error corresponds to ENOTDIR (is not a directory) +// Check if the given error corresponds to ENOTDIR (is not a directory). func isSysErrNotDir(err error) bool { if pathErr, ok := err.(*os.PathError); ok { switch pathErr.Err { @@ -53,8 +53,19 @@ func isSysErrNotDir(err error) bool { return false } +// Check if the given error corresponds to the ENAMETOOLONG (name too long). +func isSysErrTooLong(err error) bool { + if pathErr, ok := err.(*os.PathError); ok { + switch pathErr.Err { + case syscall.ENAMETOOLONG: + return true + } + } + return false +} + // Check if the given error corresponds to ENOTEMPTY for unix -// and ERROR_DIR_NOT_EMPTY for windows (directory not empty) +// and ERROR_DIR_NOT_EMPTY for windows (directory not empty). func isSysErrNotEmpty(err error) bool { if pathErr, ok := err.(*os.PathError); ok { if runtime.GOOS == "windows" { diff --git a/cmd/posix-errors_test.go b/cmd/posix-errors_test.go new file mode 100644 index 000000000..498022f50 --- /dev/null +++ b/cmd/posix-errors_test.go @@ -0,0 +1,57 @@ +/* + * Minio Cloud Storage, (C) 2016 Minio, 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 cmd + +import ( + "os" + "runtime" + "syscall" + "testing" +) + +func TestSysErrors(t *testing.T) { + pathErr := &os.PathError{Err: syscall.ENAMETOOLONG} + ok := isSysErrTooLong(pathErr) + if !ok { + t.Fatalf("Unexpected error expecting %s", syscall.ENAMETOOLONG) + } + pathErr = &os.PathError{Err: syscall.ENOTDIR} + ok = isSysErrNotDir(pathErr) + if !ok { + t.Fatalf("Unexpected error expecting %s", syscall.ENOTDIR) + } + if runtime.GOOS != "windows" { + pathErr = &os.PathError{Err: syscall.ENOTEMPTY} + ok = isSysErrNotEmpty(pathErr) + if !ok { + t.Fatalf("Unexpected error expecting %s", syscall.ENOTEMPTY) + } + } else { + pathErr = &os.PathError{Err: syscall.Errno(0x91)} + ok = isSysErrNotEmpty(pathErr) + if !ok { + t.Fatal("Unexpected error expecting 0x91") + } + } + if runtime.GOOS == "windows" { + pathErr = &os.PathError{Err: syscall.Errno(0x03)} + ok = isSysErrPathNotFound(pathErr) + if !ok { + t.Fatal("Unexpected error expecting 0x03") + } + } +} diff --git a/cmd/posix.go b/cmd/posix.go index 029a9f991..84b5ac474 100644 --- a/cmd/posix.go +++ b/cmd/posix.go @@ -34,9 +34,9 @@ import ( ) const ( - fsMinFreeSpace = 1024 * 1024 * 1024 - fsMinFreeInodesPercent = 5 - maxAllowedIOError = 5 + fsMinFreeSpace = 1024 * 1024 * 1024 // Min 1GiB free space. + fsMinFreeInodes = 10000 // Min 10000. + maxAllowedIOError = 5 ) // posix - implements StorageAPI interface. @@ -57,6 +57,9 @@ func checkPathLength(pathName string) error { return errFileNameTooLong } + // Convert any '\' to '/'. + pathName = filepath.ToSlash(pathName) + // Check each path segment length is > 255 for len(pathName) > 0 && pathName != "." && pathName != "/" { dir, file := slashpath.Dir(pathName), slashpath.Base(pathName) @@ -111,7 +114,7 @@ func newPosix(path string) (StorageAPI, error) { fs := &posix{ diskPath: diskPath, minFreeSpace: fsMinFreeSpace, - minFreeInodes: fsMinFreeInodesPercent, + minFreeInodes: fsMinFreeInodes, // 1MiB buffer pool for posix internal operations. pool: sync.Pool{ New: func() interface{} { @@ -120,17 +123,21 @@ func newPosix(path string) (StorageAPI, error) { }, }, } - st, err := os.Stat(preparePath(diskPath)) - if err != nil { - if os.IsNotExist(err) { - // Disk not found create it. - err = os.MkdirAll(diskPath, 0777) - return fs, err + fi, err := os.Stat(preparePath(diskPath)) + if err == nil { + if !fi.IsDir() { + return nil, syscall.ENOTDIR } - return fs, err } - if !st.IsDir() { - return fs, syscall.ENOTDIR + if os.IsNotExist(err) { + // Disk not found create it. + err = os.MkdirAll(preparePath(diskPath), 0777) + if err != nil { + return nil, err + } + } + if err = fs.checkDiskFree(); err != nil { + return nil, err } return fs, nil } @@ -150,7 +157,14 @@ func getDiskInfo(diskPath string) (di disk.Info, err error) { // checkDiskFree verifies if disk path has sufficient minimum free disk space and files. func (s *posix) checkDiskFree() (err error) { - di, err := getDiskInfo(s.diskPath) + // We don't validate disk space or inode utilization on windows. + // Each windows calls to 'GetVolumeInformationW' takes around 3-5seconds. + if runtime.GOOS == "windows" { + return nil + } + + var di disk.Info + di, err = getDiskInfo(preparePath(s.diskPath)) if err != nil { return err } @@ -166,8 +180,8 @@ func (s *posix) checkDiskFree() (err error) { // Allow for the available disk to be separately validate and we will validate inodes only if // total inodes are provided by the underlying filesystem. if di.Files != 0 { - availableFiles := 100 * float64(di.Ffree) / float64(di.Files) - if int64(availableFiles) <= s.minFreeInodes { + availableFiles := int64(di.Ffree) + if availableFiles <= s.minFreeInodes { return errDiskFull } } @@ -184,7 +198,7 @@ func (s *posix) String() string { // DiskInfo provides current information about disk space usage, // total free inodes and underlying filesystem. func (s *posix) DiskInfo() (info disk.Info, err error) { - return getDiskInfo(s.diskPath) + return getDiskInfo(preparePath(s.diskPath)) } // getVolDir - will convert incoming volume names to @@ -199,6 +213,20 @@ func (s *posix) getVolDir(volume string) (string, error) { return volumeDir, nil } +// checkDiskFound - validates if disk is available, +// returns errDiskNotFound if not found. +func (s *posix) checkDiskFound() (err error) { + _, err = os.Stat(preparePath(s.diskPath)) + if err != nil { + if os.IsNotExist(err) { + return errDiskNotFound + } else if isSysErrTooLong(err) { + return errFileNameTooLong + } + } + return err +} + // Make a volume entry. func (s *posix) MakeVol(volume string) (err error) { defer func() { @@ -211,8 +239,7 @@ func (s *posix) MakeVol(volume string) (err error) { return errFaultyDisk } - // Validate if disk is free. - if err = s.checkDiskFree(); err != nil { + if err = s.checkDiskFound(); err != nil { return err } @@ -246,7 +273,11 @@ func (s *posix) ListVols() (volsInfo []VolInfo, err error) { return nil, errFaultyDisk } - volsInfo, err = listVols(s.diskPath) + if err = s.checkDiskFound(); err != nil { + return nil, err + } + + volsInfo, err = listVols(preparePath(s.diskPath)) if err != nil { return nil, err } @@ -306,8 +337,7 @@ func (s *posix) StatVol(volume string) (volInfo VolInfo, err error) { return VolInfo{}, errFaultyDisk } - // Check disk availability. - if _, err = getDiskInfo(s.diskPath); err != nil { + if err = s.checkDiskFound(); err != nil { return VolInfo{}, err } @@ -346,8 +376,7 @@ func (s *posix) DeleteVol(volume string) (err error) { return errFaultyDisk } - // Check disk availability. - if _, err = getDiskInfo(s.diskPath); err != nil { + if err = s.checkDiskFound(); err != nil { return err } @@ -381,8 +410,7 @@ func (s *posix) ListDir(volume, dirPath string) (entries []string, err error) { return nil, errFaultyDisk } - // Check disk availability. - if _, err = getDiskInfo(s.diskPath); err != nil { + if err = s.checkDiskFound(); err != nil { return nil, err } @@ -419,8 +447,7 @@ func (s *posix) ReadAll(volume, path string) (buf []byte, err error) { return nil, errFaultyDisk } - // Check disk availability. - if _, err = getDiskInfo(s.diskPath); err != nil { + if err = s.checkDiskFound(); err != nil { return nil, err } @@ -439,7 +466,7 @@ func (s *posix) ReadAll(volume, path string) (buf []byte, err error) { // Validate file path length, before reading. filePath := pathJoin(volumeDir, path) - if err = checkPathLength(filePath); err != nil { + if err = checkPathLength(preparePath(filePath)); err != nil { return nil, err } @@ -483,8 +510,7 @@ func (s *posix) ReadFile(volume string, path string, offset int64, buf []byte) ( return 0, errFaultyDisk } - // Check disk availability. - if _, err = getDiskInfo(s.diskPath); err != nil { + if err = s.checkDiskFound(); err != nil { return 0, err } @@ -503,7 +529,7 @@ func (s *posix) ReadFile(volume string, path string, offset int64, buf []byte) ( // Validate effective path length before reading. filePath := pathJoin(volumeDir, path) - if err = checkPathLength(filePath); err != nil { + if err = checkPathLength(preparePath(filePath)); err != nil { return 0, err } @@ -557,8 +583,7 @@ func (s *posix) createFile(volume, path string) (f *os.File, err error) { return nil, errFaultyDisk } - // Validate if disk is free. - if err = s.checkDiskFree(); err != nil { + if err = s.checkDiskFound(); err != nil { return nil, err } @@ -576,7 +601,7 @@ func (s *posix) createFile(volume, path string) (f *os.File, err error) { } filePath := pathJoin(volumeDir, path) - if err = checkPathLength(filePath); err != nil { + if err = checkPathLength(preparePath(filePath)); err != nil { return nil, err } @@ -632,6 +657,11 @@ func (s *posix) PrepareFile(volume, path string, fileSize int64) (err error) { return errFaultyDisk } + // Validate if disk is indeed free. + if err = s.checkDiskFree(); err != nil { + return err + } + // Create file if not found w, err := s.createFile(volume, path) if err != nil { @@ -705,8 +735,7 @@ func (s *posix) StatFile(volume, path string) (file FileInfo, err error) { return FileInfo{}, errFaultyDisk } - // Check disk availability. - if _, err = getDiskInfo(s.diskPath); err != nil { + if err = s.checkDiskFound(); err != nil { return FileInfo{}, err } @@ -724,7 +753,7 @@ func (s *posix) StatFile(volume, path string) (file FileInfo, err error) { } filePath := slashpath.Join(volumeDir, path) - if err = checkPathLength(filePath); err != nil { + if err = checkPathLength(preparePath(filePath)); err != nil { return FileInfo{}, err } st, err := os.Stat(preparePath(filePath)) @@ -802,8 +831,7 @@ func (s *posix) DeleteFile(volume, path string) (err error) { return errFaultyDisk } - // Check disk availability. - if _, err = getDiskInfo(s.diskPath); err != nil { + if err = s.checkDiskFound(); err != nil { return err } @@ -823,7 +851,7 @@ func (s *posix) DeleteFile(volume, path string) (err error) { // Following code is needed so that we retain "/" suffix if any in // path argument. filePath := pathJoin(volumeDir, path) - if err = checkPathLength(filePath); err != nil { + if err = checkPathLength(preparePath(filePath)); err != nil { return err } @@ -843,8 +871,7 @@ func (s *posix) RenameFile(srcVolume, srcPath, dstVolume, dstPath string) (err e return errFaultyDisk } - // Validate if disk is free. - if err = s.checkDiskFree(); err != nil { + if err = s.checkDiskFound(); err != nil { return err } @@ -878,11 +905,11 @@ func (s *posix) RenameFile(srcVolume, srcPath, dstVolume, dstPath string) (err e return errFileAccessDenied } srcFilePath := slashpath.Join(srcVolumeDir, srcPath) - if err = checkPathLength(srcFilePath); err != nil { + if err = checkPathLength(preparePath(srcFilePath)); err != nil { return err } dstFilePath := slashpath.Join(dstVolumeDir, dstPath) - if err = checkPathLength(dstFilePath); err != nil { + if err = checkPathLength(preparePath(dstFilePath)); err != nil { return err } if srcIsDir { diff --git a/cmd/posix_test.go b/cmd/posix_test.go index 2c55f0c7f..82abc98b3 100644 --- a/cmd/posix_test.go +++ b/cmd/posix_test.go @@ -536,23 +536,6 @@ func TestListVols(t *testing.T) { if _, err = posixStorage.ListVols(); err != errDiskNotFound { t.Errorf("Expected to fail with \"%s\", but instead failed with \"%s\"", errDiskNotFound, err) } - // creating a new posix instance. - posixStorage, path, err = newPosixTestSetup() - if err != nil { - t.Fatalf("Unable to create posix test setup, %s", err) - } - defer removeAll(path) - // adding the segment of the path length of the volume to be > 255. - if posixType, ok := posixStorage.(*posix); ok { - // setting the disk Path with name whose segment length > 255. - posixType.diskPath = "my-vol-name-0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001" - } else { - t.Errorf("Expected the StorageAPI to be of type *posix") - } - if _, err = posixStorage.ListVols(); err != errFileNameTooLong { - t.Errorf("Expected to fail with \"%s\", but instead failed with \"%s\"", errFileNameTooLong, err) - } - } // TestPosixListDir - Tests validate the directory listing functionality provided by posix.ListDir . diff --git a/pkg/disk/stat_windows.go b/pkg/disk/stat_windows.go index 6054350f6..8d019d208 100644 --- a/pkg/disk/stat_windows.go +++ b/pkg/disk/stat_windows.go @@ -24,6 +24,19 @@ import ( "unsafe" ) +var ( + kernel32 = syscall.NewLazyDLL("kernel32.dll") + + // GetDiskFreeSpaceEx - https://msdn.microsoft.com/en-us/library/windows/desktop/aa364937(v=vs.85).aspx + // Retrieves information about the amount of space that is available on a disk volume, + // which is the total amount of space, the total amount of free space, and the total + // amount of free space available to the user that is associated with the calling thread. + GetDiskFreeSpaceEx = kernel32.NewProc("GetDiskFreeSpaceExW") + // GetDiskFreeSpace - https://msdn.microsoft.com/en-us/library/windows/desktop/aa364935(v=vs.85).aspx + // Retrieves information about the specified disk, including the amount of free space on the disk. + GetDiskFreeSpace = kernel32.NewProc("GetDiskFreeSpaceW") +) + // GetInfo returns total and free bytes available in a directory, e.g. `C:\`. // It returns free space available to the user (including quota limitations) // @@ -34,13 +47,6 @@ func GetInfo(path string) (info Info, err error) { return Info{}, err } - dll := syscall.MustLoadDLL("kernel32.dll") - // https://msdn.microsoft.com/en-us/library/windows/desktop/aa364937(v=vs.85).aspx - // Retrieves information about the amount of space that is available on a disk volume, - // which is the total amount of space, the total amount of free space, and the total - // amount of free space available to the user that is associated with the calling thread. - GetDiskFreeSpaceEx := dll.MustFindProc("GetDiskFreeSpaceExW") - lpFreeBytesAvailable := int64(0) lpTotalNumberOfBytes := int64(0) lpTotalNumberOfFreeBytes := int64(0) @@ -61,10 +67,6 @@ func GetInfo(path string) (info Info, err error) { info.Free = int64(lpFreeBytesAvailable) info.FSType = getFSType(path) - // https://msdn.microsoft.com/en-us/library/windows/desktop/aa364935(v=vs.85).aspx - // Retrieves information about the specified disk, including the amount of free space on the disk. - GetDiskFreeSpace := dll.MustFindProc("GetDiskFreeSpaceW") - // Return values of GetDiskFreeSpace() lpSectorsPerCluster := uint32(0) lpBytesPerSector := uint32(0) diff --git a/pkg/disk/type_windows.go b/pkg/disk/type_windows.go index 03946af09..023fd5b57 100644 --- a/pkg/disk/type_windows.go +++ b/pkg/disk/type_windows.go @@ -24,11 +24,13 @@ import ( "unsafe" ) +var ( + // GetVolumeInformation provides windows drive volume information. + GetVolumeInformation = kernel32.NewProc("GetVolumeInformationW") +) + // getFSType returns the filesystem type of the underlying mounted filesystem func getFSType(path string) string { - dll := syscall.MustLoadDLL("kernel32.dll") - GetVolumeInformation := dll.MustFindProc("GetVolumeInformationW") - var volumeNameSize uint32 = 260 var nFileSystemNameSize, lpVolumeSerialNumber uint32 var lpFileSystemFlags, lpMaximumComponentLength uint32