crimson/os/seastore/cache: initial_pending extents also do "set_io_wait/complete_io/wait_io" by xxhdx1985126 · Pull Request #62878 · ceph/ceph

xxhdx1985126 · 2025-04-18T09:27:14Z

Fixes: https://tracker.ceph.com/issues/70976
Signed-off-by: Xuehan Xu xuxuehan@qianxin.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. "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

xxhdx1985126 · 2025-04-18T09:39:24Z

jenkins test make check

xxhdx1985126 · 2025-04-18T09:39:29Z

jenkins test api

xxhdx1985126 · 2025-04-19T06:46:10Z

jenkins test make check

cyx1231st · 2025-04-21T02:51:32Z

src/crimson/os/seastore/cached_extent.h

+    // stable and visible, see prepare_record().
    //
-    // XXX: It might be good to mark this case as DIRTY from the definition,
+    // XXX: It might be good to mark this case as DIRTY/CLEAN from the definition,


The major reason is that I think it is inappropriate (and wrong) to expose the pending states that are not from (irrelavent to) this transaction. These states should actually be DIRTY/CLEAN from this transaction's perspective.

This will make things challenging (even worse when initial-pending is also affecting now) as there are lots of different places relying on deciding whether the extent is pending in this transaction.

src/crimson/os/seastore/cache.cc

xxhdx1985126 · 2025-04-21T08:35:14Z

@cyx1231st I've just modified the code as discussed offline, please take a look, thanks:-)

cyx1231st · 2025-04-21T08:43:23Z

src/crimson/os/seastore/cached_extent.h

+    // XXX: It might be good to mark this case as DIRTY/CLEAN from the definition,
    // which probably can make things simpler.
-    return is_mutation_pending() && is_pending_io();
+    if (is_pending_io()) {


I think if needs to be wrapped in DEBUG macro to build conditionally as well, otherwise non-debug build will see an empty if.

Um, IIUC, the empty if would be optimized by the compiler: https://godbolt.org/z/zd6oz6sbv

src/crimson/os/seastore/cache.h

cyx1231st · 2025-04-22T03:19:01Z

The essential gap from the discussions is that:

We cannot add stable initial-pending extents to the transaction read-set when its paddr is unknown.
We need to make sure transaction conflict still works in whatever way to work around the above gap.

xxhdx1985126 · 2025-04-23T03:48:27Z

@cyx1231st It seems that the use cases in which an initial_pending extents might be met are as follows:

In the omap tree, the user transactions might hit an initial pending omap nodes that are being committed by trimmier/cleaner transactions.
In the future, when doing bottom-up lba iterator construction, we might hit an initial pending parent node.

Both of the above cases must go through Cache::get_extent_viewable_by_trans(). So I tried to add transaction interfaces for the seperated Transaction::add_to_read_set() steps ---- adding transactions to extents' read_transactions and adding extents to transactions' read_set ---- and do the seperated steps for initial pending extents in Cache::get_extent_viewable_by_trans().

I also added asserts to make sure the seperated steps are only used for initial pending and stable writing extents and prevent other cases from wrongly using the interfaces.

Please take a look, thanks:-)

cyx1231st

Yeah, it makes sense to me that only get_extent_viewable_by_trans() is and will be related to this issue in the near future, and there are proper asserts added.

Can you help to take a look at the paddr type validations in #62938 ?

If possible, I prefer to merge the code consolidation/cleanup work before moving forward.

src/crimson/os/seastore/linked_tree_node.h

src/crimson/os/seastore/cache.h

src/crimson/os/seastore/transaction.h

github-actions · 2025-04-25T15:48:03Z

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

cyx1231st · 2025-05-15T03:38:54Z

The test shows this PR intensifies the issue https://tracker.ceph.com/issues/71317, which depends the PR #63218

See https://pulpito.ceph.com/yingxin-2025-05-15_02:31:43-crimson-rados-wip-yingxin-testing-2025-05-14-1435-distro-default-smithi/8285808/ osd2

ceph-osd: /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos9/DIST/centos9/MACHINE_SIZE/gigantic/release/20.3.0-354-g69e4144d/rpm/el9/BUILD/ceph-20.3.0-354-g69e4144d/
src/crimson/os/seastore/transaction.h:303:
void crimson::os::seastore::Transaction::replace_placeholder(
  crimson::os::seastore::CachedExtent&,
  crimson::os::seastore::CachedExtent&):
Assertion  `where->ref.get() == &placeholder' failed.

cyx1231st

See above.

cyx1231st

Regarding to "Assertion `where->ref.get() == &placeholder' failed."

I see further gaps related to retired-placeholer (probably still incomplete):

retired-placeholder is not added atomically upon get_extent_if_linked(), it is only added later into the cache index
retired-placeholder is not linked atomically in the lba tree, so it is possible that
- a retired placeholder can be added multiple times repeatedly
- a real extent can be linked in the middle, making the errors looking more confusing
even after it is linked in the lba tree, there is no logic to handle this case properly (see comments).

Probably we should merge #63218 with this PR to fix the issues together.

Let me know your thoughts.

cyx1231st · 2025-05-16T03:22:17Z

src/crimson/os/seastore/cache.h

-          touch_extent(*p_extent, &t_src, t.get_cache_hint());
+          if (ret.is_paddr_known) {
+            touch_extent(*p_extent, &t_src, t.get_cache_hint());
+          } else {


In the else case, validate p_extent must not be a retired-placeholder.

add assert:
assert(!is_retired_placeholder_type(p_extent->get_type()));

cyx1231st · 2025-05-16T03:40:17Z

src/crimson/os/seastore/cache.h

            ++stats.access.cache_lru;
          }
-          touch_extent(*p_extent, &t_src, t.get_cache_hint());
+          if (ret.is_paddr_known) {


See #63218 (comment), after retired-placeholder is linked in the lba tree, it is possible to see a retired-placeholder here, I think the proper way to handle this is to:

check if ret is a retired-placeholder.

create a real one (which means we need to know the real type here)

and replace placeholder by the real one immediately (see Cache::do_get_caching_extent())

There might be asserts needed somewhere to make sure things are working properly, still thinking.

cyx1231st · 2025-05-16T03:43:16Z

src/crimson/os/seastore/cache.h

    CachedExtent* p_extent;
+    bool needs_step_2 = false;
+    bool needs_touch = false;
    if (extent->is_stable()) {


After #63218 (comment), and starting from this case, it should be possible to see the extent is a retired-placeholder.

cyx1231st · 2025-05-16T03:44:37Z

src/crimson/os/seastore/cache.h

+    bool needs_step_2 = false;
+    bool needs_touch = false;
    if (extent->is_stable()) {
      p_extent = extent->get_transactional_view(t);


I think a retired-placeholder cannot get-transactional-view, might need some adjustments.

cyx1231st · 2025-05-16T03:45:40Z

src/crimson/os/seastore/cache.h

+            needs_touch = true;
+          }
        } else {
+          // already exists


Must not be a retired-placeholder if already exists.

add assert:
assert(!is_retired_placeholder_type(p_extent->get_type()));

cyx1231st · 2025-05-16T06:24:52Z

I'll proceed to see if we can merge #62071 first.

xxhdx1985126 · 2025-05-16T06:30:45Z

Probably we should merge #63218 with this PR to fix the issues together.

Yeah, will do.

xxhdx1985126 · 2025-05-19T10:37:02Z

jenkins test api

cyx1231st · 2025-05-20T03:02:26Z

src/crimson/os/seastore/transaction.h

-      auto it = read_set.emplace_hint(where, this, &extent);
+      placeholder.read_transactions.erase(
+	read_trans_set_t<Transaction>::s_iterator_to(*where));
+      read_items.emplace_back(this, &extent);


Add a comment:
// Note, the retired-placeholder is not removed from read_items after replace.

src/crimson/os/seastore/cache.h

cyx1231st · 2025-05-20T03:42:01Z

src/crimson/os/seastore/cache.h

-          touch_extent(*p_extent, &t_src, t.get_cache_hint());
+          if (ret.is_paddr_known) {
+            touch_extent(*p_extent, &t_src, t.get_cache_hint());
+          } else {


add assert:
assert(!is_retired_placeholder_type(p_extent->get_type()));

cyx1231st · 2025-05-20T03:42:12Z

src/crimson/os/seastore/cache.h

+            needs_touch = true;
+          }
        } else {
+          // already exists


add assert:
assert(!is_retired_placeholder_type(p_extent->get_type()));

cyx1231st · 2025-05-20T03:47:34Z

src/crimson/os/seastore/transaction.h

-      auto it = read_set.emplace_hint(where, this, &extent);
+      placeholder.read_transactions.erase(
+	read_trans_set_t<Transaction>::s_iterator_to(*where));
+      read_items.emplace_back(this, &extent);


I assume the only changed place is here compared with the last version, right?

github-actions · 2025-05-21T06:41:53Z

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

set_io_wait/complete_io/wait_io Fixes: https://tracker.ceph.com/issues/70976 Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

…ntrusive set Fixes: https://tracker.ceph.com/issues/70976 Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

…tion::do_add_to_read_set() The current implementation of Transaction::do_add_to_read_set() can be seperated into two steps: 1. attach the extent to the transaction, i.e. insert the extent into the transaction's read_set 2. attach the transaction to the extent, i.e. insert the transaction into the extent's read_transactions For initial pending and stable writing extents, we need to do the second step before doing "CachedExtent::wait_io()" and to do the first step after "CachedExtent::wait_io()". This commit add interfaces corresponding to those two steps. Fixes: https://tracker.ceph.com/issues/70976 Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com> Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

initial pending and stable writing extents Fixes: https://tracker.ceph.com/issues/70976 Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com> Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

`BaseChildNode::get_parent_node()` to `BaseChildNode::peek_parent_node()` Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

xxhdx1985126 · 2025-06-03T08:52:18Z

closing this PR as its commits have been merged to #63218

xxhdx1985126 requested a review from cyx1231st April 18, 2025 09:27

xxhdx1985126 requested a review from a team as a code owner April 18, 2025 09:27

github-actions bot added the crimson label Apr 18, 2025

xxhdx1985126 force-pushed the wip-70976 branch from 99c3a58 to 2f6fb00 Compare April 18, 2025 09:31

xxhdx1985126 added the seastore label Apr 18, 2025

xxhdx1985126 force-pushed the wip-70976 branch 3 times, most recently from 913d2bc to ee2e878 Compare April 18, 2025 09:52

cyx1231st added this to Crimson Apr 21, 2025

cyx1231st moved this to Awaits review in Crimson Apr 21, 2025

cyx1231st reviewed Apr 21, 2025

View reviewed changes

xxhdx1985126 force-pushed the wip-70976 branch from ee2e878 to 34dea0d Compare April 21, 2025 08:04

cyx1231st reviewed Apr 22, 2025

View reviewed changes

xxhdx1985126 force-pushed the wip-70976 branch from 34dea0d to 25f069e Compare April 23, 2025 03:41

xxhdx1985126 force-pushed the wip-70976 branch from 25f069e to 07b4752 Compare April 23, 2025 04:23

cyx1231st reviewed Apr 24, 2025

View reviewed changes

github-actions bot added the needs-rebase label Apr 25, 2025

xxhdx1985126 force-pushed the wip-70976 branch from 07b4752 to a6e16ba Compare May 3, 2025 07:01

github-actions bot removed the needs-rebase label May 3, 2025

xxhdx1985126 force-pushed the wip-70976 branch 4 times, most recently from 0b2430d to 330961a Compare May 5, 2025 06:00

cyx1231st removed wip-yingxin-testing needs-crimson-seastore-qa labels May 15, 2025

cyx1231st moved this from Needs QA to In Progress in Crimson May 15, 2025

cyx1231st requested changes May 15, 2025

View reviewed changes

cyx1231st reviewed May 16, 2025

View reviewed changes

xxhdx1985126 force-pushed the wip-70976 branch from 2f69961 to ab49a34 Compare May 19, 2025 09:54

cyx1231st reviewed May 20, 2025

View reviewed changes

cyx1231st mentioned this pull request May 20, 2025

crimson/os/seastore/transaction_manager: Cache::retire_absent_extent_addr should be called immediately after TransactionManager::get_extent_if_linked if necessary #63218

Merged

14 tasks

github-actions bot added the needs-rebase label May 21, 2025

xxhdx1985126 force-pushed the wip-70976 branch 2 times, most recently from ac52d80 to 4af73bd Compare May 24, 2025 09:50

github-actions bot removed the needs-rebase label May 24, 2025

cyx1231st added the wip-yingxin-testing-2 label May 26, 2025

xxhdx1985126 added 2 commits May 26, 2025 11:44

crimson/os/seastore/cache: initial_pending extents also do

d91be79

set_io_wait/complete_io/wait_io Fixes: https://tracker.ceph.com/issues/70976 Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

crimson/os/seastore/transaction: turn Transaction::read_set into an i…

1eef4f6

…ntrusive set Fixes: https://tracker.ceph.com/issues/70976 Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

xxhdx1985126 force-pushed the wip-70976 branch from 4af73bd to 64648ff Compare May 26, 2025 03:44

cyx1231st removed the wip-yingxin-testing-2 label May 26, 2025

xxhdx1985126 added 3 commits May 27, 2025 13:54

crimson/os/seastore/cache: do seperated add_to_read_set steps for

fb374c9

initial pending and stable writing extents Fixes: https://tracker.ceph.com/issues/70976 Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com> Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

crimson/os/seastore/linked_tree_node: rename

649e750

`BaseChildNode::get_parent_node()` to `BaseChildNode::peek_parent_node()` Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

xxhdx1985126 force-pushed the wip-70976 branch from 64648ff to 649e750 Compare May 27, 2025 05:54

xxhdx1985126 closed this Jun 3, 2025

Matan-B removed this from Crimson Jun 30, 2025

Conversation

xxhdx1985126 commented Apr 18, 2025

Contribution Guidelines

Checklist

Uh oh!

xxhdx1985126 commented Apr 18, 2025

Uh oh!

xxhdx1985126 commented Apr 18, 2025

Uh oh!

xxhdx1985126 commented Apr 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xxhdx1985126 commented Apr 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cyx1231st commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xxhdx1985126 commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyx1231st left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 25, 2025

Uh oh!

cyx1231st commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyx1231st left a comment

Choose a reason for hiding this comment

Uh oh!

cyx1231st left a comment • 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!

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 commented May 16, 2025

Uh oh!

xxhdx1985126 commented May 16, 2025

Uh oh!

xxhdx1985126 commented May 19, 2025

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!

cyx1231st commented Apr 22, 2025 •

edited

Loading

xxhdx1985126 commented Apr 23, 2025 •

edited

Loading

cyx1231st commented May 15, 2025 •

edited

Loading

cyx1231st left a comment •

edited

Loading