Lock free multipart backend implementation for FS #5401
Conversation
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
2 times, most recently
from
dfaa4f5
to
7762002
Compare
|
Is this still a WIP @krishnasrinivas can you fix the CI failures? |
pkg/ioutil/ioutil.go
Outdated
| ) | ||
|
|
||
| var DefaultBufferSize = humanize.MiByte |
There was a problem hiding this comment.
should this be const? - can you add a comment..
There was a problem hiding this comment.
Sure, I can make that constant too as this util func is used only by us.
pkg/ioutil/ioutil.go
Outdated
| @@ -59,3 +64,20 @@ func (w *WriteOnCloser) HasWritten() bool { return w.hasWritten } | |||
| func WriteOnClose(w io.Writer) *WriteOnCloser { | |||
| return &WriteOnCloser{w, false} | |||
| } | |||
|
|
|||
| func AppendFile(dst string, src string) error { | |||
There was a problem hiding this comment.
A comment and a test would be needed..
There was a problem hiding this comment.
yes, will add
| @@ -833,6 +833,7 @@ func (xl xlObjects) listObjectParts(bucket, object, uploadID string, partNumberM | |||
| result.Object = object | |||
| result.UploadID = uploadID | |||
| result.MaxParts = maxParts | |||
| result.PartNumberMarker = partNumberMarker | |||
There was a problem hiding this comment.
should this be fixed outside this PR?
There was a problem hiding this comment.
unit tests were failing which required this fix. The refactor exposed this bug.
There was a problem hiding this comment.
Can you add in a separate PR ? This should be reproducible outside of this PR as well right ?
There was a problem hiding this comment.
Actually, while making the changes I fixed this bug and this broke the tests (as tests were itself incorrect), so the tests needed a fix as well. I can do this in separate PR and retain the bug in the code and tests in this PR. Probably the effort is not worth it at this point since it's all fixed.
cmd/fs-v1-multipart.go
Outdated
| } | ||
| sort.SliceStable(uploadIDs, func(i int, j int) bool { |
There was a problem hiding this comment.
Any particular reason to use sort.SliceStable instead of sort.Slice if yes can you add relevant comment?
There was a problem hiding this comment.
golang doc says:
// Slice sorts the provided slice given the provided less function.
//
// The sort is not guaranteed to be stable. For a stable sort, use
// SliceStable.
//
// The function panics if the provided interface is not a slice.
func Slice(slice interface{}, less func(i, j int) bool) {
So we should replace all our sort.Slice with the stable version
There was a problem hiding this comment.
That is incorrect way to look at it. Stable sort is needed when we are sorting based on two or more distinct elements. It is not useful for us and has performance penalty.
When equal elements are indistinguishable, such as with integers, or more generally, any data where the entire element is the key, stability is not an issue. Stability is also not an issue if all keys are different.
There was a problem hiding this comment.
Nowhere in our code we have requirements for sorting based on two or more distinct elements. For example if you say we sort based on object name and object modified time then stable sort makes sense.
There was a problem hiding this comment.
I assumed that unstable = unreliable. Will fix this.
krishnasrinivas
changed the title
krishnasrinivas
changed the title
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
6 times, most recently
from
18908d5
to
e24bcbf
Compare
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
from
e24bcbf
to
0971919
Compare
cmd/format-fs.go
Outdated
| @@ -41,14 +43,20 @@ type formatFSV1 struct { | |||
| } `json:"fs"` | |||
| } | |||
|
|
|||
| // formatFSV2 - structure is same as formatFSV1. But the multipart backend | |||
| // structure is flat instead of hieararchy now. | |||
| // 1. The last PutObjectPart triggers go-routine fs.backgroundAppend, this go-routine has not started yet. | ||
| // 2. Now CompleteMultipartUpload gets called which sees that lastPart is not appended and starts appending | ||
| // from the beginning | ||
| fs.backgroundAppend(bucket, object, uploadID) |
There was a problem hiding this comment.
Should this still be called backgroundAppend() ?
There was a problem hiding this comment.
Yes, because it still appends in the background (though it is not in a long lasting go-routine listening on a channel)
| ticker.Stop() | ||
| return | ||
| case <-ticker.C: | ||
| now := time.Now() |
There was a problem hiding this comment.
do you not want to use UTCNow() ?
There was a problem hiding this comment.
no because fi.Modtime() (which we use to compare with now) does not return in UTC
There was a problem hiding this comment.
check this:
krishna@escape:~/dev/goprogs$ cat mtime.go
package main
import (
"fmt"
"log"
"os"
"time"
)
func main() {
fmt.Println(time.Now(), time.Now().UTC())
fi, err := os.Stat("mtime.go")
if err != nil {
log.Fatal(err)
}
fmt.Println(fi.ModTime())
}
krishna@escape:~/dev/goprogs$ touch mtime.go
krishna@escape:~/dev/goprogs$ go run mtime.go
2018-01-17 12:44:04.361967289 -0800 PST m=+0.000160832 2018-01-17 20:44:04.361967415 +0000 UTC
2018-01-17 12:44:00.791171574 -0800 PST
krishna@escape:~/dev/goprogs$
There was a problem hiding this comment.
Actually, looks like it does not matter as time package abstracts it:
krishna@escape:~/dev/goprogs$ cat mtime.go
package main
import (
"fmt"
"log"
"os"
"time"
)
func main() {
now := time.Now()
nowUTC := time.Now().UTC()
fi, err := os.Stat("mtime.go")
if err != nil {
log.Fatal(err)
}
fmt.Println(now.Sub(fi.ModTime()), nowUTC.Sub(fi.ModTime()))
}
krishna@escape:~/dev/goprogs$ go run mtime.go
44.118449376s 44.118449493s
krishna@escape:~/dev/goprogs$
There was a problem hiding this comment.
So they manage it for us..
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
3 times, most recently
from
dfb3f60
to
523863e
Compare
Codecov Report
@@ Coverage Diff @@
## master #5401 +/- ##
==========================================
- Coverage 61.82% 61.63% -0.19%
==========================================
Files 201 201
Lines 29516 29072 -444
==========================================
- Hits 18248 17919 -329
+ Misses 9856 9769 -87
+ Partials 1412 1384 -28
Continue to review full report at Codecov.
|
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
5 times, most recently
from
7e4ea5d
to
6ab32c0
Compare
| @@ -0,0 +1,27 @@ | |||
| package ioutil | |||
| @@ -0,0 +1,27 @@ | |||
| // +build !windows | |||
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
3 times, most recently
from
0246692
to
61aaf64
Compare
|
|
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
3 times, most recently
from
6f80cc8
to
872d718
Compare
Mint Automation
5401-872d718/mint-gateway-s3.sh.log:5401-872d718/mint-gateway-azure.sh.log:5401-872d718/mint-fs.sh.log: |
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
from
872d718
to
547b86a
Compare
cmd/format-fs.go
Outdated
| @@ -98,19 +115,56 @@ func formatFSGetVersion(r io.ReadSeeker) (string, error) { | |||
| return format.FS.Version, nil | |||
| } | |||
|
|
|||
| // Migrate from V1 to V2. V2 implements new backend format for mutipart | |||
cmd/format-fs.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if version != formatFSVersionV1 { | ||
| if version != formatFSVersionV2 { | ||
| return fmt.Errorf(`%s file: expected FS version: %s, found FS version: %s`, formatConfigFile, formatFSVersionV1, version) |
cmd/format-fs_test.go
Outdated
| @@ -62,7 +62,7 @@ func TestFSFormatFS(t *testing.T) { | |||
| if err != nil { | |||
| t.Fatal(err) | |||
| } | |||
| if version != formatFSVersionV1 { | |||
| if version != formatFSVersionV2 { | |||
| t.Fatalf(`expected: %s, got: %s`, formatFSVersionV1, version) | |||
pkg/ioutil/append-file_windows.go
Outdated
| @@ -0,0 +1,43 @@ | |||
| /* | |||
| * Minio Cloud Storage, (C) 2017 Minio, Inc. | |||
pkg/ioutil/append-file_nix.go
Outdated
| // +build !windows | ||
|
|
||
| /* | ||
| * Minio Cloud Storage, (C) 2017 Minio, Inc. |
cmd/fs-v1-multipart.go
Outdated
| func (fs *fsObjects) decodePartFile(name string) (partNumber int, md5hex string, err error) { | ||
| result := strings.Split(name, ".") | ||
| if len(result) != 2 { | ||
| return -1, "", errUnexpected |
There was a problem hiding this comment.
A special -1 part number seems unnecessary as valid part numbers start from 1 anyway. In addition go conventions-wise, when error is not nil, the other return values must be ignored.
cmd/fs-v1-multipart.go
Outdated
| uploadIDs := uploadsV1{} | ||
| if _, err = uploadIDs.ReadFrom(rlk.LockedFile); err != nil { | ||
| return nil, false, err | ||
| return -1, "", errUnexpected |
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
6 times, most recently
from
1f7b89c
to
9d4a368
Compare
cmd/fs-v1-multipart.go
Outdated
| multipartMarkerPath = pathJoin(bucket, keyMarker) | ||
| lenEntries := len(entries) | ||
| if lenEntries == 0 { | ||
| errorIf(errUnexpected, "%s is empty", uploadIDDir) |
There was a problem hiding this comment.
We don't need to do this i think, since entries range would be skipped and returned quickly. Also there is no reason to print an error lo here..
There was a problem hiding this comment.
I added it to log it as we should never be in this situation. But we dont lose much by removing this error message too, because I think we will never hit this case. One possible situation where we will hit this case is when cleanup has cleaned the uploadID and not deleted the directory and just then if backgroundAppend sees 0 entries. This is a highly improbable case,so I will delete this logging.
cmd/fs-v1-multipart.go
Outdated
| if err := checkNewMultipartArgs(bucket, object, fs); err != nil { | ||
| return "", err | ||
| return "", toObjectErr(errors.Trace(err), bucket) |
There was a problem hiding this comment.
You could avoid double tracing here err returned by checkNewMultipartArgs is already traced.
cmd/fs-v1-multipart.go
Outdated
| } | ||
|
|
||
| if _, err := fs.statBucketDir(bucket); err != nil { | ||
| return "", toObjectErr(err, bucket) | ||
| return "", toObjectErr(errors.Trace(err), bucket) |
There was a problem hiding this comment.
You could avoid double tracing here err returned by checkNewMultipartArgs is already traced.
| nextPartIndex := fsMeta.ObjectPartIndex(nextPartNum) | ||
| if nextPartIndex == -1 { | ||
| return | ||
| if err = ioutil.WriteFile(pathJoin(uploadIDDir, fsMetaJSONFile), fsMetaBytes, 0644); err != nil { |
There was a problem hiding this comment.
You can use WriteTo interface here @krishnasrinivas and change the WriteTo to be.
func (m *fsMetaV1) WriteTo(w io.Writer) (n int64, err error) {
var metadataBytes []byte
metadataBytes, err = json.Marshal(m)
if err != nil {
return 0, errors.Trace(err)
}
t, ok := w.(*lock.LockedFile)
if ok {
if err = t.Truncate(0); err != nil {
return 0, errors.Trace(err)
}
}
if _, err = lk.Write(metadataBytes); err != nil {
return 0, errors.Trace(err)
}
// Success.
return int64(len(metadataBytes)), nil
}There was a problem hiding this comment.
I thought about this, but using WriteTo() does not give any advantage over ioutil.WriteFile().
On the other hand if WriteTo and ReadFrom takes lock.LockedFile the advantage is compiler always type checks that it reads and writes from a locked file. i.e we can have a convention that all access to the main namespace fs.json should be done through fsMeta.WriteTo and fsMeta.ReadFrom
There was a problem hiding this comment.
Okay that makes sense @krishnasrinivas
cmd/fs-v1-multipart.go
Outdated
| if err := checkCompleteMultipartArgs(bucket, object, fs); err != nil { | ||
| return oi, err | ||
| return oi, toObjectErr(errors.Trace(err)) |
|
|
||
| fsMeta := fsMetaV1{} | ||
| // Read saved fs metadata for ongoing multipart. | ||
| fsMetaBuf, err := ioutil.ReadFile(pathJoin(uploadIDDir, fsMetaJSONFile)) |
There was a problem hiding this comment.
You could use ReadFrom()
cmd/fs-v1-multipart.go
Outdated
| // delete the bucket/object/uploadid | ||
| fs.rwPool.Close(fsMetaPath) | ||
| fsRemoveAll(uploadIDDir) | ||
| // if err := fsRemoveAll(uploadIDDir); err != nil { |
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
from
9d4a368
to
b508a08
Compare
|
Build is failing. |
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
3 times, most recently
from
012bebe
to
6e5a7d8
Compare
krishnasrinivas
force-pushed
the
fs-multipart-no-lock
branch
from
6e5a7d8
to
de58159
Compare
Description
https://github.com/krishnasrinivas/wikinotes/wiki/FS-multipart-backend
Motivation and Context
Implement lock free multipart backend.
How Has This Been Tested?
manual.
unit tests fail. Looking into it
Types of changes
Checklist:
mintPR # here: )