mixed balance read and rwordered in read ops by NitzanMordhai · Pull Request #62806 · ceph/ceph

NitzanMordhai · 2025-04-14T14:38:25Z

Ops with mixed flags of balance_reads and rwordered will cause errors when send to replica, we should not allow that mix of flags

Fixes: https://tracker.ceph.com/issues/70715

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

NitzanMordhai · 2025-04-15T12:29:26Z

@athanatos can you please review?

rzarzynski

LGTM.

rzarzynski · 2025-04-17T16:47:42Z

src/osdc/Objecter.h

+
+    // If the op is rwordered, strip out the balanced and localized flags.
+    if (flags & CEPH_OSD_FLAG_RWORDERED) {
+      ret &= ~(CEPH_OSD_FLAG_BALANCE_READS | CEPH_OSD_FLAG_LOCALIZE_READS);


rzarzynski · 2025-04-17T16:52:15Z

src/osd/PrimaryLogPG.cc

+      op->rwordered()) {
+    dout(4) << __func__ << ": rebalance reads with rwordered not allowed "
+       << *m << dendl;
+    osd->reply_op_error(op, -EINVAL);


As the objecter no longer supports the problematic mixed ops, why are we checking instead of asserting? Is that for the sake of older clients?

i just wanted to be sure: https://tracker.ceph.com/issues/70715#note-13 see the #2

Yes, it's for old clients.
You basically have no control over client versions and you need to be ready to handle even misbehaving ones gracefully for long time (this fits particularly well to kernel clients).
Definitely you don't want to assert in such scenario.

ljflores · 2025-04-21T18:12:34Z

jenkins test make check

ljflores · 2025-04-21T18:13:05Z

Retriggered make check since the logs expired, but there might be a related issue there. We should check if it happens again. In the meantime, marking this ready for QA since it's been approved.

Matan-B · 2025-04-22T09:05:16Z

src/osd/PrimaryLogPG.cc

 			 CEPH_OSD_FLAG_LOCALIZE_READS)) &&
      op->may_read() &&
-      !(op->may_write() || op->may_cache())) {
+      !(op->may_write() || op->may_cache() || op->rwordered())) {


IIUC the issue is accepting flag of rwordered and balanced reads - which is handled by:
if (m->has_flag(CEPH_OSD_FLAG_BALANCE_READS) && op->rwordered()) above.

Why are we also adding another check here? This will disallow also localized reads with rwordered, is that the intent?

cc @NitzanMordhai, @rzarzynski

yep, that one is redundant and will add a new issue for localize.

Matan-B

LGTM - My comment was addressed - #62806 (review)
'
@rzarzynski, would you like to give this another look as you previously reviewed it? Thanks!

This might need a re-build since the PR has changed.

rzarzynski

I'm afraid we stopped doing the error handling for rwordered, localized reads.

rzarzynski · 2025-04-23T09:03:23Z

src/osd/PrimaryLogPG.cc

+      op->rwordered()) {
+    dout(4) << __func__ << ": rebalance reads with rwordered not allowed "
+       << *m << dendl;
+    osd->reply_op_error(op, -EINVAL);


Yes, it's for old clients.
You basically have no control over client versions and you need to be ready to handle even misbehaving ones gracefully for long time (this fits particularly well to kernel clients).
Definitely you don't want to assert in such scenario.

src/osd/PrimaryLogPG.cc

ljflores · 2025-04-24T19:48:46Z

jenkins test make check

ljflores · 2025-04-24T19:48:50Z

jenkins test api

rzarzynski · 2025-04-24T19:52:26Z

@ljflores: the review comments need to be addressed; at the moment merging is blocked.

NitzanMordhai · 2025-04-30T12:14:37Z

@rzarzynski please review

ljflores · 2025-05-05T20:00:08Z

Let's put this in a new batch.

rzarzynski · 2025-05-06T14:52:29Z

src/osdc/Objecter.h

+
+    // If the op is rwordered, strip out the balanced and localized flags.
+    if (flags & CEPH_OSD_FLAG_RWORDERED) {
+      ret &= ~(CEPH_OSD_FLAG_BALANCE_READS | CEPH_OSD_FLAG_LOCALIZE_READS);


src/osd/PrimaryLogPG.cc

ljflores · 2025-05-27T19:29:40Z

jenkins test api

ljflores · 2025-05-27T19:29:53Z

jenkins test make check

Objecter shouldn't sent ops with mixed rwordered and balance_read flags Fixes: https://tracker.ceph.com/issues/70715 Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

… flags do_op can't accept mixed flag of rwordered and balance_read Fixes: https://tracker.ceph.com/issues/70715 Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

amathuria · 2025-06-16T12:38:14Z

RADOS approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues71410

amathuria · 2025-06-16T12:38:36Z

jenkins test make check arm64

SrinivasaBharath · 2025-06-16T14:04:58Z

jenkins test make check arm64

…reads This is the counterpart of ceph#62806 Which fixes classical: https://tracker.ceph.com/issues/70715 Fixes: https://tracker.ceph.com/issues/71007 Signed-off-by: Matan Breizman <mbreizma@redhat.com>

…reads This is the counterpart of ceph/ceph#62806 Which fixes classical: https://tracker.ceph.com/issues/70715 Fixes: https://tracker.ceph.com/issues/71007 Signed-off-by: Matan Breizman <mbreizma@redhat.com> (cherry picked from commit 18cfa59bc900460d666c5c686499f87e433cc79f)

…reads This is the counterpart of ceph#62806 Which fixes classical: https://tracker.ceph.com/issues/70715 Fixes: https://tracker.ceph.com/issues/71007 Signed-off-by: Matan Breizman <mbreizma@redhat.com>

…reads This is the counterpart of ceph/ceph#62806 Which fixes classical: https://tracker.ceph.com/issues/70715 Fixes: https://tracker.ceph.com/issues/71007 Signed-off-by: Matan Breizman <mbreizma@redhat.com>

NitzanMordhai requested a review from athanatos April 14, 2025 14:38

NitzanMordhai requested a review from a team as a code owner April 14, 2025 14:38

github-actions bot added the core label Apr 14, 2025

NitzanMordhai force-pushed the wip-nitzan-mixed-balance-read-and-rwordered-fix branch from 5d13b41 to 7e99410 Compare April 14, 2025 14:39

rzarzynski self-requested a review April 14, 2025 18:32

rzarzynski added the bug-fix label Apr 14, 2025

NitzanMordhai force-pushed the wip-nitzan-mixed-balance-read-and-rwordered-fix branch from 7e99410 to 7415b79 Compare April 15, 2025 12:28

rzarzynski approved these changes Apr 17, 2025

View reviewed changes

ljflores added the needs-qa label Apr 21, 2025

SrinivasaBharath added the wip-bharath3-testing label Apr 22, 2025

Matan-B reviewed Apr 22, 2025

View reviewed changes

NitzanMordhai force-pushed the wip-nitzan-mixed-balance-read-and-rwordered-fix branch from 7415b79 to e18bff7 Compare April 22, 2025 17:43

NitzanMordhai requested a review from Matan-B April 22, 2025 17:43

Matan-B approved these changes Apr 23, 2025

View reviewed changes

rzarzynski requested changes Apr 23, 2025

View reviewed changes

ljflores requested a review from rzarzynski April 24, 2025 19:48

NitzanMordhai force-pushed the wip-nitzan-mixed-balance-read-and-rwordered-fix branch from e18bff7 to 0a52f39 Compare April 30, 2025 12:13

ljflores removed the wip-bharath3-testing label May 5, 2025

rzarzynski approved these changes May 6, 2025

View reviewed changes

NitzanMordhai force-pushed the wip-nitzan-mixed-balance-read-and-rwordered-fix branch from 0a52f39 to 94c4cb4 Compare May 7, 2025 09:25

SrinivasaBharath added the wip-bharath5-testing label May 18, 2025

NitzanMordhai added 2 commits May 28, 2025 06:58

Objecter: remove balance_read and localize_read if rwordered

40292f2

Objecter shouldn't sent ops with mixed rwordered and balance_read flags Fixes: https://tracker.ceph.com/issues/70715 Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

PrimeryLogPG: don't accept ops with mixed balance_reads and rwordered…

f68b217

… flags do_op can't accept mixed flag of rwordered and balance_read Fixes: https://tracker.ceph.com/issues/70715 Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

NitzanMordhai force-pushed the wip-nitzan-mixed-balance-read-and-rwordered-fix branch from 94c4cb4 to f68b217 Compare May 28, 2025 06:58

SrinivasaBharath merged commit edf8b22 into ceph:main Jun 17, 2025
12 checks passed

SrinivasaBharath removed the wip-bharath5-testing label Jun 17, 2025

NitzanMordhai mentioned this pull request Jul 15, 2025

tentacle: mixed balance read and rwordered in read ops #64506

Merged

Conversation

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

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

NitzanMordhai commented Apr 15, 2025

Uh oh!

rzarzynski left a comment

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!

ljflores commented Apr 21, 2025

Uh oh!

ljflores commented Apr 21, 2025

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!

Matan-B left a comment

Choose a reason for hiding this comment

Uh oh!

rzarzynski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ljflores commented Apr 24, 2025

Uh oh!

ljflores commented Apr 24, 2025

Uh oh!

rzarzynski commented Apr 24, 2025

Uh oh!

NitzanMordhai commented Apr 30, 2025

Uh oh!

ljflores commented May 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ljflores commented May 27, 2025

Uh oh!

ljflores commented May 27, 2025

Uh oh!

amathuria commented Jun 16, 2025

Uh oh!

amathuria commented Jun 16, 2025

Uh oh!

SrinivasaBharath commented Jun 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

NitzanMordhai commented Apr 14, 2025 •

edited

Loading