From 14f0047295930fbaadc88438889270956689b6fe Mon Sep 17 00:00:00 2001 From: Anis Elleuch Date: Fri, 14 Apr 2017 20:06:24 +0100 Subject: [PATCH] fs: Remove fs meta lock when PutObject() fails (#4114) Removing the fs meta lock file when PutObject() encounters any error during its execution, such as upload getting permatuerly cancelled by the client. --- cmd/fs-v1-helpers.go | 47 ++++++++++++++++++++++++++++++++++ cmd/fs-v1-helpers_test.go | 54 +++++++++++++++++++++++++++++++++++++++ cmd/fs-v1-multipart.go | 48 +++------------------------------- cmd/fs-v1.go | 14 ++++++++-- 4 files changed, 116 insertions(+), 47 deletions(-) diff --git a/cmd/fs-v1-helpers.go b/cmd/fs-v1-helpers.go index 85fcce0c1..56131956b 100644 --- a/cmd/fs-v1-helpers.go +++ b/cmd/fs-v1-helpers.go @@ -20,6 +20,7 @@ import ( "io" "os" pathutil "path" + "runtime" ) // Removes only the file at given path does not remove @@ -377,3 +378,49 @@ func fsDeleteFile(basePath, deletePath string) error { return nil } + +// fsRemoveMeta safely removes a locked file and takes care of Windows special case +func fsRemoveMeta(basePath, deletePath, tmpDir string) error { + // Special case for windows please read through. + if runtime.GOOS == globalWindowsOSName { + // Ordinarily windows does not permit deletion or renaming of files still + // in use, but if all open handles to that file were opened with FILE_SHARE_DELETE + // then it can permit renames and deletions of open files. + // + // There are however some gotchas with this, and it is worth listing them here. + // Firstly, Windows never allows you to really delete an open file, rather it is + // flagged as delete pending and its entry in its directory remains visible + // (though no new file handles may be opened to it) and when the very last + // open handle to the file in the system is closed, only then is it truly + // deleted. Well, actually only sort of truly deleted, because Windows only + // appears to remove the file entry from the directory, but in fact that + // entry is merely hidden and actually still exists and attempting to create + // a file with the same name will return an access denied error. How long it + // silently exists for depends on a range of factors, but put it this way: + // if your code loops creating and deleting the same file name as you might + // when operating a lock file, you're going to see lots of random spurious + // access denied errors and truly dismal lock file performance compared to POSIX. + // + // We work-around these un-POSIX file semantics by taking a dual step to + // deleting files. Firstly, it renames the file to tmp location into multipartTmpBucket + // We always open files with FILE_SHARE_DELETE permission enabled, with that + // flag Windows permits renaming and deletion, and because the name was changed + // to a very random name somewhere not in its origin directory before deletion, + // you don't see those unexpected random errors when creating files with the + // same name as a recently deleted file as you do anywhere else on Windows. + // Because the file is probably not in its original containing directory any more, + // deletions of that directory will not fail with "directory not empty" as they + // otherwise normally would either. + + tmpPath := pathJoin(tmpDir, mustGetUUID()) + + fsRenameFile(deletePath, tmpPath) + + // Proceed to deleting the directory if empty + fsDeleteFile(basePath, pathutil.Dir(deletePath)) + + // Finally delete the renamed file. + return fsDeleteFile(tmpDir, tmpPath) + } + return fsDeleteFile(basePath, deletePath) +} diff --git a/cmd/fs-v1-helpers_test.go b/cmd/fs-v1-helpers_test.go index afaff8a80..a262868b1 100644 --- a/cmd/fs-v1-helpers_test.go +++ b/cmd/fs-v1-helpers_test.go @@ -18,7 +18,12 @@ package cmd import ( "bytes" + "io/ioutil" + "os" + "path" "testing" + + "github.com/minio/minio/pkg/lock" ) func TestFSStats(t *testing.T) { @@ -396,3 +401,52 @@ func TestFSRemoves(t *testing.T) { t.Fatal(err) } } + +func TestFSRemoveMeta(t *testing.T) { + // create posix test setup + _, fsPath, err := newPosixTestSetup() + if err != nil { + t.Fatalf("Unable to create posix test setup, %s", err) + } + defer removeAll(fsPath) + + // Setup test environment. + if err = fsMkdir(pathJoin(fsPath, "success-vol")); err != nil { + t.Fatalf("Unable to create directory, %s", err) + } + + filePath := pathJoin(fsPath, "success-vol", "success-file") + + var buf = make([]byte, 4096) + var reader = bytes.NewReader([]byte("Hello, world")) + if _, err = fsCreateFile(filePath, reader, buf, reader.Size()); err != nil { + t.Fatalf("Unable to create file, %s", err) + } + + rwPool := &fsIOPool{ + readersMap: make(map[string]*lock.RLockedFile), + } + + if _, err := rwPool.Open(filePath); err != nil { + t.Fatalf("Unable to lock file %s", filePath) + } + + defer rwPool.Close(filePath) + + tmpDir, tmpErr := ioutil.TempDir(globalTestTmpDir, "minio-") + if tmpErr != nil { + t.Fatal(tmpErr) + } + + if err := fsRemoveMeta(fsPath, filePath, tmpDir); err != nil { + t.Fatalf("Unable to remove file, %s", err) + } + + if _, err := os.Stat(filePath); !os.IsNotExist(err) { + t.Fatalf("`%s` file found though it should have been deleted.", filePath) + } + + if _, err := os.Stat(path.Dir(filePath)); !os.IsNotExist(err) { + t.Fatalf("`%s` parent directory found though it should have been deleted.", filePath) + } +} diff --git a/cmd/fs-v1-multipart.go b/cmd/fs-v1-multipart.go index ee7010e8b..104f30548 100644 --- a/cmd/fs-v1-multipart.go +++ b/cmd/fs-v1-multipart.go @@ -24,7 +24,6 @@ import ( "io" "os" pathutil "path" - "runtime" "strings" "time" @@ -46,56 +45,15 @@ func (fs fsObjects) isMultipartUpload(bucket, prefix string) bool { return true } -// Delete uploads.json file wrapper handling a tricky case on windows. +// Delete uploads.json file wrapper func (fs fsObjects) deleteUploadsJSON(bucket, object, uploadID string) error { - timeID := fmt.Sprintf("%X", UTCNow().UnixNano()) - tmpPath := pathJoin(fs.fsPath, minioMetaTmpBucket, fs.fsUUID, uploadID+"+"+timeID) - multipartBucketPath := pathJoin(fs.fsPath, minioMetaMultipartBucket) uploadPath := pathJoin(multipartBucketPath, bucket, object) uploadsMetaPath := pathJoin(uploadPath, uploadsJSONFile) - // Special case for windows please read through. - if runtime.GOOS == globalWindowsOSName { - // Ordinarily windows does not permit deletion or renaming of files still - // in use, but if all open handles to that file were opened with FILE_SHARE_DELETE - // then it can permit renames and deletions of open files. - // - // There are however some gotchas with this, and it is worth listing them here. - // Firstly, Windows never allows you to really delete an open file, rather it is - // flagged as delete pending and its entry in its directory remains visible - // (though no new file handles may be opened to it) and when the very last - // open handle to the file in the system is closed, only then is it truly - // deleted. Well, actually only sort of truly deleted, because Windows only - // appears to remove the file entry from the directory, but in fact that - // entry is merely hidden and actually still exists and attempting to create - // a file with the same name will return an access denied error. How long it - // silently exists for depends on a range of factors, but put it this way: - // if your code loops creating and deleting the same file name as you might - // when operating a lock file, you're going to see lots of random spurious - // access denied errors and truly dismal lock file performance compared to POSIX. - // - // We work-around these un-POSIX file semantics by taking a dual step to - // deleting files. Firstly, it renames the file to tmp location into multipartTmpBucket - // We always open files with FILE_SHARE_DELETE permission enabled, with that - // flag Windows permits renaming and deletion, and because the name was changed - // to a very random name somewhere not in its origin directory before deletion, - // you don't see those unexpected random errors when creating files with the - // same name as a recently deleted file as you do anywhere else on Windows. - // Because the file is probably not in its original containing directory any more, - // deletions of that directory will not fail with "directory not empty" as they - // otherwise normally would either. - fsRenameFile(uploadsMetaPath, tmpPath) - - // Proceed to deleting the directory. - if err := fsDeleteFile(multipartBucketPath, uploadPath); err != nil { - return err - } + tmpDir := pathJoin(fs.fsPath, minioMetaTmpBucket, fs.fsUUID) - // Finally delete the renamed file. - return fsDeleteFile(pathutil.Dir(tmpPath), tmpPath) - } - return fsDeleteFile(multipartBucketPath, uploadsMetaPath) + return fsRemoveMeta(multipartBucketPath, uploadsMetaPath, tmpDir) } // Removes the uploadID, called either by CompleteMultipart of AbortMultipart. If the resuling uploads diff --git a/cmd/fs-v1.go b/cmd/fs-v1.go index 8a6a3430e..32d9600d9 100644 --- a/cmd/fs-v1.go +++ b/cmd/fs-v1.go @@ -492,7 +492,9 @@ func (fs fsObjects) GetObjectInfo(bucket, object string) (ObjectInfo, error) { // until EOF, writes data directly to configured filesystem path. // Additionally writes `fs.json` which carries the necessary metadata // for future object operations. -func (fs fsObjects) PutObject(bucket string, object string, size int64, data io.Reader, metadata map[string]string, sha256sum string) (objInfo ObjectInfo, err error) { +func (fs fsObjects) PutObject(bucket string, object string, size int64, data io.Reader, metadata map[string]string, sha256sum string) (objInfo ObjectInfo, retErr error) { + var err error + // This is a special case with size as '0' and object ends with // a slash separator, we treat it like a valid operation and // return success. @@ -517,13 +519,21 @@ func (fs fsObjects) PutObject(bucket string, object string, size int64, data io. var wlk *lock.LockedFile if bucket != minioMetaBucket { - fsMetaPath := pathJoin(fs.fsPath, minioMetaBucket, bucketMetaPrefix, bucket, object, fsMetaJSONFile) + bucketMetaDir := pathJoin(fs.fsPath, minioMetaBucket, bucketMetaPrefix) + fsMetaPath := pathJoin(bucketMetaDir, bucket, object, fsMetaJSONFile) wlk, err = fs.rwPool.Create(fsMetaPath) if err != nil { return ObjectInfo{}, toObjectErr(traceError(err), bucket, object) } // This close will allow for locks to be synchronized on `fs.json`. defer wlk.Close() + defer func() { + // Remove meta file when PutObject encounters any error + if retErr != nil { + tmpDir := pathJoin(fs.fsPath, minioMetaTmpBucket, fs.fsUUID) + fsRemoveMeta(bucketMetaDir, fsMetaPath, tmpDir) + } + }() } // Uploaded object will first be written to the temporary location which will eventually