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

os/bluestore: narrow lock scope for cache trim() #10410

Merged
merged 1 commit into from Aug 10, 2016

Conversation

Projects
None yet
3 participants
@xiexingguo
Copy link
Member

commented Jul 23, 2016

The cache trim() process can become time consuming
when the cache size is huge. Since it can be operated
safely under its own internal mutex, we shall avoid
to hold irrelevant locks when possible, which is good
for performance.

@xiexingguo

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2016

2016-07-23 12:51:26.319710 7ff4dbc42680 1 bluestore(store_test_temp_dir) fsck finish with 0 errors
[ OK ] ObjectStore/StoreTest.TooManyBlobsTest/2 (22819 ms)
[----------] 59 tests from ObjectStore/StoreTest (2114529 ms total)

[----------] Global test environment tear-down
[==========] 59 tests from 1 test case ran. (2114531 ms total)
[ PASSED ] 59 tests.

if (!o || !o->exists)
r = false;
}

This comment has been minimized.

Copy link
@somnathr

somnathr Jul 25, 2016

Contributor

@xiexingguo We can't do that cache->trim needs to be within collection lock as it is modifying onode structures..

This comment has been minimized.

Copy link
@xiexingguo

xiexingguo Jul 26, 2016

Author Member

Negative, cache->trim() does cache cleanup only and is under its own mutex protection already.

We can't do that cache->trim needs to be within collection lock as it is modifying onode structures..

Otherwise shouldn't we promote to WLocker(c->lock) here?

This comment has been minimized.

Copy link
@somnathr

somnathr Jul 26, 2016

Contributor

trim() does clear blob_map and onode_map. protecting against cache mutex is not good enough.
All the onode operations should be protected by collection lock. In this case, you are right, it should be promoted to Wlock at the time of clearing the onode structures..
BTW, I don't think taking write lock in the beginning instead of Rlock will be impacting performance much as the requests are been serialized within a PG (collection).

This comment has been minimized.

Copy link
@liewegas

liewegas Aug 10, 2016

Member

The cache->lock protects the BufferSpace and OnodeSpace structures that get trimmed.

@xiexingguo xiexingguo force-pushed the xiexingguo:xxg-wip-bluestore-2016-07-23 branch 2 times, most recently from a35ba51 to 873ac5c Jul 26, 2016

@xiexingguo xiexingguo changed the title os/bluestore: narrow lock scope for cache trim() os/bluestore: os/bluestore: promote RLock to Wlock for read ops asking for cache->trim() Jul 26, 2016

@xiexingguo xiexingguo force-pushed the xiexingguo:xxg-wip-bluestore-2016-07-23 branch from 873ac5c to 8adff5c Jul 26, 2016

@xiexingguo xiexingguo changed the title os/bluestore: os/bluestore: promote RLock to Wlock for read ops asking for cache->trim() os/bluestore: narrow lock scope for cache trim() Jul 26, 2016

os/bluestore: narrow lock scope for cache trim()
The cache trim() process can become be time consuming
when the cache size is huge. Since it can be operated
safely under its own internal mutex, we shall avoid
to hold irrelevant locks when possible, which is good
for performance.

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

@xiexingguo xiexingguo force-pushed the xiexingguo:xxg-wip-bluestore-2016-07-23 branch from 8adff5c to 9fd9296 Jul 29, 2016

@liewegas

This comment has been minimized.

Copy link
Member

commented Aug 10, 2016

lgtm

@liewegas liewegas merged commit f440aec into ceph:master Aug 10, 2016

2 checks passed

Signed-off-by all commits in this PR are signed
Details
default Build finished.
Details

@xiexingguo xiexingguo deleted the xiexingguo:xxg-wip-bluestore-2016-07-23 branch Aug 10, 2016

@somnathr

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

@liewegas we saw a crash that Mark reported because the cache->trim() is not happening within c->lock() from _osr_reap_done(). The pull request #10578 seems to fix that.
onodes are accessed within c->lock() and without cache->lock() (for ex, onode->flush() ) and if from trim() it decrements reference without c->lock() , it may delete onode object while there still be user.

Am I missing anything ?

@liewegas

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

trim should operate with only cache->lock, and any place where it affects a structure needs to be protected with cache->lock in the forward path (not the collection lock).

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.