Skip to content

Disabled XFS rmapbt and reflink flags for reducing overhead #2562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/api/scylla/v1alpha1/types_nodeconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,10 @@ type FilesystemConfiguration struct {

// type is a desired filesystem type.
Type FilesystemType `json:"type"`

// flags contains filesystem-specific flags to be used during filesystem creation
// +optional
Flags []string `json:"flags,omitempty"`
Comment on lines +180 to +183
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API changes requires regenerating autogenerated code.

}

// MountConfiguration specifies mount configuration options.
Expand Down
8 changes: 7 additions & 1 deletion pkg/controller/nodesetup/sync_filesystems.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,13 @@ func (nsc *Controller) syncFilesystems(ctx context.Context, nc *scyllav1alpha1.N
continue
}

changed, err := disks.MakeFS(ctx, nsc.executor, device, blockSize, string(fs.Type))
// Add default flags for XFS filesystem
flags := fs.Flags
if fs.Type == scyllav1alpha1.XFSFilesystem {
flags = append(flags, "rmapbt=0", "reflink=0")
}

changed, err := disks.MakeFS(ctx, nsc.executor, device, blockSize, string(fs.Type), flags)
Comment on lines +38 to +44
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's intended, how would i enable those flags via fs.Flags if I wanted to?
Pass along only those which are in the API, those two (rmapbt=0 and reflin=0) should be part of the examples, not implementation.

if err != nil {
nsc.eventRecorder.Eventf(
nc,
Expand Down
12 changes: 10 additions & 2 deletions pkg/disks/fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
"k8s.io/utils/exec"
)

func MakeFS(ctx context.Context, executor exec.Interface, device string, blockSize int, fsType string) (bool, error) {
func MakeFS(ctx context.Context, executor exec.Interface, device string, blockSize int, fsType string, xfsFlags []string) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are not xfs flags anymore

existingFs, err := blkutils.GetFilesystemType(ctx, executor, device)
if err != nil {
return false, fmt.Errorf("can't determine existing filesystem type at %q: %w", device, err)
Expand All @@ -35,8 +35,16 @@ func MakeFS(ctx context.Context, executor exec.Interface, device string, blockSi
"-b", fmt.Sprintf("size=%d", blockSize),
// no discard
"-K",
device,
}

// Add XFS-specific flags if filesystem type is xfs
if fsType == "xfs" {
for _, flag := range xfsFlags {
args = append(args, "-m", flag)
}
}
Comment on lines +41 to +45
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't -m be part of flags? Why this is only for xfs?


args = append(args, device)
stdout, stderr, err := oexec.RunCommand(ctx, executor, "mkfs", args...)
if err != nil {
return false, fmt.Errorf("can't run mkfs with args %v: %w, stdout: %q, stderr: %q", args, err, stdout.String(), stderr.String())
Expand Down
28 changes: 27 additions & 1 deletion pkg/disks/fs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ func TestMakeFS(t *testing.T) {
device string
blockSize int
fsType string
flags []string
expectedCommands []exectest.Command
expectedFinished bool
expectedErr error
Expand Down Expand Up @@ -81,6 +82,31 @@ func TestMakeFS(t *testing.T) {
expectedFinished: true,
expectedErr: nil,
},
{
name: "format the device with default XFS flags",
device: "/dev/md0",
blockSize: 1024,
fsType: "xfs",
flags: []string{"rmapbt=0", "reflink=0"},
expectedCommands: []exectest.Command{
{
Cmd: "lsblk",
Args: []string{"--json", "--nodeps", "--paths", "--fs", "--output=NAME,MODEL,FSTYPE,PARTUUID", "/dev/md0"},
Stdout: []byte(`{"blockdevices":[{"name":"/dev/md0","model":"Amazon EC2 NVMe Instance Storage","fstype":"","partuuid":"1bbeb48b-101f-4d49-8ba4-67adc9878721"}]}`),
Stderr: nil,
Err: nil,
},
{
Cmd: "mkfs",
Args: []string{"-t", "xfs", "-b", "size=1024", "-K", "-m", "rmapbt=0", "-m", "reflink=0", "/dev/md0"},
Stdout: nil,
Stderr: nil,
Err: nil,
},
},
expectedFinished: true,
expectedErr: nil,
},
}

for _, tc := range tt {
Expand All @@ -92,7 +118,7 @@ func TestMakeFS(t *testing.T) {

executor := exectest.NewFakeExec(tc.expectedCommands...)

finished, err := MakeFS(ctx, executor, tc.device, tc.blockSize, tc.fsType)
finished, err := MakeFS(ctx, executor, tc.device, tc.blockSize, tc.fsType, tc.flags)
if !reflect.DeepEqual(err, tc.expectedErr) {
t.Fatalf("expected %v error, got %v", tc.expectedErr, err)
}
Expand Down