From 019fe69a576ed217f47270f779375939f151f33a Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 4 Aug 2020 12:09:41 -0700 Subject: [PATCH] fix: reduce an extra system call for writes instead fail later (#10187) --- cmd/fs-v1-helpers.go | 18 +++++++---- cmd/globals.go | 1 - cmd/storage-errors.go | 9 ++++++ cmd/xl-storage.go | 72 ++++++++---------------------------------- cmd/xl-storage_test.go | 52 ------------------------------ 5 files changed, 35 insertions(+), 117 deletions(-) diff --git a/cmd/fs-v1-helpers.go b/cmd/fs-v1-helpers.go index c12961bb6..437b5bfc5 100644 --- a/cmd/fs-v1-helpers.go +++ b/cmd/fs-v1-helpers.go @@ -285,12 +285,18 @@ func fsCreateFile(ctx context.Context, filePath string, reader io.Reader, buf [] } if err := mkdirAll(pathutil.Dir(filePath), 0777); err != nil { - logger.LogIf(ctx, err) - return 0, err - } - - if err := checkDiskFree(pathutil.Dir(filePath), fallocSize); err != nil { - logger.LogIf(ctx, err) + switch { + case os.IsPermission(err): + return 0, errFileAccessDenied + case os.IsExist(err): + return 0, errFileAccessDenied + case isSysErrIO(err): + return 0, errFaultyDisk + case isSysErrInvalidArg(err): + return 0, errUnsupportedDisk + case isSysErrNoSpace(err): + return 0, errDiskFull + } return 0, err } diff --git a/cmd/globals.go b/cmd/globals.go index 7358611de..61063e727 100644 --- a/cmd/globals.go +++ b/cmd/globals.go @@ -58,7 +58,6 @@ const ( globalMinioDefaultOwnerID = "02d6176db174dc93cb1b899f7c6078f08654445fe8cf1b6ce98d8855f66bdbf4" globalMinioDefaultStorageClass = "STANDARD" globalWindowsOSName = "windows" - globalNetBSDOSName = "netbsd" globalMacOSName = "darwin" globalMinioModeFS = "mode-server-fs" globalMinioModeErasure = "mode-server-xl" diff --git a/cmd/storage-errors.go b/cmd/storage-errors.go index 4b989cf5d..efb0e104f 100644 --- a/cmd/storage-errors.go +++ b/cmd/storage-errors.go @@ -142,5 +142,14 @@ func osErrToFileErr(err error) error { if isSysErrHandleInvalid(err) { return errFileNotFound } + if isSysErrIO(err) { + return errFaultyDisk + } + if isSysErrInvalidArg(err) { + return errUnsupportedDisk + } + if isSysErrNoSpace(err) { + return errDiskFull + } return err } diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index 9b0d58658..a9492de26 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -48,8 +48,7 @@ import ( const ( nullVersionID = "null" - diskMinFreeSpace = 900 * humanize.MiByte // Min 900MiB free space. - diskMinTotalSpace = diskMinFreeSpace // Min 900MiB total space. + diskMinTotalSpace = 900 * humanize.MiByte // Min 900MiB total space. readBlockSize = 4 * humanize.MiByte // Default read block size 4MiB. // On regular files bigger than this; @@ -175,6 +174,7 @@ func getValidPath(path string, requireDirectIO bool) (string, error) { if err != nil { return path, err } + if err = checkDiskMinTotal(di); err != nil { return path, err } @@ -276,13 +276,6 @@ func getDiskInfo(diskPath string) (di disk.Info, err error) { return di, err } -// List of operating systems where we ignore disk space -// verification. -var ignoreDiskFreeOS = []string{ - globalWindowsOSName, - globalNetBSDOSName, -} - // check if disk total has minimum required size. func checkDiskMinTotal(di disk.Info) (err error) { // Remove 5% from total space for cumulative disk space @@ -294,46 +287,6 @@ func checkDiskMinTotal(di disk.Info) (err error) { return nil } -// check if disk free has minimum required size. -func checkDiskMinFree(di disk.Info) error { - // Remove 5% from free space for cumulative disk space used for journalling, inodes etc. - availableDiskSpace := float64(di.Free) * diskFillFraction - if int64(availableDiskSpace) <= diskMinFreeSpace { - return errDiskFull - } - - // Success. - return nil -} - -// checkDiskFree verifies if disk path has sufficient minimum free disk space and files. -func checkDiskFree(diskPath string, neededSpace int64) (err error) { - // We don't validate disk space or inode utilization on windows. - // Each windows call to 'GetVolumeInformationW' takes around - // 3-5seconds. And StatDISK is not supported by Go for solaris - // and netbsd. - if contains(ignoreDiskFreeOS, runtime.GOOS) { - return nil - } - - var di disk.Info - di, err = getDiskInfo(diskPath) - if err != nil { - return err - } - - if err = checkDiskMinFree(di); err != nil { - return err - } - - // Check if we have enough space to store data - if neededSpace > int64(float64(di.Free)*diskFillFraction) { - return errDiskFull - } - - return nil -} - // Implements stringer compatible interface. func (s *xlStorage) String() string { return s.diskPath @@ -1643,14 +1596,6 @@ func (s *xlStorage) CreateFile(volume, path string, fileSize int64, r io.Reader) atomic.AddInt32(&s.activeIOCount, -1) }() - // Validate if disk is indeed free. - if err = checkDiskFree(s.diskPath, fileSize); err != nil { - if isSysErrIO(err) { - return errFaultyDisk - } - return err - } - volumeDir, err := s.getVolDir(volume) if err != nil { return err @@ -1674,8 +1619,17 @@ func (s *xlStorage) CreateFile(volume, path string, fileSize int64, r io.Reader) // Create top level directories if they don't exist. // with mode 0777 mkdir honors system umask. if err = mkdirAll(slashpath.Dir(filePath), 0777); err != nil { - if errors.Is(err, &os.PathError{}) { + switch { + case os.IsPermission(err): return errFileAccessDenied + case os.IsExist(err): + return errFileAccessDenied + case isSysErrIO(err): + return errFaultyDisk + case isSysErrInvalidArg(err): + return errUnsupportedDisk + case isSysErrNoSpace(err): + return errDiskFull } return err } @@ -1691,6 +1645,8 @@ func (s *xlStorage) CreateFile(volume, path string, fileSize int64, r io.Reader) return errFaultyDisk case isSysErrInvalidArg(err): return errUnsupportedDisk + case isSysErrNoSpace(err): + return errDiskFull default: return err } diff --git a/cmd/xl-storage_test.go b/cmd/xl-storage_test.go index dfbaa5ab8..5aa2c97ab 100644 --- a/cmd/xl-storage_test.go +++ b/cmd/xl-storage_test.go @@ -1766,55 +1766,3 @@ func TestCheckDiskTotalMin(t *testing.T) { } } } - -// Checks for restrictions for min free disk space and inodes. -func TestCheckDiskFreeMin(t *testing.T) { - testCases := []struct { - diskInfo disk.Info - err error - }{ - // Test 1 - when fstype is nfs. - { - diskInfo: disk.Info{ - Free: diskMinTotalSpace * 3, - FSType: "NFS", - }, - err: nil, - }, - // Test 2 - when fstype is xfs and total inodes are less than 10k. - { - diskInfo: disk.Info{ - Free: diskMinTotalSpace * 3, - FSType: "XFS", - Files: 9999, - Ffree: 9999, - }, - err: nil, - }, - // Test 3 - when fstype is btrfs and total inodes are empty. - { - diskInfo: disk.Info{ - Free: diskMinTotalSpace * 3, - FSType: "BTRFS", - Files: 0, - }, - err: nil, - }, - // Test 4 - when fstype is xfs and total disk space is really small. - { - diskInfo: disk.Info{ - Free: diskMinTotalSpace - diskMinTotalSpace/1024, - FSType: "XFS", - Files: 9999, - }, - err: errDiskFull, - }, - } - - // Validate all cases. - for i, test := range testCases { - if err := checkDiskMinFree(test.diskInfo); test.err != err { - t.Errorf("Test %d: Expected error %s, got %s", i+1, test.err, err) - } - } -}