From 6fe2b3f9016d18ba9784b6eaa2790633e5074b29 Mon Sep 17 00:00:00 2001
From: Harshavardhana <harsha@minio.io>
Date: Wed, 24 Jul 2024 03:22:50 -0700
Subject: [PATCH] avoid sendFile() for ranges or object lengths < 4MiB (#20141)

---
 cmd/storage-rest-server.go | 42 +++++++++++++++++++++-----------------
 cmd/xl-storage.go          |  1 -
 internal/ioutil/ioutil.go  | 23 ++++++++++-----------
 3 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/cmd/storage-rest-server.go b/cmd/storage-rest-server.go
index ed8ecd409..75fad25f0 100644
--- a/cmd/storage-rest-server.go
+++ b/cmd/storage-rest-server.go
@@ -36,6 +36,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/dustin/go-humanize"
 	"github.com/minio/minio/internal/grid"
 	"github.com/tinylib/msgp/msgp"
 
@@ -594,44 +595,47 @@ func (s *storageRESTServer) ReadFileStreamHandler(w http.ResponseWriter, r *http
 	}
 	volume := r.Form.Get(storageRESTVolume)
 	filePath := r.Form.Get(storageRESTFilePath)
-	offset, err := strconv.Atoi(r.Form.Get(storageRESTOffset))
+	offset, err := strconv.ParseInt(r.Form.Get(storageRESTOffset), 10, 64)
 	if err != nil {
 		s.writeErrorResponse(w, err)
 		return
 	}
-	length, err := strconv.Atoi(r.Form.Get(storageRESTLength))
+	length, err := strconv.ParseInt(r.Form.Get(storageRESTLength), 10, 64)
 	if err != nil {
 		s.writeErrorResponse(w, err)
 		return
 	}
 
-	w.Header().Set(xhttp.ContentLength, strconv.Itoa(length))
+	w.Header().Set(xhttp.ContentLength, strconv.FormatInt(length, 10))
 
-	rc, err := s.getStorage().ReadFileStream(r.Context(), volume, filePath, int64(offset), int64(length))
+	rc, err := s.getStorage().ReadFileStream(r.Context(), volume, filePath, offset, length)
 	if err != nil {
 		s.writeErrorResponse(w, err)
 		return
 	}
 	defer rc.Close()
 
-	rf, ok := w.(io.ReaderFrom)
-	if ok && runtime.GOOS != "windows" {
-		// Attempt to use splice/sendfile() optimization, A very specific behavior mentioned below is necessary.
-		// See https://github.com/golang/go/blob/f7c5cbb82087c55aa82081e931e0142783700ce8/src/net/sendfile_linux.go#L20
-		// Windows can lock up with this optimization, so we fall back to regular copy.
-		sr, ok := rc.(*sendFileReader)
+	noReadFrom := runtime.GOOS == "windows" || length < 4*humanize.MiByte
+	if !noReadFrom {
+		rf, ok := w.(io.ReaderFrom)
 		if ok {
-			// Sendfile sends in 4MiB chunks per sendfile syscall which is more than enough
-			// for most setups.
-			_, err = rf.ReadFrom(sr.Reader)
-			if !xnet.IsNetworkOrHostDown(err, true) { // do not need to log disconnected clients
-				storageLogIf(r.Context(), err)
-			}
-			if err == nil || !errors.Is(err, xhttp.ErrNotImplemented) {
-				return
+			// Attempt to use splice/sendfile() optimization, A very specific behavior mentioned below is necessary.
+			// See https://github.com/golang/go/blob/f7c5cbb82087c55aa82081e931e0142783700ce8/src/net/sendfile_linux.go#L20
+			// Windows can lock up with this optimization, so we fall back to regular copy.
+			sr, ok := rc.(*sendFileReader)
+			if ok {
+				// Sendfile sends in 4MiB chunks per sendfile syscall which is more than enough
+				// for most setups.
+				_, err = rf.ReadFrom(sr.Reader)
+				if !xnet.IsNetworkOrHostDown(err, true) { // do not need to log disconnected clients
+					storageLogIf(r.Context(), err)
+				}
+				if err == nil || !errors.Is(err, xhttp.ErrNotImplemented) {
+					return
+				}
 			}
 		}
-	} // Fallback to regular copy
+	} // noReadFrom means use io.Copy()
 
 	_, err = xioutil.Copy(w, rc)
 	if !xnet.IsNetworkOrHostDown(err, true) { // do not need to log disconnected clients
diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go
index dbef68ad2..cdaa79af1 100644
--- a/cmd/xl-storage.go
+++ b/cmd/xl-storage.go
@@ -2067,7 +2067,6 @@ func (s *xlStorage) ReadFileStream(ctx context.Context, volume, path string, off
 			return nil, err
 		}
 	}
-
 	return &sendFileReader{Reader: io.LimitReader(file, length), Closer: file}, nil
 }
 
diff --git a/internal/ioutil/ioutil.go b/internal/ioutil/ioutil.go
index 6ac14247d..1b93fbb60 100644
--- a/internal/ioutil/ioutil.go
+++ b/internal/ioutil/ioutil.go
@@ -243,6 +243,15 @@ func NopCloser(w io.Writer) io.WriteCloser {
 	return nopCloser{w}
 }
 
+const copyBufferSize = 32 * 1024
+
+var copyBufPool = sync.Pool{
+	New: func() interface{} {
+		b := make([]byte, copyBufferSize)
+		return &b
+	},
+}
+
 // SkipReader skips a given number of bytes and then returns all
 // remaining data.
 type SkipReader struct {
@@ -285,21 +294,11 @@ func NewSkipReader(r io.Reader, n int64) io.Reader {
 	return &SkipReader{r, n}
 }
 
-const copyBufferSize = 32 * 1024
-
-var copyBufPool = sync.Pool{
-	New: func() interface{} {
-		b := make([]byte, copyBufferSize)
-		return &b
-	},
-}
-
 // Copy is exactly like io.Copy but with reusable buffers.
 func Copy(dst io.Writer, src io.Reader) (written int64, err error) {
-	bufp := copyBufPool.Get().(*[]byte)
+	bufp := ODirectPoolLarge.Get().(*[]byte)
 	buf := *bufp
-	buf = buf[:copyBufferSize]
-	defer copyBufPool.Put(bufp)
+	defer ODirectPoolLarge.Put(bufp)
 
 	return io.CopyBuffer(dst, src, buf)
 }