fix(s3): use recursive listing to avoid excessive API calls by 0byte-coding · Pull Request #313 · longhorn/backupstore

0byte-coding · 2026-07-17T20:24:08Z

Which issue(s) this PR fixes:

What this PR does / why we need it:

getBlockNamesForVolume and getAllBlockNames walk the 2-level sharded block directory tree with one driver.List() call per directory, issuing up to 256*256 List calls per volume. On backends that bill per API request (e.g. Backblaze B2 Class C transactions), this is the main driver of the excessive call counts reported in longhorn/longhorn#1547 (one user measured up to 65,000 requests per backup).

This PR adds an optional RecursiveLister interface that a driver can implement to provide a native flat/recursive listing:

s3: ListRecursive uses ListObjectsV2 without a delimiter, returning every key under a prefix via normal pagination (1 API call per 1000 objects) instead of one call per directory.
fsops (nfs/cifs/vfs): ListRecursive walks the local mount in a single process pass instead of one List() round-trip per directory level.

getBlockNamesForVolume / getAllBlockNames prefer the recursive path when the driver supports it, and fall back to the original walk-based implementation otherwise, so drivers that don't implement RecursiveLister (e.g. azblob) keep working unchanged.

Special notes for your reviewer:

End-to-end verified, not just unit tested. Built this patch into a full longhorn engine CLI binary (same binary longhorn-manager execs for backup rm/GC) against a real k3s + Longhorn v1.10.1 cluster and a real Backblaze B2 bucket with valid, working credentials. Ran backup rm on an identical 135-block backup with both the stock and patched binary, capturing every HTTP request via a local MITM proxy:

	Stock binary	Patched binary	Change
Total S3 requests	534	296	-44.6%
Directory-listing GET calls	243	5	-97.9%
GC wall time	~24s	~13s	-46%

Both runs produced identical, correct results (Removed 135 unused blocks, bucket left with only volume.cfg) - confirming no functional regression. The 5 remaining directory-listing calls (root check, locks/, backups/ x2, blocks/) are legitimate; the blocks/ listing that used to cost 243 nested calls is now a single ListRecursive call. The other ~137 GET calls in both runs are unrelated per-block existence checks (FileSize/HeadObject) done during GC bookkeeping, outside the scope of this fix.

Unit tests (list_recursive_test.go, fsops/list_recursive_test.go) cover the recursive listing path and the "directory doesn't exist" fallback case. Pre-existing test failures (TestInspectBackup, NFS mount tests requiring superuser) are unrelated to this change and reproduce identically on unmodified master.

Additional documentation or context

See discussion in longhorn/longhorn#1547, particularly joshimoo's analysis of the O(nodes) * O(vol * 4) call cost and sedlund's independent proof-of-concept for the same recursive-listing approach.

christophersherman · 2026-08-02T17:32:44Z

@0byte-coding This looks directly relevant to a current production failure mode also covered by longhorn/longhorn#8060: a volume with roughly 1.4 million block objects can spend longer than an hourly backup interval in retention GC while the deletion lock prevents the next backup from starting.

A source audit confirms that current backupstore still walks the two-level block shard tree serially before serial per-block cleanup, so this recursive-listing change is the right first optimization and avoids changing deletion semantics. The DCO check is currently ACTION_REQUIRED; could you sign off and update the commit?

@mantissahz Since #8060 is assigned to you and marked priority/0, could you help route review once DCO is green? Per-block removal may merit a separately benchmarked bounded-concurrency follow-up, but #313 already removes the largest fixed LIST amplification.

Copilot

Pull request overview

Adds recursive block listing to reduce costly S3 API requests while retaining fallback behavior for unsupported drivers.

Changes:

Introduces the optional RecursiveLister interface.
Implements recursive listing for S3 and filesystem drivers.
Uses recursive listing for volume and backing-image block discovery.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`driver.go`	Defines the recursive listing extension.
`s3/s3.go`	Adds flat, paginated S3 listing.
`fsops/fsops.go`	Adds filesystem tree traversal.
`deltablock.go`	Uses recursive volume block discovery.
`backupbackingimage/config.go`	Uses recursive backing-image block discovery.
`list_recursive_test.go`	Tests recursive volume listing.
`fsops/list_recursive_test.go`	Tests filesystem recursive listing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+	for _, b := range blocks {
+		assert.NoError(afero.WriteFile(m.fs, b, []byte("data"), 0644))
+	}


+		paths, err := recDriver.ListRecursive(blockPathBase)
+		if err != nil {
+			// Directory doesn't exist
+			return []string{}, nil
+		}


+		paths, err := recDriver.ListRecursive(blockPathBase)
+		if err != nil {
+			// Directory doesn't exist
+			return []string{}, nil
+		}


+	base := f.LocalPath(path)
+	var result []string
+
+	err := filepath.Walk(base, func(p string, info os.FileInfo, err error) error {


getBlockNamesForVolume and getAllBlockNames walked the 2-level sharded block directory tree with one driver.List() call per directory level, issuing up to 256*256 List calls per volume. On backends that bill per API request (e.g. Backblaze B2 Class C transactions), this translates directly into runaway costs, as reported repeatedly in longhorn/longhorn#1547 (up to 65,000 requests per backup according to one user). Add an optional RecursiveLister interface that drivers can implement to provide a native flat/recursive listing: - s3: ListRecursive uses ListObjectsV2 without a delimiter, returning every key under a prefix via normal pagination (1 API call per 1000 objects) instead of one call per directory. - vfs: ListRecursive walks the local mount in a single process pass instead of one List() round-trip per directory level. nfs and cifs deliberately do NOT get this via fsops.FileSystemOperator embedding: filepath.Walk over a network mount still issues one remote metadata round-trip per directory entry, so it could turn one round-trip per directory level into one round-trip per block - worse than the List()-per-directory traversal it would replace on backends with very large block counts. Renamed the underlying fsops helper to ListRecursiveLocal to make this an explicit opt-in per driver rather than an accidental promotion. getBlockNamesForVolume and getAllBlockNames now prefer the recursive path when the driver supports it, falling back to the original walk-based implementation otherwise (azblob and any future drivers that don't implement RecursiveLister keep working unchanged). A missing directory is not an error - both the s3 and fsops implementations already return (nil, nil) for a prefix/path that doesn't exist - so any error ListRecursive does return is a genuine failure (auth, network, pagination, etc.) and is now propagated to the caller instead of being silently treated as "no blocks found", which previously risked GC persisting BlockCount = 0 for a volume whose blocks simply failed to list. Signed-off-by: 0byte <git@susnext.com>

0byte-coding · 2026-08-04T05:43:34Z

Thanks for the confirmation and the pointer to #8060 - that lines up with what I found.

Force-pushed with:

DCO sign-off (git commit -s) - now green.
Addressed all 4 Copilot review comments:
1. list_recursive_test.go: made shard directory creation explicit instead of relying on afero.MemMapFs auto-vivifying parent dirs (an implementation detail of that specific in-memory fake, not something afero.WriteFile itself guarantees).
2. deltablock.go: getBlockNamesForVolume now propagates genuine ListRecursive errors instead of treating every error as "directory doesn't exist" - both the s3 and fsops implementations already return (nil, nil) for a missing prefix/path, so any error is a real failure (auth/network/pagination) that should abort GC rather than silently persisting BlockCount = 0.
3. backupbackingimage/config.go: same fix applied to getAllBlockNames.
4. fsops/fsops.go: renamed the walk-based helper to ListRecursiveLocal and stopped it being promoted automatically via embedding. Only vfs (a plain local directory) now opts into RecursiveLister explicitly; nfs/cifs (network mounts) deliberately do not, since filepath.Walk still issues one remote metadata round-trip per directory entry there - it could turn one round-trip per directory level into one round-trip per block on a volume with a large number of blocks. Added compile-time interface-assertion tests locking this in for all three drivers.

Also added a regression test for point 2 (TestGetBlockNamesForVolumePropagatesRecursiveListError).

Agreed per-block removal in GC (the DELETE side) looks like a good separately-benchmarked follow-up - this PR only touches the LIST side.

0byte-coding mentioned this pull request Jul 17, 2026

fix(backup-target): sanitize volatile IDs from status condition messages longhorn/longhorn-manager#5030

Open

0byte-coding force-pushed the fix-excessive-s3-calls branch 2 times, most recently from 0f25306 to ca35336 Compare July 24, 2026 16:45

This was referenced Jul 24, 2026

docs(settings): document Backupstore Poll Interval cost implications longhorn/website#1407

Open

[BUG] Excessive s3 call count longhorn/longhorn#1547

Open

christophersherman mentioned this pull request Aug 2, 2026

fix(s3): delete unused blocks with bounded concurrency #315

Open

mantissahz assigned christophersherman Aug 3, 2026

mantissahz requested review from a team, Copilot and mantissahz August 3, 2026 02:04

Copilot started reviewing on behalf of mantissahz August 3, 2026 02:05 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

0byte-coding force-pushed the fix-excessive-s3-calls branch from ca35336 to 9813edd Compare August 4, 2026 05:42

Navigation Menu

Uh oh!

fix(s3): use recursive listing to avoid excessive API calls - #313

fix(s3): use recursive listing to avoid excessive API calls#313
0byte-coding wants to merge 1 commit into
longhorn:masterfrom
0byte-coding:fix-excessive-s3-calls

0byte-coding commented Jul 17, 2026 •

edited

Loading

Uh oh!

christophersherman commented Aug 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

0byte-coding commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

0byte-coding commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue(s) this PR fixes:

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

Uh oh!

christophersherman commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

0byte-coding commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

0byte-coding commented Jul 17, 2026 •

edited

Loading

christophersherman commented Aug 2, 2026 •

edited

Loading