os/bluestore: remove unused parameters. #18635

majianpeng · 2017-10-31T04:04:37Z

No description provided.

majianpeng · 2017-10-31T04:05:00Z

@xiexingguo . Can you review this PR? Thanks!

xiexingguo

yeah, sounds reasonable to me

majianpeng · 2017-10-31T06:05:50Z

retest

amitkumar50

Please update PR description why o->flush() is removed.

liewegas

the flushes are necessary!

liewegas · 2017-10-31T16:45:54Z

src/os/bluestore/BlueStore.cc

@@ -10733,7 +10733,6 @@ int BlueStore::_do_remove(
  bool is_gen = !o->oid.is_no_gen();
  _do_truncate(txc, c, o, 0, is_gen ? &maybe_unshared_blobs : nullptr);
  if (o->onode.has_omap()) {
-    o->flush();
    _do_omap_clear(txc,


_do_omap_clear has to iterator over the keys in order to delete them, so the flush() is needed here. We could move it into _do_omap_clear for better clarity, perhaps

now it use rm_range_keys . so we only consider rm_range_keys

(sorry, I am unfamiliar with RocksDB)

@liewegas The root cause here is because there are still chances we are manipulating the omap of the same object in concurrent transaction groups, such as pglogs?

for example, if following osd_op rm the key which previous op add,but the previous don't submit .so db don't know key exist.

Thanks, jianpeng.

if following osd_op rm the key which previous op add,but the previous don't submit .so db don't know key exist.

Then add_omap_keys and rm_omap_keys should be two separate ops, right? Generally PG should order write ops manipulating the same object, which means rm_omap_keys should not be handled by BlueStore until add_omap_keys is stable, but I guess there might be exceptions(e.g., pglogs?)

exactly.. when i fixed this the test failure i saw was that the pg log was removed but left a single omap key behind, confusing things down the line.

the problem is that in order to implement the rm_range_keys we have to enumerate them which means we have to flush prior writes.

someday when rocksdb let's us enable the RangeDelete we can take these out.. but not until then

it's fine to have an add followed by a delete.. just not a range delete that requires us to enumerate in order to name the keys to delete.

@liewegas Thanks for the excellent clarifications:-)

liewegas · 2017-10-31T16:46:59Z

src/os/bluestore/BlueStore.cc

@@ -11078,7 +11076,6 @@ int BlueStore::_omap_rmkey_range(TransContext *txc,
  {
    const string& prefix =
      o->onode.is_pgmeta_omap() ? PREFIX_PGMETA_OMAP : PREFIX_OMAP;
-    o->flush();
    get_omap_key(o->onode.nid, first, &key_first);
    get_omap_key(o->onode.nid, last, &key_last);
    txc->t->rm_range_keys(prefix, key_first, key_last);


rm_range_keys impl enumerates keys, so we need the flush.

we could perhaps have a KeyValueDB method to tell us whether a proper range delete is implemented and if so skip the flush, but we have that off by default still (rocksdb recommends not using yet iirc?).

liewegas · 2017-11-01T01:24:51Z

the second cleanup patch is good, though!

asdf

xiexingguo · 2017-11-01T04:21:10Z

@majianpeng Mind dropping the flush commit and repushing?

majianpeng · 2017-11-06T06:52:48Z

@xiexingguo . sorry for later response. updated by your suggestions. BTW, i has a quesiton: You said "Then add_omap_keys and rm_omap_keys should be two separate ops, right? Generally PG should order write ops manipulating the same object, which means rm_omap_keys should not be handled by BlueStore until add_omap_keys is stable, but I guess there might be exceptions(e.g., pglogs?)".

How pg do this? by on_disk_lock or others? Thanks!

xiexingguo · 2017-11-06T07:47:29Z

@majianpeng I mean flush only works if we want to make sure that the omap changes made by a previous transaction(-group) are now visible in DB and hence the relevant keys can be safely enumerated.

It is okay (and possible) to manipulate omap multiple times in a single transaction but it's then caller's responsibility to make sure there is no conflicits.

majianpeng · 2017-11-06T08:02:32Z

@xiexingguo . FileStore don't care this (in a osop: set_omap_keys, rm_omap_keys).

Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>

In some situations the IP address the Monitor wants to bind to might not be available yet. This might for example be a IPv6 Address which is still performing DAD or waiting for a Router Advertisement to be send by the Router(s). Have systemd wait for 10s before starting the Mon and increase the amount of times it does so to 5. This allows the system to bring up IP Addresses in the mean time while systemd waits with restarting the Mon. Fixes: ceph#18635 Signed-off-by: Wido den Hollander <wido@42on.com> (cherry picked from commit e73eb8c)

xiexingguo changed the title ~~Bluestore omap flush~~ os/bluestore: drop o->flush on omap update paths Oct 31, 2017

xiexingguo previously approved these changes Oct 31, 2017

View reviewed changes

xiexingguo added bluestore performance needs-qa labels Oct 31, 2017

amitkumar50 reviewed Oct 31, 2017

View reviewed changes

tchaikov added the wip-kefu-testing label Oct 31, 2017

liewegas requested changes Oct 31, 2017

View reviewed changes

xiexingguo removed needs-qa performance wip-kefu-testing labels Nov 1, 2017

majianpeng force-pushed the bluestore-omap-flush branch from 7db291e to a140996 Compare November 6, 2017 02:46

majianpeng changed the title ~~os/bluestore: drop o->flush on omap update paths~~ os/bluestore: remove unused parameters. Nov 6, 2017

xiexingguo approved these changes Nov 6, 2017

View reviewed changes

xiexingguo merged commit 7e5a8a7 into ceph:master Nov 6, 2017

majianpeng deleted the bluestore-omap-flush branch November 6, 2017 08:02

os/bluestore: remove unused paramerter.

a140996

Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os/bluestore: remove unused parameters. #18635

os/bluestore: remove unused parameters. #18635

majianpeng commented Oct 31, 2017

majianpeng commented Oct 31, 2017

xiexingguo left a comment

majianpeng commented Oct 31, 2017

amitkumar50 left a comment

liewegas left a comment

liewegas Oct 31, 2017

majianpeng Oct 31, 2017

xiexingguo Nov 1, 2017

majianpeng Nov 1, 2017

xiexingguo Nov 1, 2017

liewegas Nov 1, 2017

liewegas Nov 1, 2017

xiexingguo Nov 1, 2017 •

edited

liewegas Oct 31, 2017

majianpeng Nov 1, 2017

majianpeng Nov 1, 2017

liewegas commented Nov 1, 2017

xiexingguo commented Nov 1, 2017 •

edited

majianpeng commented Nov 6, 2017

xiexingguo commented Nov 6, 2017 •

edited

majianpeng commented Nov 6, 2017

os/bluestore: remove unused parameters. #18635

os/bluestore: remove unused parameters. #18635

Conversation

majianpeng commented Oct 31, 2017

majianpeng commented Oct 31, 2017

xiexingguo left a comment

Choose a reason for hiding this comment

majianpeng commented Oct 31, 2017

amitkumar50 left a comment

Choose a reason for hiding this comment

liewegas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiexingguo Nov 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liewegas commented Nov 1, 2017

xiexingguo commented Nov 1, 2017 • edited

majianpeng commented Nov 6, 2017

xiexingguo commented Nov 6, 2017 • edited

majianpeng commented Nov 6, 2017

xiexingguo Nov 1, 2017 •

edited

xiexingguo commented Nov 1, 2017 •

edited

xiexingguo commented Nov 6, 2017 •

edited