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

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


Discussion:
@mattcary: Thanks for raising the container memory limit. In GKE we have in fact moved to a 450Mi memory limit, it's been out in prod clusters for several months and seems to not be causing problems. So we should update the deployment here, I can get a PR for that out shortly.

For (2), probably your patch is good enough. Raise a PR and we can discuss it there.@mattcary: Oh, we actually don't have mem requests for the upstream deployment :shrug:.  Anyway, as mentioned, this is already done on GKE. The 450Mi limit works pretty well up to 256T volumes in our testing.@motiejus: #2091

Solution 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)
+			}
+		}
 	}
 }