Skip to content

osd/OSD: simple perfcounter usage in OSDService#33770

Merged
tchaikov merged 1 commit intoceph:masterfrom
majianpeng:osd-simple-logger
Jul 16, 2020
Merged

osd/OSD: simple perfcounter usage in OSDService#33770
tchaikov merged 1 commit intoceph:masterfrom
majianpeng:osd-simple-logger

Conversation

@majianpeng
Copy link
Member

@majianpeng majianpeng commented Mar 6, 2020

Move create_logger()/create_recoverystate_perf() into construct to avoid
check logger in OSDService.
And avoid in destructor delete nullptr.

Signed-off-by: Jianpeng Ma jianpeng.ma@intel.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show available Jenkins commands
  • jenkins retest this please
  • 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 backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@stale
Copy link

stale bot commented May 6, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label May 6, 2020
@majianpeng
Copy link
Member Author

majianpeng commented May 7, 2020

retest this please

@stale stale bot removed the stale label May 7, 2020
@majianpeng
Copy link
Member Author

majianpeng commented May 8, 2020

@tchaikov . ping.

@stale
Copy link

stale bot commented Jul 7, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Jul 7, 2020
tchaikov
tchaikov approved these changes Jul 7, 2020
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

LGTM. but the order of initialization could be confusing in future. as we keep a reference of pointer to logger and recoverystate_perf, do you think it would help with the readability to change create_logger() and create_recoverystate_perf() so they return a logger and recoverystate_perf respectively, and call them in the ctor's member initialization list, and init service, afterwards?

@stale stale bot removed the stale label Jul 7, 2020
@majianpeng majianpeng force-pushed the osd-simple-logger branch from e67f42b to 7b267ef Compare July 7, 2020 22:05
@majianpeng
Copy link
Member Author

majianpeng commented Jul 7, 2020

@tchaikov . update by your suggestion. please review.

@tchaikov
Copy link
Contributor

tchaikov commented Jul 8, 2020

jenkins test make check

@tchaikov
Copy link
Contributor

tchaikov commented Jul 8, 2020 

2020-07-07T22:34:34.383+0000 7effbd18e700 -1 error: monitor data filesystem reached concerning levels of available storage space (available: 2% 10 GiB)
you may adjust 'mon data avail crit' to a lower value to make this go away (default: 5%)

@tchaikov
Copy link
Contributor

tchaikov commented Jul 8, 2020

jenkins test dashboard backend

tchaikov
tchaikov reviewed Jul 11, 2020
View reviewed changes
src/osd/OSD.cc
logger(NULL),
recoverystate_perf(NULL),
logger(create_logger()),
recoverystate_perf(create_recoverystate_perf()),
Copy link
Contributor

@tchaikov tchaikov Jul 11, 2020

Choose a reason for hiding this comment

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

because get_osdmap_epoch() is used for printing the current osdmap epoch in OSD::create_logger() and in OSD::create_recoverystate_perf(), while osdmap is initialized after logger and recoverystate_perf. valgrind emitted warnings like

<error>
  <unique>0x0</unique>
  <tid>1</tid>
  <kind>UninitCondition</kind>
  <what>Conditional jump or move depends on uninitialised value(s)</what>
...
    <frame>
      <ip>0x791D98</ip>
      <obj>/usr/bin/ceph-osd</obj>
      <fn>atomic_load&lt;const OSDMap&gt;</fn>
      <dir>/usr/include/c++/8/bits</dir>
      <file>shared_ptr_atomic.h</file>
      <line>107</line>
    </frame>
    <frame>
      <ip>0x791D98</ip>
      <obj>/usr/bin/ceph-osd</obj>
      <fn>get_osdmap</fn>
      <dir>/usr/src/debug/ceph-16.0.0-3411.g73dcd530b88.el8.x86_64/src/osd</dir>
      <file>OSD.h</file>
      <line>1686</line>
    </frame>
    <frame>
      <ip>0x791D98</ip>
      <obj>/usr/bin/ceph-osd</obj>
      <fn>OSD::get_osdmap_epoch() const</fn>
      <dir>/usr/src/debug/ceph-16.0.0-3411.g73dcd530b88.el8.x86_64/src/osd</dir>
      <file>OSD.h</file>
      <line>1690</line>
    </frame>
    <frame>
      <ip>0x72842B</ip>
      <obj>/usr/bin/ceph-osd</obj>
      <fn>OSD::create_logger()</fn>
      <dir>/usr/src/debug/ceph-16.0.0-3411.g73dcd530b88.el8.x86_64/src/osd</dir>
      <file>OSD.cc</file>
      <line>4048</line>
    </frame>
    <frame>
      <ip>0x781D2E</ip>
      <obj>/usr/bin/ceph-osd</obj>
      <fn>OSD::OSD(ceph::common::CephContext*, ObjectStore*, int, Messenger*, Messenger*, Messenger*, Messenger*, Messenger*, Messenger*, Messenger*, MonClient*, std::__cxx11::basic_string&lt;char, std::char_traits&lt;char&gt;, std::allocator&lt;char&gt; &gt; const&amp;, std::__cxx11::basic_string&lt;char, std::char_traits&lt;char&gt;, std::allocator&lt;char&gt; &gt; const&amp;, ceph::async::io_context_pool&amp;)</fn>
      <dir>/usr/src/debug/ceph-16.0.0-3411.g73dcd530b88.el8.x86_64/src/osd</dir>
      <file>OSD.cc</file>
      <line>2157</line>
    </frame>
...

so i think we have at least three options here

  1. do not print debugging messages like dout(10) << "create_logger" << dendl; in the beginning of these functions.
  2. move declarations of logger and recoverystate_perf in OSD class after that of _osdmap
  3. keep existing implementation unchanged -- initialize logger and recoverystate_perf with nullptr. and set them in the body of ctor function.

i am in favor of the first option. what do you think?

tchaikov
tchaikov requested changes Jul 11, 2020
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

https://pulpito.ceph.com/kchai-2020-07-11_05:07:55-rados-wip-kefu-testing-2020-07-11-1109-distro-basic-smithi/ failed.

osd/OSD: simple perfcounter usage in OSDService
5ace640 
Move create_logger()/create_recoverystate_perf() into construct to
avoid check logger in OSDService. And avoid in destructor delete
nullptr.

Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>
@tchaikov
Copy link
Contributor

tchaikov commented Jul 15, 2020

failures tracked / fixed by

@tchaikov
Copy link
Contributor

tchaikov commented Jul 15, 2020

jenkins test make check

@tchaikov tchaikov self-assigned this Jul 15, 2020
@tchaikov tchaikov merged commit 49fa970 into ceph:master Jul 16, 2020
@sebastian-philipp
Copy link
Contributor

sebastian-philipp commented Jul 17, 2020

https://tracker.ceph.com/issues/46596 related?

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

Reviewers

@tchaikov tchaikov tchaikov approved these changes

Assignees

@tchaikov tchaikov

Labels

core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@majianpeng @tchaikov @sebastian-philipp @batrick