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

os/bluestore: move cache_trim into MempoolThread #15380

Merged
merged 1 commit into from Jun 5, 2017

Conversation

Projects
None yet
5 participants
@xiexingguo
Member

xiexingguo commented May 31, 2017

This simplifies the code a little and local FIO tests show no perfromance regressions.

Signed-off-by: xie xingguo xie.xingguo@zte.com.cn

<< dendl;
return;
}
cache->last_trim_seq = seq;

This comment has been minimized.

@ifed01

ifed01 May 31, 2017

Contributor

forget to remove last_trim_seq member from Cache?

@ifed01

ifed01 May 31, 2017

Contributor

forget to remove last_trim_seq member from Cache?

@ifed01

ifed01 approved these changes May 31, 2017

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas May 31, 2017

Member

IIRC the reason I left trim_cache in the IO thread was for cpu cache locality (since that is the thread that's doing the cache updates). Mempool thread does not. That said, this gets the potentially long-running trim out of the thread that blocks other ops' progress, and we could shard it and pin the threads in the future if the cpu caching effects are significant...

Member

liewegas commented May 31, 2017

IIRC the reason I left trim_cache in the IO thread was for cpu cache locality (since that is the thread that's doing the cache updates). Mempool thread does not. That said, this gets the potentially long-running trim out of the thread that blocks other ops' progress, and we could shard it and pin the threads in the future if the cpu caching effects are significant...

@markhpc

This comment has been minimized.

Show comment
Hide comment
@markhpc

markhpc May 31, 2017

Member

I just got done testing this PR out. It doesn't appear to affect performance dramatically in any of my tests either way, though did appear to dramatically reduce time spent in trim (even in the MempoolThread?) and as a result lowered pg::lock contention from 95% in the finisher thread to something more like 78%. Unfortunately we are still spending a lot of time in the tp_osd_tp threads simply doing reads of the onodes when there isn't enough cache to keep them in memory. Included are gdb wallclock profiles on a run before this PR and after this PR.

after_gdbtrace.txt
before_gdbtrace.txt

Member

markhpc commented May 31, 2017

I just got done testing this PR out. It doesn't appear to affect performance dramatically in any of my tests either way, though did appear to dramatically reduce time spent in trim (even in the MempoolThread?) and as a result lowered pg::lock contention from 95% in the finisher thread to something more like 78%. Unfortunately we are still spending a lot of time in the tp_osd_tp threads simply doing reads of the onodes when there isn't enough cache to keep them in memory. Included are gdb wallclock profiles on a run before this PR and after this PR.

after_gdbtrace.txt
before_gdbtrace.txt

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jun 1, 2017

Member

changelog:

  • drop last_trim_seq member from Cache as Igor kindly reminded
Member

xiexingguo commented Jun 1, 2017

changelog:

  • drop last_trim_seq member from Cache as Igor kindly reminded
@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 1, 2017

Member

retest this please

Member

liewegas commented Jun 1, 2017

retest this please

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 2, 2017

Member

rebase please?

Member

liewegas commented Jun 2, 2017

rebase please?

os/bluestore: move cache_trim into MempoolThread
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jun 3, 2017

Member

changelog:

  • rebase on #15398 (to use new (cache) mempool)
  • fix a warning introduced by #15398
Member

xiexingguo commented Jun 3, 2017

changelog:

  • rebase on #15398 (to use new (cache) mempool)
  • fix a warning introduced by #15398

@xiexingguo xiexingguo closed this Jun 3, 2017

@xiexingguo xiexingguo reopened this Jun 3, 2017

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jun 3, 2017

Member

changelog:

fix a warning introduced by #15398

Drop the above change, since it has already been addressed by #15435

Member

xiexingguo commented Jun 3, 2017

changelog:

fix a warning introduced by #15398

Drop the above change, since it has already been addressed by #15435

@liewegas liewegas merged commit 92d4866 into ceph:master Jun 5, 2017

3 checks passed

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

@xiexingguo xiexingguo deleted the xiexingguo:wip-cache-trim branch Jun 6, 2017

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