crimson/os/seastore: write "seastore block device" signature in write_superblock by mohit84 · Pull Request #64333 · ceph/ceph

mohit84 · 2025-07-03T12:30:54Z

To validate the seastore device by cephadm write a signature "seastore block device" during superblock creation.

Fixes: https://tracker.ceph.com/issues/71954

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins test classic perf Jenkins Job | Jenkins Job Definition
jenkins test crimson perf Jenkins Job | Jenkins Job Definition
jenkins test signed Jenkins Job | Jenkins Job Definition
jenkins test make check Jenkins Job | Jenkins Job Definition
jenkins test make check arm64 Jenkins Job | Jenkins Job Definition
jenkins test submodules Jenkins Job | Jenkins Job Definition
jenkins test dashboard Jenkins Job | Jenkins Job Definition
jenkins test dashboard cephadm Jenkins Job | Jenkins Job Definition
jenkins test api Jenkins Job | Jenkins Job Definition
jenkins test docs ReadTheDocs | Github Workflow Definition
jenkins test ceph-volume all Jenkins Jobs | Jenkins Jobs Definition
jenkins test windows Jenkins Job | Jenkins Job Definition
jenkins test rook e2e Jenkins Job | Jenkins Job Definition

mohit84 · 2025-07-03T12:32:20Z

jenkins test make check

Matan-B

This seems like the correct approach. Left few suggestions and let's wait for @cyx1231st approval.

Probably worth mentioning that this comes to help ceph volume's has_seastore_label helper.

Matan-B · 2025-07-03T12:47:19Z

src/crimson/os/seastore/segment_manager/block.cc

    [=, &device](auto &bp)
  {
    bufferlist bl;
+    bl.append(SUPERBLOCK_MAGIC);


Is there a reason not to move this into block_sm_superblock_t encode/decode?

There is no specific reason

Agree with matan, this should be part of block_sm_superblock_t, then the change will affect the other tools relying on block_sm_superblock_t immediately, which is actually better.

see #63604

@cyx1231st I have updated the note why can;t we add signature via DENC.

Matan-B · 2025-07-03T12:48:25Z

src/crimson/os/seastore/seastore_types.h

+constexpr const char* SUPERBLOCK_MAGIC = "seastore block device\n";
+constexpr size_t SUPERBLOCK_MAGIC_LEN = strlen(SUPERBLOCK_MAGIC);
+


nit: Can we name this label instead of magic?

cyx1231st

The change is straightforward.

If we are not going to support seastore label in the other 2 superblocks in this PR, for completeness, please add TODO comments to them to clarify that they won't be compatible to (which) related features.

cyx1231st · 2025-07-04T02:51:43Z

src/crimson/os/seastore/segment_manager/block.cc

    [=, &device](auto &bp)
  {
    bufferlist bl;
+    bl.append(SUPERBLOCK_MAGIC);


Agree with matan, this should be part of block_sm_superblock_t, then the change will affect the other tools relying on block_sm_superblock_t immediately, which is actually better.

see #63604

src/crimson/os/seastore/segment_manager/block.cc

mohit84 · 2025-07-07T10:32:44Z

jenkins test make check

Matan-B · 2025-07-07T11:13:43Z

src/crimson/os/seastore/seastore_types.h

  1 << (std::numeric_limits<device_id_t>::digits - 1);

+constexpr const char* SEASTORE_SUPERBLOCK_SIGN = "seastore block device\n";
+constexpr size_t SEASTORE_SUPERBLOCK_SIGN_LEN = strlen(SEASTORE_SUPERBLOCK_SIGN);


/home/jenkins-build/build/workspace/ceph-pull-requests/src/crimson/os/seastore/seastore_types.h:122:18: error: constexpr variable 'SEASTORE_SUPERBLOCK_SIGN_LEN' must be initialized by a constant expression 122 | constexpr size_t SEASTORE_SUPERBLOCK_SIGN_LEN = strlen(SEASTORE_SUPERBLOCK_SIGN); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/jenkins-build/build/workspace/ceph-pull-requests/src/crimson/os/seastore/seastore_types.h:122:49: note: non-constexpr function 'strlen' cannot be used in a constant expression

Matan-B · 2025-07-07T11:16:34Z

src/crimson/os/seastore/segment_manager/block.cc

+    // Write the SEASTORE_SUPERBLOCK_SIGN at offset 0 before
+    // encoding the superblock. If the signature is added inside
+    // the block_sm_superblock_t structure (via DENC), it won't be
+    // written at offset 0 because DENC encoding add some metadata.
+    // Therefore prepend the signature to the bufferlist before
+    // encoding the superblock sothat signature always appears at the
+    // start of the written data.


nit: This sections here would probably better fit in the commit message.
We can add a more specific comment here such as:
// encode SEASTORE_SUPERBLOCK_SIGN at offset 0 before encoding anything else

Matan-B · 2025-07-07T11:31:10Z

src/crimson/os/seastore/segment_manager/block.cc

+    // Write the SEASTORE_SUPERBLOCK_SIGN at offset 0 before
+    // encoding the superblock. If the signature is added inside
+    // the block_sm_superblock_t structure (via DENC), it won't be
+    // written at offset 0 because DENC encoding add some metadata.


If DENC encodes additional data, can we use encode/decode instead so that we could move this into encode/decode?
As @cyx1231st mentioned earlier encoding additional data without properly decoding it might break future users (i.e objectstore-tool).

I had already discussed with @cyx1231st he agreed with patch.

As disscussed, it's simpler to place SEASTORE_SUPERBLOCK_SIGN directly at the start of the device without headers, like bluestore.

We can switch to encode/decode if possible.

mohit84 · 2025-07-07T12:27:10Z

jenkins test make check

cyx1231st

LGTM otherwise.

src/crimson/os/seastore/seastore_types.h

src/crimson/os/seastore/segment_manager/block.cc

src/crimson/os/seastore/seastore_types.h

cyx1231st · 2025-07-08T08:32:45Z

src/crimson/os/seastore/segment_manager/block.cc

+    // Write the SEASTORE_SUPERBLOCK_SIGN at offset 0 before
+    // encoding the superblock. If the signature is added inside
+    // the block_sm_superblock_t structure (via DENC), it won't be
+    // written at offset 0 because DENC encoding add some metadata.


As disscussed, it's simpler to place SEASTORE_SUPERBLOCK_SIGN directly at the start of the device without headers, like bluestore.

We can switch to encode/decode if possible.

cyx1231st · 2025-07-08T08:36:53Z

^ can you fix the commit title as well?

Also better to explain why we need this PR in the commit message (the requirement, e.g. ceph-volume).

mohit84 · 2025-07-10T05:59:56Z

^ can you fix the commit title as well?

Also better to explain why we need this PR in the commit message (the requirement, e.g. ceph-volume).

done

src/crimson/os/seastore/seastore_types.h

mohit84 · 2025-07-10T08:03:55Z

jenkins test make check

Write the SEASTORE_SUPERBLOCK_SIGN at offset 0 before encoding the superblock. If the signature is added inside the block_sm_superblock_t structure (via DENC), it won't be written at offset 0 because DENC encoding add some metadata. Therefore prepend the signature to the bufferlist before encoding the superblock sothat signature always appears at the start of the written data Fixes: https://tracker.ceph.com/issues/71954 Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

mohit84 · 2025-07-10T08:14:12Z

jenkins test make check

mohit84 · 2025-07-11T05:06:26Z

https://shaman.ceph.com/builds/ceph/wip-moagrawa-crimson-only-seastore_magic/

mohit84 · 2025-07-14T04:48:11Z

https://pulpito.ceph.com/moagrawa-2025-07-14_01:47:30-crimson-rados-wip-moagrawa-crimson-only-seastore_magic-distro-crimson-debug-smithi/

Matan-B · 2025-07-14T06:29:22Z

https://pulpito.ceph.com/moagrawa-2025-07-14_01:47:30-crimson-rados-wip-moagrawa-crimson-only-seastore_magic-distro-crimson-debug-smithi/

Hey Mohit, can you please review the run to verify that all the failures are known?

Matan-B · 2025-07-14T06:29:32Z

jenkins test api

Matan-B · 2025-07-14T06:29:43Z

jenkins test make check

mohit84 · 2025-07-14T12:22:58Z

https://pulpito.ceph.com/moagrawa-2025-07-14_01:47:30-crimson-rados-wip-moagrawa-crimson-only-seastore_magic-distro-crimson-debug-smithi/

Hey Mohit, can you please review the run to verify that all the failures are known?

There are total two types of failure

During tear down the test case test_aio_mirror_image_create_snapshot is failing for below job IDs
8384759 8384770 8384817 8384826 8384874 8384883
ceph_test_cls_cmpomap failed for noexist test for below Job IDs
8384719 8384777 8384834

I have checked the failures were specific to the ceph branch version, on latest main branch these failures are resolved so we can say it is ok.

Matan-B · 2025-07-14T12:46:14Z

https://pulpito.ceph.com/moagrawa-2025-07-14_01:47:30-crimson-rados-wip-moagrawa-crimson-only-seastore_magic-distro-crimson-debug-smithi/

Hey Mohit, can you please review the run to verify that all the failures are known?

There are total two types of failure

During tear down the test case test_aio_mirror_image_create_snapshot is failing for below job IDs
8384759 8384770 8384817 8384826 8384874 8384883

ceph_test_cls_cmpomap failed for noexist test for below Job IDs
8384719 8384777 8384834

I have checked the failures were specific to the ceph branch version, on latest main branch these failures are resolved so we can say it is ok.

For 1, it's probably due to the branch being a bit outdated - I'm ok with merging since the issue also appeared in bluestore which is not affected by this PR (and most of the test did pass).
For 2, The qa dir used was of main which included the fix while the build didn't.

mohit84 requested a review from a team as a code owner July 3, 2025 12:30

github-actions bot added the crimson label Jul 3, 2025

mohit84 requested review from Matan-B and cyx1231st July 3, 2025 12:31

mohit84 mentioned this pull request Jul 3, 2025

ceph-volume: add seastore OSDs support #64323

Merged

Matan-B reviewed Jul 3, 2025

View reviewed changes

Matan-B added this to Crimson Jul 3, 2025

Matan-B moved this to Awaits review in Crimson Jul 3, 2025

cyx1231st reviewed Jul 4, 2025

View reviewed changes

src/crimson/os/seastore/segment_manager/block.cc Outdated Show resolved Hide resolved

Matan-B moved this from Awaits review to In Progress in Crimson Jul 6, 2025

cyx1231st added seastore seastore-disruptive-upgrade labels Jul 7, 2025

mohit84 force-pushed the seastore_magic branch from 1db586e to 713dd1a Compare July 7, 2025 10:30

Matan-B reviewed Jul 7, 2025

View reviewed changes

mohit84 force-pushed the seastore_magic branch from 713dd1a to 6ceeef9 Compare July 7, 2025 12:25

cyx1231st reviewed Jul 8, 2025

View reviewed changes

cyx1231st changed the title ~~seastore: Write "seastore block device" signature in write_superblock~~ crimson/os/seastore: write "seastore block device" signature in write_superblock Jul 8, 2025

mohit84 force-pushed the seastore_magic branch from 6ceeef9 to 7b143ba Compare July 10, 2025 05:58

cyx1231st reviewed Jul 10, 2025

View reviewed changes

src/crimson/os/seastore/seastore_types.h Outdated Show resolved Hide resolved

mohit84 force-pushed the seastore_magic branch from 7b143ba to 5fb9151 Compare July 10, 2025 08:03

mohit84 force-pushed the seastore_magic branch from 5fb9151 to 06e8fd1 Compare July 10, 2025 08:12

cyx1231st approved these changes Jul 10, 2025

View reviewed changes

cyx1231st moved this from In Progress to Needs QA in Crimson Jul 10, 2025

cyx1231st added the needs-crimson-seastore-qa label Jul 10, 2025

Matan-B merged commit 5267745 into ceph:main Jul 14, 2025
13 checks passed

Matan-B moved this from Needs QA to Merged (Main) in Crimson Jul 14, 2025

		constexpr const char* SUPERBLOCK_MAGIC = "seastore block device\n";
		constexpr size_t SUPERBLOCK_MAGIC_LEN = strlen(SUPERBLOCK_MAGIC);

Conversation

mohit84 commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

mohit84 commented Jul 3, 2025

Uh oh!

Matan-B left a comment

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!

cyx1231st left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mohit84 commented Jul 7, 2025

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!

mohit84 commented Jul 7, 2025

Uh oh!

cyx1231st left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyx1231st commented Jul 8, 2025

Uh oh!

mohit84 commented Jul 10, 2025

Uh oh!

Uh oh!

mohit84 commented Jul 10, 2025

Uh oh!

mohit84 commented Jul 10, 2025

Uh oh!

mohit84 commented Jul 11, 2025

Uh oh!

mohit84 commented Jul 14, 2025

Uh oh!

Matan-B commented Jul 14, 2025

Uh oh!

Matan-B commented Jul 14, 2025

Uh oh!

Matan-B commented Jul 14, 2025

Uh oh!

mohit84 commented Jul 14, 2025

Uh oh!

Matan-B commented Jul 14, 2025

Uh oh!

Uh oh!

mohit84 commented Jul 3, 2025 •

edited

Loading