rgw: Cleaned up fix for 57562 by adamemerson · Pull Request #49179 · ceph/ceph

adamemerson · 2022-12-01T17:51:14Z

This is a more thoroughly cleaned up fix for https://tracker.ceph.com/issues/57562

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

src/cls/fifo/cls_fifo_ops.h

src/cls/fifo/cls_fifo_types.h

github-actions · 2022-12-09T18:51:12Z

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

src/cls/fifo/cls_fifo_types.h

kchheda3

Also wondering now do we even need the back-off changes?
coz the version will now not change for same update ?

kchheda3 · 2022-12-14T22:23:43Z

src/rgw/driver/rados/cls_fifo_legacy.cc

  assert(r >= 0);
 }

+void FIFO::maybe_backoff(const DoutPrefixProvider* dpp, std::uint64_t tid,


this function can be stanalone, doesn't use any variables from Fifo class !

It uses get_backoff().

src/cls/fifo/cls_fifo_types.h

adamemerson · 2022-12-14T23:35:34Z

Also wondering now do we even need the back-off changes? coz the version will now not change for same update ?

We might not, that's why I asked if you could test without it first. But I figured I should write it up just in case we need it for a fallback.

Fix logic error where disjunction was used instead of conjunction. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

Part tags make part creation and setting the head non-idempotent, leading to issues where racing RGWs may get confused about the correct tag for a part. (Or worse, potentially have the metadata header hold different value for a part than the part's header.) Consistently only requires that all nodes agree on the number. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

Except for the version type, where only versions with the same tag are comparable. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

Since we no longer use tags, journal entries are just an operation and a part number. If an entry being added is already in the journal, skip it. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

Only increment the version if we make an actual change. Return whether we have changed something or not so the OSD side can skip writing if there's no change. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

We don't really need the overhead and complexity of a multimap. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

First, make `_prepare_new_head` take the new head part number, so two calls racing from the same push will attempt to create the same head. Also remove the neorados FIFO since it doesn't have all the bug fixes in the legacy version and will be rewritten in terms of `async_compose` anyway. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

A racing client may delete the part we're trying to push to. Use `assert_exists()` to check, and re-read metadata if we receive -ENOENT. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

To break livelocks between two clients, increase the number of retries, but pause a random number (up to 100) of milliseconds every ten retries. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

kchheda3 · 2022-12-23T14:27:41Z

src/cls/fifo/cls_fifo_types.h


-    if (update.max_push_part_num()) {
+    if (update.max_push_part_num() &&
+	(max_push_part_num != *update.max_push_part_num())) {


just a thought instead of != check we could have
*update.max_push_part_num() > max_push_part_num ??

kchheda3 · 2023-01-05T14:20:00Z

Also wondering now do we even need the back-off changes? coz the version will now not change for same update ?

We might not, that's why I asked if you could test without it first. But I figured I should write it up just in case we need it for a fallback.

We have finished our testing without the back-off changes and everything is working fine w.r.t object count/mismatch.
so you can get rid of the backoff changes.

adamemerson · 2023-01-09T19:33:31Z

Moving this to wip-57562

github-actions bot added build/ops rgw tests labels Dec 1, 2022

kchheda3 reviewed Dec 2, 2022

View reviewed changes

src/cls/fifo/cls_fifo_ops.h Show resolved Hide resolved

src/cls/fifo/cls_fifo_types.h Outdated Show resolved Hide resolved

kchheda3 reviewed Dec 7, 2022

View reviewed changes

src/cls/fifo/cls_fifo_types.h Outdated Show resolved Hide resolved

github-actions bot added the needs-rebase label Dec 9, 2022

adamemerson force-pushed the wip-57562-redux branch from dc5d43c to 51f613c Compare December 14, 2022 18:43

github-actions bot removed the needs-rebase label Dec 14, 2022

adamemerson force-pushed the wip-57562-redux branch from 51f613c to 8e478dd Compare December 14, 2022 19:13

kchheda3 reviewed Dec 14, 2022

View reviewed changes

src/cls/fifo/cls_fifo_types.h Show resolved Hide resolved

adamemerson force-pushed the wip-57562-redux branch from 8e478dd to 2a81380 Compare December 14, 2022 22:00

kchheda3 reviewed Dec 14, 2022

View reviewed changes

adamemerson force-pushed the wip-57562-redux branch from 2a81380 to cbac99a Compare December 14, 2022 23:39

adamemerson added 9 commits December 15, 2022 14:44

cls/fifo: Fix same_or_later() comparison

5fd20d9

Fix logic error where disjunction was used instead of conjunction. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

cls/fifo: Use spaceship operator to implement comparisons

be837d3

Except for the version type, where only versions with the same tag are comparable. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

cls/fifo: Merge duplicate journal entries

42cd6c0

Since we no longer use tags, journal entries are just an operation and a part number. If an entry being added is already in the journal, skip it. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

cls/fifo: Journal is flat_set, not multimap

12a622f

We don't really need the overhead and complexity of a multimap. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

cls/fifo: Retry on push to nonexistent part

ae1f8be

A racing client may delete the part we're trying to push to. Use `assert_exists()` to check, and re-read metadata if we receive -ENOENT. Fixes: https://tracker.ceph.com/issues/57562 Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

cls/fifo: Add more retries with random backoff

af9280d

To break livelocks between two clients, increase the number of retries, but pause a random number (up to 100) of milliseconds every ten retries. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

adamemerson force-pushed the wip-57562-redux branch from cbac99a to af9280d Compare December 15, 2022 19:44

cbodley mentioned this pull request Dec 16, 2022

rgw: multisite stabilization for reef #48898

Merged

kchheda3 reviewed Dec 23, 2022

View reviewed changes

adamemerson closed this Jan 9, 2023

adamemerson deleted the wip-57562-redux branch January 10, 2023 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rgw: Cleaned up fix for 57562#49179

rgw: Cleaned up fix for 57562#49179
adamemerson wants to merge 9 commits intoceph:mainfrom
adamemerson:wip-57562-redux

adamemerson commented Dec 1, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 9, 2022

Uh oh!

Uh oh!

kchheda3 left a comment

Uh oh!

kchheda3 Dec 14, 2022

Uh oh!

adamemerson Dec 14, 2022

Uh oh!

Uh oh!

adamemerson commented Dec 14, 2022

Uh oh!

kchheda3 Dec 23, 2022

Uh oh!

kchheda3 commented Jan 5, 2023

Uh oh!

adamemerson commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

adamemerson commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 9, 2022

Uh oh!

Uh oh!

kchheda3 left a comment

Choose a reason for hiding this comment

Uh oh!

kchheda3 Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

adamemerson Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamemerson commented Dec 14, 2022

Uh oh!

kchheda3 Dec 23, 2022

Choose a reason for hiding this comment

Uh oh!

kchheda3 commented Jan 5, 2023

Uh oh!

adamemerson commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adamemerson commented Dec 1, 2022 •

edited

Loading