Read ahead over SFTP: one connection with 32 requests in flight by default, more with restore_sftp_connections; a Storage Box gets slower with parallel readers

CHANGELOG.md

@@ -10,11 +10,11 @@ instead carried by the app description, the git tag, the release, and this file.
 ## [Unreleased]
 
 ### Changed
-- A full restore over SFTP reads the packs ahead on 4 connections of its own, many requests in
+- A full restore over SFTP reads the packs ahead on a connection of its own, 32 requests in
-  flight on each, into temporary files in the restore folder. From a Hetzner Storage Box to a
+  flight, into temporary files in the restore folder. From a Hetzner Storage Box to a server in
-  server in Canada (122 ms) packs now come in at about 7.6 MiB/s instead of 2.8; on a test line
+  Canada (122 ms) packs now come in at about 14 MiB/s instead of 2.8. More connections can be set
-  with 120 ms and some packet loss, a 1.6 GiB restore went from 5.1 to 17.8 MiB/s. A part that
+  with `restore_sftp_connections` (README). A part that cannot be read this way is read as
-  cannot be read this way is read as before. Restores from other locations work as before.
+  before. Restores from other locations work as before.
 
 ### Fixed
 - After a full restore stopped halfway, running it again with `--force` was refused when the

README.md

@@ -181,9 +181,10 @@ Contributions welcome via PR — add what you ran into with your own provider.
   server (or the VM/container host where TCP ends) to BBR:
   `sysctl -w net.core.default_qdisc=fq net.ipv4.tcp_congestion_control=bbr`
   (`modprobe tcp_bbr` first; make it permanent in `/etc/sysctl.d/`).
-  A full restore over SFTP reads the packs ahead on 4 connections of its own, next to the one
+  A full restore over SFTP reads the packs ahead on a second connection, with many requests in
-  NG Backup already has open: the SFTP server must allow 5 connections at once for that user (a
+  flight. A server that handles parallel reads well (not a Hetzner Storage Box, which gets slower)
-  Hetzner Storage Box allows 10). With fewer it uses the connections it gets.
+  can be read on more connections: `occ config:app:set ng_backup restore_sftp_connections
+  --value=4` (at most 8; the server must allow that many plus one at once for the user).
 
 - **NFS is not a files_external backend at all.** Mount the NFS export on the server yourself
   (e.g. via `/etc/fstab` or a systemd `.mount` unit) and point ng_backup's `local` backend at the

lib/Backend/SftpRangeFetcher.php

@@ -11,10 +11,9 @@ namespace OCA\NgBackup\Backend;
  * Reads byte ranges of files on an SFTP location ahead of use, over several connections at once,
  * into local files (StorageBackend::readAhead(), for a full restore).
  *
- * One connection is bound by the round trip and by what a single TCP connection gets over a long
+ * A restore that reads its packs as it goes is bound by the round trip: from a Hetzner Storage
- * line: a restore from a Hetzner Storage Box to Canada (RTT 122 ms) reached about 2 MiB/s, with
+ * Box to Canada (RTT 122 ms) it reached about 2.8 MiB/s, with the PHP process mostly waiting.
- * the PHP process mostly waiting. Here every connection reads its own range with DEPTH requests
+ * Here every connection reads its own range with DEPTH requests in flight, ahead of the restore. PHP runs one thing at a time: while it waits for an answer on one connection, the
- * in flight. PHP runs one thing at a time: while it waits for an answer on one connection, the
  * answers of the others arrive in the kernel's buffers, so the connections still transfer at the
  * same time.
  *
@@ -24,10 +23,17 @@ namespace OCA\NgBackup\Backend;
  * for the caller to read the usual way; a broken connection is not used again.
  */
 final class SftpRangeFetcher {
-	/** Connections used at once by a full restore. A Hetzner Storage Box allows 10 per account. */
+	/**
-	public const CONNECTIONS = 4;
+	 * Connections a full restore uses, unless the app setting restore_sftp_connections says
+	 * otherwise. One: a Hetzner Storage Box gave about 14 MiB/s on one connection with DEPTH
+	 * requests in flight, and 4-6 MiB/s on 2 or 4 (it reads from disks, and parallel readers make
+	 * them seek). On a line with packet loss to a server that handles parallel reads well, 4
+	 * connections did better (21 against 14 MiB/s).
+	 */
+	public const DEFAULT_CONNECTIONS = 1;
+	public const MAX_CONNECTIONS = 8;
 	/** READ requests in flight per connection. */
-	public const DEPTH = 16;
+	public const DEPTH = 32;
 	/** Ranges read ahead of the one the caller is at, in flight or waiting in the spool. */
 	public const AHEAD = 8;
 	/**

lib/Backend/StorageBackend.php

@@ -29,6 +29,8 @@ final class StorageBackend implements IBackend, IReadsAhead {
 	/** Seconds without a request after which an SFTP connection is checked before it is used. */
 	public const IDLE_CHECK = 60;
 	private float $lastUsed;
+	/** Connections readAhead() opens (see SftpRangeFetcher::DEFAULT_CONNECTIONS). */
+	public int $readAheadConnections = SftpRangeFetcher::DEFAULT_CONNECTIONS;
 	/** Times the storage was built anew after its connection died (for tests and diagnostics). */
 	public int $reconnects = 0;
 
@@ -244,7 +246,7 @@ final class StorageBackend implements IBackend, IReadsAhead {
 	}
 
 	/**
-	 * Over SFTP: SftpRangeFetcher, on CONNECTIONS new connections of its own (built with the
+	 * Over SFTP: SftpRangeFetcher, on readAheadConnections new connections of its own (built with the
 	 * reconnect function), so the storage's connection stays free for other requests. Elsewhere
 	 * not (yet): null.
 	 */
@@ -262,7 +264,8 @@ final class StorageBackend implements IBackend, IReadsAhead {
 		$storages = [];
 		$connections = [];
 		$root = '';
-		for ($n = 0; $n < SftpRangeFetcher::CONNECTIONS; $n++) {
+		$wanted = max(1, min(SftpRangeFetcher::MAX_CONNECTIONS, $this->readAheadConnections));
+		for ($n = 0; $n < $wanted; $n++) {
 			try {
 				$storage = $reconnect();
 				$sftp = SftpReadStream::connection($storage);

lib/Service/TargetService.php

@@ -287,7 +287,10 @@ final class TargetService {
 
 	private function buildBackend(string $backend, string $auth, array $options, string $basePath, bool $createBase = true): StorageBackend {
 		$create = fn (): IStorage => $this->factory()->create($backend, $auth, $options);
-		return new StorageBackend($create(), $basePath, $createBase, $create);
+		$storage = new StorageBackend($create(), $basePath, $createBase, $create);
+		$storage->readAheadConnections = Server::get(\OCP\IAppConfig::class)
+			->getValueInt('ng_backup', 'restore_sftp_connections', \OCA\NgBackup\Backend\SftpRangeFetcher::DEFAULT_CONNECTIONS);
+		return $storage;
 	}
 
 	private ?ExternalStorageFactory $factory = null;

tests/Unit/Backend/SftpRangeFetcherTest.php

@@ -79,9 +79,9 @@ class SftpRangeFetcherTest extends TestCase {
 	}
 
 	public function testKeepsSeveralRequestsInFlightOnEachConnection(): void {
-		$this->files['/home/repo/packs/big'] = random_bytes(6 * 1048576);
+		$this->files['/home/repo/packs/big'] = random_bytes(10 * 1048576);
 		$connections = $this->connections(2);
-		self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', [['repo/packs/big', 0, 6 * 1048576], ['repo/packs/big', 5, 6 * 1048576 - 5]], $this->tempDir()));
+		self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', [['repo/packs/big', 0, 10 * 1048576], ['repo/packs/big', 5, 10 * 1048576 - 5]], $this->tempDir()));
 
 		$this->assertSame(SftpRangeFetcher::DEPTH, $connections[0]->maxInFlight);
 		$this->assertSame(SftpRangeFetcher::DEPTH, $connections[1]->maxInFlight);

tests/Unit/Backend/StorageBackendReadAheadTest.php

@@ -53,6 +53,7 @@ class StorageBackendReadAheadTest extends TestCase {
 
 	public function testReadsAheadOnNewConnectionsOnly(): void {
 		$backend = new StorageBackend($this->storage(), 'base/repo', false, fn (): IStorage => $this->storage());
+		$backend->readAheadConnections = 4;
 		$this->assertTrue($backend->canReadAhead());
 
 		$result = [];
@@ -62,7 +63,7 @@ class StorageBackendReadAheadTest extends TestCase {
 
 		$expected = array_map(fn (int $i): string => sha1(substr($this->files["/home/base/repo/packs/p$i"], 7, 1048576 - 7)), range(0, 5));
 		$this->assertSame($expected, $result);
-		$this->assertCount(1 + SftpRangeFetcher::CONNECTIONS, $this->connections);
+		$this->assertCount(1 + 4, $this->connections);
 		$this->assertSame(0, $this->connections[0]->sent, 'the storage\'s own connection stays free');
 		foreach (array_slice($this->connections, 1) as $c) {
 			$this->assertTrue($c->disconnected);
@@ -72,6 +73,7 @@ class StorageBackendReadAheadTest extends TestCase {
 	public function testGoesOnWithTheConnectionsTheServerAllows(): void {
 		$this->connectLimit = 3;
 		$backend = new StorageBackend($this->storage(), 'base/repo', false, fn (): IStorage => $this->storage());
+		$backend->readAheadConnections = 4;
 
 		$count = 0;
 		foreach ($backend->readAhead(self::ranges(), $this->tempDir()) as $fh) {
@@ -83,6 +85,16 @@ class StorageBackendReadAheadTest extends TestCase {
 		$this->assertCount(3, $this->connections);
 	}
 
+	public function testByDefaultOneConnectionNextToTheStoragesOwn(): void {
+		$backend = new StorageBackend($this->storage(), 'base/repo', false, fn (): IStorage => $this->storage());
+		foreach ($backend->readAhead(self::ranges(), $this->tempDir()) as $fh) {
+			$this->assertIsResource($fh);
+		}
+
+		$this->assertSame(1, SftpRangeFetcher::DEFAULT_CONNECTIONS);
+		$this->assertCount(2, $this->connections);
+	}
+
 	public function testNoNewConnectionMeansNoReadingAhead(): void {
 		$this->connectLimit = 1;
 		$backend = new StorageBackend($this->storage(), 'base/repo', false, fn (): IStorage => $this->storage());

tests/integration/full-restore-roundtrip.sh

@@ -191,7 +191,7 @@ if [ "$MODE" = sftp ]; then
 	LOGINS=$(( $(sftp_logins) - LOGINS_BEFORE ))
 	[ "$LOGINS" -gt 1 ] || fail "the restore did not read ahead on connections of its own ($LOGINS SFTP logins)"
 	docker exec ngb-frt-restore sh -c 'ls -a /var/www/html/data' | grep -q ngb-restore-spool && fail "the spool folder was left behind"
-	ok "over SFTP: read ahead on $LOGINS connections, large file identical, no spool left"
+	ok "over SFTP: read ahead on connections of its own ($LOGINS SFTP logins during the restore), large file identical, no spool left"
 fi
 
 run_on ngb-frt-restore backup:run offsite -l post-restore-check >/dev/null || fail "a new backup after the restore fails"