-
Notifications
You must be signed in to change notification settings - Fork 554
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
rbd: disable reflink while creating XFS filesystems #1257
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import ( | |
"fmt" | ||
"os" | ||
"strconv" | ||
"strings" | ||
|
||
csicommon "github.com/ceph/ceph-csi/internal/csi-common" | ||
"github.com/ceph/ceph-csi/internal/journal" | ||
|
@@ -61,6 +62,13 @@ type stageTransaction struct { | |
devicePath string | ||
} | ||
|
||
const ( | ||
// values for xfsHasReflink | ||
xfsReflinkUnset int = iota | ||
xfsReflinkNoSupport | ||
xfsReflinkSupport | ||
) | ||
|
||
var ( | ||
kernelRelease = "" | ||
// deepFlattenSupport holds the list of kernel which support mapping rbd | ||
|
@@ -84,6 +92,10 @@ var ( | |
Backport: true, | ||
}, // RHEL 8.2 | ||
} | ||
|
||
// xfsHasReflink is set by xfsSupportsReflink(), use the function when | ||
// checking the support for reflink | ||
xfsHasReflink = xfsReflinkUnset | ||
) | ||
|
||
// NodeStageVolume mounts the volume to a staging path on the node. | ||
|
@@ -460,6 +472,11 @@ func (ns *NodeServer) mountVolumeToStagePath(ctx context.Context, req *csi.NodeS | |
args = []string{"-m0", "-Enodiscard,lazy_itable_init=1,lazy_journal_init=1", devicePath} | ||
} else if fsType == "xfs" { | ||
args = []string{"-K", devicePath} | ||
// always disable reflink | ||
// TODO: make enabling an option, see ceph/ceph-csi#1256 | ||
if ns.xfsSupportsReflink() { | ||
args = append(args, "-m", "reflink=0") | ||
Madhu-1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
if len(args) > 0 { | ||
cmdOut, cmdErr := diskMounter.Exec.Command("mkfs."+fsType, args...).CombinedOutput() | ||
|
@@ -870,3 +887,27 @@ func openEncryptedDevice(ctx context.Context, volOptions *rbdVolume, devicePath | |
|
||
return mapperFilePath, nil | ||
} | ||
|
||
// xfsSupportsReflink checks if mkfs.xfs supports the "-m reflink=0|1" | ||
// argument. In case it is supported, return true. | ||
func (ns *NodeServer) xfsSupportsReflink() bool { | ||
// return cached value, if set | ||
if xfsHasReflink != xfsReflinkUnset { | ||
return xfsHasReflink == xfsReflinkSupport | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nixpanic do we need this whitespace here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not required, but it splits two parts of unrelated code:
|
||
// run mkfs.xfs in the same namespace as formatting would be done in | ||
// mountVolumeToStagePath() | ||
diskMounter := &mount.SafeFormatAndMount{Interface: ns.mounter, Exec: utilexec.New()} | ||
out, err := diskMounter.Exec.Command("mkfs.xfs").CombinedOutput() | ||
if err != nil { | ||
// mkfs.xfs should fail with an error message (and help text) | ||
if strings.Contains(string(out), "reflink=0|1") { | ||
xfsHasReflink = xfsReflinkSupport | ||
return true | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nixpanic do we need this whitespace here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just clean coding, I think? This is not an else statement that would follow the if |
||
xfsHasReflink = xfsReflinkNoSupport | ||
return false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nixpanic do we need this whitespace here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if it is required, but splitting a multi-line variable with an empty line between the next multi-line comment looks clean to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so in many places we dont leave a newline before multiline comment and in some places like this we do. Thats what I am trying to understand.