objecter: use configurable to set local and balanced reads by yehudasa · Pull Request #56180 · ceph/ceph

yehudasa · 2024-03-13T23:16:39Z

This sets the relevant flags when a read op is created. The configurable allows for

'localize' : localized reads
'balance': balanced reads
'default': the default behavior (read from primary)

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

gregsfortytwo

Well, this is short and sweet. I've been leaning towards preferring per-component switches to allow more granular control of things, but the simplicity and universality here makes me think again.

gregsfortytwo · 2024-03-15T17:55:22Z

src/osdc/Objecter.h

  bool keep_balanced_budget = false;
  bool honor_pool_full = true;

+  std::atomic<int> extra_read_flags{0};


Eww do we really need to make this atomic and stick a memory barrier in the middle of all those constructor calls?
All we really need here is for the memory update to not be torn and I don't think that can happen for these sizes?

Use the atomic, read/write races are undefined behavior otherwise.

if we want to avoid the atomic here, we might consider removing the support for runtime changes. i think we care more about performance here than the convenience around config changes

We don't need sequentially consistent memory ordering (default for a C++ atomic), release-acquire ordering should be sufficient - https://en.cppreference.com/w/cpp/atomic/memory_order#Release-Acquire_ordering .

On X86 with its strong memory model this results in no memory barriers in the generated code, just constraints on the compiler not to re-order code. On other architectures (e.g. ARM and POWER) release-acquire ordering adds synchronization instructions, but these are still have less overhead than the instructions added for sequential consistent memory order.

So using code like this when reading the flags would be more efficient: extra_read_flags.load(std::memory_order_acquire)

Given that the balanced and localized flags are just performance optimizations, and as such it doesn't really matter if a race hazard between threads mean a few I/Os use an out of date copy of flags, then a weaker memory ordering of memory_order_relaxed could be used

Using a weaker memory ordering seems reasonable to me -- my main concern was simply that we use defined C++ memory ordering semantics.

Resolved by new commit

bill-scales · 2024-03-26T12:14:14Z

src/osdc/Objecter.h

  bool keep_balanced_budget = false;
  bool honor_pool_full = true;

+  std::atomic<int> extra_read_flags{0};


There is already a global_ops_flags which is also atomic and appears only to be use by the filer Client::set_filer_flags and Client::get_filer_flags to set the same LOCALIZE_READS flag.

Why do we need two separate sets of additional flags, why can't this code just set the global_ops_flags?

global_ops_flags is used for all ops, extra_read_flags is used only for read ops

src/osdc/Objecter.h

bill-scales · 2024-04-03T11:58:07Z

src/osdc/Objecter.h

  bool keep_balanced_budget = false;
  bool honor_pool_full = true;

+  std::atomic<int> extra_read_flags{0};


Resolved by new commit

cbodley

i like this approach very much. is there a natural place in the rados suite to the add test coverage necessary to validate it?

yehudasa · 2024-05-09T20:28:03Z

@cbodley see

i like this approach very much. is there a natural place in the rados suite to the add test coverage necessary to validate it?

@cbodley see 369544b (untested yet)

cbodley · 2024-05-09T20:50:08Z

@cbodley see 369544b (untested yet)

i see that the rados/verify and rados/basic subsuites both run all of the cls tests. one of those would probably be a good place for that read-affinity subdirectory. it should also include an empty default.yaml so the default configuration is still part of the matrix. i know the rados suite is already gigantic, so we might consider using read-affinity$ so each job picks one at random instead of tripling the number of jobs

cc @rzarzynski

thotz · 2024-05-15T12:21:32Z

qa/suites/rados/read-affinity/balance.yaml

+      osd:
+        crush location: 'root=default host=host-$id'
+      client:
+        rados replica read policy: balance


@idryomov does the crush location required for balance read similar to local?

Do this feature supported in ec pool ? The name suggests it is replica feature

looping @rzarzynski as well

@idryomov does the crush location required for balance read similar to local?

No, specifying just rados replica read policy: balance should be enough.

Do this feature supported in ec pool ? The name suggests it is replica feature

It's not supported on EC pools.

Thanks @idryomov

rzarzynski · 2024-05-16T19:53:21Z

src/osdc/Objecter.h

  bool keep_balanced_budget = false;
  bool honor_pool_full = true;

+  std::atomic<int> extra_read_flags{0};


rzarzynski · 2024-05-16T19:58:47Z

src/osdc/Objecter.h

  bool keep_balanced_budget = false;
  bool honor_pool_full = true;

+  std::atomic<int> extra_read_flags{0};


~~nit: is there a particular reason for using signed type for the extra_read_flags?~~

mutate() takes flags as int, I see.

rzarzynski · 2024-05-16T20:03:20Z

src/librados/IoCtxImpl.cc

 				      bufferlist *pbl,
-				      int flags)
+				      int flags,
+                                      int flags_mask)


nit: mixed indentation.

mkogan1

Graphs are showing IO operations on each of the replica 3 OSDs while performing S3 GET operations of 1M 4KB objects.
On the left ./bin/radosgw ... --rados_replica_read_policy=default and on the right ./bin/radosgw ... --rados_replica_read_policy=balance where please note the IO operations are distributed almost evenly between the drives.

cbodley · 2024-06-26T13:03:44Z

i see that the rados/verify and rados/basic subsuites both run all of the cls tests. one of those would probably be a good place for that read-affinity subdirectory. it should also include an empty default.yaml so the default configuration is still part of the matrix. i know the rados suite is already gigantic, so we might consider using read-affinity$ so each job picks one at random instead of tripling the number of jobs

cc @rzarzynski

@rzarzynski do you have any feedback on this part? i see that you've approved - do you think this is ready for qa and merge?

mattbenjamin · 2024-07-02T16:46:35Z

@mkogan1 thanks for testing balanced reads! are there any other workloads you have setups to test? @vumrao will be testing local reads at scale

github-actions · 2024-10-13T18:01:56Z

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.

gregsfortytwo · 2024-11-13T17:42:51Z

Hi folks, what needs to happen for this to merge? I thought it was in already and I think we've pulled it in to our product so I'd like to see it get in. :o

cbodley · 2024-12-04T15:19:29Z

i see that the rados/verify and rados/basic subsuites both run all of the cls tests. one of those would probably be a good place for that read-affinity subdirectory. it should also include an empty default.yaml so the default configuration is still part of the matrix. i know the rados suite is already gigantic, so we might consider using read-affinity$ so each job picks one at random instead of tripling the number of jobs
cc @rzarzynski

@rzarzynski do you have any feedback on this part? i see that you've approved - do you think this is ready for qa and merge?

ping. combinatorics aside, my main concern is that the addition of read-affinity/{balance.yaml, local.yaml} is removing test coverage of the default configuration

rzarzynski · 2024-12-04T20:58:28Z

ping. combinatorics aside, my main concern is that the addition of read-affinity/{balance.yaml, local.yaml} is removing test coverage of the default configuration

I agree. Even if this was a saving measure on the size of suite, it would be better to test for default and e.g. localize.

Let's add the missing yaml, rebase and merge. Thanks for finding this, @cbodley!

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>

Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

for localized and balanced reads. Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

Many of the exec calls were using a read version of the API while they were calling a write objclass method. This modifies the calls to use the write API. Fixes: https://tracker.ceph.com/issues/65889 Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

These calls are read calls but can and have been used as write calls, which doesn't work with local (and balanced) affinity. Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

cbodley · 2024-12-12T22:15:44Z

i pushed a rebase:

src/cls/rgw/cls_rgw_client.cc conflicts came from #58448 which had already converted cls_rgw_lc_put_head(), cls_rgw_lc_rm_entry(), cls_rgw_lc_set_entry() to Object*Operation for use with async_operate()

src/common/options/rgw.yaml.in had a temporary conflicts between the addition/removal of rgw_replica_read_policy and the new rgw_bucket_logging_obj_roll_time

with read-affinity/ at the top level of the rados suite, teuthology would try to run it as a subsuite instead of mixing its yamls into an existing subsuite. move into rados/verify instead Signed-off-by: Casey Bodley <cbodley@redhat.com>

Signed-off-by: Casey Bodley <cbodley@redhat.com>

cbodley · 2024-12-12T22:25:41Z

I agree. Even if this was a saving measure on the size of suite, it would be better to test for default and e.g. localize.

Let's add the missing yaml, rebase and merge. Thanks for finding this, @cbodley!

thanks @rzarzynski. i added the empty default.yaml and an additional commit:

qa/rados: move read-affinity/ under rados/verify subsuite

with read-affinity/ at the top level of the rados suite, teuthology
would try to run it as a subsuite instead of mixing its yamls into an
existing subsuite. move into rados/verify instead

let me know if you'd prefer it somewhere other than rados/verify, or if you want that symlinked into multiple rados subsuites

idryomov

Ack from the RBD perspective (at least one more instance of exec() misuse has sneaked in into RBD in the interim, but we can take care of it separately)

idryomov · 2025-01-21T20:28:34Z

at least one more instance of exec() misuse has sneaked in into RBD in the interim, but we can take care of it separately

#61471

github-actions bot added the common label Mar 13, 2024

yehudasa mentioned this pull request Mar 13, 2024

rgw/rados: add localise read for RGW #55563

Closed

14 tasks

cbodley added the wip-cbodley-testing label Mar 14, 2024

gregsfortytwo reviewed Mar 15, 2024

View reviewed changes

yehudasa changed the title ~~[DNM] objecter: use configurable to set local and balanced reads~~ objecter: use configurable to set local and balanced reads Mar 25, 2024

bill-scales reviewed Mar 26, 2024

View reviewed changes

bill-scales reviewed Apr 3, 2024

View reviewed changes

yehudasa force-pushed the wip-objecter-local-read branch from e209eae to 8dcf5ce Compare April 3, 2024 17:02

cbodley removed the wip-cbodley-testing label May 1, 2024

cbodley approved these changes May 9, 2024

View reviewed changes

cbodley added the needs-test label May 9, 2024

yehudasa requested review from a team as code owners May 9, 2024 16:23

github-actions bot added rbd rgw tests labels May 9, 2024

yehudasa requested a review from a team as a code owner May 9, 2024 19:36

github-actions bot added the core label May 9, 2024

yehudasa force-pushed the wip-objecter-local-read branch 2 times, most recently from 5e5ed24 to 369544b Compare May 9, 2024 20:25

thotz reviewed May 15, 2024

View reviewed changes

rzarzynski approved these changes May 16, 2024

View reviewed changes

mkogan1 self-requested a review June 24, 2024 10:42

mkogan1 approved these changes Jun 24, 2024

View reviewed changes

cbodley removed the needs-test label Jun 26, 2024

yehudasa force-pushed the wip-objecter-local-read branch from 369544b to 2bda7d4 Compare July 2, 2024 23:48

github-actions bot added the stale label Oct 13, 2024

cbodley removed the stale label Oct 14, 2024

jiffin and others added 9 commits December 12, 2024 17:01

rgw : provide multiple option for read replica policy

0b12afa

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>

objecter: configurable for localized and balanced reads

abbc9d4

Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

objecter: use read policy configurable

cadc73b

for localized and balanced reads. Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

objecter: add log messages related to read policy

59c708c

Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

objecter: weaker memory ordering for extra_read_flags

0a2f572

Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

librados: disable local affinity for IoCtx::exec() and aio_exec() calls

ab00072

These calls are read calls but can and have been used as write calls, which doesn't work with local (and balanced) affinity. Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

qa/rados: add read affinity yamls

2028e4e

Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

librados: local affinity for exec calls is configurable

24087ec

Signed-off-by: Yehuda Sadeh <ysadehwe@ibm.com>

cbodley force-pushed the wip-objecter-local-read branch from fe19a13 to 24087ec Compare December 12, 2024 22:14

github-actions bot removed the needs-rebase label Dec 12, 2024

cbodley added 2 commits December 12, 2024 17:19

qa/rados: add empty default.yaml for default replica read policy

158f7c3

Signed-off-by: Casey Bodley <cbodley@redhat.com>

rzarzynski approved these changes Dec 17, 2024

View reviewed changes

rzarzynski added the needs-qa label Dec 17, 2024

ljflores added the wip-yuri4-testing label Jan 13, 2025

idryomov approved these changes Jan 21, 2025

View reviewed changes

yuriw merged commit 13883f5 into ceph:main Jan 21, 2025

idryomov mentioned this pull request Jan 24, 2025

cls/rbd: don't use read API for write-like methods #61471

Merged

14 tasks

cbodley mentioned this pull request Aug 21, 2025

osd: Erasure code direct reads #64664

Draft

14 tasks

cbodley mentioned this pull request Nov 17, 2025

osd/rados/rgw/cephfs: Modernize cls interface with compile time safety #66258

Open

14 tasks

Conversation

yehudasa commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

gregsfortytwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athanatos Mar 20, 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!

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!

cbodley left a comment

Choose a reason for hiding this comment

Uh oh!

yehudasa commented May 9, 2024

Uh oh!

cbodley commented May 9, 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!

mkogan1 left a comment

Choose a reason for hiding this comment

Uh oh!

cbodley commented Jun 26, 2024

Uh oh!

mattbenjamin commented Jul 2, 2024

Uh oh!

github-actions bot commented Oct 13, 2024

Uh oh!

gregsfortytwo commented Nov 13, 2024

Uh oh!

cbodley commented Dec 4, 2024

Uh oh!

rzarzynski commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbodley commented Dec 12, 2024

Uh oh!

cbodley commented Dec 12, 2024

Uh oh!

idryomov left a comment

Choose a reason for hiding this comment

Uh oh!

idryomov commented Jan 21, 2025

Uh oh!

Reviewers

Assignees

yehudasa commented Mar 13, 2024 •

edited

Loading

athanatos Mar 20, 2024 •

edited

Loading

rzarzynski commented Dec 4, 2024 •

edited

Loading