Skip to content

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

Merged
idryomov merged 1 commit intoceph:mainfrom
idryomov:wip-65889-more
Jan 30, 2025
Merged

cls/rbd: don't use read API for write-like methods#61471
idryomov merged 1 commit intoceph:mainfrom
idryomov:wip-65889-more

Conversation

@idryomov
Copy link
Contributor

@idryomov idryomov commented Jan 21, 2025 •
edited
Loading

This is a follow-up for commit af17631 (objclass: change objclass exec read calls to write ones where needed) which missed mirror_uuid_set and raced with the addition of mirror_remote_namespace_set.

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

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)
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

@idryomov
cls/rbd: don't use read API for write-like methods
4612195 
This is a follow-up for commit af17631 (objclass: change objclass
exec read calls to write ones where needed) which missed mirror_uuid_set
and raced with the addition of mirror_remote_namespace_set.

Fixes: https://tracker.ceph.com/issues/65889
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
@idryomov idryomov requested a review from a team as a code owner January 21, 2025 20:27
@github-actions github-actions bot added the rbd label Jan 21, 2025
@idryomov idryomov mentioned this pull request Jan 21, 2025
Merged
14 tasks
@idryomov
Copy link
Contributor Author

idryomov commented Jan 22, 2025

jenkins test api

@bill-scales
Copy link
Contributor

bill-scales commented Jan 24, 2025

@idryomov it looks like this is an easy mistake to make. Should we be renaming the API to make it clear that it should only be used for read execs? Should we be trying to perform balanced/local optimizations at all for execs - are these performance critical enough to justify the optimization or should we just be routing all execs to the primary?

@idryomov
Copy link
Contributor Author

idryomov commented Jan 24, 2025

Should we be renaming the API to make it clear that it should only be used for read execs?

It's a public librados API that has been around in its current form since 2011, so renaming is probably not in the cards.

Note that the potential confusion extends wider than just IoCtx::exec(). The lower-level exec() variants are present both in ObjectReadOperation and ObjectWriteOperation, so even if IoCtx::exec() didn't exist, there would be nothing preventing someone from calling a write-like method through ObjectReadOperation. I guess it's just the nature of object class methods that a method can read, write or both read and write, but this all goes on the wire through a single opcode. The method name is just a string, so the Objecter on the client side has no way of knowing.

@athanatos @yehudasa Perhaps OSDs could be taught to fail CEPH_OSD_OP_CALL op if it turns out that the method is marked CLS_METHOD_WR but MOSDOp message doesn't have CEPH_OSD_FLAG_WRITE set (and same for CLS_METHOD_RD and CEPH_OSD_FLAG_READ)? This would need to be opt-in for backwards compatibility reasons, but we could at least enable this in testing.

Should we be trying to perform balanced/local optimizations at all for execs - are these performance critical enough to justify the optimization or should we just be routing all execs to the primary?

I think it depends on the workload. For RBD, it likely doesn't matter. RGW uses object class methods much more, so there the optimization might well be worth it. Routing all execs to the primary is what ended up happening in #56180 after all, albeit with a new rados_replica_read_policy_on_objclass option which defaults to false.

@idryomov
Copy link
Contributor Author

idryomov commented Jan 30, 2025

No related failures:

https://pulpito.ceph.com/dis-2025-01-29_00:32:37-rbd-wip-dis-testing-distro-default-smithi/
https://pulpito.ceph.com/dis-2025-01-29_09:25:27-rbd-wip-dis-testing-distro-default-smithi/
https://pulpito.ceph.com/dis-2025-01-29_14:05:53-rbd-wip-dis-testing-distro-default-smithi/

@idryomov idryomov merged commit 0e4a0ee into ceph:main Jan 30, 2025
5 of 6 checks passed
@idryomov idryomov deleted the wip-65889-more branch January 30, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ajarr ajarr ajarr approved these changes

Assignees

No one assigned

Labels

cleanup rbd wip-dis-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@idryomov @bill-scales @ajarr