From b048b22daffcbd9834f1cb3d8ff069b7273b366e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julio=20L=C3=B3pez?= <1953782+julio-lopez@users.noreply.github.com> Date: Fri, 26 Jun 2026 22:07:01 -0700 Subject: [PATCH] refactor(general): cleanup `iomem` hints (#5439) - rename package to iomem - update comments - unexport `directoryWithOptions` - check for `Close()` error on test cleanup - reword flag description - wrap errors to facilitate troubleshooting - update logging message --- cli/command_snapshot_create.go | 2 +- fs/localfs/local_fs.go | 37 ++++++++----------- fs/localfs/local_fs_os.go | 4 +- fs/localfs/local_fs_test.go | 2 +- .../iomem_linux.go} | 12 ++++-- .../iomem_others.go} | 6 +-- .../pagecache_test.go => iomem/iomem_test.go} | 11 ++++-- 7 files changed, 38 insertions(+), 36 deletions(-) rename internal/{pagecache/pagecache_linux.go => iomem/iomem_linux.go} (84%) rename internal/{pagecache/pagecache_others.go => iomem/iomem_others.go} (75%) rename internal/{pagecache/pagecache_test.go => iomem/iomem_test.go} (73%) diff --git a/cli/command_snapshot_create.go b/cli/command_snapshot_create.go index f2aee07c8..9d9f5d498 100644 --- a/cli/command_snapshot_create.go +++ b/cli/command_snapshot_create.go @@ -78,7 +78,7 @@ func (c *commandSnapshotCreate) setup(svc appServices, parent commandParent) { cmd.Flag("flush-per-source", "Flush writes at the end of each source").Hidden().BoolVar(&c.flushPerSource) cmd.Flag("override-source", "Override the source of the snapshot.").StringVar(&c.sourceOverride) cmd.Flag("send-snapshot-report", "Send a snapshot report notification using configured notification profiles").Default("true").BoolVar(&c.sendSnapshotReport) - cmd.Flag("hint-streaming-reads", "Release file memory after reading to reduce backup memory footprint (Linux only, best-effort)."). + cmd.Flag("hint-streaming-reads", "[EXPERIMENTAL] Hint the OS to release memory used for I/O after reading files that are being backed up, aiming at reducing the memory footprint during backups (Linux only, best-effort)."). Default("false").Hidden().BoolVar(&c.snapshotCreateStreamingReads) c.logDirDetail = -1 diff --git a/fs/localfs/local_fs.go b/fs/localfs/local_fs.go index a6c2715ca..53810de2d 100644 --- a/fs/localfs/local_fs.go +++ b/fs/localfs/local_fs.go @@ -9,7 +9,7 @@ "github.com/pkg/errors" "github.com/kopia/kopia/fs" - "github.com/kopia/kopia/internal/pagecache" + "github.com/kopia/kopia/internal/iomem" "github.com/kopia/kopia/repo/logging" ) @@ -18,12 +18,13 @@ const numEntriesToRead = 100 // number of directory entries to read in one shot // Options configures behavior of a local filesystem entry tree created by -// NewEntryWithOptions or DirectoryWithOptions. The zero value matches -// upstream kopia behavior. +// NewEntryWithOptions. The zero value means default behavior. type Options struct { - // StreamingReads, when true, hints the Linux kernel page cache for - // files opened through this entry tree: HintStreaming at open and - // HintNotNeeded at close. Advisory/best-effort; no effect on non-Linux. + // StreamingReads indicates whether to hint the OS regarding how to cache + // files opened through this entry (tree). The option is propagated to all + // the children in a subtree. + // HintStreaming is issued at Open() and HintNotNeeded at Close(). + // These hints are advisory-only and only effective on Linux. StreamingReads bool } @@ -108,8 +109,7 @@ func (fsd *filesystemDirectory) Size() int64 { type fileWithMetadata struct { *os.File - // opts is a snapshot of the parent entry's Options at Open() time, so - // Close() applies the same hint policy the file was opened with. + // opts may be used for operations on the file, including Close(). opts Options } @@ -127,11 +127,9 @@ func (f *fileWithMetadata) Entry() (fs.Entry, error) { func (f *fileWithMetadata) Close() error { if f.opts.StreamingReads { // HintNotNeeded is advisory and best-effort; failures don't affect - // correctness. The error is intentionally dropped here because - // fs.Reader.Close() has no ctx, and capturing a ctx-derived logger - // on the struct would mirror the contained-ctx anti-pattern. - // Open-time hint failures are still logged via the real ctx. - _ = pagecache.HintNotNeeded(f.File) + // correctness. The error is intentionally ignored and not logged given + // that Close() has no ctx to retrieve a ctx-derived logger. + _ = iomem.HintNotNeeded(f.File) } if err := f.File.Close(); err != nil { @@ -149,11 +147,9 @@ func (fsf *filesystemFile) Open(ctx context.Context) (fs.Reader, error) { // In streaming-reads mode, hint the kernel for readahead at open // (HintStreaming) and to drop the pages at close (HintNotNeeded). - // Incremental drops during reads were tested and removed — they add - // ~15% overhead by fighting the kernel's own LRU/readahead. if fsf.opts.StreamingReads { - if hintErr := pagecache.HintStreaming(f); hintErr != nil { - log(ctx).Debugf("page cache hint at open failed for %q: %v", f.Name(), hintErr) + if hintErr := iomem.HintStreaming(f); hintErr != nil { + log(ctx).Debugf("streaming read hint at open failed for %q: %v", f.Name(), hintErr) } } @@ -192,12 +188,11 @@ func splitDirPrefix(s string) (basename, prefix string) { // Directory returns fs.Directory for the specified path with default Options. func Directory(path string) (fs.Directory, error) { - return DirectoryWithOptions(path, Options{}) + return directoryWithOptions(path, Options{}) } -// DirectoryWithOptions behaves like Directory but configures the returned -// directory (and its descendants) with opts. -func DirectoryWithOptions(path string, opts Options) (fs.Directory, error) { +// directoryWithOptions configures the returned directory (and its descendants) with opts. +func directoryWithOptions(path string, opts Options) (fs.Directory, error) { e, err := NewEntryWithOptions(path, opts) if err != nil { return nil, err diff --git a/fs/localfs/local_fs_os.go b/fs/localfs/local_fs_os.go index 681df714a..8ed4fb222 100644 --- a/fs/localfs/local_fs_os.go +++ b/fs/localfs/local_fs_os.go @@ -136,8 +136,8 @@ func NewEntry(path string) (fs.Entry, error) { return NewEntryWithOptions(path, Options{}) } -// NewEntryWithOptions behaves like NewEntry but configures the returned -// entry (and any child entries produced by iterating it) with opts. +// NewEntryWithOptions returns fs.Entry for the specified path and configures +// the returned entry (and any child entries produced by iterating it) with opts. func NewEntryWithOptions(path string, opts Options) (fs.Entry, error) { path = filepath.Clean(path) diff --git a/fs/localfs/local_fs_test.go b/fs/localfs/local_fs_test.go index f43b67919..f8d11063d 100644 --- a/fs/localfs/local_fs_test.go +++ b/fs/localfs/local_fs_test.go @@ -311,7 +311,7 @@ func TestStreamingReads(t *testing.T) { }) t.Run("propagates to children via Iterate and Child", func(t *testing.T) { - dir, err := DirectoryWithOptions(tmp, Options{StreamingReads: true}) + dir, err := directoryWithOptions(tmp, Options{StreamingReads: true}) require.NoError(t, err) fsd, ok := dir.(*filesystemDirectory) diff --git a/internal/pagecache/pagecache_linux.go b/internal/iomem/iomem_linux.go similarity index 84% rename from internal/pagecache/pagecache_linux.go rename to internal/iomem/iomem_linux.go index 70a1fbe8c..e0decfae3 100644 --- a/internal/pagecache/pagecache_linux.go +++ b/internal/iomem/iomem_linux.go @@ -1,8 +1,8 @@ //go:build linux -// Package pagecache provides OS-specific helpers to advise the kernel +// Package iomem provides OS-specific helpers to advise the kernel // about page cache usage for accessed files. -package pagecache +package iomem import ( "os" @@ -15,17 +15,21 @@ // once. Call it immediately after opening a file for backup so the hint // lands before any reads populate the page cache. func HintStreaming(f *os.File) error { - return callWithFd(f, func(fd int) error { + err := callWithFd(f, func(fd int) error { return unix.Fadvise(fd, 0, 0, unix.FADV_SEQUENTIAL) }) + + return errors.Wrap(err, "FADV_SEQUENTIAL hint for streaming file reads failed") } // HintNotNeeded advises the kernel that cached pages for this file are no // longer needed and can be reclaimed. Call it after reading is done. func HintNotNeeded(f *os.File) error { - return callWithFd(f, func(fd int) error { + err := callWithFd(f, func(fd int) error { return unix.Fadvise(fd, 0, 0, unix.FADV_DONTNEED) }) + + return errors.Wrap(err, "FADV_DONTNEED hint for releasing file I/O memory failed") } // callWithFd runs op against f's underlying file descriptor. diff --git a/internal/pagecache/pagecache_others.go b/internal/iomem/iomem_others.go similarity index 75% rename from internal/pagecache/pagecache_others.go rename to internal/iomem/iomem_others.go index d0bec8c86..e2efe3eac 100644 --- a/internal/pagecache/pagecache_others.go +++ b/internal/iomem/iomem_others.go @@ -1,8 +1,8 @@ //go:build !linux -// Package pagecache provides OS-specific helpers to advise the kernel +// Package iomem provides OS-specific helpers to advise the kernel // about page cache usage for accessed files. -package pagecache +package iomem import ( "os" @@ -11,7 +11,7 @@ ) // HintStreaming is a no-op on non-Linux builds. It errors on a nil file to -// match the Linux implementation's contract (callWithFd in pagecache_linux.go). +// match the Linux implementation's contract (callWithFd in iomem_linux.go). func HintStreaming(f *os.File) error { if f == nil { return errors.New("nil file") diff --git a/internal/pagecache/pagecache_test.go b/internal/iomem/iomem_test.go similarity index 73% rename from internal/pagecache/pagecache_test.go rename to internal/iomem/iomem_test.go index 4792c22a9..9fe7f704b 100644 --- a/internal/pagecache/pagecache_test.go +++ b/internal/iomem/iomem_test.go @@ -1,24 +1,27 @@ -package pagecache +package iomem import ( "os" "path/filepath" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// TestPageCacheHints exercises the happy path. On Linux it issues real +// TestIOMemHints exercises the happy path. On Linux it issues real // FADV_SEQUENTIAL / FADV_DONTNEED syscalls; on other OSes it confirms the // stubs accept a real *os.File without erroring. No build tag, so it runs // on every platform. -func TestPageCacheHints(t *testing.T) { +func TestIOMemHints(t *testing.T) { p := filepath.Join(t.TempDir(), "f") require.NoError(t, os.WriteFile(p, []byte("x"), 0o644)) f, err := os.Open(p) require.NoError(t, err) - t.Cleanup(func() { f.Close() }) + + // require cannot be used inside Cleanup because it calls t.Fatal()/t.FailNow() + t.Cleanup(func() { assert.NoError(t, f.Close()) }) require.NoError(t, HintStreaming(f)) require.NoError(t, HintNotNeeded(f))