From 413c8cf4ea59dbca287cc3e9b8c8a2046cd0e250 Mon Sep 17 00:00:00 2001
From: Jakob Borg <jakob@kastelo.net>
Date: Thu, 3 Nov 2022 15:44:46 +0100
Subject: [PATCH] lib/connections: Use adaptive write size for rate limited
 connections (fixes #8630) (#8631)

---
 lib/connections/limiter.go      | 19 ++++++++++++++-----
 lib/connections/limiter_test.go | 20 ++++++++++++++------
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/lib/connections/limiter.go b/lib/connections/limiter.go
index 3f3e84bd7..9f279ba6e 100644
--- a/lib/connections/limiter.go
+++ b/lib/connections/limiter.go
@@ -37,8 +37,7 @@ type waiter interface {
 }
 
 const (
-	limiterBurstSize   = 4 * 128 << 10
-	maxSingleWriteSize = 8 << 10
+	limiterBurstSize = 4 * 128 << 10
 )
 
 func newLimiter(myId protocol.DeviceID, cfg config.Wrapper) *limiter {
@@ -251,10 +250,20 @@ func (w *limitedWriter) Write(buf []byte) (int, error) {
 	}
 
 	// This does (potentially) multiple smaller writes in order to be less
-	// bursty with large writes and slow rates.
+	// bursty with large writes and slow rates. At the same time we don't
+	// want to do hilarious amounts of tiny writes when the rate is high, so
+	// try to be a bit adaptable. We range from the minimum write size of 1
+	// KiB up to the limiter burst size, aiming for about a write every
+	// 10ms.
+	singleWriteSize := int(w.waiter.Limit() / 100)          // 10ms worth of data
+	singleWriteSize = ((singleWriteSize / 1024) + 1) * 1024 // round up to the next kibibyte
+	if singleWriteSize > limiterBurstSize {
+		singleWriteSize = limiterBurstSize
+	}
+
 	written := 0
 	for written < len(buf) {
-		toWrite := maxSingleWriteSize
+		toWrite := singleWriteSize
 		if toWrite > len(buf)-written {
 			toWrite = len(buf) - written
 		}
@@ -294,7 +303,7 @@ func (w waiterHolder) take(tokens int) {
 	// into the lower level reads so we might get a large amount of data and
 	// end up in the loop further down.
 
-	if tokens < limiterBurstSize {
+	if tokens <= limiterBurstSize {
 		// Fast path. We won't get an error from WaitN as we don't pass a
 		// context with a deadline.
 		_ = w.waiter.WaitN(context.TODO(), tokens)
diff --git a/lib/connections/limiter_test.go b/lib/connections/limiter_test.go
index caf6bf201..2379ec65c 100644
--- a/lib/connections/limiter_test.go
+++ b/lib/connections/limiter_test.go
@@ -218,7 +218,7 @@ func TestLimitedWriterWrite(t *testing.T) {
 
 	// A buffer with random data that is larger than the write size and not
 	// a precise multiple either.
-	src := make([]byte, int(12.5*maxSingleWriteSize))
+	src := make([]byte, int(12.5*8192))
 	if _, err := crand.Reader.Read(src); err != nil {
 		t.Fatal(err)
 	}
@@ -242,9 +242,14 @@ func TestLimitedWriterWrite(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Verify there were lots of writes and that the end result is identical.
-	if cw.writeCount != 13 {
-		t.Error("expected lots of smaller writes, but not too many")
+	// Verify there were lots of writes (we expect one kilobyte write size
+	// for the very low rate in this test) and that the end result is
+	// identical.
+	if cw.writeCount < 10*8 {
+		t.Error("expected lots of smaller writes")
+	}
+	if cw.writeCount > 15*8 {
+		t.Error("expected fewer larger writes")
 	}
 	if !bytes.Equal(src, dst.Bytes()) {
 		t.Error("results should be equal")
@@ -319,8 +324,11 @@ func TestLimitedWriterWrite(t *testing.T) {
 	}
 
 	// Verify there were lots of writes and that the end result is identical.
-	if cw.writeCount != 13 {
-		t.Error("expected just the one write")
+	if cw.writeCount < 10*8 {
+		t.Error("expected lots of smaller writes")
+	}
+	if cw.writeCount > 15*8 {
+		t.Error("expected fewer larger writes")
 	}
 	if !bytes.Equal(src, dst.Bytes()) {
 		t.Error("results should be equal")