os/bluestore: refactor bluestore_sync_submit_transaction #11537
Conversation
|
unit test failure is fixed in latest master |
| txc->kv_submitted = true; | ||
| int r = db->submit_transaction(txc->t); | ||
| assert(r == 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
@liewegas .
I think we in kv_sync_thread, we must make sure the transaction which include new nid_max must update and then update nid_max.
Like:
nid_max_tmp =
encode(nid_max_tmp, bl)
submit_transaction(t)
nid_max = nid_max_tmp;
Avoid later txc first submit and the txc include new nid_max later submit.
|
|
||
| virtual bool supports_parallel_transactions() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
@liewegas . I don't know for extentfreelistmanager why don't support parallel transaction? Hope your explain. Thanks!
|
Oh sorry, forgot to answer this. The extent manager stores the freelist as offset=length k/v pairs. When start with 0=100 txn2 If they commit in order all is well. If the order is reversed the This was one of the motivations for the bitmap implementation, which uses |
|
@liewegas . Thanks your detail explain. Even if it don't use parallel submit transaction, we also don't make tx1 must firstly submit to kv_queue.Is it so? |
| @@ -67,6 +67,10 @@ class BitmapFreelistManager : public FreelistManager { | |||
| void release( | |||
| uint64_t offset, uint64_t length, | |||
| KeyValueDB::Transaction txn) override; | |||
|
|
|||
| bool supports_parallel_transactions() override { | |||
|
LGTM |
|
@majianpeng _txc_finish_io() will make sure we are submitting the transaction in sequence. |
liewegas
force-pushed
the
wip-bluestore-parallel
branch
from
e63c744
to
ebe82cc
Compare
| } | ||
| { | ||
| std::lock_guard<std::mutex> l(kv_lock); | ||
| kv_queue.push_back(txc); | ||
| kv_cond.notify_one(); | ||
| if (!txc->kv_submitted) { | ||
| kv_queue_unsubmitted.push_back(txc); | ||
| ++txc->osr->kv_committing_serially; |
| dout(20) << __func__ | ||
| << " last_{nid,blobid} exceeds max, submit via kv thread" | ||
| << dendl; | ||
| } else if (txc->osr->kv_committing_serially) { |
There was a problem hiding this comment.
@liewegas I think this may hurt the parallelism, may be we should guard against a number rather than even 1 ?
I will run through my test and see how frequent we are hitting this.
|
We do, because in the non-sync mode, the _txc_finalize_kv is called from
kv_sync_thread in kv_committing order, and they are submitted to rocksdb
in that same order.
|
|
we should only fall into this case for low values of prealloc* tunables. |
And drop bluestore_sync_transaction, at least for now. The key change here is to make a per-txc flag indicating whether the txn was already submitted. This allows us to make a choice between sync and not-sync on a per-txn basis. Signed-off-by: Sage Weil <sage@redhat.com>
This ensures the txn will not commit before an update to the global max. Signed-off-by: Sage Weil <sage@redhat.com>
It doesn't support unordered transactions. Signed-off-by: Sage Weil <sage@redhat.com>
- single case to cover increases ahead of schedule or just in time - update global max only after txn commits, eliminating race - drop unneeded id_lock - improve t naming Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
liewegas
force-pushed
the
wip-bluestore-parallel
branch
from
e8c7137
to
516b0db
Compare
No description provided.