From 294ea814bf71b9e79c3ab05085f01673c9b3c308 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Sun, 6 Mar 2016 21:32:49 -0800 Subject: [PATCH 1/4] pkg/fs: for locks, prefer defer and read-only ops This commit prefers the use of 'defer' for fs.Unlock (and fs.RUnlock) because it is more idiomatic Go and reduces repetition in the code, lending to a cleaner code base. It also switches a few uses of the lock to read-only locks, which should improve performance of those functions dramatically in certain contexts. --- pkg/fs/fs-bucket-acl.go | 12 ++++++------ pkg/fs/fs-bucket.go | 12 ++++++------ pkg/fs/fs-multipart.go | 11 +++++------ 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/pkg/fs/fs-bucket-acl.go b/pkg/fs/fs-bucket-acl.go index 46a61020e..fe2c9d26b 100644 --- a/pkg/fs/fs-bucket-acl.go +++ b/pkg/fs/fs-bucket-acl.go @@ -18,8 +18,8 @@ package fs // IsPrivateBucket - is private bucket func (fs Filesystem) IsPrivateBucket(bucket string) bool { - fs.rwLock.Lock() - defer fs.rwLock.Unlock() + fs.rwLock.RLock() + defer fs.rwLock.RUnlock() bucketMetadata, ok := fs.buckets.Metadata[bucket] if !ok { return true @@ -29,8 +29,8 @@ func (fs Filesystem) IsPrivateBucket(bucket string) bool { // IsPublicBucket - is public bucket func (fs Filesystem) IsPublicBucket(bucket string) bool { - fs.rwLock.Lock() - defer fs.rwLock.Unlock() + fs.rwLock.RLock() + defer fs.rwLock.RUnlock() bucketMetadata, ok := fs.buckets.Metadata[bucket] if !ok { return true @@ -40,8 +40,8 @@ func (fs Filesystem) IsPublicBucket(bucket string) bool { // IsReadOnlyBucket - is read only bucket func (fs Filesystem) IsReadOnlyBucket(bucket string) bool { - fs.rwLock.Lock() - defer fs.rwLock.Unlock() + fs.rwLock.RLock() + defer fs.rwLock.RUnlock() bucketMetadata, ok := fs.buckets.Metadata[bucket] if !ok { return true diff --git a/pkg/fs/fs-bucket.go b/pkg/fs/fs-bucket.go index f366e286d..686bc6432 100644 --- a/pkg/fs/fs-bucket.go +++ b/pkg/fs/fs-bucket.go @@ -58,12 +58,12 @@ func (fs Filesystem) DeleteBucket(bucket string) *probe.Error { // Critical region hold write lock. fs.rwLock.Lock() + defer fs.rwLock.Unlock() + delete(fs.buckets.Metadata, bucket) if err := saveBucketsMetadata(*fs.buckets); err != nil { - fs.rwLock.Unlock() return err.Trace(bucket) } - fs.rwLock.Unlock() return nil } @@ -171,12 +171,12 @@ func (fs Filesystem) MakeBucket(bucket, acl string) *probe.Error { // Critical region hold a write lock. fs.rwLock.Lock() + defer fs.rwLock.Unlock() + fs.buckets.Metadata[bucket] = bucketMetadata if err := saveBucketsMetadata(*fs.buckets); err != nil { - fs.rwLock.Unlock() return err.Trace(bucket) } - fs.rwLock.Unlock() return nil } @@ -266,11 +266,11 @@ func (fs Filesystem) SetBucketMetadata(bucket string, metadata map[string]string // Critical region handle write lock. fs.rwLock.Lock() + defer fs.rwLock.Unlock() + fs.buckets.Metadata[bucket] = bucketMetadata if err := saveBucketsMetadata(*fs.buckets); err != nil { - fs.rwLock.Unlock() return err.Trace(bucket) } - fs.rwLock.Unlock() return nil } diff --git a/pkg/fs/fs-multipart.go b/pkg/fs/fs-multipart.go index 4ac9856ec..ddb9da6e2 100644 --- a/pkg/fs/fs-multipart.go +++ b/pkg/fs/fs-multipart.go @@ -276,6 +276,7 @@ func (fs Filesystem) NewMultipartUpload(bucket, object string) (string, *probe.E // Critical region requiring write lock. fs.rwLock.Lock() + defer fs.rwLock.Unlock() // Initialize multipart session. mpartSession := &MultipartSession{} mpartSession.TotalParts = 0 @@ -288,10 +289,8 @@ func (fs Filesystem) NewMultipartUpload(bucket, object string) (string, *probe.E fs.multiparts.ActiveSession[uploadID] = mpartSession if err := saveMultipartsSession(*fs.multiparts); err != nil { - fs.rwLock.Unlock() return "", err.Trace(objectPath) } - fs.rwLock.Unlock() return uploadID, nil } @@ -445,12 +444,12 @@ func (fs Filesystem) CreateObjectPart(bucket, object, uploadID, expectedMD5Sum s // Critical region requiring write lock. fs.rwLock.Lock() + defer fs.rwLock.Unlock() + fs.multiparts.ActiveSession[uploadID] = deserializedMultipartSession if err := saveMultipartsSession(*fs.multiparts); err != nil { - fs.rwLock.Unlock() return "", err.Trace(partPathPrefix) } - fs.rwLock.Unlock() // Return etag. return partMetadata.ETag, nil @@ -693,11 +692,11 @@ func (fs Filesystem) AbortMultipartUpload(bucket, object, uploadID string) *prob // Critical region requiring write lock. fs.rwLock.Lock() + defer fs.rwLock.Unlock() + delete(fs.multiparts.ActiveSession, uploadID) if err := saveMultipartsSession(*fs.multiparts); err != nil { - fs.rwLock.Unlock() return err.Trace(partPathPrefix) } - fs.rwLock.Unlock() return nil } From 0a0451a0fb8f5666222ee3473e8512da6f7f8f49 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Sun, 6 Mar 2016 22:08:44 -0800 Subject: [PATCH 2/4] pkg/fs: DRY SetBucketMetadata It had a lot of code that was the same as GetBucketMetadata, so instead call GBM from SBM so as to reduce doing the same thing in two different spots. Theoretically this will induce a small overhead as now at least two calls of denormalizeBucket are made, although this shouldn't be noticeable. --- pkg/fs/fs-bucket.go | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/pkg/fs/fs-bucket.go b/pkg/fs/fs-bucket.go index 686bc6432..e7ff8d358 100644 --- a/pkg/fs/fs-bucket.go +++ b/pkg/fs/fs-bucket.go @@ -230,45 +230,27 @@ func (fs Filesystem) GetBucketMetadata(bucket string) (BucketMetadata, *probe.Er // SetBucketMetadata - set bucket metadata. func (fs Filesystem) SetBucketMetadata(bucket string, metadata map[string]string) *probe.Error { - // Input validation. - if !IsValidBucketName(bucket) { - return probe.NewError(BucketNameInvalid{Bucket: bucket}) + bucketMetadata, err := fs.GetBucketMetadata(bucket) + if err != nil { + return err } + // Save the acl. acl := metadata["acl"] if !IsValidBucketACL(acl) { return probe.NewError(InvalidACL{ACL: acl}) - } - if acl == "" { + } else if acl == "" { acl = "private" } - bucket = fs.denormalizeBucket(bucket) - bucketDir := filepath.Join(fs.path, bucket) - fi, e := os.Stat(bucketDir) - if e != nil { - // Check if bucket exists. - if os.IsNotExist(e) { - return probe.NewError(BucketNotFound{Bucket: bucket}) - } - return probe.NewError(e) - } - - // Critical region handle read lock. - fs.rwLock.RLock() - bucketMetadata, ok := fs.buckets.Metadata[bucket] - fs.rwLock.RUnlock() - if !ok { - bucketMetadata = &BucketMetadata{} - bucketMetadata.Name = fi.Name() - bucketMetadata.Created = fi.ModTime() - } bucketMetadata.ACL = BucketACL(acl) + bucket = fs.denormalizeBucket(bucket) + // Critical region handle write lock. fs.rwLock.Lock() defer fs.rwLock.Unlock() - fs.buckets.Metadata[bucket] = bucketMetadata + fs.buckets.Metadata[bucket] = &bucketMetadata if err := saveBucketsMetadata(*fs.buckets); err != nil { return err.Trace(bucket) } From 7399d8ceaa1659d853ea0db711e5c2f5314f7fbc Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Sun, 6 Mar 2016 23:54:43 -0800 Subject: [PATCH 3/4] pkg/fs: skip unnecessary os.Stat system call --- pkg/fs/fs-bucket.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pkg/fs/fs-bucket.go b/pkg/fs/fs-bucket.go index e7ff8d358..1a598e82a 100644 --- a/pkg/fs/fs-bucket.go +++ b/pkg/fs/fs-bucket.go @@ -36,14 +36,11 @@ func (fs Filesystem) DeleteBucket(bucket string) *probe.Error { } bucket = fs.denormalizeBucket(bucket) bucketDir := filepath.Join(fs.path, bucket) - // Check if bucket exists. - if _, e := os.Stat(bucketDir); e != nil { + if e := os.Remove(bucketDir); e != nil { + // Error if there was no bucket in the first place. if os.IsNotExist(e) { return probe.NewError(BucketNotFound{Bucket: bucket}) } - return probe.NewError(e) - } - if e := os.Remove(bucketDir); e != nil { // On windows the string is slightly different, handle it here. if strings.Contains(e.Error(), "directory is not empty") { return probe.NewError(BucketNotEmpty{Bucket: bucket}) From fab45aae40e4a4f2ccbc7d41bbd9b991e9f83594 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Sun, 6 Mar 2016 23:55:15 -0800 Subject: [PATCH 4/4] pkg/fs: add bucket test and benchmarks Lots of useful benchmarks and a simple test addition! --- pkg/fs/fs-bucket_test.go | 145 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 pkg/fs/fs-bucket_test.go diff --git a/pkg/fs/fs-bucket_test.go b/pkg/fs/fs-bucket_test.go new file mode 100644 index 000000000..c50ac1eb6 --- /dev/null +++ b/pkg/fs/fs-bucket_test.go @@ -0,0 +1,145 @@ +/* + * Minio Cloud Storage, (C) 2016 Minio, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package fs + +import ( + "io/ioutil" + "os" + "strings" + "testing" +) + +func TestDeleteBucket(t *testing.T) { + // Make a temporary directory to use as the filesystem. + directory, fserr := ioutil.TempDir("", "minio-benchmark") + if fserr != nil { + t.Fatal(fserr) + } + defer os.RemoveAll(directory) + + // Create the filesystem. + filesystem, err := New(directory, 0) + if err != nil { + t.Fatal(err) + } + + // Deleting a bucket that doesn't exist should error. + err = filesystem.DeleteBucket("bucket") + if !strings.Contains(err.Cause.Error(), "Bucket not found:") { + t.Fail() + } +} + +func BenchmarkDeleteBucket(b *testing.B) { + // Make a temporary directory to use as the filesystem. + directory, fserr := ioutil.TempDir("", "minio-benchmark") + if fserr != nil { + b.Fatal(fserr) + } + defer os.RemoveAll(directory) + + // Create the filesystem. + filesystem, err := New(directory, 0) + if err != nil { + b.Fatal(err) + } + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + // Creating buckets takes time, so stop and start the timer. + b.StopTimer() + + // Create and delete the bucket over and over. + err = filesystem.MakeBucket("bucket", "public-read-write") + if err != nil { + b.Fatal(err) + } + + b.StartTimer() + + err = filesystem.DeleteBucket("bucket") + if err != nil { + b.Fatal(err) + } + } +} + +func BenchmarkGetBucketMetadata(b *testing.B) { + // Make a temporary directory to use as the filesystem. + directory, fserr := ioutil.TempDir("", "minio-benchmark") + if fserr != nil { + b.Fatal(fserr) + } + defer os.RemoveAll(directory) + + // Create the filesystem. + filesystem, err := New(directory, 0) + if err != nil { + b.Fatal(err) + } + + // Put up a bucket with some metadata. + err = filesystem.MakeBucket("bucket", "public-read-write") + if err != nil { + b.Fatal(err) + } + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + // Retrieve the metadata! + _, err := filesystem.GetBucketMetadata("bucket") + if err != nil { + b.Fatal(err) + } + } +} + +func BenchmarkSetBucketMetadata(b *testing.B) { + // Make a temporary directory to use as the filesystem. + directory, fserr := ioutil.TempDir("", "minio-benchmark") + if fserr != nil { + b.Fatal(fserr) + } + defer os.RemoveAll(directory) + + // Create the filesystem. + filesystem, err := New(directory, 0) + if err != nil { + b.Fatal(err) + } + + // Put up a bucket with some metadata. + err = filesystem.MakeBucket("bucket", "public-read-write") + if err != nil { + b.Fatal(err) + } + + metadata := make(map[string]string) + metadata["acl"] = "public-read-write" + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + // Set all the metadata! + err = filesystem.SetBucketMetadata("bucket", metadata) + if err != nil { + b.Fatal(err) + } + } +}