From c95f552cc6a321ecd00fa187d3360ebfb3675f8d Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 07:46:12 +0200 Subject: [PATCH] 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 +++++----- README.md | 7 ++++--- lib/Backend/SftpRangeFetcher.php | 20 ++++++++++++------- lib/Backend/StorageBackend.php | 7 +++++-- lib/Service/TargetService.php | 5 ++++- tests/Unit/Backend/SftpRangeFetcherTest.php | 4 ++-- .../Backend/StorageBackendReadAheadTest.php | 14 ++++++++++++- tests/integration/full-restore-roundtrip.sh | 2 +- 8 files changed, 47 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5496a03..a3fdc86 100644 --- a/CHANGELOG.md +++ b/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 - flight on each, into temporary files in the restore folder. From a Hetzner Storage Box to a - server in Canada (122 ms) packs now come in at about 7.6 MiB/s instead of 2.8; on a test line - with 120 ms and some packet loss, a 1.6 GiB restore went from 5.1 to 17.8 MiB/s. A part that - cannot be read this way is read as before. Restores from other locations work as before. +- A full restore over SFTP reads the packs ahead on a connection of its own, 32 requests in + flight, into temporary files in the restore folder. From a Hetzner Storage Box to a server in + Canada (122 ms) packs now come in at about 14 MiB/s instead of 2.8. More connections can be set + with `restore_sftp_connections` (README). A part that cannot be read this way is read as + before. Restores from other locations work as before. ### Fixed - After a full restore stopped halfway, running it again with `--force` was refused when the diff --git a/README.md b/README.md index 79e8258..db2595c 100644 --- a/README.md +++ b/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 - NG Backup already has open: the SFTP server must allow 5 connections at once for that user (a - Hetzner Storage Box allows 10). With fewer it uses the connections it gets. + A full restore over SFTP reads the packs ahead on a second connection, with many requests in + flight. A server that handles parallel reads well (not a Hetzner Storage Box, which gets slower) + 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 diff --git a/lib/Backend/SftpRangeFetcher.php b/lib/Backend/SftpRangeFetcher.php index 153b13e..74651b5 100644 --- a/lib/Backend/SftpRangeFetcher.php +++ b/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 - * line: a restore from a Hetzner Storage Box to Canada (RTT 122 ms) reached about 2 MiB/s, with - * the PHP process mostly waiting. Here every connection reads its own range with DEPTH requests - * in flight. PHP runs one thing at a time: while it waits for an answer on one connection, the + * A restore that reads its packs as it goes is bound by the round trip: from a Hetzner Storage + * Box to Canada (RTT 122 ms) it reached about 2.8 MiB/s, with the PHP process mostly waiting. + * 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 * 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; /** diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index 24ee719..ec5897d 100644 --- a/lib/Backend/StorageBackend.php +++ b/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); diff --git a/lib/Service/TargetService.php b/lib/Service/TargetService.php index b0e914e..abb8989 100644 --- a/lib/Service/TargetService.php +++ b/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; diff --git a/tests/Unit/Backend/SftpRangeFetcherTest.php b/tests/Unit/Backend/SftpRangeFetcherTest.php index b27e8ff..51dedc1 100644 --- a/tests/Unit/Backend/SftpRangeFetcherTest.php +++ b/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); diff --git a/tests/Unit/Backend/StorageBackendReadAheadTest.php b/tests/Unit/Backend/StorageBackendReadAheadTest.php index 5bf7b7c..87818b2 100644 --- a/tests/Unit/Backend/StorageBackendReadAheadTest.php +++ b/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()); diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index 0d5139e..7b65ed7 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/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"