[VDO-5752] [VDOSTORY-292] Add formatting logic #334

bjohnsto · 2025-09-16T17:37:13Z

This series of commits is the final set of changes for moving vdoformat into the kernel. The changes add the logic for when to format or not and the actual layout initialization and writing of the layout to disk when formatting is needed. Additionally, it adds unit test support and more functional tests to the VDOFormatInKernel.pm perl test.

github-actions · 2025-09-16T18:28:57Z

Unit Test Results

  11 files   96 suites 1h 27m 57s ⏱️
272 tests 272 ✔️ 0 💤 0 ❌
276 runs 276 ✔️ 0 💤 0 ❌

Results for commit 57230c6.

lorelei-sakai

Okay, ran out of steam by the time I got to the tests, but tey looks familiar and mostly reasonable.

Most comments are on the way the code is structured and ordered, and it's very likely I don't remember some of the nuances involved. Probably we should sit down and talk some of these things out face-to-face. We can do it Thursday if there's no time earlier.

src/c++/vdo/base/vdo.c

lorelei-sakai · 2025-09-23T21:43:37Z

src/c++/vdo/base/vdo.c

 	return vdo_get_admin_state_code(&vdo->admin.state);
 }

+


extra blank line added. Reminds me to check my own work-in-progress, I think I've seen cursor do this to me.

I think I removed this properly

lorelei-sakai · 2025-09-23T21:51:35Z

src/c++/vdo/base/vdo.c

 	result = read_geometry_block(vdo);
 	if (result != VDO_SUCCESS) {
-		*reason = "Could not load geometry block";
+		*reason = "Could not read geometry block";


I think I see what you're trying to clarify here, but I would hesitate to change an error message like this; there could be scripts or users that are relying on the current error string. (They really shouldn't, but this sort of thing can also cause trouble for very little benefit.)
Fine if you still want to do it, just making sure you're aware of the hazards.

I turned the message back to load

lorelei-sakai · 2025-09-23T21:59:38Z

src/c++/vdo/base/encodings.h

 				   struct volume_geometry *geometry);

-int vdo_encode_volume_geometry(u8 *buffer, const struct volume_geometry *geometry);
+int vdo_encode_volume_geometry(u8 *buffer, const struct volume_geometry *geometry,


The version parameter being added to vdo_encode_volume() here (commit 3) doesn't really make sense to me. I expected it to get included with the rest of the function when you "added" it in commit 2. Could we move these bits into commit 2?

After looking at this, I am not sure why encode_volume_geometry ended up in commit 2 at all. Its for writing to disk. I think i should move everything to commit 3 instead

I've reworked the commits. Hopefully they make more sense now.

lorelei-sakai · 2025-09-23T22:15:22Z

src/c++/vdo/base/vdo.c

+	result = vdo_initialize_component_states(&vdo_config, &vdo->geometry, nonce, &vdo->states);
+	if (result != VDO_SUCCESS) {
+		*error_ptr = "Could not initialize data layout during format";
+		if (result == VDO_NO_SPACE) {


Spelling this error condition as
if (result == VDO_NO_SPACE) { ... } else if (result != VDO_SUCCESS) { ... }
would save you an indentation level, and the slighty confusing oddity of setting the error_ptr string twice.

I've done this

src/c++/vdo/user/userVDO.c

lorelei-sakai · 2025-09-23T23:31:21Z

src/c++/vdo/user/userVDO.c

  if (result != VDO_SUCCESS)
    return result;

-  memcpy(block, VDO_GEOMETRY_MAGIC_NUMBER, VDO_GEOMETRY_MAGIC_NUMBER_SIZE);


I'd like it better if the userspace changes got added immediately next to the kernel code they enabled.
And the encode_geometry changes seem completely separable from the blkdev_issue_zeroout and uuid additions. The corresponding kernel code bits are in different commits, the userspace parts could be, too.

As a reviewer, it's easier if the userspace changes are close (in terms of commit count distance) to the matching kernel changes. And also this commit (commit 4) feels a bit overstuffed in general. It's fine-ish, but it feels like it's near the line.

I've reworked the commits to hopefully do what you are asking here

lorelei-sakai · 2025-09-23T23:33:50Z

src/c++/uds/userLinux/uds/linux/types.h

 typedef int pid_t;
 typedef u64 sector_t;

+typedef unsigned int gfp_t;


Why did you move this typedef? I mean the move seems fine but it's three files that seem like they didn't actually have to change?

the new function is in uds. I believe you wanted it there. the original typedef was in vdo/fake, so I moved it to uds as well

lorelei-sakai · 2025-09-23T23:36:54Z

src/c++/vdo/tests/FormatVDOInKernel_t1.c

@@ -0,0 +1,93 @@
+/*


I think I'm too tired to read this carefully, so just one question for now:
This unit test, we talked about it before, right? And it's basically what you had before, there aren't any changes to it?
It looks mostly familiar, and I definitely don't want to waste time re-litigating anything we've already hashed out just because I'm too tired to remember the previous discussion.

I haven't changed the unit test. and yes, we've talked about it before. I used to have 3 separate unit tests and you wanted one, as well as some other cleanup.

lorelei-sakai · 2025-09-23T23:40:30Z

src/perl/vdotest/VDOTest/VDOFormatInKernel.pm

+
+  $storageDevice->extend($lessThanNeeded);
+
+  my $extend1Cursor = $machine->getKernelJournalCursor();


This mostly looks familiar, too, I'll read it carefully later. Just one comment for now, these albMemory => 2 caes should maybe get a comment explaining why we expect them to fail in tests, because in general it's a perfectly reasonable VDO configuration. (If I recall correctly, it's that our pfarms are too small to have the required memory? But we should definitely document a hardware/platform dependency like that.)

bjohnsto

extra blank line added. Reminds me to check my own work-in-progress, I think I've seen cursor do this to me.

Yeah cursor does this too me a lot.

Add additional admin states for in kernel formatting. Signed-off-by: Bruce Johnston <[email protected]>

Add logic to decide when to format and initialize the appropriate structures. Formatting is decided upon by reading the geometry block of the VDO (block 0 on the storage) and determining if that block is all zeroes. If it is we format the device. If not we assume there is already an existing vdo on disk and simply attempt to load that. Signed-off-by: Bruce Johnston <[email protected]>

Add user mode version of uuid_gen function. Signed-off-by: Bruce Johnston <[email protected]>

Write the initialized structures to the underlying disk. We should not be writing to disk during dmsetup create, so do the write during the dm target's pre-resume operation. Signed-off-by: Bruce Johnston <[email protected]>

Add user mode version of blkdev_issue_zeroout function. Since the function is being put in the uds code, we also need to move the definition of gfp_t to uds code. Signed-off-by: Bruce Johnston <[email protected]>

Adds support for in kernel formatting in unit tests and adds a new unit test to test such formatting. One subtest in FormatVDOInKernel_t1,c mimics FormatVDO_t1.c and one mimics FormatVDO_t3.c. There is no version for FormatVDO_t2.c because it makes no sense to have 0 as the logical size passed into the dmsetup code since dmsetup blocks such a thing with an error. Signed-off-by: Bruce Johnston <[email protected]>

Adds additional tests now that formatting actually works. Signed-off-by: Bruce Johnston <[email protected]>

github-actions bot assigned bjohnsto Sep 16, 2025

github-actions bot requested a review from C2Redhat September 16, 2025 17:37

vdo-devel added the untested label Sep 16, 2025

github-actions bot added public testing running public testing failed and removed untested public testing running labels Sep 16, 2025

bjohnsto force-pushed the feature/add_formatting branch from 57230c6 to 185361e Compare September 16, 2025 19:43

github-actions bot removed the public testing failed label Sep 16, 2025

vdo-devel added the untested label Sep 16, 2025

github-actions bot added public testing running public testing done private testing running and removed untested public testing running public testing done private testing running labels Sep 16, 2025

vdo-devel added the private testing done label Sep 16, 2025

bjohnsto force-pushed the feature/add_formatting branch from 185361e to f42f855 Compare September 17, 2025 15:27

github-actions bot removed the private testing done label Sep 17, 2025

vdo-devel added the untested label Sep 17, 2025

github-actions bot added public testing running public testing done private testing running and removed untested public testing running public testing done private testing running labels Sep 17, 2025

vdo-devel added the private testing done label Sep 17, 2025

lorelei-sakai requested changes Sep 23, 2025

View reviewed changes

bjohnsto commented Sep 26, 2025

View reviewed changes

bjohnsto added the on hold PR is on hold for now label Oct 10, 2025

bjohnsto added 7 commits November 17, 2025 12:59

dm vdo: Update admin states for formatting

6174e13

Add additional admin states for in kernel formatting. Signed-off-by: Bruce Johnston <[email protected]>

dm vdo user: Add support for uuid_gen

b9685f6

Add user mode version of uuid_gen function. Signed-off-by: Bruce Johnston <[email protected]>

dm vdo: Save the formatted vdo to disk.

8af9dce

Write the initialized structures to the underlying disk. We should not be writing to disk during dmsetup create, so do the write during the dm target's pre-resume operation. Signed-off-by: Bruce Johnston <[email protected]>

dm vdo user: Add support for blkdev_issue_zeroout

13f843d

Add user mode version of blkdev_issue_zeroout function. Since the function is being put in the uds code, we also need to move the definition of gfp_t to uds code. Signed-off-by: Bruce Johnston <[email protected]>

dm vdo test: Update perl test for in kernel formatting

95ccd11

Adds additional tests now that formatting actually works. Signed-off-by: Bruce Johnston <[email protected]>

bjohnsto removed the on hold PR is on hold for now label Nov 17, 2025

bjohnsto force-pushed the feature/add_formatting branch from f42f855 to 95ccd11 Compare November 17, 2025 20:15

github-actions bot removed the private testing done label Nov 17, 2025

vdo-devel added the untested label Nov 17, 2025

github-actions bot added public testing running public testing done private testing running and removed untested public testing running public testing done private testing running labels Nov 17, 2025

vdo-devel added the private testing done label Nov 17, 2025


		$storageDevice->extend($lessThanNeeded);

		my $extend1Cursor = $machine->getKernelJournalCursor();

[VDO-5752] [VDOSTORY-292] Add formatting logic #334

Are you sure you want to change the base?

[VDO-5752] [VDOSTORY-292] Add formatting logic #334

Uh oh!

Conversation

bjohnsto commented Sep 16, 2025

Uh oh!

github-actions bot commented Sep 16, 2025

Unit Test Results

Uh oh!

lorelei-sakai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjohnsto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants