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: In osd_tp_thread, call _txc_finalize_kv. #14709

Merged
merged 1 commit into from May 4, 2017

Conversation

Projects
None yet
3 participants
@majianpeng
Member

majianpeng commented Apr 21, 2017

To reduce overhead of _kv_sync_thread, make _txc_finalize_kv do
in_osd_tp_thread.

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

@@ -7893,7 +7892,6 @@ void BlueStore::_kv_sync_thread()
}
for (auto txc : kv_submitting) {
assert(txc->state == TransContext::STATE_KV_QUEUED);
_txc_finalize_kv(txc, txc->t);

This comment has been minimized.

@liewegas

liewegas Apr 21, 2017

Member

This has to happen in kv_sync_thread when using ExtentFreelistManager. For the bitmap one it could be moved. Perhaps we should just drop the extent-based implementation....

@liewegas

liewegas Apr 21, 2017

Member

This has to happen in kv_sync_thread when using ExtentFreelistManager. For the bitmap one it could be moved. Perhaps we should just drop the extent-based implementation....

@liewegas liewegas changed the title from os/BlueStore: In osd_tp_thread, call _txc_finalize_kv. to os/blueStore: In osd_tp_thread, call _txc_finalize_kv. Apr 21, 2017

@liewegas liewegas added the bluestore label Apr 21, 2017

@liewegas

we need to either remove extentfreelistmanager, or we need to make the call site conditional on whether the freelist calls can be reordered (there is a bool method to report this in the interface).

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 21, 2017

Member

@ifed01 what do you think?

Member

liewegas commented Apr 21, 2017

@ifed01 what do you think?

@ifed01

This comment has been minimized.

Show comment
Hide comment
@ifed01

ifed01 Apr 24, 2017

Contributor

When choosing between additional flag verification at different locations and eliminating extentfreelist I'd select the second.
Wondering what's wrong with extentfreelist in this context though.

Contributor

ifed01 commented Apr 24, 2017

When choosing between additional flag verification at different locations and eliminating extentfreelist I'd select the second.
Wondering what's wrong with extentfreelist in this context though.

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 24, 2017

Member
Member

liewegas commented Apr 24, 2017

@majianpeng

This comment has been minimized.

Show comment
Hide comment
@majianpeng

majianpeng Apr 24, 2017

Member

So we can remove extent allocator for bluestore. Still keep for bluefs. Is it ok?

Member

majianpeng commented Apr 24, 2017

So we can remove extent allocator for bluestore. Still keep for bluefs. Is it ok?

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 24, 2017

Member
Member

liewegas commented Apr 24, 2017

@majianpeng

This comment has been minimized.

Show comment
Hide comment
@majianpeng

majianpeng Apr 24, 2017

Member

Sorry, I missed the freelistmanger && allocator. I'd like to send another PR to remove extentlistmanager.

Member

majianpeng commented Apr 24, 2017

Sorry, I missed the freelistmanger && allocator. I'd like to send another PR to remove extentlistmanager.

@majianpeng

This comment has been minimized.

Show comment
Hide comment
@majianpeng

majianpeng Apr 26, 2017

Member

update . please review.

Member

majianpeng commented Apr 26, 2017

update . please review.

os/BlueStore: In osd_tp_thread, call _txc_finalize_kv.
To reduce overhead of _kv_sync_thread, make _txc_finalize_kv do
in_osd_tp_thread.

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

@ifed01 ifed01 added the performance label May 3, 2017

@ifed01

ifed01 approved these changes May 3, 2017

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas May 3, 2017

Member

We could take this one step further and combine finalize_kv and write_nodes. This looks good though!

Member

liewegas commented May 3, 2017

We could take this one step further and combine finalize_kv and write_nodes. This looks good though!

@liewegas liewegas added the needs-qa label May 3, 2017

@ifed01

This comment has been minimized.

Show comment
Hide comment
@ifed01

ifed01 May 3, 2017

Contributor

There is still another call for txc_finalize_kv from kv_sync_thread for deferred traansactions hence not sure if we can merge the funcs

Contributor

ifed01 commented May 3, 2017

There is still another call for txc_finalize_kv from kv_sync_thread for deferred traansactions hence not sure if we can merge the funcs

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas May 3, 2017

Member
Member

liewegas commented May 3, 2017

@ifed01

This comment has been minimized.

Show comment
Hide comment
@ifed01

ifed01 May 3, 2017

Contributor

As far as I understand it's there for kraken WAL compatibility. And we should update freelist here as well. Hence we can eliminate this call only if we don't bother that compatibility any more.

Contributor

ifed01 commented May 3, 2017

As far as I understand it's there for kraken WAL compatibility. And we should update freelist here as well. Hence we can eliminate this call only if we don't bother that compatibility any more.

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas May 3, 2017

Member
Member

liewegas commented May 3, 2017

@ifed01

This comment has been minimized.

Show comment
Hide comment
@ifed01

ifed01 May 3, 2017

Contributor

BTW it looks like there is another potential issue with that in Kraken - one might call txc_finalize_kv twice for the same txc. Probably we need to clear txc's allocated/released containers in txc_finalize_kv. At least for Kraken

Contributor

ifed01 commented May 3, 2017

BTW it looks like there is another potential issue with that in Kraken - one might call txc_finalize_kv twice for the same txc. Probably we need to clear txc's allocated/released containers in txc_finalize_kv. At least for Kraken

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas May 3, 2017

Member
Member

liewegas commented May 3, 2017

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas May 3, 2017

Member
Member

liewegas commented May 3, 2017

@liewegas liewegas merged commit 1de3820 into ceph:master May 4, 2017

2 of 4 checks passed

arm build Build triggered. sha1 is merged.
Details
default Build triggered. sha1 is merged.
Details
Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment