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

@@ -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

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

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)
 

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)

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.

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")

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))