-
Notifications
You must be signed in to change notification settings - Fork 553
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: Use assume_storage_prezeroed when formatting #4996
base: devel
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
|
@@ -67,6 +67,10 @@ const ( | |
xfsReflinkNoSupport | ||
xfsReflinkSupport | ||
|
||
ext4PrezeroedUnset | ||
ext4PrezeroedNoSupport | ||
ext4PrezeroedSupport | ||
|
||
staticVol = "staticVolume" | ||
volHealerCtx = "volumeHealerContext" | ||
tryOtherMounters = "tryOtherMounters" | ||
|
@@ -100,10 +104,9 @@ var ( | |
// checking the support for reflink. | ||
xfsHasReflink = xfsReflinkUnset | ||
|
||
mkfsDefaultArgs = map[string][]string{ | ||
"ext4": {"-m0", "-Enodiscard,lazy_itable_init=1,lazy_journal_init=1"}, | ||
"xfs": {"-K"}, | ||
} | ||
// ext4HasPrezeroed is set by ext4SupportsPrezeroed(), use the function when | ||
// checking the support for assume_storage_prezeroed. | ||
ext4HasPrezeroedSupport = ext4PrezeroedUnset | ||
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. I understand you copied the |
||
|
||
mountDefaultOpts = map[string][]string{ | ||
"xfs": {"nouuid"}, | ||
|
@@ -793,7 +796,7 @@ func (ns *NodeServer) mountVolumeToStagePath( | |
} | ||
|
||
if existingFormat == "" && !staticVol && !readOnly && !isBlock { | ||
args := mkfsDefaultArgs[fsType] | ||
args := ns.getMkfsArgs(fsType) | ||
|
||
// if the VolumeContext contains "mkfsOptions", use those as args instead | ||
volumeCtx := req.GetVolumeContext() | ||
|
@@ -1314,6 +1317,55 @@ func (ns *NodeServer) xfsSupportsReflink() bool { | |
return false | ||
} | ||
|
||
// ext4SupportsPrezeroed checks if the ext4 filesystem supports the | ||
// "assume_storage_prezeroed" option. It does this by creating a temporary | ||
// image file, attempting to format it with the ext4 filesystem using the | ||
// "assume_storage_prezeroed" option, and checking the output for errors. | ||
func (ns *NodeServer) ext4SupportsPrezeroed() bool { | ||
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. how about if we check if this is supported or not in the driver.go(if we are stating it as nodeplugin) before starting the node plugin and store it in the Nodeserver struct like its suggested here https://github.com/ceph/ceph-csi/pull/4996/files#r1910308642? |
||
if ext4HasPrezeroedSupport != ext4PrezeroedUnset { | ||
return ext4HasPrezeroedSupport == ext4PrezeroedSupport | ||
} | ||
|
||
ctx := context.TODO() | ||
tempImgFile, err := os.CreateTemp(os.TempDir(), "prezeroed.img") | ||
if err != nil { | ||
log.WarningLog(ctx, "failed to create temporary image file: %v", err) | ||
|
||
return false | ||
} | ||
defer os.Remove(tempImgFile.Name()) | ||
|
||
if err = createSparseFile(tempImgFile, 1); err != nil { | ||
log.WarningLog(ctx, "failed to create sparse file: %v", err) | ||
|
||
return false | ||
} | ||
|
||
diskMounter := &mount.SafeFormatAndMount{Interface: ns.Mounter, Exec: utilexec.New()} | ||
|
||
// `-n` Causes mke2fs to not actually create a file system | ||
// but display what it would do if it were to create a file system. | ||
out, err := diskMounter.Exec.Command( | ||
"mkfs.ext4", | ||
"-n", | ||
"-Enodiscard,assume_storage_prezeroed=1", | ||
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. I think it is nice to have the options mentioned as a const somewhere, they are here, but also in Maybe keep the default map, and use 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.
I don't have an opinion about it being expressed as a const, but I think it should be a single option. This function is supposed to be checking whether ext4 supports "prezeroed", so it should be just that -- pass only |
||
tempImgFile.Name(), | ||
).CombinedOutput() | ||
if err != nil { | ||
if strings.Contains(string(out), "Bad option(s) specified") { | ||
ext4HasPrezeroedSupport = ext4PrezeroedNoSupport | ||
} | ||
|
||
// if the error is not due to the missing option, we can't be sure | ||
// if the option is supported or not, return false optimistically | ||
return false | ||
} | ||
|
||
ext4HasPrezeroedSupport = ext4PrezeroedSupport | ||
|
||
return true | ||
} | ||
|
||
// NodeGetVolumeStats returns volume stats. | ||
func (ns *NodeServer) NodeGetVolumeStats( | ||
ctx context.Context, | ||
|
@@ -1397,3 +1449,42 @@ func getDeviceSize(ctx context.Context, devicePath string) (uint64, error) { | |
|
||
return size, nil | ||
} | ||
|
||
// getMkfsArgs returns the appropriate mkfs arguments for the given filesystem type. | ||
// Supported filesystem types are "ext4" and "xfs". For "ext4", it checks if the | ||
// ext4 filesystem supports pre-zeroed storage and returns the corresponding arguments. | ||
// For "xfs", it returns the arguments to disable discard. If an unknown filesystem | ||
// type is provided, a warning is logged and empty options are returned. | ||
func (ns *NodeServer) getMkfsArgs(fsType string) []string { | ||
switch fsType { | ||
case "ext4": | ||
if ns.ext4SupportsPrezeroed() { | ||
return []string{"-m0", "-Enodiscard,assume_storage_prezeroed=1"} | ||
} | ||
|
||
return []string{"-m0", "-Enodiscard,lazy_itable_init=1,lazy_journal_init=1"} | ||
case "xfs": | ||
return []string{"-K"} | ||
default: | ||
log.WarningLogMsg(fmt.Sprintf("unknown fsType: %q, using default mkfs options", fsType)) | ||
|
||
return []string{} | ||
} | ||
} | ||
|
||
// createSpareFile makes `file` a sparse file of size `sizeMB`. | ||
func createSparseFile(file *os.File, sizeMB int64) error { | ||
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. this is not specific to the nodeserver, move it to the internal/util package please, and add a unit-test for it |
||
sizeBytes := sizeMB * 1024 * 1024 | ||
|
||
// seek to the end of the file. | ||
if _, err := file.Seek(sizeBytes-1, 0); err != nil { | ||
return fmt.Errorf("failed to seek to the end of the file: %w", err) | ||
} | ||
|
||
// write a single byte, effectively making it a sparse file. | ||
if _, err := file.Write([]byte{0}); err != nil { | ||
return fmt.Errorf("failed to write to the end of the file: %w", err) | ||
} | ||
|
||
return nil | ||
} |
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.
restart counting again, use
ext4PrezeroedUnset = itoa
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.
go doesn't consider
iota
twice in the same const block :(