os/bluestore: Recompression, part 2. New write path. by aclamk · Pull Request #54504 · ceph/ceph

aclamk · 2023-11-14T17:21:09Z

Refactor write path.
The goal is to make write path like:

punch hole
stream blobs into empty place
The difference is that one is no longer limited by the shape and locations of blobs that were present before.
If deferred write will be in use, reused allocations can be used in any blob and any place,
and not only by the blobs that had it before. As a result, a compressed blobs of sizes different then before can replace previous blobs.

#54075 Nice debugs.
#54504 New write path.
#57448 Segmented onode.
#56975 Main
#57450 Test

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 retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

ifed01 · 2024-01-25T14:25:35Z

src/os/bluestore/BlueStore.cc

  _dump_extent_map<LogLevelV>(cct, o.extent_map);
 }

+std::ostream& operator<<(std::ostream& out, const BlueStore::Onode::printer &p)


Shouldn't this go to BlueStore_debug.cc module?

More generally - doesn't it make sense to move all Bluestore's operator<< to BlueStore_debug.cc module?

Moved.
I think we may move all operator<< to there, but I prefer another PR for that.

src/os/bluestore/BlueStore.h

ifed01 · 2024-01-25T14:47:39Z

src/os/bluestore/BlueStore.h

    }
+    struct printer : public BlueStore::printer {
+      const Extent& ext;
+      uint8_t mode;


shouldn't this be uint16_t to match the other printers?

It only used first few bits, but I synchronized it to uint16_t.

ifed01 · 2024-01-25T14:50:51Z

src/os/bluestore/BlueStore.h

 	blob->shared_blob->get_cache()->rm_extent();
      }
    }
+    struct printer : public BlueStore::printer {


We can declare BlueStore::printer as a template class implementing all this type specific logic? Hence wouldn't need to copy-paste for each "rinted" struct/class.

We could, but I tied verbosity of printing to dout levels. It works very nicely I don't want to lose it.

src/os/bluestore/BlueStore_debug.cc

rzarzynski · 2024-02-05T19:39:58Z

Ping.

github-actions · 2024-02-06T14:50:51Z

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

aclamk · 2024-02-29T11:54:30Z

Now #54075 is a base PR for this one.

github-actions · 2024-04-29T12:02:01Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

ifed01 · 2024-01-30T14:37:37Z

src/os/bluestore/Writer.cc

+/// shared_changed - set of shared blobs that are modified,
+///                  including the case of shared blob being empty
+/// statfs_delta - delta of stats
+BlueStore::extent_map_t::iterator BlueStore::_punch_hole_2(


I really don't like the idea to have methods from non-Writer classes being defined in Writer.cc

Makes sense.
New file then? It will never be inlined, so there is no penalty.

since this is a BlueStore's method likely this has to be in BlueStore.cc

We might want to split BlueStore.cc into multiple modules later though.

ifed01 · 2024-01-30T14:38:55Z

src/os/bluestore/Writer.cc

+}
+
+// should _maybe_expand_blob go to Blob ?
+inline void BlueStore::Writer::_maybe_expand_blob(


IMO better move close to other Writer's methods

Or indeed move to Blob class and locate accordingly

ifed01 · 2024-01-30T14:49:16Z

src/os/bluestore/Writer.cc

+      for (auto loc : disk_allocs) {
+        ceph_assert(initial_offset <= loc.length);
+        bufferlist data_chunk;
+        uint32_t data_to_write = std::min(data.length(), loc.length - initial_offset);


IMO better to check data_to_write for zero explicitly before submitting an I/O. Just to be sure there is no any unexpected behavior due to that.

ifed01 · 2024-01-30T14:51:50Z

src/os/bluestore/Writer.cc

+      bluestore_deferred_op_t *op = bstore->_get_deferred_op(txc, data.length());
+      op->op = bluestore_deferred_op_t::OP_WRITE;
+      for (auto& e : disk_allocs) {
+        op->extents.emplace_back(e);


We can resize op->extents explicitly before the loop.
And curious if we need disk_allocs after the call and whether we can start using swap instead.

const on disk_allocs is not useful.
Dropping. swap() will be used.

Update. Cant do. In some cases disk_allocs are used.

but I can op->extents = disk_allocs; instead of loop

src/os/bluestore/Writer.cc

ifed01 · 2024-05-13T08:48:12Z

src/os/bluestore/bluestore_types.cc

+    if (p->length - offset) {
+      released_disk->emplace_back(p->offset + offset, p->length - offset);
+    }
+    released_length += p->length - offset;


This and two next lines could be moved under the above 'if' statement.

On the other hand - (p->length - offset) is always greater than zero so no need in the above 'if' at all...

Lets optimize, even if a bit.
On the other hand, I wonder if optimizer will not squeeze it anyways.

ifed01 · 2024-05-13T08:59:35Z

src/os/bluestore/bluestore_types.cc

+    auto anchor_it = extents.begin() + (anchor - begin);
+    if (insert_element_cnt != 0) {
+      if (insert_element_cnt > 0) {
+        anchor_it = extents.insert(anchor_it, insert_element_cnt, bluestore_pextent_t(0, 0));


nit: IMO better insert bluestore_pextent_t(EMPTY, 0)

ifed01 · 2024-05-13T09:01:08Z

src/os/bluestore/bluestore_types.cc

+    // and insert hold in this place
+    int32_t insert_element_cnt = hold_size - (p - anchor);
+    auto anchor_it = extents.begin() + (anchor - begin);
+    if (insert_element_cnt != 0) {


IMO more readable/elegant would be:
if (insert_element_cnt >0) {
...
} else if ((insert_element_cnt < 0) {
...
}

changed it; now two independent ifs.

ifed01 · 2024-05-13T09:08:47Z

src/os/bluestore/bluestore_types.cc

+        anchor_it = extents.erase(anchor_it, anchor_it + (-insert_element_cnt));
+      }
+    }
+    for (uint32_t i = 0; i < hold_size; i++) {


hint: one could use std::copy if define hold as std::array.

I loot at this function and I think it is too clever.
What do you think about refactor to make it 2 phased:

find it_begin, offset_begin; it_end, offset_end

move to released, cut out KNOWN region
do you feel it might produce clearer code?

pereman2 · 2024-05-15T11:05:49Z

src/os/bluestore/Writer.cc

+    for (auto q: bd) {
+      *_dout << " " << lof << "~" << q.disk_data.length();
+      lof += q.disk_data.length();
+    }


skip this if not on level 25?

We do code to execute only on level 25+:

dout(25); ... conditional code in new scope... *_dout << dendl;

pereman2 · 2024-05-15T13:08:10Z

src/os/bluestore/Writer.cc

+  dout(25) << "after punch_hole_2: " << std::endl << onode->print(pp_mode) << dendl;
+
+  uint32_t au_size = bstore->min_alloc_size;
+  if (au_size != bstore->block_size) {


why can't au_size != block-size for this to work?

That would be an empty run.
The code is intended to use portions of blobs that are allocated (AU=64K) but have never been written.

pereman2 · 2024-05-15T13:45:00Z

src/os/bluestore/Writer.cc

+        if (left_do_pad) {
+          tmp.append_zero(location - left_location);
+        } else {
+          tmp = _read_self(left_location, location - left_location);


we may be able to make this read async.

I am considering it, but first lets get metrics how often it actually happens.
Maybe telemetry can help in this case.

pereman2 · 2024-05-15T14:33:57Z

src/os/bluestore/Writer.cc

+// whether to use deferred or direct writes.
+// The assumption behind it is that having parts of write executed as
+// deferred and other parts as direct is suboptimal in any case.
+void BlueStore::Writer::_deferred_decision(uint32_t need_size)


add something like "allocates" to function name so that we know this is where disk allocations really happen?

_defer_or_allocate ?

pereman2 · 2024-05-15T14:35:08Z

src/os/bluestore/Writer.cc

+    _try_put_data_on_allocated(location, data_end, ref_end, bd, after_punch_it);
+  }
+  if (location != data_end) {
+    uint32_t need_size = p2roundup(data_end, au_size) - p2align(location, au_size);


dumb question: what if need_size is too big it conflicts with other writes causing a enospc?

This particular code got changed under pressure of compressed blobs.
Now need_size is calculated differently.

New "write_lat" tracks wallclock time spent in execution of BlueStore::_write(). Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Added _do_write_v2 function to BlueStore. Function is not integrated yet. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Add new conf variable. bluestore_write_v2 = true : use new _do_write_v2() function bluestore_write_v2 = false : use legacy _do_write() function This variable is read only at start time. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

We need to scan all extents that are under proposed read / padding. It was broken and we could clear out something defined. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

The search must go over entire range where we can have aligned mutable blobs. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

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

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

Result of ongoing review. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

It was not checked if necessary location is still within blob range. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

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

Plus making const& variables in `for`. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

BufferSpace is not with Onode, not Blob. Modify code to adapt to this change. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Usually the data we put to disk is AU aligned. In weird cases like AU=16K we put less data than we allocated. _crop_allocs_to_io trims allocated extents into disk block extents to reflect real IO. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Added missing files to alienstore CMake list. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Clang fails at _construct_at(). Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

More diligent calcualtion algorithm of need_size. Takes into account front and back alignment. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

1) Algorithm assumed that blob->blob_start() is aligned to csum size. It is true for blobs created by write_v2, but write_v1 can generate blob like: begin = 0x9000, size = 0x6000, csum = 0x2000. 2) Blobs with unused were selected even if those need to be expanded. This is illegal since we cannot expand unused. Fixed blob selection algorithm. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

For write_v2 create fallback to write_v1 if compression is selected. This is temporary until compression dedicated to benefit from v2 is merged. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Added conf.bluestore_write_v2_random. This is useful only for testing. If set, it overrides value of bluestore_write_v2 with a random true/false selection. It is useful for v1 / v2 compatibility testing. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Add framework for various random options for debug bluestore. Use framework to select: - write_v1 - write_v2 - write_v1 / write_v2 selected at random Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

…e_extents The #1 and #2 elements could form a continuous sequence but still not joined: Expected equality of these values: result Which is: { 0x7b138000~48000, 0x883b0000~48000, 0xf0c10000~10000, 0x727b8000~38000 } mid Which is: { 0x7b138000~30000, 0x7b168000~18000, 0x883b0000~48000, 0xf0c10000~10000, 0x727b8000~38000 } Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

1) moved stats and blobs update to Writer::do_write 2) preallocate space in Writer:_split_data 3) fixed Writer::_write_expand_l that could check one extent too much Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

4) remove Writer::shared_changed and use txc::shared_blobs directly Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

ifed01 · 2024-08-08T11:04:29Z

jenkins test make check

aclamk · 2024-08-08T13:54:14Z

jenkins test make check

aclamk · 2024-08-12T11:46:47Z

jenkins test make check

aclamk requested a review from a team as a code owner November 14, 2023 17:21

github-actions bot added bluestore build/ops core tests labels Nov 14, 2023

aclamk marked this pull request as draft November 14, 2023 17:21

aclamk requested a review from pereman2 November 14, 2023 17:31

aclamk force-pushed the wip-aclamk-bs-refactor-write-path branch from 9c7fb12 to fb1615f Compare January 5, 2024 09:12

aclamk force-pushed the wip-aclamk-bs-refactor-write-path branch from fb1615f to 21b5dfb Compare January 16, 2024 11:21

github-actions bot added the common label Jan 16, 2024

aclamk force-pushed the wip-aclamk-bs-refactor-write-path branch from 21b5dfb to 736de0c Compare January 16, 2024 11:22

aclamk marked this pull request as ready for review January 23, 2024 14:36

ifed01 reviewed Jan 25, 2024

View reviewed changes

github-actions bot added the needs-rebase label Feb 6, 2024

aclamk force-pushed the wip-aclamk-bs-refactor-write-path branch from 736de0c to b0235a6 Compare February 28, 2024 20:56

github-actions bot removed the needs-rebase label Feb 28, 2024

github-actions bot added the stale label Apr 29, 2024

ifed01 reviewed May 13, 2024

View reviewed changes

github-actions bot removed the stale label May 13, 2024

aclamk force-pushed the wip-aclamk-bs-refactor-write-path branch from b0235a6 to 9566e0f Compare May 13, 2024 12:15

aclamk changed the title ~~os/bluestore: Refactor write path~~ os/bluestore: Recompression, part 2. New write path. May 13, 2024

pereman2 reviewed May 15, 2024

View reviewed changes

aclamk added 23 commits August 7, 2024 10:55

os/bluestore: Add "write_lat" perf counter

03bac5d

New "write_lat" tracks wallclock time spent in execution of BlueStore::_write(). Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Added new write path with BlueStore

f0c82b3

Added _do_write_v2 function to BlueStore. Function is not integrated yet. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Add bluestore_write_v2 conf variable

df43f47

Add new conf variable. bluestore_write_v2 = true : use new _do_write_v2() function bluestore_write_v2 = false : use legacy _do_write() function This variable is read only at start time. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Modify _write_expand_l/r

3f4bb3f

We need to scan all extents that are under proposed read / padding. It was broken and we could clear out something defined. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Fix _try_reuse_allocated_r in Writer.cc

5500454

The search must go over entire range where we can have aligned mutable blobs. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Review fixes of Writer

bf824f7

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

os/bluestore: Writer, rename _deferred_decision -> _defer_or_allocate

d19b998

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

os/bluestore: Cleanup on bluestore_blob_t::release_extents

83d6a4e

Result of ongoing review. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Writer, fix _try_reuse_allocated_l/r

9e847a9

It was not checked if necessary location is still within blob range. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Add perf counters to Writer

22fa03e

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

os/bluestore: Review fixes to Writer

96065b1

Plus making const& variables in `for`. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Fix after rebase

1c5fd0f

BufferSpace is not with Onode, not Blob. Modify code to adapt to this change. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

crimson/alienstore: New write path & debug

255ad42

Added missing files to alienstore CMake list. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Writer, fix for clang

d2fda6c

Clang fails at _construct_at(). Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Writer, improved calculation of need_size

938a0f1

More diligent calcualtion algorithm of need_size. Takes into account front and back alignment. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Add compression fallback

3b5b7fa

For write_v2 create fallback to write_v1 if compression is selected. This is temporary until compression dedicated to benefit from v2 is merged. Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

qa/bluestore: Add write_v1/v2 selection

8bd233b

Add framework for various random options for debug bluestore. Use framework to select: - write_v1 - write_v2 - write_v1 / write_v2 selected at random Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Write_v2 changes

1b24c7c

1) moved stats and blobs update to Writer::do_write 2) preallocate space in Writer:_split_data 3) fixed Writer::_write_expand_l that could check one extent too much Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

os/bluestore: Write_v2 changes

59857a7

4) remove Writer::shared_changed and use txc::shared_blobs directly Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

aclamk force-pushed the wip-aclamk-bs-refactor-write-path branch from 746e82c to 59857a7 Compare August 7, 2024 10:56

github-actions bot removed the needs-rebase label Aug 7, 2024

ifed01 approved these changes Aug 8, 2024

View reviewed changes

aclamk merged commit a787a91 into ceph:main Aug 13, 2024

Conversation

aclamk commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ifed01 Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

rzarzynski commented Feb 5, 2024

Uh oh!

github-actions bot commented Feb 6, 2024

Uh oh!

aclamk commented Feb 29, 2024

Uh oh!

github-actions bot commented Apr 29, 2024

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!

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!

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!

aclamk commented Nov 14, 2023 •

edited

Loading

ifed01 Jan 25, 2024 •

edited

Loading