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.

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

lttng: Trace rgw data transfer, bi entry and object header update processes #20556

Merged
merged 1 commit into from Mar 29, 2018

Conversation

Projects
None yet
4 participants
@yanghonggang
Copy link
Contributor

commented Feb 23, 2018

Signed-off-by: Yang Honggang joseph.yang@xtaotech.com

@tchaikov tchaikov added the rgw label Feb 23, 2018

@@ -45,8 +47,10 @@ set(osd_traces oprequest.tp osd.tp pg.tp)
add_tracing_library(osd_tp "${osd_traces}" 1.0.0)
add_tracing_library(rados_tp librados.tp 2.0.0)
add_tracing_library(os_tp objectstore.tp 1.0.0)
add_tracing_library(rgw_op_tp rgw_op.tp 1.0.0)
add_tracing_library(rgw_rados_tp rgw_rados.tp 1.0.0)

This comment has been minimized.

Copy link
@cbodley

cbodley Feb 23, 2018

Contributor

there's a build failure in radosgw:

rgw_op.cc:55:10: fatal error: tracing/rgw_op.h: No such file or directory

it looks like radosgw will need to add dependencies to make sure that the headers get generated first. something like this in src/rgw/CMakeLists.txt:

if(WITH_LTTNG)
  add_dependencies(rgw_a rgw_op-tp rgw_rados-tp)
endif()

This comment has been minimized.

Copy link
@yanghonggang

yanghonggang Feb 24, 2018

Author Contributor

@cbodley Yes, I forgot to add this dependence.

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2018

@cbodley Please help to review.

@@ -3532,6 +3542,9 @@ void RGWPutObj::execute()
}
}

#ifdef WITH_LTTNG
tracepoint(rgw_op, before_data_transfer, s->req_id.c_str());
#endif

This comment has been minimized.

Copy link
@cbodley

cbodley Feb 26, 2018

Contributor

it doesn't look like these #ifdef WITH_LTTNG blocks are necessary around tracepoint() calls - when WITH_LTTNG isn't defined, #define tracepoint(...) means that the call will be removed by the preprocessor

This comment has been minimized.

Copy link
@yanghonggang

yanghonggang Feb 28, 2018

Author Contributor

@cbodley You are right. :)

@@ -2873,6 +2873,9 @@ class RGWRados : public AdminSocketHook
int write_meta(uint64_t size, uint64_t accounted_size,
map<std::string, bufferlist>& attrs);
int write_data(const char *data, uint64_t ofs, uint64_t len, bool exclusive);
const struct req_state* get_req_state() {
return (const struct req_state*)(target->get_ctx().user_ctx);
}

This comment has been minimized.

Copy link
@cbodley

cbodley Feb 26, 2018

Contributor

i think this piece is going to need more work

first, i didn't see anywhere that actually initialized RGWObjectCtx::user_ctx. but more importantly, there are a lot of places where we do bucket index operations that aren't part of a request (stuff like radosgw-admin commands, object lifecycle expiration, multisite sync, etc.), so we're not guaranteed to have a req_state

are we interested in tracing these background operations too? if so, the code paths that don't have an associated req_state would need to fake up some kind of unique req_id string to pass into tracepoint() instead

either way, we'll need to pass the req_state's req_id string into _do_write_meta() somehow - preferably as a const char* instead of req_state*, and as an extra argument to either _do_write_meta() or Write's constructor instead of coupling it with RGWObjectCtx

This comment has been minimized.

Copy link
@yanghonggang

yanghonggang Mar 1, 2018

Author Contributor

@cbodley
Yes, RGWObjectCtxx::user_ctx may be NULL. All write_meta() calls are in rgw_op.cc and rgw_rados.cc. Every place where write_meta() is called, there must be a related req_state.

req_state is initialized in process_request().

  struct req_state rstate(g_ceph_context, &rgw_env, &userinfo);    
  struct req_state *s = &rstate;    
  RGWObjectCtx rados_ctx(store, s);  // s => user_ctx   
  ...     
  s->req_id = store->unique_id(req->id);

Now only put objects related bi updates are concerned about.

As RGWObjectCtx already has req_state/req_id info, do we really need to add a req_id item in Write?

This comment has been minimized.

Copy link
@cbodley

cbodley Mar 1, 2018

Contributor

ok, thanks for pointing out where process_request() initializes the user_ctx. that rados_ctx gets stored in s->obj_ctx and used throughout rgw_op.cc

but there are a lot of other cases where RGWObjectCtx is initialized on the stack without that user_ctx argument, so we'll still have to make the tracepoints in _do_write_meta() deal with the cases where this get_req_state() function returns null

This comment has been minimized.

Copy link
@cbodley

cbodley Mar 5, 2018

Contributor

thinking more about void* RGWObjectCtx::user_ctx.. now that we have code that requires it to be a req_state* if non-null, then its name and type really should be changed to reflect that. can you please change void *user_ctx to req_state *s, and also make the second RGWObjectCtx constructor explicit?

This comment has been minimized.

Copy link
@yanghonggang

yanghonggang Mar 6, 2018

Author Contributor

@cbodley Done

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2018

@cbodley ping

@cbodley
Copy link
Contributor

left a comment

looking good! just a couple cleanups and i think this is ready

@@ -2873,6 +2873,9 @@ class RGWRados : public AdminSocketHook
int write_meta(uint64_t size, uint64_t accounted_size,
map<std::string, bufferlist>& attrs);
int write_data(const char *data, uint64_t ofs, uint64_t len, bool exclusive);
const req_state* get_req_state() {
return (const req_state*)(target->get_ctx().s);

This comment has been minimized.

Copy link
@cbodley

cbodley Mar 6, 2018

Contributor

don't need the cast anymore - and the function itself can be const

This comment has been minimized.

Copy link
@yanghonggang

yanghonggang Mar 7, 2018

Author Contributor

ok

foreach(tp ${tps})
message(STATUS "process ${tp}")

This comment has been minimized.

Copy link
@cbodley

cbodley Mar 6, 2018

Contributor

i think we can drop these debug messages

This comment has been minimized.

Copy link
@yanghonggang

yanghonggang Mar 7, 2018

Author Contributor

ok

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2018

@cbodley Updated

/home/jenkins-build/build/workspace/ceph-pull-requests/qa/standalone/ceph-helpers.sh:535: run_mgr: return 1
2018-03-07 05:39:41.264 7f6f93d7f700 -1 auth: unable to find a keyring on /etc/ceph/ceph.client.admin.keyring,/etc/ceph/ceph.keyring,/etc/ceph/keyring,/etc/ceph/keyring.bin,: (2) No such file or directory

This build failure is unrelated

@cbodley

cbodley approved these changes Mar 7, 2018

Copy link
Contributor

left a comment

nice work!

@cbodley

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

jenkins test this please

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

@cbodley ping

@cbodley cbodley added the needs-qa label Mar 12, 2018

@cbodley

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

@yanghonggang could you please rebase to resolve the conflict?

@cbodley cbodley removed the needs-qa label Mar 12, 2018

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

@cbodley done

@yuriw

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

@yuriw yuriw removed the wip-yuri-testing label Mar 19, 2018

@cbodley

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

@yanghonggang it looks like the rpm builds are failing with:

RPM build errors:
Installed (but unpackaged) file(s) found:
/usr/lib64/librgw_op_tp.so
/usr/lib64/librgw_op_tp.so.1
/usr/lib64/librgw_op_tp.so.1.0.0
/usr/lib64/librgw_rados_tp.so
/usr/lib64/librgw_rados_tp.so.1
/usr/lib64/librgw_rados_tp.so.1.0.0

could you please update ceph.spec.in to match what it's doing for the other tp libs? (ie libos_tp.so.*)

lttng: Trace rgw data transfer, bi entry and object header update pro…
…cesses

Signed-off-by: Yang Honggang <joseph.yang@xtaotech.com>
@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2018

@yuriw After this update, I can successfully build rpm packages now. Please help to test.

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2018

@tchaikov tchaikov merged commit 4fb21e1 into ceph:master Mar 29, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
@tchaikov

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2018

@yanghonggang sorry, i forgot to merge your PR after testing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.