From 86e0d272f37f326c509102193dfe479b864f104d Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Mon, 2 Nov 2020 16:14:31 -0800 Subject: [PATCH] Reduce WriteAll allocs (#10810) WriteAll saw 127GB allocs in a 5 minute timeframe for 4MiB buffers used by `io.CopyBuffer` even if they are pooled. Since all writers appear to write byte buffers, just send those instead and write directly. The files are opened through the `os` package so they have no special properties anyway. This removes the alloc and copy for each operation. REST sends content length so a precise alloc can be made. --- cmd/erasure-healing_test.go | 4 ++-- cmd/format-erasure.go | 5 ++--- cmd/naughty-disk_test.go | 4 ++-- cmd/storage-interface.go | 3 ++- cmd/storage-rest-client.go | 4 ++-- cmd/storage-rest-server.go | 9 +++++++-- cmd/xl-storage-disk-id-check.go | 4 ++-- cmd/xl-storage.go | 22 ++++++++++++---------- cmd/xl-storage_test.go | 4 ++-- 9 files changed, 33 insertions(+), 26 deletions(-) diff --git a/cmd/erasure-healing_test.go b/cmd/erasure-healing_test.go index 1d5c742d6..490051390 100644 --- a/cmd/erasure-healing_test.go +++ b/cmd/erasure-healing_test.go @@ -226,7 +226,7 @@ func TestHealObjectCorrupted(t *testing.T) { t.Errorf("Failure during deleting part.1 - %v", err) } - err = firstDisk.WriteAll(context.Background(), bucket, pathJoin(object, fi.DataDir, "part.1"), bytes.NewReader([]byte{})) + err = firstDisk.WriteAll(context.Background(), bucket, pathJoin(object, fi.DataDir, "part.1"), []byte{}) if err != nil { t.Errorf("Failure during creating part.1 - %v", err) } @@ -252,7 +252,7 @@ func TestHealObjectCorrupted(t *testing.T) { } bdata := bytes.Repeat([]byte("b"), int(nfi.Size)) - err = firstDisk.WriteAll(context.Background(), bucket, pathJoin(object, fi.DataDir, "part.1"), bytes.NewReader(bdata)) + err = firstDisk.WriteAll(context.Background(), bucket, pathJoin(object, fi.DataDir, "part.1"), bdata) if err != nil { t.Errorf("Failure during creating part.1 - %v", err) } diff --git a/cmd/format-erasure.go b/cmd/format-erasure.go index 08d259425..5e96b20d0 100644 --- a/cmd/format-erasure.go +++ b/cmd/format-erasure.go @@ -17,7 +17,6 @@ package cmd import ( - "bytes" "context" "encoding/hex" "encoding/json" @@ -363,7 +362,7 @@ func saveFormatErasure(disk StorageAPI, format *formatErasureV3, heal bool) erro defer disk.Delete(context.TODO(), minioMetaBucket, tmpFormat, false) // write to unique file. - if err = disk.WriteAll(context.TODO(), minioMetaBucket, tmpFormat, bytes.NewReader(formatBytes)); err != nil { + if err = disk.WriteAll(context.TODO(), minioMetaBucket, tmpFormat, formatBytes); err != nil { return err } @@ -383,7 +382,7 @@ func saveFormatErasure(disk StorageAPI, format *formatErasureV3, heal bool) erro } return disk.WriteAll(context.TODO(), minioMetaBucket, pathJoin(bucketMetaPrefix, slashSeparator, healingTrackerFilename), - bytes.NewReader(htrackerBytes)) + htrackerBytes) } return nil } diff --git a/cmd/naughty-disk_test.go b/cmd/naughty-disk_test.go index 2fd8a4783..3d1952509 100644 --- a/cmd/naughty-disk_test.go +++ b/cmd/naughty-disk_test.go @@ -273,11 +273,11 @@ func (d *naughtyDisk) ReadVersion(ctx context.Context, volume, path, versionID s return d.disk.ReadVersion(ctx, volume, path, versionID, checkDataDir) } -func (d *naughtyDisk) WriteAll(ctx context.Context, volume string, path string, reader io.Reader) (err error) { +func (d *naughtyDisk) WriteAll(ctx context.Context, volume string, path string, b []byte) (err error) { if err := d.calcError(); err != nil { return err } - return d.disk.WriteAll(ctx, volume, path, reader) + return d.disk.WriteAll(ctx, volume, path, b) } func (d *naughtyDisk) ReadAll(ctx context.Context, volume string, path string) (buf []byte, err error) { diff --git a/cmd/storage-interface.go b/cmd/storage-interface.go index 3688ed4cb..65761c4e6 100644 --- a/cmd/storage-interface.go +++ b/cmd/storage-interface.go @@ -78,7 +78,8 @@ type StorageAPI interface { VerifyFile(ctx context.Context, volume, path string, fi FileInfo) error // Write all data, syncs the data to disk. - WriteAll(ctx context.Context, volume string, path string, reader io.Reader) (err error) + // Should be used for smaller payloads. + WriteAll(ctx context.Context, volume string, path string, b []byte) (err error) // Read all. ReadAll(ctx context.Context, volume string, path string) (buf []byte, err error) diff --git a/cmd/storage-rest-client.go b/cmd/storage-rest-client.go index 8e20b3e8f..45284ee6a 100644 --- a/cmd/storage-rest-client.go +++ b/cmd/storage-rest-client.go @@ -315,11 +315,11 @@ func (client *storageRESTClient) DeleteVersion(ctx context.Context, volume, path } // WriteAll - write all data to a file. -func (client *storageRESTClient) WriteAll(ctx context.Context, volume string, path string, reader io.Reader) error { +func (client *storageRESTClient) WriteAll(ctx context.Context, volume string, path string, b []byte) error { values := make(url.Values) values.Set(storageRESTVolume, volume) values.Set(storageRESTFilePath, path) - respBody, err := client.call(ctx, storageRESTMethodWriteAll, values, reader, -1) + respBody, err := client.call(ctx, storageRESTMethodWriteAll, values, bytes.NewBuffer(b), int64(len(b))) defer http.DrainBody(respBody) return err } diff --git a/cmd/storage-rest-server.go b/cmd/storage-rest-server.go index 947411923..315ae7d45 100644 --- a/cmd/storage-rest-server.go +++ b/cmd/storage-rest-server.go @@ -384,8 +384,13 @@ func (s *storageRESTServer) WriteAllHandler(w http.ResponseWriter, r *http.Reque s.writeErrorResponse(w, errInvalidArgument) return } - - err := s.storage.WriteAll(r.Context(), volume, filePath, io.LimitReader(r.Body, r.ContentLength)) + tmp := make([]byte, r.ContentLength) + _, err := io.ReadFull(r.Body, tmp) + if err != nil { + s.writeErrorResponse(w, err) + return + } + err = s.storage.WriteAll(r.Context(), volume, filePath, tmp) if err != nil { s.writeErrorResponse(w, err) } diff --git a/cmd/xl-storage-disk-id-check.go b/cmd/xl-storage-disk-id-check.go index 07ff52001..11975e885 100644 --- a/cmd/xl-storage-disk-id-check.go +++ b/cmd/xl-storage-disk-id-check.go @@ -264,12 +264,12 @@ func (p *xlStorageDiskIDCheck) VerifyFile(ctx context.Context, volume, path stri return p.storage.VerifyFile(ctx, volume, path, fi) } -func (p *xlStorageDiskIDCheck) WriteAll(ctx context.Context, volume string, path string, reader io.Reader) (err error) { +func (p *xlStorageDiskIDCheck) WriteAll(ctx context.Context, volume string, path string, b []byte) (err error) { if err = p.checkDiskStale(); err != nil { return err } - return p.storage.WriteAll(ctx, volume, path, reader) + return p.storage.WriteAll(ctx, volume, path, b) } func (p *xlStorageDiskIDCheck) DeleteVersion(ctx context.Context, volume, path string, fi FileInfo) (err error) { diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index 90d83992e..3d21ea8dc 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -1203,7 +1203,7 @@ func (s *xlStorage) DeleteVersion(ctx context.Context, volume, path string, fi F } if !lastVersion { - return s.WriteAll(ctx, volume, pathJoin(path, xlStorageFormatFile), bytes.NewReader(buf)) + return s.WriteAll(ctx, volume, pathJoin(path, xlStorageFormatFile), buf) } // Delete the meta file, if there are no more versions the @@ -1251,7 +1251,7 @@ func (s *xlStorage) WriteMetadata(ctx context.Context, volume, path string, fi F } } - return s.WriteAll(ctx, volume, pathJoin(path, xlStorageFormatFile), bytes.NewReader(buf)) + return s.WriteAll(ctx, volume, pathJoin(path, xlStorageFormatFile), buf) } func (s *xlStorage) renameLegacyMetadata(volume, path string) error { @@ -1861,7 +1861,7 @@ func (s *xlStorage) CreateFile(ctx context.Context, volume, path string, fileSiz return nil } -func (s *xlStorage) WriteAll(ctx context.Context, volume string, path string, reader io.Reader) (err error) { +func (s *xlStorage) WriteAll(ctx context.Context, volume string, path string, b []byte) (err error) { atomic.AddInt32(&s.activeIOCount, 1) defer func() { atomic.AddInt32(&s.activeIOCount, -1) @@ -1873,12 +1873,14 @@ func (s *xlStorage) WriteAll(ctx context.Context, volume string, path string, re } defer w.Close() - - bufp := s.pool.Get().(*[]byte) - defer s.pool.Put(bufp) - - _, err = io.CopyBuffer(w, reader, *bufp) - return err + n, err := w.Write(b) + if err != nil { + return err + } + if n != len(b) { + return io.ErrShortWrite + } + return nil } // AppendFile - append a byte array at path, if file doesn't exist at @@ -2311,7 +2313,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath, dataDir, return errFileCorrupt } - if err = s.WriteAll(ctx, srcVolume, pathJoin(srcPath, xlStorageFormatFile), bytes.NewReader(dstBuf)); err != nil { + if err = s.WriteAll(ctx, srcVolume, pathJoin(srcPath, xlStorageFormatFile), dstBuf); err != nil { return err } diff --git a/cmd/xl-storage_test.go b/cmd/xl-storage_test.go index 5ab1317fa..29a129122 100644 --- a/cmd/xl-storage_test.go +++ b/cmd/xl-storage_test.go @@ -133,7 +133,7 @@ func newXLStorageTestSetup() (*xlStorageDiskIDCheck, string, error) { return nil, "", err } // Create a sample format.json file - err = storage.WriteAll(context.Background(), minioMetaBucket, formatConfigFile, bytes.NewBufferString(`{"version":"1","format":"xl","id":"592a41c2-b7cc-4130-b883-c4b5cb15965b","xl":{"version":"3","this":"da017d62-70e3-45f1-8a1a-587707e69ad1","sets":[["e07285a6-8c73-4962-89c6-047fb939f803","33b8d431-482d-4376-b63c-626d229f0a29","cff6513a-4439-4dc1-bcaa-56c9e880c352","da017d62-70e3-45f1-8a1a-587707e69ad1","9c9f21d5-1f15-4737-bce6-835faa0d9626","0a59b346-1424-4fc2-9fa2-a2e80541d0c1","7924a3dc-b69a-4971-9a2e-014966d6aebb","4d2b8dd9-4e48-444b-bdca-c89194b26042"]],"distributionAlgo":"CRCMOD"}}`)) + err = storage.WriteAll(context.Background(), minioMetaBucket, formatConfigFile, []byte(`{"version":"1","format":"xl","id":"592a41c2-b7cc-4130-b883-c4b5cb15965b","xl":{"version":"3","this":"da017d62-70e3-45f1-8a1a-587707e69ad1","sets":[["e07285a6-8c73-4962-89c6-047fb939f803","33b8d431-482d-4376-b63c-626d229f0a29","cff6513a-4439-4dc1-bcaa-56c9e880c352","da017d62-70e3-45f1-8a1a-587707e69ad1","9c9f21d5-1f15-4737-bce6-835faa0d9626","0a59b346-1424-4fc2-9fa2-a2e80541d0c1","7924a3dc-b69a-4971-9a2e-014966d6aebb","4d2b8dd9-4e48-444b-bdca-c89194b26042"]],"distributionAlgo":"CRCMOD"}}`)) if err != nil { return nil, "", err } @@ -1659,7 +1659,7 @@ func TestXLStorageVerifyFile(t *testing.T) { h := algo.New() h.Write(data) hashBytes := h.Sum(nil) - if err := xlStorage.WriteAll(context.Background(), volName, fileName, bytes.NewBuffer(data)); err != nil { + if err := xlStorage.WriteAll(context.Background(), volName, fileName, data); err != nil { t.Fatal(err) } if err := xlStorage.storage.bitrotVerify(pathJoin(path, volName, fileName), size, algo, hashBytes, 0); err != nil {