mimic: kv/RocksDBStore: ignore empty rocksdb transaction #36108

wuhongsong · 2020-07-15T08:19:39Z

otherwise, disks associated to osds have small write io every bluefs_balance_interval seconds even on an idle ceph cluster.

Problem only exists in Luminous/Mimic.

Fixes: https://tracker.ceph.com/issues/46411
Signed-off-by: hzwuhongsong wojiaowugen@163.com

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard backend
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

wuhongsong · 2020-07-15T08:32:35Z

jenkins test make check

tchaikov · 2020-07-15T11:14:09Z

@wuhongsong do you know where the empty transaction is created and submitted ?

wuhongsong · 2020-07-15T12:12:35Z

@wuhongsong do you know where the empty transaction is created and submitted ?

@tchaikov

The empty rocksdb transaction is created and submitted in BlueStore::_kv_sync_thread().

After the pr( #26866 ) is applied, even the ceph cluster is idle, it will create a rocksdb transaction ( KeyValueDB::Transaction synct = db->get_transaction() ) and be submitted by submit_transaction_sync every bluefs_balance_interval seconds,But there is no real bufferlist to set the rocksdb transaction, so it’s empty.

After my change in this pr, there is no longer have the small write on an idel ceph cluster.

aclamk

Submitting empty transactions to RocksDB does not make sense, unless it is used as synchronization point.
So, when woptions.sync==true, we should pass even empty transaction to RocksDB.

wuhongsong · 2020-07-18T04:46:24Z

Submitting empty transactions to RocksDB does not make sense, unless it is used as synchronization point.
So, when woptions.sync==true, we should pass even empty transaction to RocksDB.

@aclamk
Maybe I can move the judgement of empty transaction to BlueStore::_kv_sync_thread()，then if the transaction is empty and bluefs_do_check_balance is true, it will not call submit_transaction_sync any more in _kv_sync_thread().

Do you think it’a a good idea?

smithfarm

Please amend the commit message (git commit --amend) so it includes an explanation of why this fix cannot be cherry-picked from master, octopus, or nautilus.

For details, see https://github.com/ceph/ceph/blob/master/SubmittingPatches-backports.rst and, in particular, https://github.com/ceph/ceph/blob/master/SubmittingPatches-backports.rst#cherry-picking-rules

(In short, we do not ordinarily accept fixes implemented directly in stable branch. Only in special cases when the fix is not applicable to any of the more recent branches.)

wuhongsong · 2020-07-21T03:00:26Z

Please amend the commit message (git commit --amend) so it includes an explanation of why this fix cannot be cherry-picked from master, octopus, or nautilus.

For details, see https://github.com/ceph/ceph/blob/master/SubmittingPatches-backports.rst and, in particular, https://github.com/ceph/ceph/blob/master/SubmittingPatches-backports.rst#cherry-picking-rules

(In short, we do not ordinarily accept fixes implemented directly in stable branch. Only in special cases when the fix is not applicable to any of the more recent branches.)

Thanks for your advice.

aclamk

I think getting rid of empty transactions is a good idea.

aclamk · 2020-07-21T08:57:59Z

src/os/bluestore/BlueStore.cc

+        ceph_assert(r == 0);
+      }
+
+      bluefs_do_check_balance = false;


Was there a reason to move "bluefs_do_check_balance = false"?

Because I used it in the previous. like below:
https://github.com/ceph/ceph/pull/36108/files#diff-a9faffcf40600fd57aea5451cef5abe9R9509

aclamk · 2020-07-21T09:07:40Z

src/os/bluestore/BlueStore.cc

+        static_cast<RocksDBStore::RocksDBTransactionImpl *>(synct.get());
+      RocksDBStore::RocksWBHandler bat_txc;
+      _t->bat.Iterate(&bat_txc);
+


Iterating over submit batch just to check if it is empty is problematic, especially because kv_sync_thread is known bottleneck. Moreover, synct can be empty, but we could have already submitted some other batches to db ( _txc_apply_kv).
We should gather information whether we need to submit sync by other means.

@aclamk Thanks for your review.

In addition to the judgment of synct is empty, there is also the judgment of whether the bluefs_do_check_balance is true.

As you konw，when the bluefs_do_check_balance is true，the kv_queue should be empty , so we impossibility have already submitted some other batches to db. Right?

Thanks again for your review.

@wuhongsong

"As you konw，when the bluefs_do_check_balance is true，the kv_queue should be empty , so we impossibility have already submitted some other batches to db."
I did not actually realized that. True, each time we add to kv_queue we signal kv_cond, so it IS impossible to get both timeout and non-empty kv_queue.
I am only afraid that relying on this might bring us an error when we "optimize" notification frequency, as we did with deferred_done_queue.

Iterating over bat_txc should be done only when bluefs_do_check_balance is true. Unless there any other situation for us to get empty transaction; which I do not see.

@wuhongsong

@wuhongsong Thanks for including the sentence "Problem only exists in Luminous/Mimic" in the commit message. This is nice, but it would be even nicer if the commit message explained why the problem can't be fixed by cherry-picking one or more commits from master. For example, the problem doesn't exist in master because something was completely reworked, and that rework is not going to be backported. That said, I don't want to make problems for anyone. The sentence you added is enough for me.

aclamk

Getting rid of empty transaction is good idea.

aclamk · 2020-07-30T08:50:17Z

src/os/bluestore/BlueStore.cc

+        static_cast<RocksDBStore::RocksDBTransactionImpl *>(synct.get());
+      RocksDBStore::RocksWBHandler bat_txc;
+      _t->bat.Iterate(&bat_txc);
+


@wuhongsong

"As you konw，when the bluefs_do_check_balance is true，the kv_queue should be empty , so we impossibility have already submitted some other batches to db."
I did not actually realized that. True, each time we add to kv_queue we signal kv_cond, so it IS impossible to get both timeout and non-empty kv_queue.
I am only afraid that relying on this might bring us an error when we "optimize" notification frequency, as we did with deferred_done_queue.

Iterating over bat_txc should be done only when bluefs_do_check_balance is true. Unless there any other situation for us to get empty transaction; which I do not see.

aclamk · 2020-08-04T12:55:35Z

src/os/bluestore/BlueStore.cc

+        _t->bat.Iterate(&bat_txc);
+      }
+      if (!(bluefs_do_check_balance && bat_txc.seen.empty())) {
+        // submit synct synchronously (block and wait for it to commit)


@wuhongsong Have you considered using rocksdb::WriteBatch::Count() for testing if transaction is empty?

@aclamk Thank you for your advice, I have modified it.

otherwise, disks associated to osds have small write io even on an idel ceph cluster. Problem only exists in Luminous/Mimic. Fixes: https://tracker.ceph.com/issues/46411 Signed-off-by: hzwuhongsong <hzwuhongsong@corp.netease.com>

wuhongsong · 2020-08-25T10:39:32Z

@aclamk can you help to have a review again，thanks very much.

tchaikov · 2021-01-03T03:48:50Z

@aclamk ping?

smithfarm · 2021-01-27T20:09:28Z

mimic EOL

10199962 · 2021-01-28T13:22:04Z

Ｉhave met the same problem ,the growth rate of block.wal space is 20M per hour......
so huge...

Gangbiao · 2021-04-19T06:44:29Z

How to fix this problem at last?

wuhongsong · 2023-03-13T12:11:30Z

How to fix this problem at last?

the pr maybe can solve the problem, but mimic EOL

wuhongsong · 2023-03-13T12:12:25Z

Ｉhave met the same problem ,the growth rate of block.wal space is 20M per hour...... so huge...

maybe you can have a look：
https://access.redhat.com/solutions/5173092

wuhongsong requested a review from a team July 15, 2020 08:19

wuhongsong requested review from a team as code owners July 15, 2020 08:19

wuhongsong changed the base branch from master to mimic July 15, 2020 08:21

wuhongsong changed the title ~~Abnormal io~~ kv/RocksDBStore: ignore empty rocksdb transaction Jul 15, 2020

wuhongsong force-pushed the abnormal_io branch from fc4e4e2 to a55f4aa Compare July 15, 2020 08:35

tchaikov requested a review from aclamk July 15, 2020 11:12

tchaikov added bluestore bug-fix labels Jul 15, 2020

aclamk requested changes Jul 16, 2020

View reviewed changes

smithfarm added this to the mimic milestone Jul 20, 2020

smithfarm previously requested changes Jul 20, 2020

View reviewed changes

smithfarm changed the title ~~kv/RocksDBStore: ignore empty rocksdb transaction~~ mimic: kv/RocksDBStore: ignore empty rocksdb transaction Jul 20, 2020

wuhongsong force-pushed the abnormal_io branch from a55f4aa to 91db456 Compare July 21, 2020 02:49

wuhongsong force-pushed the abnormal_io branch from 91db456 to 5eba5ee Compare July 21, 2020 06:38

wuhongsong requested review from aclamk and smithfarm and removed request for a team July 21, 2020 08:22

aclamk requested changes Jul 21, 2020

View reviewed changes

smithfarm removed their request for review July 21, 2020 09:17

aclamk requested changes Jul 30, 2020

View reviewed changes

wuhongsong force-pushed the abnormal_io branch from 5eba5ee to f768900 Compare August 3, 2020 06:53

wuhongsong requested a review from aclamk August 3, 2020 07:28

aclamk reviewed Aug 4, 2020

View reviewed changes

kv/RocksDBStore: ignore empty rocksdb transaction

6188d8d

otherwise, disks associated to osds have small write io even on an idel ceph cluster. Problem only exists in Luminous/Mimic. Fixes: https://tracker.ceph.com/issues/46411 Signed-off-by: hzwuhongsong <hzwuhongsong@corp.netease.com>

wuhongsong force-pushed the abnormal_io branch from f768900 to 6188d8d Compare August 8, 2020 02:19

wuhongsong requested a review from aclamk August 8, 2020 02:56

smithfarm closed this Jan 27, 2021

wuhongsong deleted the abnormal_io branch March 30, 2023 06:52

mimic: kv/RocksDBStore: ignore empty rocksdb transaction #36108

mimic: kv/RocksDBStore: ignore empty rocksdb transaction #36108

Conversation

wuhongsong commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

wuhongsong commented Jul 15, 2020

Uh oh!

tchaikov commented Jul 15, 2020

Uh oh!

wuhongsong commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aclamk left a comment

Choose a reason for hiding this comment

Uh oh!

wuhongsong commented Jul 18, 2020

Uh oh!

smithfarm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wuhongsong commented Jul 21, 2020

Uh oh!

aclamk left a comment

Choose a reason for hiding this comment

Uh oh!

aclamk Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

wuhongsong Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aclamk Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

wuhongsong Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

aclamk Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

aclamk left a comment

Choose a reason for hiding this comment

Uh oh!

aclamk Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

aclamk Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wuhongsong Aug 8, 2020

Choose a reason for hiding this comment

Uh oh!

wuhongsong commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tchaikov commented Jan 3, 2021

Uh oh!

smithfarm commented Jan 27, 2021

Uh oh!

10199962 commented Jan 28, 2021

Uh oh!

Gangbiao commented Apr 19, 2021

Uh oh!

wuhongsong commented Mar 13, 2023

Uh oh!

wuhongsong commented Mar 13, 2023

Uh oh!

wuhongsong commented Jul 15, 2020 •

edited

Loading

wuhongsong commented Jul 15, 2020 •

edited

Loading

smithfarm left a comment •

edited

Loading

wuhongsong Jul 23, 2020 •

edited

Loading

aclamk Aug 4, 2020 •

edited

Loading

wuhongsong commented Aug 25, 2020 •

edited

Loading