From 43f973c4cf954491cd7df34adce6d05e5c8b7bb3 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 22 Jan 2021 15:38:21 -0800 Subject: [PATCH] fix: check for O_DIRECT support for reads and writes (#11331) In-case user enables O_DIRECT for reads and backend does not support it we shall proceed to turn it off instead and print a warning. This validation avoids any unexpected downtimes that users may incur. --- cmd/config/storageclass/storage-class.go | 2 +- cmd/fs-v1.go | 2 +- cmd/storage-rest_test.go | 4 +- cmd/xl-storage.go | 130 +++++++++++------------ cmd/xl-storage_test.go | 70 ++---------- 5 files changed, 72 insertions(+), 136 deletions(-) diff --git a/cmd/config/storageclass/storage-class.go b/cmd/config/storageclass/storage-class.go index 3fde56f94..b2d774ab1 100644 --- a/cmd/config/storageclass/storage-class.go +++ b/cmd/config/storageclass/storage-class.go @@ -64,7 +64,7 @@ const ( defaultRRSParity = minParityDisks // Default DMA value - defaultDMA = DMAWrite + defaultDMA = DMAReadWrite ) // DefaultKVS - default storage class config diff --git a/cmd/fs-v1.go b/cmd/fs-v1.go index 49f509c90..c3095c2e7 100644 --- a/cmd/fs-v1.go +++ b/cmd/fs-v1.go @@ -123,7 +123,7 @@ func NewFSObjectLayer(fsPath string) (ObjectLayer, error) { } var err error - if fsPath, err = getValidPath(fsPath, false); err != nil { + if fsPath, err = getValidPath(fsPath); err != nil { if err == errMinDiskSize { return nil, config.ErrUnableToWriteInBackend(err).Hint(err.Error()) } diff --git a/cmd/storage-rest_test.go b/cmd/storage-rest_test.go index 66e5ff9fb..b810b1bb4 100644 --- a/cmd/storage-rest_test.go +++ b/cmd/storage-rest_test.go @@ -82,8 +82,8 @@ func testStorageAPIListVols(t *testing.T, storage StorageAPI) { expectedResult []VolInfo expectErr bool }{ - {nil, []VolInfo{}, false}, - {[]string{"foo"}, []VolInfo{{Name: "foo"}}, false}, + {nil, []VolInfo{{Name: ".minio.sys"}}, false}, + {[]string{"foo"}, []VolInfo{{Name: ".minio.sys"}, {Name: "foo"}}, false}, } for i, testCase := range testCases { diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index 8ad2cd7a2..0cf7c4c62 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -53,10 +53,9 @@ import ( ) const ( - nullVersionID = "null" - diskMinTotalSpace = 900 * humanize.MiByte // Min 900MiB total space. - blockSizeLarge = 1 * humanize.MiByte // Default r/w block size for larger objects. - blockSizeSmall = 128 * humanize.KiByte // Default r/w block size for smaller objects. + nullVersionID = "null" + blockSizeLarge = 1 * humanize.MiByte // Default r/w block size for larger objects. + blockSizeSmall = 128 * humanize.KiByte // Default r/w block size for smaller objects. // On regular files bigger than this; readAheadSize = 16 << 20 @@ -103,6 +102,8 @@ type xlStorage struct { rootDisk bool + readODirectSupported bool + diskID string formatFileInfo os.FileInfo @@ -155,7 +156,7 @@ func checkPathLength(pathName string) error { return nil } -func getValidPath(path string, requireDirectIO bool) (string, error) { +func getValidPath(path string) (string, error) { if path == "" { return path, errInvalidArgument } @@ -173,7 +174,7 @@ func getValidPath(path string, requireDirectIO bool) (string, error) { } if osIsNotExist(err) { // Disk not found create it. - if err = os.MkdirAll(path, 0777); err != nil { + if err = reliableMkdirAll(path, 0777); err != nil { return path, err } } @@ -181,42 +182,6 @@ func getValidPath(path string, requireDirectIO bool) (string, error) { return path, errDiskNotDir } - // check if backend is writable. - var rnd [8]byte - _, _ = rand.Read(rnd[:]) - - fn := pathJoin(path, ".writable-check-"+hex.EncodeToString(rnd[:])+".tmp") - defer os.Remove(fn) - - var file *os.File - - if requireDirectIO { - // only erasure coding needs direct-io support - file, err = disk.OpenFileDirectIO(fn, os.O_CREATE|os.O_EXCL, 0666) - } else { - file, err = os.OpenFile(fn, os.O_CREATE|os.O_EXCL, 0666) - } - - // open file in direct I/O and use default umask, this also verifies - // if direct i/o failed. - if err != nil { - if isSysErrInvalidArg(err) { - // O_DIRECT not supported - return path, errUnsupportedDisk - } - return path, osErrToFileErr(err) - } - file.Close() - - di, err := getDiskInfo(path) - if err != nil { - return path, err - } - - if err = checkDiskMinTotal(di); err != nil { - return path, err - } - return path, nil } @@ -245,7 +210,7 @@ func newLocalXLStorage(path string) (*xlStorage, error) { func newXLStorage(ep Endpoint) (*xlStorage, error) { path := ep.Path var err error - if path, err = getValidPath(path, true); err != nil { + if path, err = getValidPath(path); err != nil { return nil, err } @@ -279,6 +244,35 @@ func newXLStorage(ep Endpoint) (*xlStorage, error) { rootDisk: rootDisk, } + // Create all necessary bucket folders if possible. + if err = p.MakeVolBulk(context.TODO(), minioMetaBucket, minioMetaTmpBucket, minioMetaMultipartBucket, dataUsageBucket); err != nil { + return nil, err + } + + // Check if backend is writable and supports O_DIRECT + var rnd [8]byte + _, _ = rand.Read(rnd[:]) + tmpFile := ".writable-check-" + hex.EncodeToString(rnd[:]) + ".tmp" + if err = p.CreateFile(GlobalContext, minioMetaTmpBucket, tmpFile, 1, strings.NewReader("0")); err != nil { + return p, err + } + defer os.Remove(pathJoin(p.diskPath, minioMetaTmpBucket, tmpFile)) + + volumeDir, err := p.getVolDir(minioMetaTmpBucket) + if err != nil { + return p, err + } + + // Check if backend is readable, and optionally supports O_DIRECT. + if _, err = p.readAllData(volumeDir, pathJoin(volumeDir, tmpFile), true); err != nil { + if err != errUnsupportedDisk { + return p, err + } + // error is unsupported disk, turn-off directIO for reads + logger.Info(fmt.Sprintf("Drive %s does not support O_DIRECT for reads, proceeding to use the drive without O_DIRECT", ep)) + p.readODirectSupported = false + } + // Success. return p, nil } @@ -301,17 +295,6 @@ func getDiskInfo(diskPath string) (di disk.Info, err error) { return di, err } -// check if disk total has minimum required size. -func checkDiskMinTotal(di disk.Info) (err error) { - // Remove 5% from total space for cumulative disk space - // used for journalling, inodes etc. - totalDiskSpace := float64(di.Total) * diskFillFraction - if int64(totalDiskSpace) <= diskMinTotalSpace { - return errMinDiskSize - } - return nil -} - // Implements stringer compatible interface. func (s *xlStorage) String() string { return s.diskPath @@ -583,11 +566,11 @@ func (s *xlStorage) SetDiskID(id string) { // storage rest server for remote disks. } -func (s *xlStorage) MakeVolBulk(ctx context.Context, volumes ...string) (err error) { +func (s *xlStorage) MakeVolBulk(ctx context.Context, volumes ...string) error { for _, volume := range volumes { - if err = s.MakeVol(ctx, volume); err != nil { - if osIsPermission(err) { - return errVolumeAccessDenied + if err := s.MakeVol(ctx, volume); err != nil { + if errors.Is(err, errDiskAccessDenied) { + return errDiskAccessDenied } } } @@ -595,7 +578,7 @@ func (s *xlStorage) MakeVolBulk(ctx context.Context, volumes ...string) (err err } // Make a volume entry. -func (s *xlStorage) MakeVol(ctx context.Context, volume string) (err error) { +func (s *xlStorage) MakeVol(ctx context.Context, volume string) error { if !isValidVolname(volume) { return errInvalidArgument } @@ -614,7 +597,7 @@ func (s *xlStorage) MakeVol(ctx context.Context, volume string) (err error) { // Volume does not exist we proceed to create. if osIsNotExist(err) { // Make a volume entry, with mode 0777 mkdir honors system umask. - err = os.MkdirAll(volumeDir, 0777) + err = reliableMkdirAll(volumeDir, 0777) } if osIsPermission(err) { return errDiskAccessDenied @@ -1138,7 +1121,10 @@ func (s *xlStorage) ReadVersion(ctx context.Context, volume, path, versionID str // - object size lesser than 32KiB // - object has maximum of 1 parts if fi.TransitionStatus == "" && fi.DataDir != "" && fi.Size <= smallFileThreshold && len(fi.Parts) == 1 { - fi.Data, err = s.readAllData(volumeDir, pathJoin(volumeDir, path, fi.DataDir, fmt.Sprintf("part.%d", fi.Parts[0].Number))) + // Enable O_DIRECT optionally only if drive supports it. + requireDirectIO := globalStorageClass.GetDMA() == storageclass.DMAReadWrite && s.readODirectSupported + partPath := fmt.Sprintf("part.%d", fi.Parts[0].Number) + fi.Data, err = s.readAllData(volumeDir, pathJoin(volumeDir, path, fi.DataDir, partPath), requireDirectIO) if err != nil { return FileInfo{}, err } @@ -1148,9 +1134,9 @@ func (s *xlStorage) ReadVersion(ctx context.Context, volume, path, versionID str return fi, nil } -func (s *xlStorage) readAllData(volumeDir, filePath string) (buf []byte, err error) { +func (s *xlStorage) readAllData(volumeDir, filePath string, requireDirectIO bool) (buf []byte, err error) { var f *os.File - if globalStorageClass.GetDMA() == storageclass.DMAReadWrite { + if requireDirectIO { f, err = disk.OpenFileDirectIO(filePath, os.O_RDONLY, 0666) } else { f, err = os.Open(filePath) @@ -1176,7 +1162,7 @@ func (s *xlStorage) readAllData(volumeDir, filePath string) (buf []byte, err err } else if isSysErrTooManyFiles(err) { return nil, errTooManyOpenFiles } else if isSysErrInvalidArg(err) { - return nil, errFileNotFound + return nil, errUnsupportedDisk } return nil, err } @@ -1185,7 +1171,11 @@ func (s *xlStorage) readAllData(volumeDir, filePath string) (buf []byte, err err rd := &odirectReader{f, nil, nil, true, true, s, nil} defer rd.Close() - return ioutil.ReadAll(rd) + buf, err = ioutil.ReadAll(rd) + if err != nil { + err = osErrToFileErr(err) + } + return buf, err } // ReadAll reads from r until an error or EOF and returns the data it read. @@ -1233,7 +1223,7 @@ func (s *xlStorage) ReadAll(ctx context.Context, volume string, path string) (bu } else if isSysErrTooManyFiles(err) { return nil, errTooManyOpenFiles } else if isSysErrInvalidArg(err) { - return nil, errFileNotFound + return nil, errUnsupportedDisk } return nil, err } @@ -1486,7 +1476,7 @@ func (s *xlStorage) ReadFileStream(ctx context.Context, volume, path string, off var file *os.File // O_DIRECT only supported if offset is zero - if offset == 0 && globalStorageClass.GetDMA() == storageclass.DMAReadWrite { + if offset == 0 && globalStorageClass.GetDMA() == storageclass.DMAReadWrite && s.readODirectSupported { file, err = disk.OpenFileDirectIO(filePath, os.O_RDONLY, 0666) } else { // Open the file for reading. @@ -1508,6 +1498,8 @@ func (s *xlStorage) ReadFileStream(ctx context.Context, volume, path string, off return nil, errFaultyDisk case isSysErrTooManyFiles(err): return nil, errTooManyOpenFiles + case isSysErrInvalidArg(err): + return nil, errUnsupportedDisk default: return nil, err } @@ -1525,7 +1517,7 @@ func (s *xlStorage) ReadFileStream(ctx context.Context, volume, path string, off } atomic.AddInt32(&s.activeIOCount, 1) - if offset == 0 && globalStorageClass.GetDMA() == storageclass.DMAReadWrite { + if offset == 0 && globalStorageClass.GetDMA() == storageclass.DMAReadWrite && s.readODirectSupported { if length <= smallFileThreshold { return &odirectReader{file, nil, nil, true, true, s, nil}, nil } @@ -2145,7 +2137,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath, dataDir, legacyDataPath := pathJoin(dstVolumeDir, dstPath, legacyDataDir) // legacy data dir means its old content, honor system umask. - if err = os.MkdirAll(legacyDataPath, 0777); err != nil { + if err = reliableMkdirAll(legacyDataPath, 0777); err != nil { return osErrToFileErr(err) } diff --git a/cmd/xl-storage_test.go b/cmd/xl-storage_test.go index 43a6645b7..ce5a72cb7 100644 --- a/cmd/xl-storage_test.go +++ b/cmd/xl-storage_test.go @@ -31,7 +31,6 @@ import ( "testing" "github.com/minio/minio/cmd/config/storageclass" - "github.com/minio/minio/pkg/disk" ) func TestCheckPathLength(t *testing.T) { @@ -128,10 +127,6 @@ func newXLStorageTestSetup() (*xlStorageDiskIDCheck, string, error) { if err != nil { return nil, "", err } - err = storage.MakeVol(context.Background(), minioMetaBucket) - if err != nil { - return nil, "", err - } // Create a sample format.json file err = storage.WriteAll(context.Background(), minioMetaBucket, formatConfigFile, []byte(`{"version":"1","format":"xl","id":"592a41c2-b7cc-4130-b883-c4b5cb15965b","xl":{"version":"3","this":"da017d62-70e3-45f1-8a1a-587707e69ad1","sets":[["e07285a6-8c73-4962-89c6-047fb939f803","33b8d431-482d-4376-b63c-626d229f0a29","cff6513a-4439-4dc1-bcaa-56c9e880c352","da017d62-70e3-45f1-8a1a-587707e69ad1","9c9f21d5-1f15-4737-bce6-835faa0d9626","0a59b346-1424-4fc2-9fa2-a2e80541d0c1","7924a3dc-b69a-4971-9a2e-014966d6aebb","4d2b8dd9-4e48-444b-bdca-c89194b26042"]],"distributionAlgo":"CRCMOD"}}`)) if err != nil { @@ -258,7 +253,7 @@ func TestXLStorageReadVersion(t *testing.T) { // create xlStorage test setup xlStorage, path, err := newXLStorageTestSetup() if err != nil { - t.Fatalf("Unable to create xlStorage test setup, %s", err) + t.Fatalf("Unable to cfgreate xlStorage test setup, %s", err) } defer os.RemoveAll(path) @@ -539,7 +534,7 @@ func TestXLStorageMakeVol(t *testing.T) { // Initialize xlStorage storage layer for permission denied error. _, err = newLocalXLStorage(permDeniedDir) - if err != nil && err != errFileAccessDenied { + if err != nil && err != errDiskAccessDenied { t.Fatalf("Unable to initialize xlStorage, %s", err) } @@ -638,7 +633,7 @@ func TestXLStorageDeleteVol(t *testing.T) { // Initialize xlStorage storage layer for permission denied error. _, err = newLocalXLStorage(permDeniedDir) - if err != nil && err != errFileAccessDenied { + if err != nil && err != errDiskAccessDenied { t.Fatalf("Unable to initialize xlStorage, %s", err) } @@ -890,7 +885,7 @@ func TestXLStorageListDir(t *testing.T) { // Initialize xlStorage storage layer for permission denied error. _, err = newLocalXLStorage(permDeniedDir) - if err != nil && err != errFileAccessDenied { + if err != nil && err != errDiskAccessDenied { t.Fatalf("Unable to initialize xlStorage, %s", err) } @@ -1014,7 +1009,7 @@ func TestXLStorageDeleteFile(t *testing.T) { // Initialize xlStorage storage layer for permission denied error. _, err = newLocalXLStorage(permDeniedDir) - if err != nil && err != errFileAccessDenied { + if err != nil && err != errDiskAccessDenied { t.Fatalf("Unable to initialize xlStorage, %s", err) } @@ -1221,7 +1216,7 @@ func TestXLStorageReadFile(t *testing.T) { // Initialize xlStorage storage layer for permission denied error. _, err = newLocalXLStorage(permDeniedDir) - if err != nil && err != errFileAccessDenied { + if err != nil && err != errDiskAccessDenied { t.Fatalf("Unable to initialize xlStorage, %s", err) } @@ -1391,7 +1386,7 @@ func TestXLStorageAppendFile(t *testing.T) { var xlStoragePermStorage StorageAPI // Initialize xlStorage storage layer for permission denied error. _, err = newLocalXLStorage(permDeniedDir) - if err != nil && err != errFileAccessDenied { + if err != nil && err != errDiskAccessDenied { t.Fatalf("Unable to initialize xlStorage, %s", err) } @@ -1824,54 +1819,3 @@ func TestXLStorageVerifyFile(t *testing.T) { t.Fatal("expected to fail bitrot check") } } - -// Checks for restrictions for min total disk space and inodes. -func TestCheckDiskTotalMin(t *testing.T) { - testCases := []struct { - diskInfo disk.Info - err error - }{ - // Test 1 - when fstype is nfs. - { - diskInfo: disk.Info{ - Total: diskMinTotalSpace * 3, - FSType: "NFS", - }, - err: nil, - }, - // Test 2 - when fstype is xfs and total inodes are less than 10k. - { - diskInfo: disk.Info{ - Total: diskMinTotalSpace * 3, - FSType: "XFS", - Files: 9999, - }, - err: nil, - }, - // Test 3 - when fstype is btrfs and total inodes is empty. - { - diskInfo: disk.Info{ - Total: diskMinTotalSpace * 3, - FSType: "BTRFS", - Files: 0, - }, - err: nil, - }, - // Test 4 - when fstype is xfs and total disk space is really small. - { - diskInfo: disk.Info{ - Total: diskMinTotalSpace - diskMinTotalSpace/1024, - FSType: "XFS", - Files: 9999, - }, - err: errMinDiskSize, - }, - } - - // Validate all cases. - for i, test := range testCases { - if err := checkDiskMinTotal(test.diskInfo); test.err != err { - t.Errorf("Test %d: Expected error %s, got %s", i+1, test.err, err) - } - } -}