From f538b47070caaab370b0619e67696c2ccf6ae9ff Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Sat, 9 May 2026 12:15:18 +0200 Subject: [PATCH] chore(model): slightly improve handling of pulling empty blocks (#10679) In the common case (sparse files enabled, not reusing old data) we'd optimise away pulling & writing zero blocks. However in the corner cases we'd go through the whole processing of pulling the block over the network which is of course entirely unnecessary. Now, instead, always take an optimised path for all-zeroes blocks. In the clean case we do nothing, otherwise we materialise a block of zeroes and write it directly. --------- Signed-off-by: Jakob Borg --- .golangci.yml | 3 ++ lib/model/folder_sendrecv.go | 24 ++++++--- lib/model/folder_sendrecv_test.go | 82 +++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 7 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 9087d1a3d..ceaff8e2b 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -76,6 +76,9 @@ linters: # Don't necessarily rewrite !(foo || bar) to !foo && !bar - linters: [staticcheck] text: QF1001 + # Don't necessarily pass the context just for the sake of the logger + - linters: [contextcheck] + text: "->log`" settings: sloglint: context: "scope" diff --git a/lib/model/folder_sendrecv.go b/lib/model/folder_sendrecv.go index 26f1c0fbc..7d7238b19 100644 --- a/lib/model/folder_sendrecv.go +++ b/lib/model/folder_sendrecv.go @@ -1558,10 +1558,21 @@ func (f *sendReceiveFolder) pullBlock(ctx context.Context, state pullBlockState, return } - if !f.DisableSparseFiles && state.reused == 0 && state.block.IsEmpty() { + if state.block.IsEmpty() { // There is no need to request a block of all zeroes. Pretend we // requested it and handled it correctly. - state.pullDone(state.block) + if state.reused != 0 || f.DisableSparseFiles { + // We are reusing a file (contents apparently weren't all-zeroes + // previously), or sparse files are disabled, so we need to + // actually write the block. + zeroes := make([]byte, state.block.Size) + err = f.limitedWriteAt(ctx, fd, zeroes, state.block.Offset) + } + if err != nil { + state.fail(fmt.Errorf("save: %w", err)) + } else { + state.pullDone(state.block) + } out <- state.sharedPullerState return } @@ -1607,11 +1618,10 @@ loop: } // Verify that the received block matches the desired hash, if not - // try pulling it from another device. - // For receive-only folders, the hash is not SHA256 as it's an - // encrypted hash token. In that case we can't verify the block - // integrity so we'll take it on trust. (The other side can and - // will verify.) + // try pulling it from another device. For receive-encrypted + // folders, the hash is not SHA256 as it's an encrypted hash token. + // In that case we can't verify the block integrity so we'll take it + // on trust. (The other side can and will verify.) if f.Type != config.FolderTypeReceiveEncrypted { lastError = f.verifyBuffer(buf, state.block) } diff --git a/lib/model/folder_sendrecv_test.go b/lib/model/folder_sendrecv_test.go index a84de43be..8bd56b088 100644 --- a/lib/model/folder_sendrecv_test.go +++ b/lib/model/folder_sendrecv_test.go @@ -917,6 +917,88 @@ func TestPullCtxCancel(t *testing.T) { } } +// TestPullEmptyBlock exercises pullBlock for blocks of all zeroes. +// pullBlock distinguishes three cases: +// +// - Sparse files enabled, reused == 0: nothing to write, the offset +// reads as zero anyway because the temp file is sparse-allocated. +// - reused != 0: the temp file is being reused and may already hold +// non-zero data at this offset, so zeroes must be written explicitly. +// - DisableSparseFiles: cannot rely on a sparse hole reading as zero, +// so zeroes must be written explicitly. +func TestPullEmptyBlock(t *testing.T) { + const blockSize = protocol.MinBlockSize + emptyBlock := protocol.BlockInfo{ + Offset: 0, + Size: blockSize, + Hash: blocks[0].Hash, // sha256 of a 128 KiB all-zeroes block + } + if !emptyBlock.IsEmpty() { + t.Fatal("test setup: block not recognised as empty") + } + + cases := []struct { + name string + reused []int + disableSparse bool + preFillTempFile bool + }{ + {name: "sparse-clean"}, + {name: "reused-temp-file", reused: []int{1}, preFillTempFile: true}, + {name: "disable-sparse-files", disableSparse: true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, f := setupSendReceiveFolder(t) + f.folder.FolderConfiguration.DisableSparseFiles = tc.disableSparse + + file := protocol.FileInfo{ + Name: "file", + Size: int64(blockSize), + Permissions: 0o644, + Blocks: []protocol.BlockInfo{emptyBlock}, + } + tempName := fs.TempName(file.Name) + + // Pre-fill the temp file with non-zero data so that a missing + // overwrite would be detectable. + if tc.preFillTempFile { + writeFile(t, f.Filesystem(), tempName, bytes.Repeat([]byte{0xff}, blockSize)) + } + + state := pullBlockState{ + sharedPullerState: newSharedPullerState(file, f.Filesystem(), f.folderID, tempName, file.Blocks, tc.reused, false, false, protocol.FileInfo{}, !tc.disableSparse, false), + block: emptyBlock, + } + + out := make(chan *sharedPullerState, 1) + f.pullBlock(t.Context(), state, out) + + s := <-out + if err := s.failed(); err != nil { + t.Fatalf("pullBlock failed: %v", err) + } + cleanupSharedPullerState(s) + + fd, err := f.Filesystem().Open(tempName) + must(t, err) + defer fd.Close() + data, err := io.ReadAll(fd) + must(t, err) + + if len(data) != blockSize { + t.Fatalf("temp file too short: got %d bytes, want at least %d", len(data), emptyBlock.Offset+int64(blockSize)) + } + for i, b := range data { + if b != 0 { + t.Fatalf("byte %d: got %#x, want 0", i, b) + } + } + }) + } +} + func TestPullDeleteUnscannedDir(t *testing.T) { _, f := setupSendReceiveFolder(t) ffs := f.Filesystem()