os/bluestore: Fast WAL for RocksDB by aclamk · Pull Request #62224 · ceph/ceph

aclamk · 2025-03-11T11:27:53Z

The work here is based on #56927.
It implements the design principles showed there, but with a bit different implementation details.

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

github-actions · 2025-03-11T11:28:10Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

aclamk · 2025-03-11T21:24:33Z

jenkins test make check

aclamk · 2025-03-12T08:33:17Z

jenkins test make check

aclamk · 2025-03-18T09:06:38Z

jenkins test api

anthonyeleven · 2025-03-18T14:24:27Z

src/common/options/global.yaml.in

+  type: bool
+  level: advanced
+  desc: Enables a faster backend in BlueFS for WAL writes.
+  long_desc: Enabling this feature will reduce ~50% the amount of fdatasync syscalls issued by WAL writes. This happens because we embed metadata


fdatasync double backticks

anthonyeleven · 2025-03-18T14:24:58Z

src/common/options/global.yaml.in

+  desc: Enables a faster backend in BlueFS for WAL writes.
+  long_desc: Enabling this feature will reduce ~50% the amount of fdatasync syscalls issued by WAL writes. This happens because we embed metadata
+    with the data itself. Downgrading from a version that uses v2 to v1 will require running `ceph-bluestore-tool --command downgrade-wal-to-v1`
+    to move wal files to previous format.


to migrate WAL entries to the previous format.

ifed01 · 2025-03-19T11:48:39Z

src/include/encoding.h

  using ::ceph::encode;					     \
  do {

+#define ENCODE_START_FILLER(v, compat, filler_in)			     \


To be removed, not used any more

ifed01 · 2025-03-19T11:48:50Z

src/include/encoding.h

+  * @param bl bufferlist we were encoding to
+  * @param new_struct_compat struct-compat value to use
+  */
+#define ENCODE_FINISH_FILLER()      \


To be removed, not used any more

src/common/options/global.yaml.in

src/os/bluestore/BlueFS.h

src/os/bluestore/BlueFS.cc

ifed01 · 2025-03-20T16:28:39Z

src/os/bluestore/BlueFS.cc

+  h->file->fnode.wal_size += content_length;
+  ceph_le64 flush_length_le(content_length);
+  h->wal_header_filler.copy_in(sizeof(flush_length_le), (char*)&flush_length_le);
+  uint64_t length = sizeof(File::wal_flush_t::wal_length_t) + content_length + sizeof(h->file->wal_marker);


Wouldn't it be better to use head_size()/tail_size() helpers instead of sizeof?

Replaced all sizeofs with header_size() / tail_size() calls.

src/os/bluestore/BlueFS.cc

ifed01 · 2025-03-20T16:40:31Z

src/os/bluestore/BlueFS.h

+        hashed_ino ^= hashed_ino << 11;
+        hashed_ino ^= hashed_ino << 23;
+        // use hashed ino in a endiness-agnostic way
+        for (int i = 0; i < 8; i++) {


8 to be replaced with sizeof(wal_marker) or something

I'm not sure with "8", but I added comment to explain the code.

AFAIU 8 comes from marker's length. Shouldn't it be sizeof(marker) then?

src/os/bluestore/BlueFS.h

ifed01 · 2025-03-20T16:49:01Z

src/os/bluestore/BlueFS.cc

  }
-  if (const unsigned tail = bl.length() & ~super.block_mask(); tail) {
-    const auto padding_len = super.block_size - tail;
+  unsigned padding_len = 0;


Apaprently no need to declare outside 'if' statement

ifed01 · 2025-03-27T14:49:47Z

src/common/options/global.yaml.in

-    to move wal files to previous format.
+  long_desc: In envelope mode BlueFS files do not need to update metadata. When applied to RocksDB WAL files,
+    it reduces by ~50% the amount of fdatasync syscalls.
+    Downgrading from an envelope mode to legacy mode requires `ceph-bluestore-tool --command downgrade-wal-to-v1`.


'wal-to-v1' is kinda weird now...

github-actions · 2025-03-28T16:01:57Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

WAL file mode allows to cut by 50% of fdatasync(). For regular files, we independently sync file data and file metadata. In WAL mode we are able to recover most metadata from file bytestream. Hence - we only sync file data. Signed-off-by: Pere Diaz Bou <pere-altea@hotmail.com> (cherry picked from commit 38519ff) Amended-by: Adam Kupczyk

Signed-off-by: Pere Diaz Bou <pere-altea@hotmail.com> (cherry picked from commit 4961dca)

This is useful to have ability to create a contigous_filler variable. Otherwise one needs std::optional<>. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Main changes: - remove bluefs_wal_header_t; we no longer use denc directly, instead we read length and marker directly - get rid of wal_limit; this is replaced with splitting WAL_V2=>{WAL_V2,WAL_V2_FIN}. WAL_V2 means there could be more wal data, WAL_V2_FIN means file was orderly closed = no more data. - modified wal_marker_t (previously WALMarker) to be calculated in endiness-agnostic way. - replaced _wal_update_size with _wal_index_file; it does a similar function, but is less convoluted now - simplified _wal_read - now BlueFS::close_writer() flushes data - BlueRocksEnv's BlueRocksWritableFile::truncate() now does the action Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Macros are not longer used. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

It is just not used. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

If we happen to shutdown with dirty.pending_releases, after mount we will most likely want to free them. And its an error - they will already be free. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

The fact that test was broken was hidden by the error in BlueFS::_init_alloc, that created the allocator with invalid alloc unit size. After that was fixed, also test needed to be fixed. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Signed-off-by: Pere Diaz Bou <pere-altea@hotmail.com> (cherry picked from commit 041edad)

Signed-off-by: Pere Diaz Bou <pere-altea@hotmail.com> (cherry picked from commit a65118d)

Dropped checks around free space. Now downgrades one file at a time. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Changed logic for selecting bluefs_super_t wal_version field. Now it is more a result of the data that is in bluefs log and less directly controlled. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

The check involves timing check. Jenkins is too unpredictable with performance. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

This refactor turns WAL_v2 into globally available ENVELOPE_MODE. The RocksDB WAL files are now just using existing feature. Now adding new file encoding schemes will be easier. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

…pe_mode No logic change, just conf name and constants adaptation. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

This is the variant of DENC_START that manually controls encoding version. As being incompatible with static compile-time checking of StructVChecker, new (legacy) mode had to be introduced. Also, Removed struct_v and struct_compat from denc_finish. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

aclamk · 2025-03-28T16:19:00Z

@markhpc I removed your "need-qa" label. This thing passed preliminary teuthology, and this version is not final.

Note: critical bluestore PRs we are weekly testing / reviewing with aclamk-testing-* labels.

+ relocated config bluefs_wal_envelope_mode close to other bluefs configs Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

aclamk · 2025-03-29T06:33:08Z

jenkins test make check arm64

aclamk · 2025-04-07T12:26:19Z

Approved by https://tracker.ceph.com/issues/70787.

aclamk requested a review from a team as a code owner March 11, 2025 11:27

aclamk marked this pull request as draft March 11, 2025 11:28

github-actions bot added bluestore build/ops common core tests needs-rebase labels Mar 11, 2025

aclamk force-pushed the wip-aclamk-pere-wal-fsync branch from 31e4731 to c114aea Compare March 11, 2025 17:59

github-actions bot added documentation and removed needs-rebase labels Mar 11, 2025

aclamk added aclamk-testing-phoebe bluestore testing and removed aclamk-testing-phoebe bluestore testing labels Mar 11, 2025

aclamk force-pushed the wip-aclamk-pere-wal-fsync branch from c114aea to ab34d64 Compare March 11, 2025 18:42

aclamk added the aclamk-testing-phoebe bluestore testing label Mar 12, 2025

markhpc added the performance label Mar 13, 2025

aclamk force-pushed the wip-aclamk-pere-wal-fsync branch from ab34d64 to 41e8d24 Compare March 17, 2025 17:16

aclamk force-pushed the wip-aclamk-pere-wal-fsync branch from 6780770 to 10842c6 Compare March 18, 2025 10:42

aclamk marked this pull request as ready for review March 18, 2025 12:38

aclamk requested a review from a team as a code owner March 18, 2025 12:38

anthonyeleven approved these changes Mar 18, 2025

View reviewed changes

ifed01 reviewed Mar 20, 2025

View reviewed changes

aclamk force-pushed the wip-aclamk-pere-wal-fsync branch 2 times, most recently from 1032eea to c2fdd21 Compare March 21, 2025 21:49

ifed01 approved these changes Mar 27, 2025

View reviewed changes

markhpc added the needs-qa label Mar 27, 2025

github-actions bot added the needs-rebase label Mar 28, 2025

pereman2 and others added 17 commits March 28, 2025 16:05

os/bluestore: test bluefs wal v2

90bf207

Signed-off-by: Pere Diaz Bou <pere-altea@hotmail.com> (cherry picked from commit 4961dca)

buffer.h: Add empty constructor to contigous_filler

35c1d4a

This is useful to have ability to create a contigous_filler variable. Otherwise one needs std::optional<>. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

include/encoding.h: Remove unused _FILLER_ macros

167bba6

Macros are not longer used. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Get rid of unused FileReader's "random"

8cdf4fb

It is just not used. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

test/unittest_bluefs: Bunch of tests for wal_v2

db452e6

Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore/bluefs: Clear pending releases

09fd806

If we happen to shutdown with dirty.pending_releases, after mount we will most likely want to free them. And its an error - they will already be free. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

test/objectstore: Fix unittest for truncation

f6ef7b5

The fact that test was broken was hidden by the error in BlueFS::_init_alloc, that created the allocator with invalid alloc unit size. After that was fixed, also test needed to be fixed. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: bluestore tool downgrade-wal-to-v1 command

f0b0526

Signed-off-by: Pere Diaz Bou <pere-altea@hotmail.com> (cherry picked from commit 041edad)

os/bluestore: document downgrade-wal-to-v1

7265bde

Signed-off-by: Pere Diaz Bou <pere-altea@hotmail.com> (cherry picked from commit a65118d)

os/bluestore: Simplify downgrade-wal-to-v1

4fb2e03

Dropped checks around free space. Now downgrades one file at a time. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Make wal_v2 selection more robust

24ec5c7

Changed logic for selecting bluefs_super_t wal_version field. Now it is more a result of the data that is in bluefs log and less directly controlled. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

test/bluefs: Disable check_wal_v2 feature on jenkins

c065abc

The check involves timing check. Jenkins is too unpredictable with performance. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Generalize WAL_v2 => envelope_mode

613ff6c

This refactor turns WAL_v2 into globally available ENVELOPE_MODE. The RocksDB WAL files are now just using existing feature. Now adding new file encoding schemes will be easier. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

tests/objectstore/bluefs_tests: Adapted to transition "WALv2"=>envelo…

7631716

…pe_mode No logic change, just conf name and constants adaptation. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

aclamk removed the needs-qa label Mar 28, 2025

aclamk force-pushed the wip-aclamk-pere-wal-fsync branch from 13ed631 to 11f9a67 Compare March 28, 2025 16:28

github-actions bot removed the needs-rebase label Mar 28, 2025

os/bluestore: Rename downgrade_wal_to_v1 => revert_wal_to_plain

014528d

+ relocated config bluefs_wal_envelope_mode close to other bluefs configs Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

aclamk added aclamk-testing-ganymede bluestore testing and removed aclamk-testing-phoebe bluestore testing labels Apr 2, 2025

aclamk merged commit 09e8bd4 into ceph:main Apr 9, 2025
12 checks passed

Conversation

aclamk commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

github-actions bot commented Mar 11, 2025

Uh oh!

aclamk commented Mar 11, 2025

Uh oh!

aclamk commented Mar 12, 2025

Uh oh!

aclamk commented Mar 18, 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!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 28, 2025

Uh oh!

aclamk commented Mar 28, 2025

Uh oh!

aclamk commented Mar 29, 2025

Uh oh!

aclamk commented Apr 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aclamk commented Mar 11, 2025 •

edited

Loading