Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rgw/aio: remove RGWSI_RADOS from generic Aio::get() #50347

Merged
merged 5 commits into from
Mar 14, 2023
Merged

rgw/aio: remove RGWSI_RADOS from generic Aio::get() #50347

merged 5 commits into from
Mar 14, 2023

Conversation

cbodley
Copy link
Contributor

@cbodley cbodley commented Mar 2, 2023 •
edited
Loading

so that rgw::Aio and its AioThrottle subclasses can be used outside of the rados backend

the idea is to move all of the rados-specifics into the librados_op() calls. this only changes the base rgw::Aio class, so doesn't compile yet. next we'd need to update the AioThrottle classes, and the Aio callers in rgw_rados.cc, rgw_putobj_processor.cc, and rgw_d3n_datacache.h

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 cbodley mentioned this pull request Mar 2, 2023
Closed
14 tasks
@github-actions github-actions bot added the rgw label Mar 2, 2023
cbodley
cbodley commented Mar 2, 2023
View reviewed changes
src/rgw/rgw_aio.cc
@@ -28,10 +28,11 @@ void cb(librados::completion_t, void* arg);

struct state {
Aio* aio;
librados::IoCtx ctx;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AioResult::user_data has enough space for 3 pointers, and librados::IoCtx should only contain one pointer to IoCtxImpl, so state should still fit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doh, IoCtx is ref-counted so fails this static assertion:

rgw_aio.cc:41:22: error: static assertion failed
   41 |   static_assert(std::is_trivially_destructible_v<state>);
      |                 ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamemerson can't the librados callback just call state's destructor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamemerson can't the librados callback just call state's destructor?

No reason it couldn't so long as we document that it will have to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, i added some extra comments here. i was also missing a call to the destructor in aio_abstract()'s error path

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valgrind complains about a use-after-free of this librados::IoCtx; an io_context_pool thread calls IoCtx::close() after this ~state() destructor runs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just had to call the destructor before aio->put(r); woke up the calling thread

@cbodley
Copy link
Contributor Author

cbodley commented Mar 2, 2023

from IRC:

(10:58:48 AM) Aemerson: cbodley: Believe it or not I actually have a branch that removes RGWSI_RADOS from Aio that I'd been holding on to until after reef
(10:58:57 AM) Aemerson: Though it also happens to just get rid of RGWSI_RADOS entirely
(10:59:01 AM) Aemerson: And makes everyone use the same RADOS handle
(10:59:06 AM) Aemerson: And gets rid of all the 'open' crap.
(11:00:17 AM) cbodley: joy!
(11:00:30 AM) Aemerson: If you don't object to it I can make a PR out of that.
(11:00:37 AM) Aemerson: I haven't run it through Sepia so maybe I should do that first.
(11:01:11 AM) cbodley: great Aemerson; whatever we can do to unblock Pritha on #50198
(11:01:40 AM) Aemerson: Sure, I'll rebase it.

@cbodley cbodley marked this pull request as ready for review March 2, 2023 20:18
@cbodley cbodley requested a review from a team as a code owner March 2, 2023 20:18
pritha-srivastava
pritha-srivastava reviewed Mar 3, 2023
View reviewed changes
src/rgw/rgw_aio.h
@@ -34,7 +32,7 @@ struct D3nGetObjData;
namespace rgw {

struct AioResult {
RGWSI_RADOS::Obj obj;
rgw_raw_obj obj;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbodley : In case of non rados stores/ filters, can pool and loc be left uninitialised?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can I use this PR now, to get past the segfault that I encountered?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbodley : In case of non rados stores/ filters, can pool and loc be left uninitialised?

yeah. i used rgw_raw_obj isntead of just std::string here because the rados backend needs to know the pool/loc of the write completions it gets, but other backends don't need them

Also, can I use this PR now, to get past the segfault that I encountered?

it builds and passes s3tests, so it's worth a shot!

@cbodley cbodley force-pushed the wip-rgw-sal-aio branch 2 times, most recently from c0be574 to 8f7517f Compare March 3, 2023 17:56
@cbodley
rgw/aio: librados_op() requires librados::IoCtx
1bc8d66 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
rgw/aio: remove RGWSI_RADOS from generic Aio::get()
5c3f8da 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
rgw/aio: update Aio callers in rados and d3n
d4eec9d 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
test/rgw: ceph_test_rgw_throttle no longer needs rados
e587a71 
run it as a unittest instead of a workunit

Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley cbodley force-pushed the wip-rgw-sal-aio branch 2 times, most recently from 5d8c519 to 585b6c1 Compare March 12, 2023 23:02
@cbodley
rgw/rados: copy_obj_data() can use yielding throttle
b929958 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
Copy link
Contributor Author

cbodley commented Mar 13, 2023

@adamemerson could i ask for your final review please?

adamemerson
adamemerson approved these changes Mar 13, 2023
View reviewed changes
Copy link
Contributor

@adamemerson adamemerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good PR and I approve of it.

@cbodley
Copy link
Contributor Author

cbodley commented Mar 14, 2023

passed qa in https://pulpito.ceph.com/cbodley-2023-03-13_18:22:40-rgw-wip-cbodley-testing-distro-default-smithi/

@cbodley cbodley merged commit a22b26e into ceph:main Mar 14, 2023
@cbodley cbodley deleted the wip-rgw-sal-aio branch March 14, 2023 13:43
@cbodley cbodley mentioned this pull request Oct 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pritha-srivastava pritha-srivastava pritha-srivastava left review comments

@adamemerson adamemerson adamemerson approved these changes

Assignees
No one assigned
Labels
build/ops rgw tests wip-cbodley-testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cbodley @adamemerson @pritha-srivastava