rbd: Use assume_storage_prezeroed when formatting #4996
Conversation
internal/rbd/nodeserver.go
Outdated
| @@ -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"}, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
assume_storage_prezeroedoption became available in e2fsprogs 1.47.0 last year, so it should probably be "discovered" similar toxfsSupportsReflink().
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The check for the version isn't always reliable in the face of downstream backports.
There was a problem hiding this comment.
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.
black-dragon74
force-pushed
the
stor-prezeroed
branch
from
60069c4 to
434ce4e
Compare
black-dragon74
force-pushed
the
stor-prezeroed
branch
2 times, most recently
from
2699bc7 to
6b52f64
Compare
internal/rbd/nodeserver.go
Outdated
| @@ -67,6 +67,10 @@ const ( | |||
| xfsReflinkNoSupport | |||
| xfsReflinkSupport | |||
|
|
|||
| ext4PrezeroedUnset | |||
There was a problem hiding this comment.
restart counting again, use ext4PrezeroedUnset = itoa
There was a problem hiding this comment.
go doesn't consider iota twice in the same const block :(
black-dragon74
force-pushed
the
stor-prezeroed
branch
2 times, most recently
from
3d4e46b to
ed5b89a
Compare
| // "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.
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?
There was a problem hiding this comment.
As the result for this is cached, let's check it when we need it instead of doing it preemptively? This way also has the added benefit of keeping driver.go simple. WDYT?
There was a problem hiding this comment.
I think this is sufficient for now. It could be placed in some internal/util/... package, and keep a single initialization with sync.Once there. That could be an improvement, but I don't think it is really needed at the moment.
black-dragon74
force-pushed
the
stor-prezeroed
branch
2 times, most recently
from
288c82e to
d782438
Compare
internal/util/file/file_test.go
Outdated
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| file, err := os.CreateTemp("", "test-sparse-") |
There was a problem hiding this comment.
place this in the t.TempDir() directory for automatic cleanup, no need to have to remove the file in a defer.
There was a problem hiding this comment.
Rebased and fixed this nit for you :-)
nixpanic
force-pushed
the
stor-prezeroed
branch
from
d782438 to
4b5c43e
Compare
|
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at c308e09 |
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]>
black-dragon74
force-pushed
the
stor-prezeroed
branch
from
4b5c43e to
e32b9dd
Compare
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/k8s-e2e-external-storage/1.30 |
|
/test ci/centos/upgrade-tests-rbd |
|
/test ci/centos/mini-e2e-helm/k8s-1.30 |
|
/test ci/centos/k8s-e2e-external-storage/1.32 |
|
/test ci/centos/k8s-e2e-external-storage/1.31 |
|
/test ci/centos/mini-e2e/k8s-1.30 |
|
/test ci/centos/mini-e2e-helm/k8s-1.31 |
|
/test ci/centos/mini-e2e-helm/k8s-1.32 |
|
/test ci/centos/mini-e2e/k8s-1.31 |
|
/test ci/centos/mini-e2e/k8s-1.32 |
Describe what this PR does
Instead of passing
lazy_itable_init=1andlazy_journal_init=1tomkfs.ext4, passassume_storage_prezeroed=1which isstronger 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.ext4and checking itsSTDERR.Closes: #4948