From 800a4b8e57ccceb386d4a49d6e249d8b36f81441 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sun, 9 Aug 2026 18:35:04 +0200 Subject: [PATCH] chunkers: only use the direct read path for known regular files The direct read path (big readv directly into the caller's buffer) was seen blocking forever on NetBSD 10 when reading a FIFO via create --read-special (test_create_read_special_symlink hung NetBSD CI deterministically, at the same spot in two runs), while the buffered block reader path's 1 MiB os.read() calls work fine there. Restrict the direct path to known regular files: FileReader only goes direct for S_ISREG modes. chunkify() accepts the file's os.stat_result if the caller has it anyway (process_file does, so the create hot path adds no stat syscall); otherwise FileReader fstats fh / fd.fileno() itself, staying on the buffered path for objects without an OS-level fd. Special files (--read-special) route onto the buffered path via their stat mode, exactly as before this branch. Co-Authored-By: Claude Fable 5 --- src/borg/archive.py | 8 ++- src/borg/chunkers/base.pyx | 8 ++- src/borg/chunkers/buzhash.pyi | 5 +- src/borg/chunkers/buzhash64.pyi | 5 +- src/borg/chunkers/failing.py | 6 +- src/borg/chunkers/fastcdc.pyi | 5 +- src/borg/chunkers/fixed.py | 9 ++- src/borg/chunkers/goldilocks_aes.pyi | 5 +- src/borg/chunkers/rabin_aes.pyi | 5 +- src/borg/chunkers/reader.pyi | 2 + src/borg/chunkers/reader.pyx | 26 +++++++-- src/borg/chunkers/toeplitz_aes.pyi | 5 +- src/borg/testsuite/chunkers/reader_test.py | 66 ++++++++++++++++++++++ 13 files changed, 138 insertions(+), 17 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index 271118ac86..727b6e5cac 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -1593,7 +1593,13 @@ def process_file(self, *, path, parent_fd, name, st, cache, flags=flags_normal, # stored. An unwrapped repository OSError is critical and aborts create before # archive.save() runs (see the BackupOSError docstring). self.process_file_chunks( - item, cache, self.stats, self.show_progress, backup_io_iter(self.chunker.chunkify(None, fd)) + item, + cache, + self.stats, + self.show_progress, + # passing st saves FileReader a stat call; regular files take the + # direct read path, special files (--read-special) the buffered one. + backup_io_iter(self.chunker.chunkify(None, fd, st=st)), ) self.stats.chunking_time = self.chunker.chunking_time end_reading = time.time_ns() diff --git a/src/borg/chunkers/base.pyx b/src/borg/chunkers/base.pyx index d969adb624..89ae15cf7a 100644 --- a/src/borg/chunkers/base.pyx +++ b/src/borg/chunkers/base.pyx @@ -228,7 +228,7 @@ cdef class ChunkerBase: self.bytes_yielded += n return memoryview((self.data + old_last)[:n]) - def chunkify(self, fd, fh=-1, fmap=None): + def chunkify(self, fd, fh=-1, fmap=None, st=None): """ Cut a file into chunks. @@ -236,10 +236,14 @@ cdef class ChunkerBase: :param fh: OS-level file handle (if available), defaults to -1 which means not to use OS-level fd. :param fmap: a file map, same format as generated by sparsemap + :param st: the file's os.stat_result, if the caller has it anyway - + saves FileReader a stat call, see there. """ self._fd = fd self.fh = fh - self.file_reader = FileReader(fd=fd, fh=fh, read_size=self.reader_block_size, sparse=self.sparse, fmap=fmap) + self.file_reader = FileReader( + fd=fd, fh=fh, read_size=self.reader_block_size, sparse=self.sparse, fmap=fmap, st=st + ) self.done = 0 self.remaining = 0 self.bytes_read = 0 diff --git a/src/borg/chunkers/buzhash.pyi b/src/borg/chunkers/buzhash.pyi index e8f1ae0670..f910d17317 100644 --- a/src/borg/chunkers/buzhash.pyi +++ b/src/borg/chunkers/buzhash.pyi @@ -1,3 +1,4 @@ +import os from collections.abc import Iterator from typing import BinaryIO @@ -16,4 +17,6 @@ class Chunker: hash_window_size: int, sparse: bool = False, ) -> None: ... - def chunkify(self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None) -> Iterator: ... + def chunkify( + self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None, st: os.stat_result = None + ) -> Iterator: ... diff --git a/src/borg/chunkers/buzhash64.pyi b/src/borg/chunkers/buzhash64.pyi index aa40dc1d5d..7cd9f4b7b9 100644 --- a/src/borg/chunkers/buzhash64.pyi +++ b/src/borg/chunkers/buzhash64.pyi @@ -1,3 +1,4 @@ +import os from collections.abc import Iterator from typing import BinaryIO @@ -19,4 +20,6 @@ class ChunkerBuzHash64: normal_size: int = 0, sparse: bool = False, ) -> None: ... - def chunkify(self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None) -> Iterator: ... + def chunkify( + self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None, st: os.stat_result = None + ) -> Iterator: ... diff --git a/src/borg/chunkers/failing.py b/src/borg/chunkers/failing.py index 5dc1755476..86a8292837 100644 --- a/src/borg/chunkers/failing.py +++ b/src/borg/chunkers/failing.py @@ -27,13 +27,17 @@ def __init__(self, block_size: int, map: str) -> None: self.count = 0 self.chunking_time = 0.0 # not updated, just provided so that caller does not crash - def chunkify(self, fd: BinaryIO | None = None, fh: int = -1) -> Iterator: + def chunkify( + self, fd: BinaryIO | None = None, fh: int = -1, fmap: list | None = None, st: os.stat_result | None = None + ) -> Iterator: """ Cut a file into chunks. :param fd: Python file object :param fh: OS-level file handle (if available), defaults to -1 which means not to use OS-level fd. + :param fmap: ignored + :param st: ignored """ use_fh = fh >= 0 wanted = self.block_size diff --git a/src/borg/chunkers/fastcdc.pyi b/src/borg/chunkers/fastcdc.pyi index b9fb63d0e1..085489b28c 100644 --- a/src/borg/chunkers/fastcdc.pyi +++ b/src/borg/chunkers/fastcdc.pyi @@ -1,3 +1,4 @@ +import os from collections.abc import Iterator from typing import BinaryIO @@ -16,4 +17,6 @@ class ChunkerFastCDC: normal_size: int = 0, sparse: bool = False, ) -> None: ... - def chunkify(self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None) -> Iterator: ... + def chunkify( + self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None, st: os.stat_result = None + ) -> Iterator: ... diff --git a/src/borg/chunkers/fixed.py b/src/borg/chunkers/fixed.py index 5db28ed635..944ca3d825 100644 --- a/src/borg/chunkers/fixed.py +++ b/src/borg/chunkers/fixed.py @@ -2,6 +2,7 @@ from typing import BinaryIO +import os import time from .reader import FileReader, Chunk @@ -42,7 +43,9 @@ def __init__(self, block_size: int, header_size: int = 0, sparse: bool = False) self.reader: FileReader | None = None self.sparse = sparse - def chunkify(self, fd: BinaryIO | None = None, fh: int = -1, fmap: list | None = None) -> Iterator: + def chunkify( + self, fd: BinaryIO | None = None, fh: int = -1, fmap: list | None = None, st: os.stat_result | None = None + ) -> Iterator: """ Cut a file into chunks. @@ -50,9 +53,11 @@ def chunkify(self, fd: BinaryIO | None = None, fh: int = -1, fmap: list | None = :param fh: OS-level file handle (if available), defaults to -1 which means not to use OS-level fd. :param fmap: a file map, same format as generated by sparsemap + :param st: the file's os.stat_result, if the caller has it anyway - + saves FileReader a stat call, see there. """ # Initialize the reader with the file descriptors - self.reader = FileReader(fd=fd, fh=fh, read_size=self.reader_block_size, sparse=self.sparse, fmap=fmap) + self.reader = FileReader(fd=fd, fh=fh, read_size=self.reader_block_size, sparse=self.sparse, fmap=fmap, st=st) # Read the header block first, if there is one; after it, read block_size sized blocks. wanted = self.header_size if self.header_size > 0 else self.block_size diff --git a/src/borg/chunkers/goldilocks_aes.pyi b/src/borg/chunkers/goldilocks_aes.pyi index aa6c29aaf0..0b9a35357d 100644 --- a/src/borg/chunkers/goldilocks_aes.pyi +++ b/src/borg/chunkers/goldilocks_aes.pyi @@ -1,3 +1,4 @@ +import os from collections.abc import Iterator from typing import BinaryIO @@ -22,4 +23,6 @@ class ChunkerGoldilocksAES: normal_size: int = 0, sparse: bool = False, ) -> None: ... - def chunkify(self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None) -> Iterator: ... + def chunkify( + self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None, st: os.stat_result = None + ) -> Iterator: ... diff --git a/src/borg/chunkers/rabin_aes.pyi b/src/borg/chunkers/rabin_aes.pyi index ee52b1a7cb..928f240955 100644 --- a/src/borg/chunkers/rabin_aes.pyi +++ b/src/borg/chunkers/rabin_aes.pyi @@ -1,3 +1,4 @@ +import os from collections.abc import Iterator from typing import BinaryIO @@ -19,4 +20,6 @@ class ChunkerRabinAES: normal_size: int = 0, sparse: bool = False, ) -> None: ... - def chunkify(self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None) -> Iterator: ... + def chunkify( + self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None, st: os.stat_result = None + ) -> Iterator: ... diff --git a/src/borg/chunkers/reader.pyi b/src/borg/chunkers/reader.pyi index 7870952f61..83337abb7c 100644 --- a/src/borg/chunkers/reader.pyi +++ b/src/borg/chunkers/reader.pyi @@ -1,3 +1,4 @@ +import os from collections.abc import Iterator from typing import Any, BinaryIO, NamedTuple @@ -36,6 +37,7 @@ class FileReader: read_size: int = 0, sparse: bool = False, fmap: list[fmap_entry] = None, + st: os.stat_result = None, ) -> None: ... def _fill_buffer(self) -> bool: ... def _readinto_direct(self, tv: memoryview, size: int) -> int: ... diff --git a/src/borg/chunkers/reader.pyx b/src/borg/chunkers/reader.pyx index f6866de03f..dccfbc9129 100644 --- a/src/borg/chunkers/reader.pyx +++ b/src/borg/chunkers/reader.pyx @@ -4,6 +4,7 @@ import os import errno +import stat import time from collections import namedtuple @@ -227,7 +228,7 @@ class FileReader: Most complexity in here comes from the desired size when a user calls FileReader.read does not need to match the Chunk sizes we got from the FileFMAPReader. """ - def __init__(self, *, fd=None, fh=-1, read_size=0, sparse=False, fmap=None): + def __init__(self, *, fd=None, fh=-1, read_size=0, sparse=False, fmap=None, st=None): assert read_size > 0 self.reader = FileFMAPReader(fd=fd, fh=fh, read_size=read_size, sparse=sparse, fmap=fmap) self.buffer = [] # list of Chunk objects @@ -241,7 +242,22 @@ class FileReader: # consider - the file is read start to end. readinto() then reads directly # from the file into the caller's buffer (see there), instead of going # through the block reader. - self.direct = not sparse and fmap is None + # Only known regular files take the direct path: reading FIFOs / devices via + # --read-special must keep using the proven block reader path - e.g. NetBSD 10 + # was seen blocking forever in the direct path's big readv() on a FIFO, where + # the block reader's 1 MiB os.read() calls work fine. + self.direct = False + if not sparse and fmap is None: + if st is None: + # caller did not have a stat result at hand - determine the file type here. + try: + if fh >= 0: + st = os.fstat(fh) + elif fd is not None: + st = os.fstat(fd.fileno()) + except (AttributeError, OSError): + pass # no OS-level fd (e.g. BytesIO, wrapper objects) - stay on the block reader path + self.direct = st is not None and stat.S_ISREG(st.st_mode) self.direct_offset = 0 # bytes read so far via the direct path (for fadvise) def _fill_buffer(self): @@ -399,9 +415,9 @@ class FileReader: Read up to 'size' bytes from the file directly into 'target' (a writable buffer, e.g. a memoryview over the caller's scan buffer). - Fast path (no sparse processing, no fmap given): the file data is read - by the OS directly into 'target' - zero copies in user space and one - syscall per request instead of one per block. + Fast path (known regular file, no sparse processing, no fmap given): + the file data is read by the OS directly into 'target' - zero copies + in user space and one syscall per request instead of one per block. Otherwise, unlike read(), this does not allocate or combine intermediate byte objects: each byte is copied exactly once, from the buffered file diff --git a/src/borg/chunkers/toeplitz_aes.pyi b/src/borg/chunkers/toeplitz_aes.pyi index 82096514ed..4016f34ea9 100644 --- a/src/borg/chunkers/toeplitz_aes.pyi +++ b/src/borg/chunkers/toeplitz_aes.pyi @@ -1,3 +1,4 @@ +import os from collections.abc import Iterator from typing import BinaryIO @@ -21,4 +22,6 @@ class ChunkerToeplitzAES: normal_size: int = 0, sparse: bool = False, ) -> None: ... - def chunkify(self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None) -> Iterator: ... + def chunkify( + self, fd: BinaryIO = None, fh: int = -1, fmap: list[fmap_entry] = None, st: os.stat_result = None + ) -> Iterator: ... diff --git a/src/borg/testsuite/chunkers/reader_test.py b/src/borg/testsuite/chunkers/reader_test.py index dfebc24b0e..b4bf3a9981 100644 --- a/src/borg/testsuite/chunkers/reader_test.py +++ b/src/borg/testsuite/chunkers/reader_test.py @@ -344,3 +344,69 @@ def test_filefmapreader_build_fmap(): assert fmap[0][0] == 0 # start assert fmap[0][1] == 2**62 # size assert fmap[0][2] is True # is_data + + +def test_filereader_readinto_direct(tmpdir): + """readinto uses the direct read path for a known regular file (st given).""" + fn = str(tmpdir / "regular_file") + file_content = b"data1234" * 100 + with open(fn, "wb") as fd: + fd.write(file_content) + + fh = os.open(fn, os.O_RDONLY) + try: + st = os.fstat(fh) + reader = FileReader(fd=None, fh=fh, read_size=1024, sparse=False, fmap=None, st=st) + assert reader.direct # regular file, no sparse, no fmap -> direct read path + target = bytearray(len(file_content) + 42) + got = reader.readinto(target, len(file_content) + 42) + assert got == len(file_content) + assert target[:got] == file_content + assert reader.readinto(target, 42) == 0 # EOF + finally: + os.close(fh) + + +def test_filereader_direct_without_st(tmpdir): + """Without a given stat result, FileReader stats the file itself: regular file -> direct.""" + fn = str(tmpdir / "regular_file") + file_content = b"data1234" + with open(fn, "wb") as fd: + fd.write(file_content) + + # OS-level file handle, no st given + fh = os.open(fn, os.O_RDONLY) + try: + reader = FileReader(fd=None, fh=fh, read_size=1024, sparse=False, fmap=None) + assert reader.direct # fstat(fh) says regular file + finally: + os.close(fh) + + # Python file object, no st given + with open(fn, "rb") as fd: + reader = FileReader(fd=fd, fh=-1, read_size=1024, sparse=False, fmap=None) + assert reader.direct # fstat(fd.fileno()) says regular file + target = bytearray(8) + assert reader.readinto(target, 8) == 8 + assert bytes(target) == file_content + + +def test_filereader_no_direct_without_os_fd(): + """A file-like object without an OS-level fd must use the buffered block reader path.""" + file_content = b"data1234" + reader = FileReader(fd=BytesIO(file_content), fh=-1, read_size=1024, sparse=False, fmap=None) + assert not reader.direct # no OS-level fd, unknown file type -> buffered path + target = bytearray(8) + assert reader.readinto(target, 8) == 8 + assert bytes(target) == file_content + + +def test_filereader_no_direct_for_fifo(tmpdir): + """A FIFO's stat result must not enable the direct read path (hung on NetBSD).""" + if not hasattr(os, "mkfifo"): + pytest.skip("no FIFOs on this platform") + fifo_fn = str(tmpdir / "fifo") + os.mkfifo(fifo_fn) + st = os.stat(fifo_fn) + reader = FileReader(fd=BytesIO(b""), fh=-1, read_size=1024, sparse=False, fmap=None, st=st) + assert not reader.direct