rgw: replace null_yields with optional ones by TRYTOBE8TME · Pull Request #50206 · ceph/ceph

TRYTOBE8TME · 2023-02-22T06:54:42Z

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

cbodley

looks great overall! 'make check' is catching some unit tests under src/test/rgw/ that need updating also

cbodley · 2023-02-22T14:07:09Z

src/rgw/driver/rados/rgw_gc.cc

    const uint64_t queue_size = cct->_conf->rgw_gc_max_queue_size, num_deferred_entries = cct->_conf->rgw_gc_max_deferred;
    gc_log_init2(op, queue_size, num_deferred_entries);
-    store->gc_operate(this, obj_names[i], &op);
+    store->gc_operate(this, obj_names[i], &op, null_yield);


you added an optional_yield y argument to RGWGC::initialize(), so you can pass that here. same with RGWGC::send_split_chain() and several other functions in rgw_gc.cc

dang · 2023-02-22T15:48:53Z

BTW, you can "make tests" (or "ninja tests") to build the tests that make check uses without running them.

alimaredia · 2023-02-22T22:26:39Z

src/rgw/driver/rados/rgw_object_expirer_core.cc

  auto& ref = obj.get_ref();
  int ret = cls_timeindex_trim_repeat(dpp, ref, oid, utime_t(start_time), utime_t(end_time),
-          from_marker, to_marker);
+          from_marker, to_marker, null_yield);


You added an optional_yield to objexp_hint_trim so you don't need a null_yield here.

alimaredia · 2023-02-22T22:27:30Z

src/rgw/driver/rados/rgw_object_expirer_core.cc


  int ret = exp_store.objexp_hint_trim(dpp, shard, rt_from, rt_to,
-                                       from_marker, to_marker);
+                                       from_marker, to_marker, null_yield);


Same here, there's an optional_yield added to trim_chunk.

alimaredia · 2023-02-22T22:28:06Z

src/rgw/driver/rados/rgw_object_expirer_core.cc


    if (need_trim) {
-      trim_chunk(dpp, shard, last_run, round_start, marker, out_marker);
+      trim_chunk(dpp, shard, last_run, round_start, marker, out_marker, null_yield);


Same here, there's an optional_yield added to process_single_shard.

alimaredia · 2023-02-22T22:28:37Z

src/rgw/driver/rados/rgw_object_expirer_core.cc

    ldpp_dout(dpp, 20) << "processing shard = " << shard << dendl;

-    if (! process_single_shard(dpp, shard, last_run, round_start)) {
+    if (! process_single_shard(dpp, shard, last_run, round_start, null_yield)) {


Same here, there's an optional_yield added to inspect_all_shards.

alimaredia · 2023-02-22T22:35:41Z

src/rgw/driver/rados/rgw_rados.cc

 int RGWRados::try_refresh_bucket_info(RGWBucketInfo& info,
                                      ceph::real_time *pmtime,
-                                      const DoutPrefixProvider *dpp,
+                                      const DoutPrefixProvider *dpp, optional_yield y,


@cbodley It's a knit, but there are dozens of instances in this PR where the optional_yield argument in a function definition is added on the same line as another argument even though every other argument in the function is on it's own line. I think these optional_yields should go on their own line. What do you think?

i'm generally fine with arguments on the same line as long as it doesn't go over ~80 columns

but if doubling up arguments looks inconsistent with other nearby function definitions, you're welcome to request changes

alimaredia · 2023-02-22T22:38:39Z

src/rgw/driver/rados/rgw_rados.cc

                                  bucket->get_info().owner, obj_ctx,
                                  dest_obj->clone(), olh_epoch, tag,
-				  dpp, null_yield);
+				  dpp, y);


@kapandya @cbodley just a heads up I added the comment on the lines above this AtomicObjectProcessor initialization saying that changing this null_yield to a y causes ragweed tests to fail. I wouldn't be surprised if it happens when we run this PR through teuthology again.

alimaredia · 2023-02-22T22:47:05Z

src/rgw/rgw_crypt.cc

        break;
      }
-      res = s->bucket->try_refresh_info(s, nullptr);
+      res = s->bucket->try_refresh_info(s, nullptr, null_yield);


you can use s->yield here and anywhere else there is a req_state which is commonly called s.

alimaredia · 2023-02-22T22:48:31Z

src/rgw/rgw_op.cc

    s->bucket->get_info().set_sync_policy(std::move(sync_policy));

-    int ret = s->bucket->put_info(this, false, real_time());
+    int ret = s->bucket->put_info(this, false, real_time(), null_yield);


This function takes a y.

alimaredia · 2023-02-22T22:48:45Z

src/rgw/rgw_op.cc

    s->bucket->get_info().set_sync_policy(std::move(sync_policy));

-    int ret = s->bucket->put_info(this, false, real_time());
+    int ret = s->bucket->put_info(this, false, real_time(), null_yield);


This function takes a y.

alimaredia · 2023-02-22T22:48:54Z

src/rgw/rgw_op.cc

      s->bucket->get_info().has_website = true;
      s->bucket->get_info().website_conf = website_conf;
-      op_ret = s->bucket->put_info(this, false, real_time());
+      op_ret = s->bucket->put_info(this, false, real_time(), null_yield);


This function takes a y.

alimaredia · 2023-02-22T22:49:01Z

src/rgw/rgw_op.cc

      s->bucket->get_info().has_website = false;
      s->bucket->get_info().website_conf = RGWBucketWebsiteConf();
-      op_ret = s->bucket->put_info(this, false, real_time());
+      op_ret = s->bucket->put_info(this, false, real_time(), null_yield);


This function takes a y.

alimaredia · 2023-02-22T22:49:19Z

src/rgw/rgw_op.cc

  op_ret = retry_raced_bucket_write(this, s->bucket.get(), [this] {
    s->bucket->get_info().obj_lock = obj_lock;
-    op_ret = s->bucket->put_info(this, false, real_time());
+    op_ret = s->bucket->put_info(this, false, real_time(), null_yield);


This function takes a y.

alimaredia · 2023-02-22T22:53:20Z

@kapandya great start to this work! On top of the other comments that were left do you have a list of which ops don't have anymore librados blocking warnings anymore and which still do or are untested? What happens with a full run of s3-tests?

github-actions · 2023-03-01T16:32:48Z

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

Passing optional_yield to rgw_rados.cc Signed-off-by: Kalpesh Pandya <[email protected]>

Changing null_yield in rgw_reshard.cc Signed-off-by: Kalpesh Pandya <[email protected]>

Changing null_yield in rgw_sal_rados.cc Signed-off-by: Kalpesh Pandya <[email protected]>

Signed-off-by: Kalpesh Pandya <[email protected]>

mattbenjamin · 2023-06-20T13:01:05Z

we must be very close here, thanks for the rebase, @TRYTOBE8TME :)

mattbenjamin · 2023-06-20T16:07:48Z

I see that make check failed:

[ RUN ] Queue.SpawnAsyncRequest
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/rgw/test_rgw_dmclock_scheduler.cc:425: Failure
Value of: context.stopped()
Actual: false
Expected: true
[ FAILED ] Queue.SpawnAsyncRequest (9 ms)
[----------] 8 tests from Queue (42 ms total)

TRYTOBE8TME · 2023-06-21T03:47:24Z

Latest teuthology run: https://pulpito.ceph.com/soumyakoduri-2023-06-20_12:37:42-rgw-wip-rgw-yield-work-distro-default-smithi/

TRYTOBE8TME · 2023-06-21T03:48:21Z

jenkins test make check

TRYTOBE8TME · 2023-06-21T03:54:05Z

jenkins test api

TRYTOBE8TME · 2023-06-21T14:18:50Z

I see that make check failed:

[ RUN ] Queue.SpawnAsyncRequest /home/jenkins-build/build/workspace/ceph-pull-requests/src/test/rgw/test_rgw_dmclock_scheduler.cc:425: Failure Value of: context.stopped() Actual: false Expected: true [ FAILED ] Queue.SpawnAsyncRequest (9 ms) [----------] 8 tests from Queue (42 ms total)

These went away but now I see ceph API tests check failing and it doesn't show any reason

cbodley · 2023-06-21T17:24:58Z

Latest teuthology run: https://pulpito.ceph.com/soumyakoduri-2023-06-20_12:37:42-rgw-wip-rgw-yield-work-distro-default-smithi/

looks great! the s3test failure against dbstore is a new regression, and i've pushed a fix in ~~ceph/s3-tests#516~~ ceph/s3-tests#517. cc @soumyakoduri

cbodley · 2023-06-21T17:25:02Z

jenkins test api

cbodley · 2023-06-22T13:20:58Z

thanks @TRYTOBE8TME! are there more null_yields that we need to catch? @adamemerson had found some in #52036

as a next step, i'd like to find a way to add test coverage in teuthology to make sure that no requests are blocking. that might involve a config variable (something like rgw_debug_require_beast_async) that causes us to assert instead of just logging WARNING: blocking librados call. what do you think?

mattbenjamin · 2023-06-22T13:22:46Z

@cbodley does this PR need to block on adding tests which find more blocking calls we could fix? I think we want to get this one in when it's complete and stable, no?

yuvalif · 2023-06-22T13:29:35Z

thanks @TRYTOBE8TME! are there more null_yields that we need to catch? @adamemerson had found some in #52036

as a next step, i'd like to find a way to add test coverage in teuthology to make sure that no requests are blocking. that might involve a config variable (something like rgw_debug_require_beast_async) that causes us to assert instead of just logging WARNING: blocking librados call. what do you think?

maybe there should be a compilation flag to compile out all the blocking rados calls, and compile the RGW with this flag?

TRYTOBE8TME · 2023-06-22T13:39:13Z

@cbodley yes there are some places where null_yield needs to be replaced and basically all those places are somewhere part of an op which is remaining to be done.

cbodley · 2023-06-22T13:41:35Z

@cbodley does this PR need to block on adding tests which find more blocking calls we could fix? I think we want to get this one in when it's complete and stable, no?

@mattbenjamin this PR has merged, sir! i think we're close to the finish line here, so i'd like to add some regression testing to catch any remaining blocking calls, and keep us from adding new ones. then we can do some performance measurements to figure out how to size the thread pool optimally

cbodley · 2023-06-22T13:44:31Z

maybe there should be a compilation flag to compile out all the blocking rados calls, and compile the RGW with this flag?

@yuvalif we still need the blocking mode for radosgw-admin calls, librgw requests, and background threads. it's just the asio threads (like the beast frontend and rgw::notify::Manager) that should enforce async-only

yuvalif · 2023-06-22T13:56:05Z

maybe there should be a compilation flag to compile out all the blocking rados calls, and compile the RGW with this flag?

@yuvalif we still need the blocking mode for radosgw-admin calls, librgw requests, and background threads. it's just the asio threads (like the beast frontend and rgw::notify::Manager) that should enforce async-only

so, can we force an explicit null_yield in these cases?
or do you want tio catch beast+null_yield cases as well?

cbodley · 2023-06-23T16:13:38Z

i added this topic to next week's agenda https://pad.ceph.com/p/rgw-weekly

Also clears stale pending entries prior to attempting to clear OLH index entries and remove the OLH object. Signed-off-by: Cory Snyder <[email protected]> (cherry picked from commit 3437897) Conflicts: src/rgw/driver/rados/rgw_rados.cc src/rgw/driver/rados/rgw_rados.h Cherry-pick notes: - conflicts due to ceph#50206 on main but not reef

…ror scenarios Makes request threads clean up after themselves and remove their pending xattrs from OLH objects before they return an error from set_olh. Fixes: https://tracker.ceph.com/issues/59663 Signed-off-by: Cory Snyder <[email protected]> (cherry picked from commit 38133e5) Conflicts: src/rgw/driver/rados/rgw_rados.cc src/rgw/driver/rados/rgw_rados.h Cherry-pick notes: - conflicts due to ceph#50206 on main but not reef

When pending_removal is true and the log is trimmed prior to clearing olh, it allows for the possibility that the olh is not cleared (e.g. due to new pending xattrs on the olh obj), but the olh log entries are removed. Signed-off-by: Cory Snyder <[email protected]> (cherry picked from commit dfbe4f9) Conflicts: src/rgw/driver/rados/rgw_rados.cc Cherry-pick notes: - conflicts due to ceph#50206 on main

github-actions bot added rgw tests labels Feb 22, 2023

TRYTOBE8TME requested review from alimaredia and cbodley February 22, 2023 06:55

TRYTOBE8TME force-pushed the wip-rgw-yield-work branch from c92af68 to 9c7e220 Compare February 22, 2023 06:58

cbodley changed the title ~~Wip rgw yield work~~ rgw: replace null_yields with optional ones Feb 22, 2023

cbodley reviewed Feb 22, 2023

View reviewed changes

alimaredia reviewed Feb 22, 2023

View reviewed changes

github-actions bot added the needs-rebase label Mar 1, 2023

TRYTOBE8TME force-pushed the wip-rgw-yield-work branch from e047c57 to 6816554 Compare March 2, 2023 17:49

TRYTOBE8TME force-pushed the wip-rgw-yield-work branch from 6816554 to 7abf53e Compare March 13, 2023 11:04

TRYTOBE8TME force-pushed the wip-rgw-yield-work branch from 7abf53e to 870c141 Compare March 27, 2023 09:03

github-actions bot removed the needs-rebase label Mar 27, 2023

TRYTOBE8TME force-pushed the wip-rgw-yield-work branch from 870c141 to 0a8ada6 Compare March 27, 2023 15:06

cbodley mentioned this pull request Mar 28, 2023

rgw: pass yield context #44195

Closed

14 tasks

TRYTOBE8TME force-pushed the wip-rgw-yield-work branch from 0a8ada6 to bd9c8ea Compare March 29, 2023 06:05

kapandya added 4 commits June 20, 2023 03:18

src/rgw: Changing null_yield to optional_yield

402ebca

Passing optional_yield to rgw_rados.cc Signed-off-by: Kalpesh Pandya <[email protected]>

src/rgw: Changing null_yield to optional_yield

60a8a08

Changing null_yield in rgw_reshard.cc Signed-off-by: Kalpesh Pandya <[email protected]>

src/rgw: Changing null_yield to optional_yield

02e604f

Changing null_yield in rgw_sal_rados.cc Signed-off-by: Kalpesh Pandya <[email protected]>

src/rgw: Additional changes based on comments

05ddbb7

Signed-off-by: Kalpesh Pandya <[email protected]>

TRYTOBE8TME force-pushed the wip-rgw-yield-work branch from 6a85f47 to 05ddbb7 Compare June 19, 2023 21:48

github-actions bot removed the needs-rebase label Jun 19, 2023

cbodley mentioned this pull request Jun 21, 2023

rgw/sal: list_buckets() returns RGWBucketEnts #50611

Merged

cbodley approved these changes Jun 21, 2023

View reviewed changes

cbodley added the ready-to-merge label Jun 21, 2023

cbodley merged commit 7c94864 into ceph:main Jun 21, 2023

cbodley mentioned this pull request Jun 21, 2023

rgw: fix consistency bug with OLH objects #51700

Merged

14 tasks

Comments

Conversation

TRYTOBE8TME commented Feb 22, 2023

Contribution Guidelines

Checklist

Uh oh!

cbodley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dang commented Feb 22, 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!

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!

alimaredia commented Feb 22, 2023

Uh oh!

github-actions bot commented Mar 1, 2023

Uh oh!

mattbenjamin commented Jun 20, 2023

Uh oh!

mattbenjamin commented Jun 20, 2023

Uh oh!

TRYTOBE8TME commented Jun 21, 2023

Uh oh!

TRYTOBE8TME commented Jun 21, 2023

Uh oh!

TRYTOBE8TME commented Jun 21, 2023

Uh oh!

TRYTOBE8TME commented Jun 21, 2023

Uh oh!

cbodley commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbodley commented Jun 21, 2023

Uh oh!

cbodley commented Jun 22, 2023

Uh oh!

mattbenjamin commented Jun 22, 2023

Uh oh!

yuvalif commented Jun 22, 2023

Uh oh!

TRYTOBE8TME commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbodley commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbodley commented Jun 22, 2023

Uh oh!

yuvalif commented Jun 22, 2023

Uh oh!

cbodley commented Jun 23, 2023

Uh oh!

Reviewers

Assignees

cbodley commented Jun 21, 2023 •

edited

Loading

TRYTOBE8TME commented Jun 22, 2023 •

edited

Loading

cbodley commented Jun 22, 2023 •

edited

Loading