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

osd: Preserve OSDOp information for historic ops #15265

Merged
merged 1 commit into from Jul 5, 2017

Conversation

Projects
None yet
4 participants
@cooldavid
Contributor

cooldavid commented May 24, 2017

clear_buffers() actually clears all OSDOp informations, so that we can only see empty ops with dump_historic_ops. This commit keeps the op information, and only minimal buffers for printing op information.

osd: Preserve OSDOp information for historic ops
clear_buffers() actually clears all OSDOp informations, so that we can
only see empty ops with `dump_historic_ops`. This commit keeps the op
information, and only minimal buffers for printing op information.

Signed-off-by: Guo-Fu Tseng <david_tseng@bigtera.com>
@gregsfortytwo

This comment has been minimized.

Member

gregsfortytwo commented May 24, 2017

I like the idea but don't quite get how it's functioning with the bufferlist copying and claiming . Can you demonstrate the difference in output and explain that a little more?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jun 25, 2017

retest this please.

@cooldavid

This comment has been minimized.

Contributor

cooldavid commented Jun 27, 2017

Something wrong with the patch?

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 27, 2017

Nope, just needs to go through qa. Probably tomorrow!

void OSDOp::clear_data(vector<OSDOp>& ops)
{
for (unsigned i = 0; i < ops.size(); i++) {

This comment has been minimized.

@tchaikov

tchaikov Jun 27, 2017

Contributor

could make this method a non-static member method of OSDOp.

This comment has been minimized.

@cooldavid

cooldavid Jun 29, 2017

Contributor

Do you mean having non-static member method of OSDOp, and loop it by caller?
I just feel that it might be cleaner to implement OSDOp related function in OSDOp, since there should be no caller want to clear just one OSDOp data.

This comment has been minimized.

@tchaikov

tchaikov Jun 29, 2017

Contributor

exactly.

I just feel that it might be cleaner to implement OSDOp related function in OSDOp, since there should be no caller want to clear just one OSDOp data.

the same to me. that's why i think this should be a method of OSDOp.

This comment has been minimized.

@cooldavid

cooldavid Jun 29, 2017

Contributor

@tchaikov
One of the reason I decide to use static function is seeing the functions defined above clear_data()

ex: static void merge_osd_op_vector_out_data(vector<OSDOp>& ops, bufferlist& out);
(in osd_types.h)

The use cases are very similar.

This comment has been minimized.

@tchaikov

tchaikov Jun 29, 2017

Contributor

fair enough.

bufferlist bl;
bl.append(bp);
bl.copy_in(0, op.op.xattr.name_len, op.indata);
op.indata.claim(bl);

This comment has been minimized.

@tchaikov

tchaikov Jun 27, 2017

Contributor

when/how will op.indata be referenced by dump_historic_ops()? and why you want to rebuild op.indata? BTW, what you accomplish here is equivalent to what ceph::buffer::list::rebuild() does, i think.

This comment has been minimized.

@liewegas

liewegas Jun 27, 2017

Member

I think the idea is that you only keep enough buffer around to render operator<<, but can free the rest of the memory.

This whole problem is annoying. A simple solution would just be to render the operator<< output as a string and store that in OpRequest, but it turns out it is the slowest part (or close to it) of the whole optracker. And most of the time nobody queries this info anyway.

TBH this method looks pretty slow too.. I'm not sure the slowdown here isn't too much that we're better off with the (yes, annoying) status quo?

This comment has been minimized.

@tchaikov

tchaikov Jun 27, 2017

Contributor

got it, Message::print() is used to print out the "description" field. but my question of

and why you want to rebuild op.indata? BTW, what you accomplish here is equivalent to what ceph::buffer::list::rebuild() does, i think.

still holds. bufferlist::write(int off, int len, std::ostream& out) is able to handle non-contiguous bufferlist.

and yeah, i am a little bit concern about the performance impact of this change. this change

  1. keeps the payload around for awhile, and
  2. rebuilds the op.indata (which i don't think is necessary, though)

This comment has been minimized.

@cooldavid

cooldavid Jun 29, 2017

Contributor
  1. Most of the data is freed, only minimal data kept in memory.

  2. By using fixed size buffer pointer, and the kept info is just several bytes allocated via tcmalloc/jemalloc, which should be efficient.
    With the action of copy those several bytes, not rebuilding whole bufferlist.

This comment has been minimized.

@tchaikov

tchaikov Jun 29, 2017

Contributor

Most of the data is freed, only minimal data kept in memory.

ahh, right! you are copying only the name of xattr, or the cls and method name.

By using fixed size buffer pointer, and the kept info is just several bytes allocated via tcmalloc/jemalloc, which should be efficient.
With the action of copy those several bytes, not rebuilding whole bufferlist.

okay, assuming the names are very short.

@tchaikov tchaikov merged commit b0de4e2 into ceph:master Jul 5, 2017

4 of 5 checks passed

default Build finished.
Details
Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details

@cooldavid cooldavid deleted the cooldavid:pr-osd-historic-op branch Jul 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment