Skip to content
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

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from
Open
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
101 changes: 96 additions & 5 deletions internal/rbd/nodeserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ const (
xfsReflinkNoSupport
xfsReflinkSupport

ext4PrezeroedUnset
Copy link
Member

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

Copy link
Member Author

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 :(

ext4PrezeroedNoSupport
ext4PrezeroedSupport

staticVol = "staticVolume"
volHealerCtx = "volumeHealerContext"
tryOtherMounters = "tryOtherMounters"
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

I understand you copied the xfsHasReflink idea. But I think it is cleaner if the NodeServer struct has a field for these kind of detectable parameters. Have you thought about doing it that way?


mountDefaultOpts = map[string][]string{
"xfs": {"nouuid"},
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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",
Copy link
Member

Choose a reason for hiding this comment

The 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 getMkfsArgs().

Maybe keep the default map, and use ext4.prezeroed as fstype to get the options?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

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 assume_storage_prezeroed=1, nodiscard isn't relevant here.

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,
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The 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
}
Loading