On Wed, 19 Oct 2016, Somnath Roy wrote:
> Hi Sage,
> I was trying to benchmark bluestore by enabling bluestore_sync_transaction and it seems the db method in the below code is called wrongly ?
> If we enable bluestore_sync_submit_transaction , shouldn't we be doing db-> submit_transaction_sync() and if we enable only bluestore_sync_transaction shouldn't we be doing db->submit_transaction() ?
>
>
>
> if (!g_conf->bluestore_sync_transaction) {
> if (g_conf->bluestore_sync_submit_transaction) {
> _txc_finalize_kv(txc, txc->t);
> int r = db->submit_transaction(txc->t);
> assert(r == 0);
> }
> } else {
> _txc_finalize_kv(txc, txc->t);
> int r = db->submit_transaction_sync(txc->t);
> assert(r == 0);
> }
This code is pretty broken. FWIW I did a quick refactor earlier today
that improves it somewhat, see
https://github.com/ceph/ceph/pull/11537
and Ma Jianpeng is working on addressing the nid/blobid max issue in this
PR
https://github.com/ceph/ceph/pull/11489
(although I think that can be safely ignored for now if you don't restart
the OSDs).
> We are anyway calling one submit_transaction_sync from _kv_sync_thread().
>
> Now, if I do that (for bluestore_sync_transaction = true) , I am seeing
> ~10% performance improvement with min_alloc_size = 16K and seeing much
> lower cpu usage with _kv_sync_thread. This is also telling us that even
> if we are not utilizing 100% cpu core for _kv_sync_thread , more work
> you do from this thread will degrade performance. IMO,
> bluestore_sync_transaction should be default true if you agree that we
> should be replacing the *_sync call.
That's great news! Can you try my PR and see if the result is similar?
It is still iterating over the txn list in the sync thread but that should
be very cheap. If not we can partition the lists.
s
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
