From 65f34cd823337e85bd6f9e7ee31cca8758de7cd1 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Sat, 9 Dec 2023 10:17:51 -0800 Subject: [PATCH] fix: remove ODirectReader entirely since we do not need it anymore (#18619) --- cmd/xl-storage.go | 55 +++++------- internal/ioutil/ioutil.go | 30 +++++++ internal/ioutil/odirect_reader.go | 136 ------------------------------ internal/ioutil/read_file.go | 44 ++-------- 4 files changed, 60 insertions(+), 205 deletions(-) delete mode 100644 internal/ioutil/odirect_reader.go diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index ad3bd315f..336545925 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -1005,8 +1005,10 @@ func (s *xlStorage) deleteVersions(ctx context.Context, volume, path string, fis return err } + discard := true + var legacyJSON bool - buf, _, err := s.readAllData(ctx, volume, volumeDir, pathJoin(volumeDir, path, xlStorageFormatFile), false) + buf, _, err := s.readAllData(ctx, volume, volumeDir, pathJoin(volumeDir, path, xlStorageFormatFile), discard) if err != nil { if !errors.Is(err, errFileNotFound) { return err @@ -1016,7 +1018,7 @@ func (s *xlStorage) deleteVersions(ctx context.Context, volume, path string, fis legacy := s.formatLegacy s.RUnlock() if legacy { - buf, _, err = s.readAllData(ctx, volume, volumeDir, pathJoin(volumeDir, path, xlStorageFormatFileV1), false) + buf, _, err = s.readAllData(ctx, volume, volumeDir, pathJoin(volumeDir, path, xlStorageFormatFileV1), discard) if err != nil { return err } @@ -1163,7 +1165,7 @@ func (s *xlStorage) DeleteVersion(ctx context.Context, volume, path string, fi F } var legacyJSON bool - buf, _, err := s.readAllData(ctx, volume, volumeDir, pathJoin(filePath, xlStorageFormatFile), false) + buf, _, err := s.readAllData(ctx, volume, volumeDir, pathJoin(filePath, xlStorageFormatFile), true) if err != nil { if !errors.Is(err, errFileNotFound) { return err @@ -1401,7 +1403,7 @@ func (s *xlStorage) readRaw(ctx context.Context, volume, volumeDir, filePath str xlPath := pathJoin(filePath, xlStorageFormatFile) if readData { - buf, dmTime, err = s.readAllData(ctx, volume, volumeDir, xlPath, true) + buf, dmTime, err = s.readAllData(ctx, volume, volumeDir, xlPath, false) } else { buf, dmTime, err = s.readMetadataWithDMTime(ctx, xlPath) if err != nil { @@ -1421,7 +1423,7 @@ func (s *xlStorage) readRaw(ctx context.Context, volume, volumeDir, filePath str s.RUnlock() if err != nil && errors.Is(err, errFileNotFound) && legacy { - buf, dmTime, err = s.readAllData(ctx, volume, volumeDir, pathJoin(filePath, xlStorageFormatFileV1), true) + buf, dmTime, err = s.readAllData(ctx, volume, volumeDir, pathJoin(filePath, xlStorageFormatFileV1), false) if err != nil { return nil, time.Time{}, err } @@ -1538,7 +1540,7 @@ func (s *xlStorage) ReadVersion(ctx context.Context, volume, path, versionID str len(fi.Parts) == 1 { partPath := fmt.Sprintf("part.%d", fi.Parts[0].Number) dataPath := pathJoin(volumeDir, path, fi.DataDir, partPath) - fi.Data, _, err = s.readAllData(ctx, volume, volumeDir, dataPath, true) + fi.Data, _, err = s.readAllData(ctx, volume, volumeDir, dataPath, false) if err != nil { return FileInfo{}, err } @@ -1564,7 +1566,7 @@ func (s *xlStorage) ReadVersion(ctx context.Context, volume, path, versionID str return fi, nil } -func (s *xlStorage) readAllData(ctx context.Context, volume, volumeDir string, filePath string, sync bool) (buf []byte, dmTime time.Time, err error) { +func (s *xlStorage) readAllData(ctx context.Context, volume, volumeDir string, filePath string, discard bool) (buf []byte, dmTime time.Time, err error) { if filePath == "" { return nil, dmTime, errFileNotFound } @@ -1573,13 +1575,7 @@ func (s *xlStorage) readAllData(ctx context.Context, volume, volumeDir string, f return nil, time.Time{}, ctx.Err() } - odirectEnabled := globalAPIConfig.odirectEnabled() && s.oDirect - var f *os.File - if odirectEnabled && sync { - f, err = OpenFileDirectIO(filePath, readMode, 0o666) - } else { - f, err = OpenFile(filePath, readMode, 0o666) - } + f, err := OpenFile(filePath, readMode, 0o666) if err != nil { switch { case osIsNotExist(err): @@ -1614,23 +1610,20 @@ func (s *xlStorage) readAllData(ctx context.Context, volume, volumeDir string, f } return nil, dmTime, err } - r := io.Reader(f) - var dr *xioutil.ODirectReader - if odirectEnabled { - dr = &xioutil.ODirectReader{ - File: f, - SmallFile: true, - } - defer dr.Close() - r = dr - } else { - defer f.Close() + + if discard { + // This discard is mostly true for DELETEs + // so we need to make sure we do not keep + // page-cache references after. + defer disk.Fdatasync(f) } + defer f.Close() + // Get size for precise allocation. stat, err := f.Stat() if err != nil { - buf, err = io.ReadAll(r) + buf, err = io.ReadAll(f) return buf, dmTime, osErrToFileErr(err) } if stat.IsDir() { @@ -1645,11 +1638,9 @@ func (s *xlStorage) readAllData(ctx context.Context, volume, volumeDir string, f } else { buf = make([]byte, sz) } - if dr != nil { - dr.SmallFile = sz <= xioutil.BlockSizeSmall*2 - } + // Read file... - _, err = io.ReadFull(r, buf) + _, err = io.ReadFull(f, buf) return buf, stat.ModTime().UTC(), osErrToFileErr(err) } @@ -2780,7 +2771,7 @@ func (s *xlStorage) ReadMultiple(ctx context.Context, req ReadMultipleReq, resp if req.MetadataOnly { data, mt, err = s.readMetadataWithDMTime(ctx, fullPath) } else { - data, mt, err = s.readAllData(ctx, req.Bucket, volumeDir, fullPath, false) + data, mt, err = s.readAllData(ctx, req.Bucket, volumeDir, fullPath, true) } return err }); err != nil { @@ -2875,7 +2866,7 @@ func (s *xlStorage) CleanAbandonedData(ctx context.Context, volume string, path } baseDir := pathJoin(volumeDir, path+slashSeparator) metaPath := pathutil.Join(baseDir, xlStorageFormatFile) - buf, _, err := s.readAllData(ctx, volume, volumeDir, metaPath, false) + buf, _, err := s.readAllData(ctx, volume, volumeDir, metaPath, true) if err != nil { return err } diff --git a/internal/ioutil/ioutil.go b/internal/ioutil/ioutil.go index 196f19572..b34e5b7b5 100644 --- a/internal/ioutil/ioutil.go +++ b/internal/ioutil/ioutil.go @@ -28,9 +28,39 @@ import ( "sync" "time" + "github.com/dustin/go-humanize" "github.com/minio/minio/internal/disk" ) +// Block sizes constant. +const ( + BlockSizeSmall = 32 * humanize.KiByte // Default r/w block size for smaller objects. + BlockSizeLarge = 2 * humanize.MiByte // Default r/w block size for larger objects. + BlockSizeReallyLarge = 4 * humanize.MiByte // Default write block size for objects per shard >= 64MiB +) + +// aligned sync.Pool's +var ( + ODirectPoolXLarge = sync.Pool{ + New: func() interface{} { + b := disk.AlignedBlock(BlockSizeReallyLarge) + return &b + }, + } + ODirectPoolLarge = sync.Pool{ + New: func() interface{} { + b := disk.AlignedBlock(BlockSizeLarge) + return &b + }, + } + ODirectPoolSmall = sync.Pool{ + New: func() interface{} { + b := disk.AlignedBlock(BlockSizeSmall) + return &b + }, + } +) + // WriteOnCloser implements io.WriteCloser and always // executes at least one write operation if it is closed. // diff --git a/internal/ioutil/odirect_reader.go b/internal/ioutil/odirect_reader.go deleted file mode 100644 index 4e0f6f10c..000000000 --- a/internal/ioutil/odirect_reader.go +++ /dev/null @@ -1,136 +0,0 @@ -// Copyright (c) 2015-2021 MinIO, Inc. -// -// This file is part of MinIO Object Storage stack -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU Affero General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// This program is distributed in the hope that it will be useful -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU Affero General Public License for more details. -// -// You should have received a copy of the GNU Affero General Public License -// along with this program. If not, see . - -package ioutil - -import ( - "errors" - "io" - "os" - "sync" - "syscall" - - "github.com/dustin/go-humanize" - "github.com/minio/minio/internal/disk" -) - -// ODirectReader - to support O_DIRECT reads for erasure backends. -type ODirectReader struct { - File *os.File - SmallFile bool - err error - buf []byte - bufp *[]byte - seenRead bool -} - -// Block sizes constant. -const ( - BlockSizeSmall = 32 * humanize.KiByte // Default r/w block size for smaller objects. - BlockSizeLarge = 2 * humanize.MiByte // Default r/w block size for larger objects. - BlockSizeReallyLarge = 4 * humanize.MiByte // Default write block size for objects per shard >= 64MiB -) - -// O_DIRECT aligned sync.Pool's -var ( - ODirectPoolXLarge = sync.Pool{ - New: func() interface{} { - b := disk.AlignedBlock(BlockSizeReallyLarge) - return &b - }, - } - ODirectPoolLarge = sync.Pool{ - New: func() interface{} { - b := disk.AlignedBlock(BlockSizeLarge) - return &b - }, - } - ODirectPoolSmall = sync.Pool{ - New: func() interface{} { - b := disk.AlignedBlock(BlockSizeSmall) - return &b - }, - } -) - -// Invalid argument, unsupported flags such as O_DIRECT -func isSysErrInvalidArg(err error) bool { - return errors.Is(err, syscall.EINVAL) -} - -// Read - Implements Reader interface. -func (o *ODirectReader) Read(buf []byte) (n int, err error) { - if o.err != nil && (len(o.buf) == 0 || !o.seenRead) { - return 0, o.err - } - if o.buf == nil { - if o.SmallFile { - o.bufp = ODirectPoolSmall.Get().(*[]byte) - } else { - o.bufp = ODirectPoolLarge.Get().(*[]byte) - } - } - if !o.seenRead { - o.buf = *o.bufp - n, err = o.File.Read(o.buf) - if err != nil && err != io.EOF { - if isSysErrInvalidArg(err) { - if err = disk.DisableDirectIO(o.File); err != nil { - o.err = err - return n, err - } - n, err = o.File.Read(o.buf) - } - if err != nil && err != io.EOF { - o.err = err - return n, err - } - } - if n == 0 { - // err is likely io.EOF - o.err = err - return n, err - } - o.err = err - o.buf = o.buf[:n] - o.seenRead = true - } - if len(buf) >= len(o.buf) { - n = copy(buf, o.buf) - o.seenRead = false - return n, o.err - } - n = copy(buf, o.buf) - o.buf = o.buf[n:] - // There is more left in buffer, do not return any EOF yet. - return n, nil -} - -// Close - Release the buffer and close the file. -func (o *ODirectReader) Close() error { - if o.bufp != nil { - if o.SmallFile { - ODirectPoolSmall.Put(o.bufp) - } else { - ODirectPoolLarge.Put(o.bufp) - } - o.bufp = nil - o.buf = nil - } - o.err = errors.New("internal error: ODirectReader Read after Close") - return o.File.Close() -} diff --git a/internal/ioutil/read_file.go b/internal/ioutil/read_file.go index 6559040df..29d023b26 100644 --- a/internal/ioutil/read_file.go +++ b/internal/ioutil/read_file.go @@ -62,48 +62,18 @@ func ReadFileWithFileInfo(name string) ([]byte, fs.FileInfo, error) { // // passes NOATIME flag for reads on Unix systems to avoid atime updates. func ReadFile(name string) ([]byte, error) { - if !disk.ODirectPlatform { - // Don't wrap with un-needed buffer. - // Don't use os.ReadFile, since it doesn't pass NO_ATIME when present. - f, err := OsOpenFile(name, readMode, 0o666) - if err != nil { - return nil, err - } - defer f.Close() - st, err := f.Stat() - if err != nil { - return io.ReadAll(f) - } - dst := make([]byte, st.Size()) - _, err = io.ReadFull(f, dst) - return dst, err - } - - f, err := OpenFileDirectIO(name, readMode, 0o666) + // Don't wrap with un-needed buffer. + // Don't use os.ReadFile, since it doesn't pass NO_ATIME when present. + f, err := OsOpenFile(name, readMode, 0o666) if err != nil { - // fallback if there is an error to read - // 'name' with O_DIRECT - f, err = OsOpenFile(name, readMode, 0o666) - if err != nil { - return nil, err - } + return nil, err } - - r := &ODirectReader{ - File: f, - SmallFile: true, - } - defer r.Close() - + defer f.Close() st, err := f.Stat() if err != nil { - return io.ReadAll(r) + return io.ReadAll(f) } - - // Select bigger blocks when reading would do more than 2 reads. - r.SmallFile = st.Size() <= BlockSizeSmall*2 - dst := make([]byte, st.Size()) - _, err = io.ReadFull(r, dst) + _, err = io.ReadFull(f, dst) return dst, err }