From b92a220db1416490e1cd9f19e2153b9a4054c391 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 18 Mar 2021 20:16:50 -0700 Subject: [PATCH] fix: handle weird drives sporadic read O_DIRECT behavior (#11832) on freshReads if drive returns errInvalidArgument, we should simply turn-off DirectIO and read normally, there are situations in k8s like environments where the drives behave sporadically in a single deployment and may not have been implemented properly to handle O_DIRECT for reads. --- cmd/erasure-metadata-utils.go | 4 ++- cmd/xl-storage.go | 51 +++++++++++++++-------------------- pkg/ioutil/ioutil_test.go | 4 +-- 3 files changed, 26 insertions(+), 33 deletions(-) diff --git a/cmd/erasure-metadata-utils.go b/cmd/erasure-metadata-utils.go index 84b13f595..63639e738 100644 --- a/cmd/erasure-metadata-utils.go +++ b/cmd/erasure-metadata-utils.go @@ -19,6 +19,7 @@ package cmd import ( "context" "errors" + "fmt" "hash/crc32" "github.com/minio/minio/cmd/logger" @@ -134,7 +135,8 @@ func readAllFileInfo(ctx context.Context, disks []StorageAPI, bucket, object, ve errFileVersionNotFound, errDiskNotFound, }...) { - logger.LogOnceIf(ctx, err, disks[index].String()) + logger.LogOnceIf(ctx, fmt.Errorf("Drive %s returned an error (%w)", disks[index], err), + disks[index].String()) } } return err diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index df3bb2196..84946e0f0 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -105,8 +105,6 @@ type xlStorage struct { rootDisk bool - readODirectSupported bool - diskID string // Indexes, will be -1 until assigned a set. @@ -271,13 +269,12 @@ func newXLStorage(ep Endpoint) (*xlStorage, error) { return &b }, }, - globalSync: env.Get(config.EnvFSOSync, config.EnableOff) == config.EnableOn, - ctx: GlobalContext, - rootDisk: rootDisk, - readODirectSupported: true, - poolIndex: -1, - setIndex: -1, - diskIndex: -1, + globalSync: env.Get(config.EnvFSOSync, config.EnableOff) == config.EnableOn, + ctx: GlobalContext, + rootDisk: rootDisk, + poolIndex: -1, + setIndex: -1, + diskIndex: -1, } // Create all necessary bucket folders if possible. @@ -301,21 +298,6 @@ func newXLStorage(ep Endpoint) (*xlStorage, error) { w.Close() defer os.Remove(filePath) - 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 !errors.Is(err, errUnsupportedDisk) { - return p, err - } - // error is unsupported disk, turn-off directIO for reads - logger.LogOnceIf(GlobalContext, fmt.Errorf("Drive %s does not support O_DIRECT for reads, proceeding to use the drive without O_DIRECT", ep), ep.String()) - p.readODirectSupported = false - } - // Success. return p, nil } @@ -1045,7 +1027,7 @@ func (s *xlStorage) ReadVersion(ctx context.Context, volume, path, versionID str // - object has maximum of 1 parts if fi.TransitionStatus == "" && fi.DataDir != "" && fi.Size <= smallFileThreshold && len(fi.Parts) == 1 { // Enable O_DIRECT optionally only if drive supports it. - requireDirectIO := globalStorageClass.GetDMA() == storageclass.DMAReadWrite && s.readODirectSupported + requireDirectIO := globalStorageClass.GetDMA() == storageclass.DMAReadWrite 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 { @@ -1125,7 +1107,7 @@ func (s *xlStorage) ReadAll(ctx context.Context, volume string, path string) (bu return nil, err } - requireDirectIO := globalStorageClass.GetDMA() == storageclass.DMAReadWrite && s.readODirectSupported + requireDirectIO := globalStorageClass.GetDMA() == storageclass.DMAReadWrite return s.readAllData(volumeDir, filePath, requireDirectIO) } @@ -1297,8 +1279,17 @@ func (o *odirectReader) Read(buf []byte) (n int, err error) { o.buf = *o.bufp n, err = o.f.Read(o.buf) if err != nil && err != io.EOF { - o.err = err - return n, err + if isSysErrInvalidArg(err) { + if err = disk.DisableDirectIO(o.f); err != nil { + o.err = err + return n, err + } + n, err = o.f.Read(o.buf) + } + if err != nil && err != io.EOF { + o.err = err + return n, err + } } if n == 0 { // err is io.EOF @@ -1347,7 +1338,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 && s.readODirectSupported { + if offset == 0 && globalStorageClass.GetDMA() == storageclass.DMAReadWrite { file, err = disk.OpenFileDirectIO(filePath, readMode, 0666) } else { // Open the file for reading. @@ -1389,7 +1380,7 @@ func (s *xlStorage) ReadFileStream(ctx context.Context, volume, path string, off return nil, errIsNotRegular } - if offset == 0 && globalStorageClass.GetDMA() == storageclass.DMAReadWrite && s.readODirectSupported { + if offset == 0 && globalStorageClass.GetDMA() == storageclass.DMAReadWrite { or := &odirectReader{file, nil, nil, true, false, s, nil} if length <= smallFileThreshold { or = &odirectReader{file, nil, nil, true, true, s, nil} diff --git a/pkg/ioutil/ioutil_test.go b/pkg/ioutil/ioutil_test.go index aad6066ef..3d083339c 100644 --- a/pkg/ioutil/ioutil_test.go +++ b/pkg/ioutil/ioutil_test.go @@ -50,14 +50,14 @@ func TestDeadlineWriter(t *testing.T) { if err != context.Canceled { t.Error("DeadlineWriter shouldn't be successful - should return context.Canceled") } - w = NewDeadlineWriter(&sleepWriter{timeout: 500 * time.Millisecond}, 600*time.Millisecond) + w = NewDeadlineWriter(&sleepWriter{timeout: 100 * time.Millisecond}, 600*time.Millisecond) n, err := w.Write([]byte("abcd")) w.Close() if err != nil { t.Errorf("DeadlineWriter should succeed but failed with %s", err) } if n != 4 { - t.Errorf("DeadlineWriter should succeed but should have only written 0 bytes, but returned %d instead", n) + t.Errorf("DeadlineWriter should succeed but should have only written 4 bytes, but returned %d instead", n) } }