os/bluestore: refactor ExtentMap::update to avoid preceeding db updat… #12394
Conversation
There was a problem hiding this comment.
I think we can simplify this: the only reason we ever call update() twice is when update() itself returns true. That means that update() itself can stage the changes and only put them in t if it is sure it won't return true. Which means this change coudl be local to update() only.. right?
Also, if we can avoid the memory allocations of map<>, that would be nice. Like, put the bufferlist in Shard, and string the dirty ones together in an intrusive_list or something. Or just put single local vector<Shard*> on the local stack or something.
ifed01
force-pushed
the
wip-bluestore-fix-reshard
branch
from
584c8a5
to
b12c444
Compare
|
@liewegas fixed and rebased |
|
|
||
| struct dirty_shard_t{ | ||
| string* key = nullptr; | ||
| bufferlist bl; |
There was a problem hiding this comment.
If we put the bufferlist dirty_bl in Shard, we can avoid this structure entirely, right?
| &dirty_shard_t::dirty_list_item> > dirty_shard_list_t; | ||
|
|
||
|
|
||
| vector<dirty_shard_t> encoded_shards; |
| unsigned nn; | ||
| bufferlist& bl = encoded_shards[pos].bl; |
| //schedule DB update for dirty shards | ||
| auto it = dirty_shards.begin(); | ||
| while( it != dirty_shards.end()) { | ||
| t->set(PREFIX_OBJ, *(it->key), it->bl); |
There was a problem hiding this comment.
and here we'd clear dirty_bl again?
I guess this uses a bit more RAM...
There was a problem hiding this comment.
yeah, more RAM usage for a shard was the root cause why I selected the way above. I believe that we might need to think about Onode in-memory size reduction one day. Since this affects onode caching effectiveness: larger onode - less entries in the cache - worse performance. And even empty bufferlist isn't that small. Hence IMHO that's an overkill to have a bufferlist instance for each onode's shard. Especially taking into the consideration it's local use.
Still thinking we want to have it along with shard?
There was a problem hiding this comment.
And a small bonus of my approach - we don't need to enumerate ALL the shards twice. I'm iterating over dirty blobs only at the final stage.
|
Yeah, let's go with what you ahve. Can you clean up whitespace, and
remove the unneed intrusive member item in Shard?
|
…e if reshard takes place Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>
Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>
ifed01
force-pushed
the
wip-bluestore-fix-reshard
branch
from
b12c444
to
c66977b
Compare
|
@liewegas - cleaned-up |
…e if reshard takes place.
update method might add some 'set' ops to the transaction prior to detection that resharding is needed.
No need to apply these ops after reshard takes place - correspoding records to be removed anyway.
Signed-off-by: Igor Fedotov ifed@mirantis.com