crimson/os/seastore: add fine-grained cache by ljx023 · Pull Request #50421 · ceph/ceph

ljx023 · 2023-03-07T11:35:49Z

The fine-grained cache is introduced to improve the performance and reduce read amplification when reading small chunks of data from large extents. Overall, we add a structure named "BufferSpace" to control the small buffers in CachedExtent so that it can load the exact required data from disk. Now the ObjectDataBlock can be incomplete. Some paths are modified to adapt to this change.

Signed-off-by: Jianxin Li jianxin1.li@intel.com

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. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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

Signed-off-by: Jianxin Li <jianxin1.li@intel.com>

construct BufferSpace only for ObjectDataBlock. Signed-off-by: Jianxin Li <jianxin1.li@intel.com>

ljx023 · 2023-03-07T11:40:21Z

The performance of randread from large extents does increase after adding this patch.

get unload interval in extent and load them in parallel. Signed-off-by: Jianxin Li <jianxin1.li@intel.com>

Signed-off-by: Jianxin Li <jianxin1.li@intel.com>

full extent got from transaction if not fully loaded Signed-off-by: Jianxin Li <jianxin1.li@intel.com>

xxhdx1985126 · 2023-03-22T03:01:39Z

src/crimson/os/seastore/cached_extent.h

-      version(other.version),
-      poffset(other.poffset) {}
+  CachedExtent(extent_len_t length) : ptr(ceph::bufferptr(
+    buffer::create_page_aligned(length))) {}


Is this new Constructor necessary? It seems a little bit similar to CachedExtent::(ceph::bufferptr &&), maybe only one of them needs to be preserved?

Who is the user of this new constructor?

It is prepared for all types of CachedExtent other than ObjectBlock. Since I modify the constructor in get_extent, I add the new constructor in case. The old one is still used in some places. If we ensure CachedExtent(extent_len_t length) is used only for ObjectBlock or modify the CachedExtent(ceph::bufferptr &&ptr) in other places, only one of them is needed. (I haven't checked them)

If we ensure CachedExtent(extent_len_t length) is used only for ObjectBlock or modify the CachedExtent(ceph::bufferptr &&ptr) in other places, only one of them is needed.

There is no user of CachedExtent(extent_len_t) because it is newly introduced in this PR.

Looks ObjectDataBlock is using another constructor CachedExtent(extent_len_t length, build_space_t) below.

Probably we only need explicit CachedExtent(extent_len_t length) : buffer_space(length) {}, right?

Just realized where CachedExtent(extent_len_t) is used, refer to the comment #50421 (comment)

xxhdx1985126 · 2023-03-22T03:11:09Z

src/crimson/os/seastore/cache.h

+        if (ret->is_ptr()) {
+	  return seastar::make_ready_future<CachedExtentRef>(ret);
+        } else {
+          touch_extent(*ret);


Seems that we should touch_extent no matter whether ret->is_ptr(), am I right?

BTW, this seems to be a duplicated return clause

Seems that we should touch_extent no matter whether ret->is_ptr(), am I right?

I have no idea since the old implementation did not. Should we touch_extent when we find it in a transaction? @cyx1231st

I have no idea since the old implementation did not. Should we touch_extent when we find it in a transaction? @cyx1231st

Ah, sorry, I didn't notice it's retrieved from the transaction, just ignore this comment, please.

Should we touch_extent when we find it in a transaction?

Following the existing logic, touch_extent is only called when an extent is firstly added to the transaction, or it is added to the cache.

So looks it is not necessary to touch here.

xxhdx1985126 · 2023-03-22T05:09:40Z

src/crimson/os/seastore/cache.h

@@ -572,6 +572,16 @@ class Cache {
      return ret->wait_io().then([ret] {
 	return seastar::make_ready_future<CachedExtentRef>(ret);
      });


Should this return be removed?

Sure, I forgot it. It's a mistake.

cyx1231st

Still looking.

Mostly code style suggestions to make the PR easier to read.

The comments are examples, please scan the PR and fix the others.

src/crimson/os/seastore/cache.h

cyx1231st · 2023-03-22T08:19:59Z

src/crimson/os/seastore/cache.h

          get_extent_ertr::ready_future_marker{},
          std::move(ret));
+        }
+        else {


Should be in the same line.

} else {

cyx1231st · 2023-03-22T08:31:07Z

src/crimson/os/seastore/cache.h

+            return seastar::do_with(
+              alloc_cache_buf(aligned_length),
+              [extent, this, start, aligned_length](auto &ptr){
+                return epm.read(


Let's reduce the unnecessary indention: make sure that the nested logic only has 2 extra spaces, not 4.

For example:

ceph/src/crimson/os/seastore/async_cleaner.cc

Lines 1068 to 1079 in 6e67bf8

return extent_callback->with_transaction_intr(

src,

"clean_reclaim_space",

[this, &backref_extents, &pin_list, &reclaimed](auto &t)

{

return seastar::do_with(

std::vector<CachedExtentRef>(backref_extents),

[this, &t, &reclaimed, &pin_list](auto &extents)

{

LOG_PREFIX(SegmentCleaner::do_reclaim_space);

// calculate live extents

auto cached_backref_entries =

cyx1231st · 2023-03-22T08:33:31Z

src/crimson/os/seastore/cache.h

+        if (ret->is_interval_contained(e_off, e_len)) {
+          // ret may be invalid, caller must check
+          return get_extent_ret<T>(
          get_extent_ertr::ready_future_marker{},


indention needs to be increased by 2.

cyx1231st · 2023-03-22T08:45:48Z

src/crimson/os/seastore/cached_extent.h

+          }
+      }
+
+      friend class CachedExtent;


Drop the friend declaration, and define public interfaces instead. And don't expose unnecessary interfaces as public.

Organize the public interfaces before the private interfaces.

No protected section is needed if there is no inheritance.

cyx1231st · 2023-03-22T08:46:35Z

src/crimson/os/seastore/cached_extent.h

-  /// Actual data contents
-  ceph::bufferptr ptr;
+  /// Actual data contents of extent if present
+  std::optional<ceph::bufferptr>  ptr;


Fix the extra space before ptr.

cyx1231st · 2023-03-22T08:47:16Z

src/crimson/os/seastore/cached_extent.h

+    poffset(other.poffset) {
+    if (other.ptr.has_value()) {
+      ptr = std::make_optional<ceph::bufferptr>
+        (other.ptr->c_str(), other.ptr->length());


Normally:

ptr = std::make_optional<ceph::bufferptr>( other.ptr->c_str(), other.ptr->length());

cyx1231st · 2023-03-22T08:48:18Z

src/crimson/os/seastore/cached_extent.h

+    if(offset == 0 && bptr.length() == get_length()) {
+      ptr = std::move(bptr);
+    }
+    else { buffer_space._add_buffer(offset, std::move(bptr)); }


Start with a new line:

} else { buffer_space._add_buffer(offset, std::move(bptr)); }

cyx1231st · 2023-03-22T08:48:47Z

src/crimson/os/seastore/transaction_manager.h

    LOG_PREFIX(TransactionManager::pin_to_extent);
-    SUBTRACET(seastore_tm, "getting extent {}", t, *pin);
+    SUBTRACET(seastore_tm, "getting extent {}, data offset {} length {}",
+    t, *pin, offset, length);


wrong indention

cyx1231st · 2023-03-23T06:02:37Z

src/crimson/os/seastore/cache.h

    if (!cached) {
      auto ret = CachedExtent::make_cached_extent_ref<T>(
-        alloc_cache_buf(length));
+        length);


Looks this is where (also line 313 below) the new constructor CachedExtent(extent_len_t length) is used. ~~Why the original CachedExtent::make_cached_extent_ref<T>(alloc_cache_buf(length)) cannot work?~~

Looks to me there is no need to introduce 2 new constructors to CachedExtent for the purpose.

CachedExtent(extent_len_t length) should be sufficient to reserve the length in this path.

cyx1231st

The general logic looks correct to me.

cyx1231st · 2023-03-23T06:21:55Z

src/crimson/os/seastore/cached_extent.h

+  }
+
+  /// Returns length of valid extent
+  extent_len_t get_valid_length() const {


get_valid_length() -> get_loaded_length()

cyx1231st · 2023-03-23T06:22:49Z

src/crimson/os/seastore/cached_extent.h

    return ret;
  }

+  bool is_ptr() {


bool is_ptr()
->
bool is_fully_loaded() const

cyx1231st · 2023-03-23T06:23:52Z

src/crimson/os/seastore/transaction_manager.cc

  ).si_then([=, this, &t](auto extent)
 	    -> get_extents_if_live_ret {
-    if (extent && extent->get_length() == len) {
+    if (extent && extent->get_valid_length() == len) {


Looks not accurate.

How about:
if (extent && extent->get_length() == len && extent->is_fully_loaded()) {
?

cyx1231st · 2023-03-23T06:48:34Z

src/crimson/os/seastore/cached_extent.h

+    return ptr.has_value();
+  }
+
+  bool is_interval_contained(extent_len_t offset, extent_len_t length) {


bool is_range_loaded(offset, length) const

cyx1231st · 2023-03-23T07:16:27Z

src/crimson/os/seastore/cache.h

    if (!cached) {
      auto ret = CachedExtent::make_cached_extent_ref<T>(
-        alloc_cache_buf(length));
+        length);


Looks to me there is no need to introduce 2 new constructors to CachedExtent for the purpose.

CachedExtent(extent_len_t length) should be sufficient to reserve the length in this path.

cyx1231st · 2023-03-23T08:41:04Z

src/crimson/os/seastore/cached_extent.h

  }

+  /// add buffer to bufferspace if fragmented
+  void add_buffer(extent_len_t offset, ceph::bufferptr&& bptr) {


Rename to load_range() to be consistent.

cyx1231st · 2023-03-23T08:46:47Z

src/crimson/os/seastore/cached_extent.h

+  }
+
+  /// get buffer by given offset and length.
+  ceph::bufferlist get_buffer(extent_len_t offset, extent_len_t length) {


rename to get_range(), seems can be const.

cyx1231st · 2023-03-23T08:58:06Z

src/crimson/os/seastore/cache.h

+    extent->set_io_wait();
+    auto old_length = extent->get_valid_length();
+    return seastar::do_with(
+      extent->read_buffer(offset, length),


Even for the complete read, the following path of read_buffer() -> add_buffer() -> update_lru_size() can still be reused correctly with minor adjustions, right?

cyx1231st · 2023-03-23T09:02:05Z

src/crimson/os/seastore/cache.h

+              epm.get_block_size());
+            extent_len_t aligned_length = end - start;
+            return seastar::do_with(
+              alloc_cache_buf(aligned_length),


I think alloc_cache_buf() (i.e. allocating page aligned buffer) is not necessary for partial reads that are fragmented, block aligned should be fine and has better performance.

cyx1231st · 2023-03-23T09:11:01Z

src/crimson/os/seastore/cached_extent.h

+  }
+
+  /// convert bufferspace to ptr if full
+  void check_and_rebuild() {


rename to maybe_build_bptr()

github-actions · 2023-05-09T10:54:24Z

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

github-actions · 2023-09-07T03:01:51Z

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.

github-actions · 2023-10-07T04:01:33Z

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

cyx1231st · 2024-05-13T07:34:09Z

~~I'm going to implement it based on the latest code,~~ starting from cleanups e.g. #57432

Let me know if anyone is also working on this.

Update: @ljx023 will start to work on this.

cyx1231st · 2024-05-13T07:49:45Z

Refering to #55449 (comment), seastore_max_data_allocation_size might be considerred to be disabled by default ~~/removed~~ after this feature.

xxhdx1985126 · 2024-05-13T07:53:29Z

Refering to #55449 (comment), seastore_max_data_allocation_size might be considerred to be disabled/removed after this feature.

I think we still need seastore_max_data_allocation_size when full integrity check enabled, so that the read amplification won't be so large. I think we can drop seastore_max_data_allocation_size once we have fine-grained checksums.

cyx1231st · 2024-05-13T08:00:44Z

You're correct @xxhdx1985126 , I have adjusted my comments, see above.

Jianxin Li added 2 commits March 1, 2023 11:42

crimson/os/seastore: add BufferSpace to manage small buffers

c70873b

Signed-off-by: Jianxin Li <jianxin1.li@intel.com>

crimson/os/seastore: add interface for BufferSpace

e314e35

construct BufferSpace only for ObjectDataBlock. Signed-off-by: Jianxin Li <jianxin1.li@intel.com>

ljx023 requested a review from a team as a code owner March 7, 2023 11:35

github-actions bot added crimson tests labels Mar 7, 2023

Jianxin Li added 4 commits March 7, 2023 13:34

crimson/os/seastore: modify read path for fine-grained cache

2c3d277

get unload interval in extent and load them in parallel. Signed-off-by: Jianxin Li <jianxin1.li@intel.com>

crimson/os/seastore: update lru size based on valid extent size

4cfb801

Signed-off-by: Jianxin Li <jianxin1.li@intel.com>

crimson/os/seastore: modify crc calculation

d4adcb4

Signed-off-by: Jianxin Li <jianxin1.li@intel.com>

crimson/os/seastore: modify related get_extent

65f7d8d

full extent got from transaction if not fully loaded Signed-off-by: Jianxin Li <jianxin1.li@intel.com>

ljx023 force-pushed the wip-seastore-fine-grained-cache branch from d0309da to 65f7d8d Compare March 7, 2023 13:48

cyx1231st requested review from athanatos, cyx1231st and xxhdx1985126 March 9, 2023 02:48

markhpc added the performance label Mar 9, 2023

xxhdx1985126 reviewed Mar 22, 2023

View reviewed changes

cyx1231st reviewed Mar 22, 2023

View reviewed changes

cyx1231st reviewed Mar 23, 2023

View reviewed changes

cyx1231st mentioned this pull request Apr 25, 2023

crimson/os/seastore: realize lazy read in split overwrite with overwrite refactor #51179

Merged

14 tasks

github-actions bot added the needs-rebase label May 9, 2023

github-actions bot added the stale label Sep 7, 2023

github-actions bot closed this Oct 7, 2023

cyx1231st mentioned this pull request Apr 8, 2024

crimson/os/seastore: support extent checksum verification #55449

Merged

14 tasks

cyx1231st added the crimson-has-followup label Apr 8, 2024

cyx1231st added the crimson-perf label Apr 18, 2024

ljx023 deleted the wip-seastore-fine-grained-cache branch May 23, 2024 05:48

ljx023 mentioned this pull request May 30, 2024

crimson/os/seastore: add fine-grained cache #57787

Closed

14 tasks

	return extent_callback->with_transaction_intr(
	src,
	"clean_reclaim_space",
	[this, &backref_extents, &pin_list, &reclaimed](auto &t)
	{
	return seastar::do_with(
	std::vector<CachedExtentRef>(backref_extents),
	[this, &t, &reclaimed, &pin_list](auto &extents)
	{
	LOG_PREFIX(SegmentCleaner::do_reclaim_space);
	// calculate live extents
	auto cached_backref_entries =

Conversation

ljx023 commented Mar 7, 2023

Contribution Guidelines

Checklist

Uh oh!

ljx023 commented Mar 7, 2023

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!

xxhdx1985126 Mar 22, 2023 • 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!

cyx1231st left a comment

Choose a reason for hiding this comment

Uh oh!

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!

cyx1231st Mar 23, 2023 • 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!

cyx1231st 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!

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

xxhdx1985126 Mar 22, 2023 •

edited

Loading

cyx1231st Mar 23, 2023 •

edited

Loading

cyx1231st commented May 13, 2024 •

edited

Loading

cyx1231st commented May 13, 2024 •

edited

Loading

xxhdx1985126 commented May 13, 2024 •

edited

Loading