From 65e3e2e768aa1f1820d5d15235c86940a6f3aef4 Mon Sep 17 00:00:00 2001 From: Santhi Prakash <38608178+santhiprakash@users.noreply.github.com> Date: Thu, 1 Oct 2026 03:52:51 +0530 Subject: [PATCH] fix(agent): count ZFS snapshot reads in pool I/O stats (#2474) --- agent/zfs/zfs_linux.go | 82 ++++++++++++++++++++++-- agent/zfs/zfs_linux_test.go | 121 ++++++++++++++++++++++++++++++++++++ 2 files changed, 198 insertions(+), 5 deletions(-) diff --git a/agent/zfs/zfs_linux.go b/agent/zfs/zfs_linux.go index b5a25779d..b297678a5 100644 --- a/agent/zfs/zfs_linux.go +++ b/agent/zfs/zfs_linux.go @@ -18,6 +18,11 @@ var ( devZfsPath = "/dev/zfs" ) +// errNoPoolIOStats marks an "iostats" file that predates OpenZFS 2.3 and +// reports no pool read/write counters (TRIM statistics only), so callers may +// fall back to older counter sources. +var errNoPoolIOStats = errors.New("no pool I/O counters") + func ARCSize() (uint64, error) { file, err := os.Open(filepath.Join(procZfsPath, "arcstats")) if err != nil { @@ -61,7 +66,7 @@ func checkZfsDevice() error { // ZFS collector and avoid keeping a `zpool iostat` subprocess alive. func PoolKernelStats() ([]PoolKernelStat, error) { poolDirs := make(map[string]struct{}) - for _, filename := range []string{"state", "io", "objset-*"} { + for _, filename := range []string{"state", "io", "iostats", "objset-*"} { paths, err := filepath.Glob(filepath.Join(procZfsPath, "*", filename)) if err != nil { return nil, err @@ -97,11 +102,25 @@ func PoolKernelStats() ([]PoolKernelStat, error) { return pools, nil } -// readPoolCounters supports both ZFS kernel interfaces. OpenZFS through 2.3 -// exposes aggregate vdev counters in "io". When that file is unavailable, sum -// the logical I/O counters exposed for each dataset in the pool. +// readPoolCounters supports the ZFS kernel interfaces. OpenZFS 2.3+ exposes +// pool-wide read/write counters in "iostats" that cover every objset, +// including mounted snapshots, which never get an "objset-*" kstat. OpenZFS +// through 2.0 exposes aggregate vdev counters in "io". When neither pool-level +// interface is usable, sum the logical I/O counters exposed for each dataset. func readPoolCounters(poolDir string) (uint64, uint64, error) { - nread, nwrite, err := readPoolIO(filepath.Join(poolDir, "io")) + nread, nwrite, err := readPoolIOStats(filepath.Join(poolDir, "iostats")) + if err == nil { + return nread, nwrite, nil + } + // Older sources are tried only when "iostats" is absent or predates + // OpenZFS 2.3. A read or parse failure on a usable file is returned: + // silently switching counter sources would feed kernelStats counters + // from different interfaces under the same pool name and produce a + // false I/O spike once "iostats" recovers. + if !errors.Is(err, os.ErrNotExist) && !errors.Is(err, errNoPoolIOStats) { + return 0, 0, err + } + nread, nwrite, err = readPoolIO(filepath.Join(poolDir, "io")) if err == nil || !errors.Is(err, os.ErrNotExist) { return nread, nwrite, err } @@ -144,6 +163,59 @@ func readPoolIO(path string) (uint64, uint64, error) { return 0, 0, fmt.Errorf("I/O counters not found in %s", path) } +// readPoolIOStats reads the pool I/O counters exported by OpenZFS 2.3+ in the +// "iostats" kstat. Reads are counted on ARC misses at their compressed size, so +// reads served from the ARC are excluded; writes are counted at their logical +// size. The file exists on earlier releases but then only reports TRIM +// statistics, so all four byte counters are required for the file to be usable. +func readPoolIOStats(path string) (uint64, uint64, error) { + file, err := os.Open(path) + if err != nil { + return 0, 0, err + } + defer file.Close() + + var arcRead, arcWrite, directRead, directWrite uint64 + var found uint8 + scanner := bufio.NewScanner(file) + for scanner.Scan() { + fields := strings.Fields(scanner.Text()) + if len(fields) < 3 { + continue + } + var target *uint64 + var bit uint8 + switch fields[0] { + case "arc_read_bytes": + target, bit = &arcRead, 1<<0 + case "direct_read_bytes": + target, bit = &directRead, 1<<1 + case "arc_write_bytes": + target, bit = &arcWrite, 1<<2 + case "direct_write_bytes": + target, bit = &directWrite, 1<<3 + default: + continue + } + value, err := strconv.ParseUint(fields[2], 10, 64) + if err != nil { + return 0, 0, fmt.Errorf("parsing %s in %s: %w", fields[0], path, err) + } + *target = value + found |= bit + } + if err := scanner.Err(); err != nil { + return 0, 0, err + } + if found == 0 { + return 0, 0, fmt.Errorf("%w in %s", errNoPoolIOStats, path) + } + if found != 0x0f { + return 0, 0, fmt.Errorf("incomplete pool I/O counters in %s", path) + } + return arcRead + directRead, arcWrite + directWrite, nil +} + func readPoolObjsets(poolDir string) (uint64, uint64, error) { paths, err := filepath.Glob(filepath.Join(poolDir, "objset-*")) if err != nil { diff --git a/agent/zfs/zfs_linux_test.go b/agent/zfs/zfs_linux_test.go index 7ecb59a0b..41f216ba6 100644 --- a/agent/zfs/zfs_linux_test.go +++ b/agent/zfs/zfs_linux_test.go @@ -66,6 +66,111 @@ func TestPoolKernelStatsOpenZfs24(t *testing.T) { }, stats[0]) } +func TestPoolKernelStatsOpenZfs23(t *testing.T) { + root := t.TempDir() + oldPath := procZfsPath + procZfsPath = root + t.Cleanup(func() { procZfsPath = oldPath }) + + // OpenZFS 2.3+ exposes logical pool read/write counters in "iostats". + // They cover every objset, including mounted snapshots, which never get + // an "objset-*" kstat, so the objset sum alone under-reports reads. + poolDir := filepath.Join(root, "tank") + require.NoError(t, os.MkdirAll(poolDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(poolDir, "state"), []byte("ONLINE\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(poolDir, "iostats"), []byte( + "15 1 0x01 26 5176 227423 2127661979232\n"+ + "name type data\n"+ + "trim_extents_written 4 0\n"+ + "arc_read_count 4 12\n"+ + "arc_read_bytes 4 5000\n"+ + "arc_write_count 4 7\n"+ + "arc_write_bytes 4 3000\n"+ + "direct_read_count 4 1\n"+ + "direct_read_bytes 4 500\n"+ + "direct_write_count 4 1\n"+ + "direct_write_bytes 4 200\n", + ), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(poolDir, "objset-0x1"), []byte( + "34 1 0x01 28 7872 0 0\n"+ + "name type data\n"+ + "dataset_name 7 tank\n"+ + "nwritten 4 2000\n"+ + "nread 4 1000\n", + ), 0o644)) + + stats, err := PoolKernelStats() + require.NoError(t, err) + require.Len(t, stats, 1) + assert.Equal(t, PoolKernelStat{ + Name: "tank", Health: "ONLINE", NRead: 5500, NWrite: 3200, + }, stats[0]) +} + +func TestPoolKernelStatsIOStatsTrimOnly(t *testing.T) { + root := t.TempDir() + oldPath := procZfsPath + procZfsPath = root + t.Cleanup(func() { procZfsPath = oldPath }) + + // Before OpenZFS 2.3 the "iostats" file only reports TRIM counters, so + // the per-dataset "objset-*" files remain the only usable source. + poolDir := filepath.Join(root, "tank") + require.NoError(t, os.MkdirAll(poolDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(poolDir, "state"), []byte("ONLINE\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(poolDir, "iostats"), []byte( + "15 1 0x01 18 3736 227423 2127661979232\n"+ + "name type data\n"+ + "trim_extents_written 4 10\n"+ + "trim_bytes_written 4 4096\n", + ), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(poolDir, "objset-0x1"), []byte( + "34 1 0x01 28 7872 0 0\n"+ + "name type data\n"+ + "dataset_name 7 tank\n"+ + "nwritten 4 2000\n"+ + "nread 4 1000\n", + ), 0o644)) + + stats, err := PoolKernelStats() + require.NoError(t, err) + require.Len(t, stats, 1) + assert.Equal(t, PoolKernelStat{ + Name: "tank", Health: "ONLINE", NRead: 1000, NWrite: 2000, + }, stats[0]) +} + +func TestPoolKernelStatsIOStatsErrorDoesNotFallback(t *testing.T) { + root := t.TempDir() + oldPath := procZfsPath + procZfsPath = root + t.Cleanup(func() { procZfsPath = oldPath }) + + // A malformed "iostats" on a kernel that supports it must surface an + // error. Silently switching to the per-dataset sum would drop snapshot + // reads and, when "iostats" recovers, make kernelStats report a false + // I/O spike by comparing counters from two different interfaces. + poolDir := filepath.Join(root, "tank") + require.NoError(t, os.MkdirAll(poolDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(poolDir, "state"), []byte("ONLINE\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(poolDir, "iostats"), []byte( + "arc_read_bytes 4 notanumber\n"+ + "arc_write_bytes 4 3000\n"+ + "direct_read_bytes 4 500\n"+ + "direct_write_bytes 4 200\n", + ), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(poolDir, "objset-0x1"), []byte( + "34 1 0x01 28 7872 0 0\n"+ + "name type data\n"+ + "dataset_name 7 tank\n"+ + "nwritten 4 2000\n"+ + "nread 4 1000\n", + ), 0o644)) + + _, err := PoolKernelStats() + require.Error(t, err) +} + func TestPoolKernelStatsNoZfs(t *testing.T) { oldPath := procZfsPath procZfsPath = t.TempDir() @@ -89,6 +194,22 @@ func TestReadObjsetIORequiresAllCounters(t *testing.T) { require.Error(t, err) } +func TestReadPoolIOStatsRequiresAllCounters(t *testing.T) { + path := filepath.Join(t.TempDir(), "iostats") + require.NoError(t, os.WriteFile(path, []byte( + "arc_read_bytes 4 10\ndirect_read_bytes 4 5\narc_write_bytes 4 7\n"), 0o644)) + _, _, err := readPoolIOStats(path) + require.Error(t, err) +} + +func TestReadPoolIOStatsTrimOnlySignalsFallback(t *testing.T) { + path := filepath.Join(t.TempDir(), "iostats") + require.NoError(t, os.WriteFile(path, []byte( + "trim_extents_written 4 10\ntrim_bytes_written 4 4096\n"), 0o644)) + _, _, err := readPoolIOStats(path) + assert.ErrorIs(t, err, errNoPoolIOStats) +} + func TestCollectorsSkipCommandsWhenDevZfsMissing(t *testing.T) { root := t.TempDir() oldDevZfsPath := devZfsPath