Repository: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver (go)

Issue:
btrfs: next steps
We at Chronosphere converted a significant chunk of our production workloads to btrfs using a forked CSI driver. Now that bare-bones btrfs support was merged upstream in #2001 (thanks!), I will summarize what's still lacking, so perhaps we can finalize btrfs support, so others can use it at full potential.

There are a few suggestions, I'd love to align on the direction where to bring this.

## Issue 1: CSI container memory limits

Default GKE CSI container limits of 50M is insufficient to mount a large btrfs volume (2TiB+): `mount` syscall just uses more and gets terminated. We have bumped the CSI limit to 256M and it has worked well. This works in a fork, but to my knowledge one cannot bump memory limits of the GKE-managed CSI driver. Suggestions:

1. Myself: investigate whether memory usage of `mount` can be reduced. If it can, problem solved.
2. GKE side: If (1) does not yield results, perhaps find a way to allow users bump CSI container memory limits per GKE cluster?

## Issue 2: tracking disk space and reclaim/rebalancing

To make sure one doesn't suddenly run out of disk space, btrfs requires periodic rebalancing (also known in btrfs lingo as `reclaim`). Kernel v5.19 (GKE 1.32+ on ubuntu) makes it quite straightforward: `echo 90 > /sys/fs/btrfs/UUID/{data,metadata}/bg_reclaim_threshold` after mounting the FS. There is already a fake mount option [`read_ahead_kb`][1] and I piggy-backed the btrfs settings [the same way][2]. The change is simple on the CSI driver side, there is a precedent, it has worked well over the last couple of months in production.

Proposed next steps:
1. create a patch and leave it as-is?
2. If (1) is unacceptable, set reclaim parameters via [volumeAttributesClassName](https://kubernetes.io/blog/2023/12/15/kubernetes-1-29-volume-attributes-class/) instead of the fake mount options. Note that this is a bit more involved, since there are no usages of `volumeAttributesClassName` in this CSI driver.
3. GKE side: find a way to run integration tests on Ubuntu, or work with OCI team to [enable `CONFIG_FS_BTRFS`](https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/2001#issuecomment-2743928175).

I feel like we can get away with what we did in #2001 without an integration test, but btrfs-specific code in CSI driver without an integration test is just asking for trouble.

Thoughts?

[1]: https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/c84e01f3d53df8366fd237fa90cfed4fde062d56/pkg/gce-pd-csi-driver/node.go#L104
[2]: https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/compare/v1.17.2...motiejus:gcp-compute-persistent-disk-csi-driver:public_btrfs-v1.17.2?expand=1


Pull request description:
btrfs recalim on kernel v5.19+: use bg_reclaim_threshold
Add a sysfs knob `btrfs-allocation-{,meta}data-bg_reclaim_threshold`, which will do the equivalent of:

```
echo VALUE > /sys/fs/btrfs/FS-UUID/allocation/data/bg_reclaim_threshold
```

Or, in case of metadata, equivalently:

```
echo VALUE > /sys/fs/btrfs/FS-UUID/allocation/metadata/bg_reclaim_threshold
```

Where VALUE is a number between `0` and `99` inclusive.

Adding it as a "special" mount option, similarly to `read_ahead_kb`, as that's quite convenient.

Some resources about `bg_reclaim_threshold` and more broadly balancing of the btrfs filesystem:

- https://btrfs.readthedocs.io/en/latest/Administration.html#uuid-allocations-data-metadata-system
- https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=18bb8bbf13c1839b43c9e09e76d397b753989af2
- https://lwn.net/Articles/978826/ Linux v6.11+, this may obsolete `bg_reclaim_threshold`.

Author's interpretation
-----------------------

The higher the reclaim threshold, the more accurately btrfs will show unused space (`Device Unallocated` row of `btrfs filesystem usage`) at the expense of sometimes needlessly moving data around. The lower the threshold, the less rebalancing, the less accurate metrics of the remaining space.

The author of this commit prefers more IO in order to see more accurate `Device Unallocated` metrics, and therefore sets
`btrfs-allocation-data-bg_reclaim_threshold=90`.


**What type of PR is this?**
/kind feature

**What this PR does / why we need it**:

More control to the operator to manage btrfs free space tracking.

**Which issue(s) this PR fixes**:

Fixes #2088

**Does this PR introduce a user-facing change?**:
```release-note
`btrfs` filesystems gain special mount options `btrfs-allocation-(data|metadata)-bg_reclaim_threshold`. The driver will write the provided value to `/sys/fs/btrfs/FS-UUID/allocation/(data|metadata)/bg_reclaim_threshold` respectively.
```

Patch:
diff --git a/README.md b/README.md
index 9d7b7714..34457340 100644
--- a/README.md
+++ b/README.md
@@ -96,7 +96,13 @@ Controller-level and node-level deployments will both have priorityClassName set
 
 As noted in [GCP PD documentation](https://cloud.google.com/kubernetes-engine/docs/how-to/persistent-volumes/gce-pd-csi-driver), `ext4` and `xfs` are officially supported. `btrfs` support is experimental:
 - As of writing, Ubuntu VM images support btrfs, but [COS does not](https://cloud.google.com/container-optimized-os/docs/concepts/supported-filesystems).
-- Early testers have observed CSI driver OOMs when mounting larger (1TiB+) btrfs volumes under default memory constraints. The default constraint, as of writing, is 50MiB.
+
+`btrfs` filesystem accepts two "special" mount options:
+
+- `btrfs-data-bg_reclaim_threshold`
+- `btrfs-metadata-bg_reclaim_threshold`
+
+Which writes to `/sys/fs/btrfs/FS-UUID/allocation/{,meta}data/bg_reclaim_threshold`, as documented [in btrfs docs](https://btrfs.readthedocs.io/en/latest/ch-sysfs.html#uuid-allocations-data-metadata-system).
 
 ## Further Documentation
 
diff --git a/cmd/gce-pd-csi-driver/main.go b/cmd/gce-pd-csi-driver/main.go
index 618b45a3..cf487763 100644
--- a/cmd/gce-pd-csi-driver/main.go
+++ b/cmd/gce-pd-csi-driver/main.go
@@ -265,6 +265,7 @@ func handle() {
 			DeviceInUseTimeout:       *deviceInUseTimeout,
 			EnableDataCache:          *enableDataCacheFlag,
 			DataCacheEnabledNodePool: isDataCacheEnabledNodePool,
+			SysfsPath:                "/sys",
 		}
 		nodeServer = driver.NewNodeServer(gceDriver, mounter, deviceUtils, meta, statter, nsArgs)
 
diff --git a/pkg/gce-pd-csi-driver/gce-pd-driver.go b/pkg/gce-pd-csi-driver/gce-pd-driver.go
index 6ec50844..30102748 100644
--- a/pkg/gce-pd-csi-driver/gce-pd-driver.go
+++ b/pkg/gce-pd-csi-driver/gce-pd-driver.go
@@ -157,6 +157,7 @@ func NewNodeServer(gceDriver *GCEDriver, mounter *mount.SafeFormatAndMount, devi
 		deviceInUseErrors:        newDeviceErrMap(args.DeviceInUseTimeout),
 		EnableDataCache:          args.EnableDataCache,
 		DataCacheEnabledNodePool: args.DataCacheEnabledNodePool,
+		SysfsPath:                args.SysfsPath,
 	}
 }
 
diff --git a/pkg/gce-pd-csi-driver/node.go b/pkg/gce-pd-csi-driver/node.go
index 54950b48..25e3b907 100644
--- a/pkg/gce-pd-csi-driver/node.go
+++ b/pkg/gce-pd-csi-driver/node.go
@@ -15,6 +15,7 @@ limitations under the License.
 package gceGCEDriver
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -51,6 +52,7 @@ type GCENodeServer struct {
 	MetadataService          metadataservice.MetadataService
 	EnableDataCache          bool
 	DataCacheEnabledNodePool bool
+	SysfsPath                string
 
 	// A map storing all volumes with ongoing operations so that additional operations
 	// for that same volume (as defined by VolumeID) return an Aborted error
@@ -87,6 +89,9 @@ type NodeServerArgs struct {
 	EnableDataCache bool
 
 	DataCacheEnabledNodePool bool
+
+	// SysfsPath defaults to "/sys", except if it's a unit test.
+	SysfsPath string
 }
 
 var _ csi.NodeServer = &GCENodeServer{}
@@ -112,12 +117,17 @@ const (
 	defaultLinuxFsType         = "ext4"
 	defaultWindowsFsType       = "ntfs"
 	fsTypeExt3                 = "ext3"
+	fsTypeBtrfs                = "btrfs"
 
 	readAheadKBMountFlagRegexPattern = "^read_ahead_kb=(.+)$"
+	btrfsReclaimDataRegexPattern     = "^btrfs-allocation-data-bg_reclaim_threshold=(\\d{1,2})$"     // 0-99 are valid, incl. 00
+	btrfsReclaimMetadataRegexPattern = "^btrfs-allocation-metadata-bg_reclaim_threshold=(\\d{1,2})$" // ditto ^
 )
 
 var (
 	readAheadKBMountFlagRegex = regexp.MustCompile(readAheadKBMountFlagRegexPattern)
+	btrfsReclaimDataRegex     = regexp.MustCompile(btrfsReclaimDataRegexPattern)
+	btrfsReclaimMetadataRegex = regexp.MustCompile(btrfsReclaimMetadataRegexPattern)
 )
 
 func getDefaultFsType() string {
@@ -390,6 +400,7 @@ func (ns *GCENodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStage
 	// Part 3: Mount device to stagingTargetPath
 	fstype := getDefaultFsType()
 
+	var btrfsReclaimData, btrfsReclaimMetadata string
 	shouldUpdateReadAhead := false
 	var readAheadKB int64
 	options := []string{}
@@ -403,6 +414,10 @@ func (ns *GCENodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStage
 		if err != nil {
 			return nil, status.Errorf(codes.InvalidArgument, "failure parsing mount flags: %v", err.Error())
 		}
+
+		if mnt.FsType == fsTypeBtrfs {
+			btrfsReclaimData, btrfsReclaimMetadata = extractBtrfsReclaimFlags(mnt.MountFlags)
+		}
 	} else if blk := volumeCapability.GetBlock(); blk != nil {
 		// Noop for Block NodeStageVolume
 		klog.V(4).Infof("NodeStageVolume succeeded on %v to %s, capability is block so this is a no-op", volumeID, stagingTargetPath)
@@ -454,10 +469,64 @@ func (ns *GCENodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStage
 		}
 	}
 
+	// Part 6: if configured, write sysfs values
+	if !readonly {
+		sysfs := map[string]string{}
+		if btrfsReclaimData != "" {
+			sysfs["allocation/data/bg_reclaim_threshold"] = btrfsReclaimData
+		}
+		if btrfsReclaimMetadata != "" {
+			sysfs["allocation/metadata/bg_reclaim_threshold"] = btrfsReclaimMetadata
+		}
+
+		if len(sysfs) > 0 {
+			args := []string{"--match-tag", "UUID", "--output", "value", stagingTargetPath}
+			cmd := ns.Mounter.Exec.Command("blkid", args...)
+			var stderr bytes.Buffer
+			cmd.SetStderr(&stderr)
+			klog.V(4).Infof(
+				"running %q for volume %s",
+				strings.Join(append([]string{"blkid"}, args...), " "),
+				volumeID,
+			)
+			uuid, err := cmd.Output()
+			if err != nil {
+				klog.Errorf("blkid failed for %s. stderr:\n%s", volumeID, stderr.String())
+				return nil, status.Errorf(codes.Internal, "blkid failed: %v", err)
+			}
+			uuid = bytes.TrimRight(uuid, "\n")
+
+			for key, value := range sysfs {
+				path := fmt.Sprintf("%s/fs/btrfs/%s/%s", ns.SysfsPath, uuid, key)
+				if err := writeSysfs(path, value); err != nil {
+					return nil, status.Error(codes.Internal, err.Error())
+				}
+				klog.V(4).Infof("NodeStageVolume set %s %s=%s", volumeID, key, value)
+			}
+		}
+	}
+
 	klog.V(4).Infof("NodeStageVolume succeeded on %v to %s", volumeID, stagingTargetPath)
 	return &csi.NodeStageVolumeResponse{}, nil
 }
 
+func writeSysfs(path, value string) (_err error) {
+	f, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC, 0o644)
+	if err != nil {
+		return err
+	}
+
+	defer func() {
+		_err = errors.Join(_err, f.Close())
+	}()
+
+	if _, err := f.Write([]byte(value)); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func (ns *GCENodeServer) updateReadAhead(devicePath string, readAheadKB int64) error {
 	isBlock, err := ns.VolumeStatter.IsBlockDevice(devicePath)
 	if err != nil {
@@ -474,6 +543,18 @@ func (ns *GCENodeServer) updateReadAhead(devicePath string, readAheadKB int64) e
 	return nil
 }
 
+func extractBtrfsReclaimFlags(mountFlags []string) (string, string) {
+	var reclaimData, reclaimMetadata string
+	for _, mountFlag := range mountFlags {
+		if got := btrfsReclaimDataRegex.FindStringSubmatch(mountFlag); len(got) == 2 {
+			reclaimData = got[1]
+		} else if got := btrfsReclaimMetadataRegex.FindStringSubmatch(mountFlag); len(got) == 2 {
+			reclaimMetadata = got[1]
+		}
+	}
+	return reclaimData, reclaimMetadata
+}
+
 func extractReadAheadKBMountFlag(mountFlags []string) (int64, bool, error) {
 	for _, mountFlag := range mountFlags {
 		if readAheadKB := readAheadKBMountFlagRegex.FindStringSubmatch(mountFlag); len(readAheadKB) == 2 {
diff --git a/pkg/gce-pd-csi-driver/utils.go b/pkg/gce-pd-csi-driver/utils.go
index 9a94995b..9e64778b 100644
--- a/pkg/gce-pd-csi-driver/utils.go
+++ b/pkg/gce-pd-csi-driver/utils.go
@@ -311,6 +311,14 @@ func collectMountOptions(fsType string, mntFlags []string) []string {
 			// passed directly as an option to the mount command.
 			continue
 		}
+
+		if btrfsReclaimDataRegex.FindString(opt) != "" {
+			continue
+		}
+		if btrfsReclaimMetadataRegex.FindString(opt) != "" {
+			continue
+		}
+
 		options = append(options, opt)
 	}
 


Test patch:
diff --git a/pkg/gce-pd-csi-driver/node_test.go b/pkg/gce-pd-csi-driver/node_test.go
index f4ada1d8..866615d6 100644
--- a/pkg/gce-pd-csi-driver/node_test.go
+++ b/pkg/gce-pd-csi-driver/node_test.go
@@ -537,18 +537,34 @@ func TestNodeStageVolume(t *testing.T) {
 	defer os.RemoveAll(tempDir)
 	stagingPath := filepath.Join(tempDir, defaultStagingPath)
 
+	btrfsUUID := "00000000-0000-0000-0000-000000000001"
+	btrfsPrefix := fmt.Sprintf("%s/sys/fs/btrfs/%s/allocation", tempDir, btrfsUUID)
+
+	for _, suffix := range []string{"data", "metadata"} {
+		dir := btrfsPrefix + "/" + suffix
+		if err := os.MkdirAll(dir, 0755); err != nil {
+			t.Fatalf("Failed to set up fake sysfs dir %q: %v", dir, err)
+		}
+		fname := dir + "/bg_reclaim_threshold"
+		if err := os.WriteFile(fname, []byte("0\n"), 0644); err != nil {
+			t.Fatalf("write %q: %v", fname, err)
+		}
+	}
+
 	testCases := []struct {
-		name               string
-		req                *csi.NodeStageVolumeRequest
-		deviceSize         int
-		blockExtSize       int
-		readonlyBit        string
-		expResize          bool
-		expReadAheadUpdate bool
-		expReadAheadKB     string
-		readAheadSectors   string
-		sectorSizeInBytes  int
-		expErrCode         codes.Code
+		name                 string
+		req                  *csi.NodeStageVolumeRequest
+		deviceSize           int
+		blockExtSize         int
+		readonlyBit          string
+		expResize            bool
+		expReadAheadUpdate   bool
+		expReadAheadKB       string
+		readAheadSectors     string
+		btrfsReclaimData     string
+		btrfsReclaimMetadata string
+		sectorSizeInBytes    int
+		expErrCode           codes.Code
 	}{
 		{
 			name: "Valid request, no resize because block and filesystem sizes match",
@@ -598,6 +614,76 @@ func TestNodeStageVolume(t *testing.T) {
 			readonlyBit:  "0",
 			expResize:    false,
 		},
+		{
+			name: "btrfs-allocation-data-bg_reclaim_threshold is ignored on non-btrfs",
+			req: &csi.NodeStageVolumeRequest{
+				VolumeId:          volumeID,
+				StagingTargetPath: stagingPath,
+				VolumeCapability: &csi.VolumeCapability{
+					AccessType: &csi.VolumeCapability_Mount{
+						Mount: &csi.VolumeCapability_MountVolume{
+							FsType:     "ext4",
+							MountFlags: []string{"btrfs-allocation-data-bg_reclaim_threshold=90"},
+						},
+					},
+					AccessMode: &csi.VolumeCapability_AccessMode{
+						Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+					},
+				},
+			},
+			deviceSize:       1,
+			blockExtSize:     1,
+			readonlyBit:      "0",
+			btrfsReclaimData: "0",
+		},
+		{
+			name: "Valid request, set btrfs-allocation-data-bg_reclaim_threshold=90",
+			req: &csi.NodeStageVolumeRequest{
+				VolumeId:          volumeID,
+				StagingTargetPath: stagingPath,
+				VolumeCapability: &csi.VolumeCapability{
+					AccessType: &csi.VolumeCapability_Mount{
+						Mount: &csi.VolumeCapability_MountVolume{
+							FsType:     "btrfs",
+							MountFlags: []string{"btrfs-allocation-data-bg_reclaim_threshold=90"},
+						},
+					},
+					AccessMode: &csi.VolumeCapability_AccessMode{
+						Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+					},
+				},
+			},
+			deviceSize:       1,
+			blockExtSize:     1,
+			readonlyBit:      "0",
+			btrfsReclaimData: "90",
+		},
+		{
+			name: "Valid request, set btrfs-allocation-{,meta}data-bg_reclaim_threshold",
+			req: &csi.NodeStageVolumeRequest{
+				VolumeId:          volumeID,
+				StagingTargetPath: stagingPath,
+				VolumeCapability: &csi.VolumeCapability{
+					AccessType: &csi.VolumeCapability_Mount{
+						Mount: &csi.VolumeCapability_MountVolume{
+							FsType: "btrfs",
+							MountFlags: []string{
+								"btrfs-allocation-data-bg_reclaim_threshold=90",
+								"btrfs-allocation-metadata-bg_reclaim_threshold=91",
+							},
+						},
+					},
+					AccessMode: &csi.VolumeCapability_AccessMode{
+						Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+					},
+				},
+			},
+			deviceSize:           1,
+			blockExtSize:         1,
+			readonlyBit:          "0",
+			btrfsReclaimData:     "90",
+			btrfsReclaimMetadata: "91",
+		},
 		{
 			name: "Valid request, update readahead",
 			req: &csi.NodeStageVolumeRequest{
@@ -730,6 +816,7 @@ func TestNodeStageVolume(t *testing.T) {
 		t.Logf("Test case: %s", tc.name)
 		resizeCalled := false
 		readAheadUpdateCalled := false
+		blkidCalled := false
 		actionList := []testingexec.FakeCommandAction{
 			makeFakeCmd(
 				&testingexec.FakeCmd{
@@ -853,9 +940,26 @@ func TestNodeStageVolume(t *testing.T) {
 				),
 			}...)
 		}
+		if tc.btrfsReclaimData != "" || tc.btrfsReclaimMetadata != "" {
+			actionList = append(actionList, []testingexec.FakeCommandAction{
+				makeFakeCmd(
+					&testingexec.FakeCmd{
+						OutputScript: []testingexec.FakeAction{
+							func() ([]byte, []byte, error) {
+								blkidCalled = true
+								return []byte(btrfsUUID + "\n"), nil, nil
+							},
+						},
+					},
+					"blkid",
+					[]string{"--match-tag", "UUID", "--output", "value", stagingPath}...,
+				),
+			}...)
+		}
 		mounter := mountmanager.NewFakeSafeMounterWithCustomExec(&testingexec.FakeExec{CommandScript: actionList})
 		gceDriver := getTestGCEDriverWithCustomMounter(t, mounter)
 		ns := gceDriver.ns
+		ns.SysfsPath = tempDir + "/sys"
 		_, err := ns.NodeStageVolume(context.Background(), tc.req)
 		if err != nil {
 			serverError, ok := status.FromError(err)
@@ -882,6 +986,30 @@ func TestNodeStageVolume(t *testing.T) {
 		if tc.expReadAheadUpdate == false && readAheadUpdateCalled == true {
 			t.Fatalf("Test updated read ahead, but it was not expected.")
 		}
+		if tc.btrfsReclaimData == "" && tc.btrfsReclaimMetadata == "" && blkidCalled {
+			t.Fatalf("blkid was called, but was not expected.")
+		}
+
+		if tc.btrfsReclaimData != "" {
+			fname := btrfsPrefix + "/data/bg_reclaim_threshold"
+			got, err := os.ReadFile(fname)
+			if err != nil {
+				t.Fatalf("read %q: %v", fname, err)
+			}
+			if s := strings.TrimSpace(string(got)); s != tc.btrfsReclaimData {
+				t.Fatalf("%q: expected %q, got %q", fname, tc.btrfsReclaimData, s)
+			}
+		}
+		if tc.btrfsReclaimMetadata != "" {
+			fname := btrfsPrefix + "/metadata/bg_reclaim_threshold"
+			got, err := os.ReadFile(fname)
+			if err != nil {
+				t.Fatalf("read %q: %v", fname, err)
+			}
+			if s := strings.TrimSpace(string(got)); s != tc.btrfsReclaimMetadata {
+				t.Fatalf("%q: expected %q, got %q", fname, tc.btrfsReclaimMetadata, s)
+			}
+		}
 	}
 }