rbd: Use assume_storage_prezeroed when formatting #4996

black-dragon74 · 2024-12-03T10:45:46Z

Describe what this PR does

Instead of passing lazy_itable_init=1 and lazy_journal_init=1 to
mkfs.ext4, pass assume_storage_prezeroed=1 which is
stronger and allows the filesystem to skip inode table zeroing
completely instead of simply doing it lazily.

The support for this flag is checked by trying to format a fake
temporary image with mkfs.ext4 and checking its STDERR.

Closes: #4948

nixpanic · 2024-12-09T11:02:32Z

internal/rbd/nodeserver.go

@@ -101,7 +101,7 @@ var (
 	xfsHasReflink = xfsReflinkUnset

 	mkfsDefaultArgs = map[string][]string{
-		"ext4": {"-m0", "-Enodiscard,lazy_itable_init=1,lazy_journal_init=1"},
+		"ext4": {"-m0", "-Enodiscard,assume_storage_prezeroed=1"},


make sure that you only do this when mkfs.ext4 supports the option, you'll need to add some form of detection, ideally with something like sync.Once.

assume_storage_prezeroed option became available in e2fsprogs 1.47.0 last year, so it should probably be "discovered" similar to xfsSupportsReflink().

My suggestion to do it similar to xfsSupportsReflink() isn't viable because mkfs.ext4 doesn't include extended options in its help output, but trying with assume_storage_prezeroed=1 and falling back to lazy_itable_init=1,lazy_journal_init=1 upon parsing out Bad option(s) specified: assume_storage_prezeroed error should work.

I and Madhu had a discussion on this. Since this flag is part of e2fsprogs >= 1.47 and we have control over which version of it we bundle in the container, do we really need this check?

Sidenote: Detection is possible by parsing the semver of e2fsprogs. It is standard flag so it must be there?

The check for the version isn't always reliable in the face of downstream backports.

as @idryomov mentioned we cannot depend on the version check, as the base image includes the required version, we could say what is supported by this version but that could be a problem if someone wants to build/test custom images. let's depend on the error check and fall back that works for all the cases.

nixpanic · 2025-01-07T15:52:45Z

internal/rbd/nodeserver.go

@@ -67,6 +67,10 @@ const (
 	xfsReflinkNoSupport
 	xfsReflinkSupport

+	ext4PrezeroedUnset


restart counting again, use ext4PrezeroedUnset = itoa

go doesn't consider iota twice in the same const block :(

nixpanic · 2025-01-07T15:55:52Z

internal/rbd/nodeserver.go

@@ -748,6 +756,7 @@ func (ns *NodeServer) NodePublishVolume(
 	return &csi.NodePublishVolumeResponse{}, nil
 }

+//nolint:cyclop,gocyclo // function is required to have multiple conditional checks


This is rather ugly. Maybe it is not required if you add a function like

args := ns.GetMKFSDefaultArgs(fsType)

a function like that can add the assume_storage_prezeroed=1 option if support is detected.

nixpanic · 2025-01-07T15:55:55Z

internal/rbd/nodeserver.go

 	if existingFormat == "" && !staticVol && !readOnly && !isBlock {
 		args := mkfsDefaultArgs[fsType]
+		if fsType == "ext4" && ns.ext4SupportsPrezeroed() {
+			args = []string{"-m0", "-Enodiscard,assume_storage_prezeroed=1"}


this drops the default options from line 112, is that intentional?

Yes, lazy_itable_init=1,lazy_journal_init=1 are not required when using assume_storage_prezeroed=1. Rest of the options (-m0, -Enodiscard) from line 112 are included.

nixpanic · 2025-01-07T15:56:44Z

internal/rbd/nodeserver.go

+		return ext4HasPrezeroedSupport == ext4PrezeroedSupport
+	}
+
+	tempImage := "/tmp/prezeroed.img"


use a temporary filename, please

nixpanic · 2025-01-07T15:57:33Z

internal/rbd/nodeserver.go

+
+	tempImage := "/tmp/prezeroed.img"
+	ctx := context.TODO()
+	_, _, err := util.ExecCommand(ctx, "dd", "if=/dev/zero", "of="+tempImage, "bs=1M", "count=1")


Instead of executing a dd command, open the file, seek to 1mb and write a singled zero byte. This makes a sparse file, which should be much quicker and can easily be done with plain Go.

Thank you for this insight :)

Instead of passing lazy_itable_init=1 and lazy_journal_init=1 to mkfs.ext4, pass assume_storage_prezeroed=1 which is stronger and allows the filesystem to skip inode table zeroing completely instead of simply doing it lazily. The support for this flag is checked by trying to format a fake temporary image with mkfs.ext4 and checking its STDERR. Closes: ceph#4948 Signed-off-by: Niraj Yadav <[email protected]>

nixpanic

looks good, just a few nits and potential improvements

nixpanic · 2025-01-10T12:16:11Z

internal/rbd/nodeserver.go

+}
+
+// createSpareFile makes `file` a sparse file of size `sizeMB`.
+func createSparseFile(file *os.File, sizeMB int64) error {


this is not specific to the nodeserver, move it to the internal/util package please, and add a unit-test for it

nixpanic · 2025-01-10T12:22:26Z

internal/rbd/nodeserver.go

+	out, err := diskMounter.Exec.Command(
+		"mkfs.ext4",
+		"-n",
+		"-Enodiscard,assume_storage_prezeroed=1",


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?

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.

nixpanic · 2025-01-10T12:24:29Z

internal/rbd/nodeserver.go

-	}
+	// ext4HasPrezeroed is set by ext4SupportsPrezeroed(), use the function when
+	// checking the support for assume_storage_prezeroed.
+	ext4HasPrezeroedSupport = ext4PrezeroedUnset


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?

Madhu-1 · 2025-01-10T12:57:51Z

internal/rbd/nodeserver.go

+// "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 {


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?

black-dragon74 added the DNM DO NOT MERGE label Dec 3, 2024

mergify bot added the component/rbd Issues related to RBD label Dec 3, 2024

nixpanic reviewed Dec 9, 2024

View reviewed changes

idryomov mentioned this pull request Dec 27, 2024

Pass assume_storage_prezeroed option when formatting ext4 if possible #4948

Open

black-dragon74 force-pushed the stor-prezeroed branch from 60069c4 to 434ce4e Compare January 6, 2025 11:56

black-dragon74 marked this pull request as ready for review January 6, 2025 11:58

black-dragon74 removed the DNM DO NOT MERGE label Jan 6, 2025

black-dragon74 requested review from nixpanic, idryomov and Madhu-1 January 6, 2025 11:58

black-dragon74 force-pushed the stor-prezeroed branch 2 times, most recently from 2699bc7 to 6b52f64 Compare January 6, 2025 12:25

nixpanic reviewed Jan 7, 2025

View reviewed changes

nixpanic requested changes Jan 7, 2025

View reviewed changes

black-dragon74 force-pushed the stor-prezeroed branch from 6b52f64 to 3d4e46b Compare January 8, 2025 09:17

black-dragon74 force-pushed the stor-prezeroed branch from 3d4e46b to ed5b89a Compare January 8, 2025 09:33

black-dragon74 requested a review from nixpanic January 9, 2025 08:20

nixpanic reviewed Jan 10, 2025

View reviewed changes

Madhu-1 reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd: Use assume_storage_prezeroed when formatting #4996

rbd: Use assume_storage_prezeroed when formatting #4996

black-dragon74 commented Dec 3, 2024 •

edited

Loading

nixpanic Dec 9, 2024

idryomov Dec 27, 2024

black-dragon74 Dec 27, 2024 •

edited

Loading

idryomov Dec 27, 2024

Madhu-1 Jan 3, 2025

nixpanic Jan 7, 2025

black-dragon74 Jan 8, 2025

nixpanic Jan 7, 2025

nixpanic Jan 7, 2025

black-dragon74 Jan 8, 2025

nixpanic Jan 7, 2025

black-dragon74 Jan 8, 2025

nixpanic Jan 7, 2025

black-dragon74 Jan 8, 2025

nixpanic left a comment

nixpanic Jan 10, 2025

nixpanic Jan 10, 2025

idryomov Jan 10, 2025

nixpanic Jan 10, 2025

Madhu-1 Jan 10, 2025

rbd: Use assume_storage_prezeroed when formatting #4996

Are you sure you want to change the base?

rbd: Use assume_storage_prezeroed when formatting #4996

Conversation

black-dragon74 commented Dec 3, 2024 • edited Loading

Describe what this PR does

Choose a reason for hiding this comment

Choose a reason for hiding this comment

black-dragon74 Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpanic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

black-dragon74 commented Dec 3, 2024 •

edited

Loading

black-dragon74 Dec 27, 2024 •

edited

Loading