Skip to content

Commit

Permalink
use Access() instead of Lstat() for frequent use (minio#11911)
Browse files Browse the repository at this point in the history 
using Lstat() is causing tiny memory allocations,
that are usually wasted and never used, instead
we can simply uses Access() call that does 0
memory allocations.
  • Loading branch information
@harshavardhana
harshavardhana authored Mar 29, 2021
1 parent 7c5b35d commit d93c6cb
Show file tree
Hide file tree
Showing 9 changed files with 101 additions and 67 deletions.
9 changes: 9 additions & 0 deletions cmd/os-instrumented.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const (
osMetricLstat
osMetricRemove
osMetricStat
osMetricAccess
// .... add more

osMetricLast
Expand Down Expand Up @@ -94,6 +95,14 @@ func OpenFile(name string, flag int, perm os.FileMode) (*os.File, error) {
return os.OpenFile(name, flag, perm)
}

// Access captures time taken to call syscall.Access()
// on windows, plan9 and solaris syscall.Access uses
// os.Lstat()
func Access(name string) error {
defer updateOSMetrics(osMetricAccess, name)()
return access(name)
}

// Open captures time taken to call os.Open
func Open(name string) (*os.File, error) {
defer updateOSMetrics(osMetricOpen, name)()
Expand Down
5 changes: 5 additions & 0 deletions cmd/os-readdir_other.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ import (
"syscall"
)

func access(name string) error {
_, err := os.Lstat(name)
return err
}

// Return all the entries at the directory dirPath.
func readDir(dirPath string) (entries []string, err error) {
return readDirN(dirPath, -1)
Expand Down
9 changes: 9 additions & 0 deletions cmd/os-readdir_unix.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,17 @@ import (
"sync"
"syscall"
"unsafe"

"golang.org/x/sys/unix"
)

func access(name string) error {
if err := unix.Access(name, unix.R_OK|unix.W_OK); err != nil {
return &os.PathError{Op: "lstat", Path: name, Err: err}
}
return nil
}

// The buffer must be at least a block long.
// refer https://github.com/golang/go/issues/24015
const blockSize = 8 << 10 // 8192
Expand Down
5 changes: 5 additions & 0 deletions cmd/os-readdir_windows.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ import (
"syscall"
)

func access(name string) error {
_, err := os.Lstat(name)
return err
}

// Return all the entries at the directory dirPath.
func readDir(dirPath string) (entries []string, err error) {
return readDirN(dirPath, -1)
Expand Down
7 changes: 4 additions & 3 deletions cmd/osmetric_string.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

55 changes: 26 additions & 29 deletions cmd/xl-storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -529,8 +529,7 @@ func (s *xlStorage) GetDiskID() (string, error) {
if err != nil {
// If the disk is still not initialized.
if osIsNotExist(err) {
_, err = Lstat(s.diskPath)
if err == nil {
if err = Access(s.diskPath); err == nil {
// Disk is present but missing `format.json`
return "", errUnformattedDisk
}
Expand Down Expand Up @@ -560,8 +559,7 @@ func (s *xlStorage) GetDiskID() (string, error) {
if err != nil {
// If the disk is still not initialized.
if osIsNotExist(err) {
_, err = Lstat(s.diskPath)
if err == nil {
if err = Access(s.diskPath); err == nil {
// Disk is present but missing `format.json`
return "", errUnformattedDisk
}
Expand Down Expand Up @@ -623,7 +621,7 @@ func (s *xlStorage) MakeVol(ctx context.Context, volume string) error {
return err
}

if _, err := Lstat(volumeDir); err != nil {
if err = Access(volumeDir); err != nil {
// Volume does not exist we proceed to create.
if osIsNotExist(err) {
// Make a volume entry, with mode 0777 mkdir honors system umask.
Expand Down Expand Up @@ -675,6 +673,7 @@ func (s *xlStorage) StatVol(ctx context.Context, volume string) (vol VolInfo, er
if err != nil {
return VolInfo{}, err
}

// Stat a volume entry.
var st os.FileInfo
st, err = Lstat(volumeDir)
Expand Down Expand Up @@ -736,8 +735,7 @@ func (s *xlStorage) isLeaf(volume string, leafPath string) bool {
return false
}

_, err = Lstat(pathJoin(volumeDir, leafPath, xlStorageFormatFile))
if err == nil {
if err = Access(pathJoin(volumeDir, leafPath, xlStorageFormatFile)); err == nil {
return true
}
if osIsNotExist(err) {
Expand Down Expand Up @@ -767,10 +765,10 @@ func (s *xlStorage) ListDir(ctx context.Context, volume, dirPath string, count i
}
if err != nil {
if err == errFileNotFound {
if _, verr := Lstat(volumeDir); verr != nil {
if osIsNotExist(verr) {
if ierr := Access(volumeDir); ierr != nil {
if osIsNotExist(ierr) {
return nil, errVolumeNotFound
} else if isSysErrIO(verr) {
} else if isSysErrIO(ierr) {
return nil, errFaultyDisk
}
}
Expand Down Expand Up @@ -1042,8 +1040,7 @@ func (s *xlStorage) readAllData(volumeDir string, filePath string, requireDirect
if osIsNotExist(err) {
// Check if the object doesn't exist because its bucket
// is missing in order to return the correct error.
_, err = Lstat(volumeDir)
if err != nil && osIsNotExist(err) {
if err = Access(volumeDir); err != nil && osIsNotExist(err) {
return nil, errVolumeNotFound
}
return nil, errFileNotFound
Expand Down Expand Up @@ -1129,12 +1126,13 @@ func (s *xlStorage) ReadFile(ctx context.Context, volume string, path string, of
var n int

// Stat a volume entry.
_, err = Lstat(volumeDir)
if err != nil {
if err = Access(volumeDir); err != nil {
if osIsNotExist(err) {
return 0, errVolumeNotFound
} else if isSysErrIO(err) {
return 0, errFaultyDisk
} else if osIsPermission(err) {
return 0, errFileAccessDenied
}
return 0, err
}
Expand Down Expand Up @@ -1329,8 +1327,7 @@ func (s *xlStorage) ReadFileStream(ctx context.Context, volume, path string, off
if err != nil {
switch {
case osIsNotExist(err):
_, err = Lstat(volumeDir)
if err != nil && osIsNotExist(err) {
if err = Access(volumeDir); err != nil && osIsNotExist(err) {
return nil, errVolumeNotFound
}
return nil, errFileNotFound
Expand Down Expand Up @@ -1530,9 +1527,13 @@ func (s *xlStorage) AppendFile(ctx context.Context, volume string, path string,
}

// Stat a volume entry.
if _, err = Lstat(volumeDir); err != nil {
if err = Access(volumeDir); err != nil {
if osIsNotExist(err) {
return errVolumeNotFound
} else if osIsPermission(err) {
return errVolumeAccessDenied
} else if isSysErrIO(err) {
return errFaultyDisk
}
return err
}
Expand Down Expand Up @@ -1571,7 +1572,7 @@ func (s *xlStorage) CheckParts(ctx context.Context, volume string, path string,
}

// Stat a volume entry.
if _, err = Lstat(volumeDir); err != nil {
if err = Access(volumeDir); err != nil {
if osIsNotExist(err) {
return errVolumeNotFound
}
Expand Down Expand Up @@ -1721,8 +1722,7 @@ func (s *xlStorage) Delete(ctx context.Context, volume string, path string, recu
}

// Stat a volume entry.
_, err = Lstat(volumeDir)
if err != nil {
if err = Access(volumeDir); err != nil {
if osIsNotExist(err) {
return errVolumeNotFound
} else if osIsPermission(err) {
Expand Down Expand Up @@ -1757,8 +1757,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath, dataDir,
}

// Stat a volume entry.
_, err = Lstat(srcVolumeDir)
if err != nil {
if err = Access(srcVolumeDir); err != nil {
if osIsNotExist(err) {
return errVolumeNotFound
} else if isSysErrIO(err) {
Expand All @@ -1767,7 +1766,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath, dataDir,
return err
}

if _, err = Lstat(dstVolumeDir); err != nil {
if err = Access(dstVolumeDir); err != nil {
if osIsNotExist(err) {
return errVolumeNotFound
} else if isSysErrIO(err) {
Expand Down Expand Up @@ -1998,17 +1997,16 @@ func (s *xlStorage) RenameFile(ctx context.Context, srcVolume, srcPath, dstVolum
return err
}
// Stat a volume entry.
_, err = Lstat(srcVolumeDir)
if err != nil {
if err = Access(srcVolumeDir); err != nil {
if osIsNotExist(err) {
return errVolumeNotFound
} else if isSysErrIO(err) {
return errFaultyDisk
}
return err
}
_, err = Lstat(dstVolumeDir)
if err != nil {

if err = Access(dstVolumeDir); err != nil {
if osIsNotExist(err) {
return errVolumeNotFound
} else if isSysErrIO(err) {
Expand Down Expand Up @@ -2142,8 +2140,7 @@ func (s *xlStorage) VerifyFile(ctx context.Context, volume, path string, fi File
}

// Stat a volume entry.
_, err = Lstat(volumeDir)
if err != nil {
if err = Access(volumeDir); err != nil {
if osIsNotExist(err) {
return errVolumeNotFound
} else if isSysErrIO(err) {
Expand Down
20 changes: 12 additions & 8 deletions cmd/xl-storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,7 @@ func TestXLStorageListDir(t *testing.T) {
var dirList []string
dirList, err = xlStorage.ListDir(context.Background(), testCase.srcVol, testCase.srcPath, -1)
if err != testCase.expectedErr {
t.Fatalf("TestXLStorage case %d: Expected: \"%s\", got: \"%s\"", i+1, testCase.expectedErr, err)
t.Errorf("TestXLStorage case %d: Expected: \"%s\", got: \"%s\"", i+1, testCase.expectedErr, err)
}
if err == nil {
for _, expected := range testCase.expectedListDir {
Expand Down Expand Up @@ -900,8 +900,8 @@ func TestXLStorageListDir(t *testing.T) {
t.Fatalf("Unable to initialize xlStorage, %s", err)
}

if err = xlStorageNew.Delete(context.Background(), "mybucket", "myobject", false); err != errFileAccessDenied {
t.Errorf("expected: %s, got: %s", errFileAccessDenied, err)
if err = xlStorageNew.Delete(context.Background(), "mybucket", "myobject", false); err != errVolumeAccessDenied {
t.Errorf("expected: %s, got: %s", errVolumeAccessDenied, err)
}
}

Expand All @@ -915,6 +915,10 @@ func TestXLStorageListDir(t *testing.T) {

// TestXLStorageDeleteFile - Series of test cases construct valid and invalid input data and validates the result and the error response.
func TestXLStorageDeleteFile(t *testing.T) {
if runtime.GOOS == globalWindowsOSName {
t.Skip()
}

// create xlStorage test setup
xlStorage, path, err := newXLStorageTestSetup()
if err != nil {
Expand Down Expand Up @@ -994,7 +998,7 @@ func TestXLStorageDeleteFile(t *testing.T) {
{
srcVol: "no-permissions",
srcPath: "dir/file",
expectedErr: nil,
expectedErr: errVolumeAccessDenied,
},
}

Expand Down Expand Up @@ -1024,8 +1028,8 @@ func TestXLStorageDeleteFile(t *testing.T) {
t.Fatalf("Unable to initialize xlStorage, %s", err)
}

if err = xlStorageNew.Delete(context.Background(), "mybucket", "myobject", false); err != errFileAccessDenied {
t.Errorf("expected: %s, got: %s", errFileAccessDenied, err)
if err = xlStorageNew.Delete(context.Background(), "mybucket", "myobject", false); err != errVolumeAccessDenied {
t.Errorf("expected: %s, got: %s", errVolumeAccessDenied, err)
}
}

Expand Down Expand Up @@ -1401,8 +1405,8 @@ func TestXLStorageAppendFile(t *testing.T) {
t.Fatalf("Unable to initialize xlStorage, %s", err)
}

if err = xlStoragePermStorage.AppendFile(context.Background(), "mybucket", "myobject", []byte("hello, world")); err != errFileAccessDenied {
t.Fatalf("expected: Permission error, got: %s", err)
if err = xlStoragePermStorage.AppendFile(context.Background(), "mybucket", "myobject", []byte("hello, world")); err != errVolumeAccessDenied {
t.Fatalf("expected: errVolumeAccessDenied error, got: %s", err)
}
}

Expand Down
13 changes: 6 additions & 7 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ require (
github.com/klauspost/pgzip v1.2.5
github.com/klauspost/readahead v1.3.1
github.com/klauspost/reedsolomon v1.9.11
github.com/lib/pq v1.8.0
github.com/lib/pq v1.9.0
github.com/mattn/go-colorable v0.1.8
github.com/mattn/go-ieproxy v0.0.1 // indirect
github.com/mattn/go-isatty v0.0.12
Expand All @@ -55,10 +55,10 @@ require (
github.com/mitchellh/go-homedir v1.1.0
github.com/montanaflynn/stats v0.5.0
github.com/nats-io/nats-server/v2 v2.1.9
github.com/nats-io/nats-streaming-server v0.19.0 // indirect
github.com/nats-io/nats-streaming-server v0.21.1 // indirect
github.com/nats-io/nats.go v1.10.0
github.com/nats-io/nkeys v0.2.0 // indirect
github.com/nats-io/stan.go v0.7.0
github.com/nats-io/stan.go v0.8.3
github.com/ncw/directio v1.0.5
github.com/nsqio/go-nsq v1.0.8
github.com/olivere/elastic/v7 v7.0.22
Expand All @@ -67,7 +67,7 @@ require (
github.com/pkg/errors v0.9.1
github.com/prometheus/client_golang v1.8.0
github.com/prometheus/client_model v0.2.0
github.com/prometheus/procfs v0.2.0
github.com/prometheus/procfs v0.6.0
github.com/rjeczalik/notify v0.9.2
github.com/rs/cors v1.7.0
github.com/secure-io/sio-go v0.3.1
Expand All @@ -77,16 +77,15 @@ require (
github.com/tidwall/gjson v1.6.8
github.com/tidwall/sjson v1.0.4
github.com/tinylib/msgp v1.1.3
github.com/ttacon/chalk v0.0.0-20160626202418-22c06c80ed31 // indirect
github.com/valyala/tcplisten v0.0.0-20161114210144-ceec8f93295a
github.com/willf/bitset v1.1.11 // indirect
github.com/willf/bloom v2.0.3+incompatible
github.com/xdg/scram v0.0.0-20180814205039-7eeb5667e42c
go.etcd.io/etcd v0.0.0-20201125193152-8a03d2e9614b
go.uber.org/zap v1.13.0
golang.org/x/crypto v0.0.0-20201124201722-c8d3bf9c5392
golang.org/x/crypto v0.0.0-20210220033148-5ea612d1eb83
golang.org/x/net v0.0.0-20201216054612-986b41b23924
golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4
golang.org/x/sys v0.0.0-20210225134936-a50acf3fe073
golang.org/x/tools v0.1.0 // indirect
google.golang.org/api v0.5.0
gopkg.in/yaml.v2 v2.3.0
Expand Down
Loading

0 comments on commit d93c6cb

Please sign in to comment.