crimson/osd/recovery_backend: scan_for_backill tries to load the metadata even if the obc is already in the cache while its existed is false by xxhdx1985126 · Pull Request #60989 · ceph/ceph

xxhdx1985126 · 2024-12-09T09:07:24Z

Fixes: https://tracker.ceph.com/issues/69154
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 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

xxhdx1985126 · 2024-12-11T04:54:10Z

jenkins test api

Matan-B

This PR contains 2 changes, the first is the avoiding the obc reload (which lgtm) while the second commit is the removal of the !existed case. Previously, we returned in that case and now we reload the metadata. Can you either explain why or separate the commits? Thank you!

Matan-B · 2024-12-12T10:29:06Z

src/crimson/osd/recovery_backend.cc

-	  } else {
-	    // if the object does not exist here, it must have been removed
-	    // between the collection_list_partial and here.  This can happen
-	    // for the first item in the range, which is usually last_backfill.
-	  }


Can you please explain why this case is no longer relevant?

I think this case still exists, in which the later "load_metadata" also returns an empty obc and the object won't be put into the backfill interval. So it looks to me that this PR doesn't conflict with the main branch in this case, am I right?

Yes, we will still hit this case when trying to reload although it's less explicit now.

Matan-B · 2025-01-20T12:39:32Z

src/crimson/osd/recovery_backend.cc

 	  obc = pg.obc_registry.maybe_get_cached_obc(object);
 	}
-	if (obc) {
-	  if (obc->obs.exists) {


scan_for_backill tries to load the metadata even if the obc is already in the cache while its existed is false

I might be missing something, when the above if case (obc->obs.exists) will be false and we will return in line 251, without loading the metadata. Right?

Meaning we have 3 cases (before the patch):

obc found in cache && exists is true -> emplace it.

obc found in cache && exists is false -> return

obc not found -> load_metadata

With this patch the 3 cases are:

obc found in cache && exists is true -> emplace it.

obc found in cache && exists is false -> load_metadata

obc not found -> load_metadata

This change is not reflected in the commit message/title, am I right? Is this the intended behavior?

Let me know if this is correct:

crimson/osd/recovery_backend: fix scan_for_backill metadata reload Currently there are 3 cases (before the patch): obc found in cache && exists is true -> emplace it. obc found in cache && exists is false -> return obc not found -> load_metadata We should reload the metadata **also** when exists is false in order to exclude the possibility that there's an ongoing obc load. Fixes: https://tracker.ceph.com/issues/69154 Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

Yes, that's right:-) Will update the commit message.

Matan-B · 2025-01-20T12:42:12Z

src/crimson/osd/recovery_backend.cc

-	  } else {
-	    // if the object does not exist here, it must have been removed
-	    // between the collection_list_partial and here.  This can happen
-	    // for the first item in the range, which is usually last_backfill.
-	  }


Yes, we will still hit this case when trying to reload although it's less explicit now.

metadata even if the obc is already in the cache while its existed is false Currently there are 3 cases (before the patch): obc found in cache && exists is true -> emplace it. obc found in cache && exists is false -> return obc not found -> load_metadata We should reload the metadata **also** when exists is false in order to exclude the possibility that there's an ongoing obc load. Fixes: https://tracker.ceph.com/issues/69154 Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

xxhdx1985126 · 2025-01-22T05:55:04Z

@Matan-B I've just updated the commit message, please take a look again, thanks:-)

Matan-B

This seems to work. However, looking again, I think that we can reuse the object loader logic here and avoid being responsible for the reload/load.
What do you think about: #61536?

athanatos · 2025-01-30T22:50:02Z

I also think it's probably better to use the new obc manager machinery rather than explicitly checking loaded.

github-actions · 2025-01-31T19:03:23Z

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

xxhdx1985126 · 2025-02-07T03:47:06Z

closing this PR in favour of #61536

xxhdx1985126 requested a review from a team as a code owner December 9, 2024 09:07

github-actions bot added the crimson label Dec 9, 2024

xxhdx1985126 requested review from Matan-B and athanatos December 9, 2024 09:07

xxhdx1985126 added bug-fix crimson-replicated-recovery labels Dec 9, 2024

Matan-B reviewed Dec 12, 2024

View reviewed changes

Matan-B reviewed Jan 20, 2025

View reviewed changes

xxhdx1985126 force-pushed the wip-69154 branch from 47b6c4b to fc80387 Compare January 22, 2025 05:54

Matan-B self-requested a review January 23, 2025 10:23

Matan-B reviewed Jan 27, 2025

View reviewed changes

Matan-B mentioned this pull request Jan 27, 2025

crimson/osd/recovery_backend: scan_for_backfill to use obc_manager #61536

Merged

14 tasks

github-actions bot added the needs-rebase label Jan 31, 2025

xxhdx1985126 closed this Feb 7, 2025

Conversation

xxhdx1985126 commented Dec 9, 2024

Contribution Guidelines

Checklist

Uh oh!

xxhdx1985126 commented Dec 11, 2024

Uh oh!

Matan-B 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!

Matan-B Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

xxhdx1985126 Dec 15, 2024

Choose a reason for hiding this comment

Uh oh!

Matan-B Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

Matan-B Jan 20, 2025 • 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!

Matan-B Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

xxhdx1985126 Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

Matan-B Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

xxhdx1985126 commented Jan 22, 2025

Uh oh!

Matan-B left a comment

Choose a reason for hiding this comment

Uh oh!

athanatos commented Jan 30, 2025

Uh oh!

github-actions bot commented Jan 31, 2025

Uh oh!

xxhdx1985126 commented Feb 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Matan-B left a comment •

edited

Loading

Matan-B Jan 20, 2025 •

edited

Loading