osd, os: drop the OmapIterator in favor of omap_iterate. Remove ObjectMap by rzarzynski · Pull Request #60890 · ceph/ceph

rzarzynski · 2024-11-29T20:37:12Z

This is a follow-up on #60278. In contrast to it, this PR is not supposed to be backported.

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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 cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

rzarzynski · 2024-12-11T00:11:30Z

It turned out that dropping the OmapIterator allowed to squeeze DBObjectMap and, finally, ObjectMap as well.

src/os/ObjectStore.h

src/osd/PrimaryLogPG.cc

ronen-fr · 2024-12-11T17:13:45Z

src/kv/KeyValueDB.h

 #include <map>
 #include <optional>
 #include <string>
+#include <string_view>


(a note that refers to the commit description)
Maybe add, after the perf data, a word about what it is replaced with?

ronen-fr · 2024-12-11T17:15:42Z

src/kv/RocksDBStore.cc

+  std::string_view value_as_sv() override
+  {
+    if (smaller == on_main) {
+      return main->value_as_sv();


a comment line reminding the reader what the equality means? (I know it is explained in the way 'smaller' is set, but still...)

ronen-fr

WIP - I'll continue tomorrow

src/osd/SnapMapper.cc

ronen-fr · 2024-12-11T17:34:42Z

src/osd/SnapMapper.cc

+               mapping.snap < end &&
+               mapping.hoid.pool == pool);
+        // invalid
+        dout(10) << __func__ << " stray " << mapping.hoid


this one really deserves an fmt::format()!

Will send a follow up.

The commit is ready, will push after addressing other comments.

ronen-fr · 2024-12-11T17:36:11Z

src/common/ceph_time.h

 };

+template <class ClockT = mono_clock>
+class time_guard {


is 'guard' the correct name? it's just a collector, with no alarm, right?

I think it's analogy to the scope_guard we already have.

No sure that's a good reason...

<BETTER NAME NEED>!

src/kv/RocksDBStore.cc

ronen-fr · 2024-12-11T17:48:43Z

src/kv/RocksDBStore.cc

 }

+// TODO: deduplicate the code, preferrably by removing the string variant
+int RocksDBStore::split_key(rocksdb::Slice in, string_view *prefix, string_view *key)


I'd rather simply return an optional

This would need to be an optional of pair of two strings (and string_views) – there are callers who rely on the if (param != nullptr) behavior. Sounds complex.

ronen-fr

LGTM (and a nice improvement).
Some notes/questions etc.
I'd prefer that Matan also comments on the removal of the snap-mapper format repair code.

ronen-fr · 2024-12-12T08:39:48Z

src/osd/ReplicatedBackend.cc

+static uint32_t crc32_netstring(const uint32_t orig_crc, T data)
+{
+  // this MUST be compliant marshalling format for bufferlist!
+  // otherwise with scrub during upgrade will happen.


What does the 2nd comment line means?

There is the assumption on the on-wire format of netstring. Reworking the comment. Thanks!

ronen-fr · 2024-12-12T08:47:27Z

src/osd/ReplicatedBackend.cc

+  auto crc = ceph_crc32c(orig_crc, (unsigned char*)&len, sizeof(len));
+  crc = ceph_crc32c(crc, (unsigned char*)data.c_str(), data.length());
+
+#ifndef _NDEBUG


It seems that we rarely use _NDEBUG in Ceph code. Is this the right ifdef?

I'm pretty sure it is. My intention is to have this only in debug builds due to performance penalty.

ronen-fr · 2024-12-12T08:48:43Z

src/osd/osd_types.h

  std::string omap_pos;
  int ret = 0;
-  ceph::buffer::hash data_hash, omap_hash;  ///< accumulatinng hash value
+  ceph::buffer::hash data_hash;  ///< accumulatinng hash value


sp in the comment

Fixed; will push soon.

ronen-fr · 2024-12-12T09:00:07Z

src/osd/ReplicatedBackend.cc

+      if (--max == 0) {
+        more = true;
+        pos.omap_pos = key;
+        return ObjectStore::omap_iter_ret_t::STOP;


If 'max' is 1 when entering, do we update the hashes?

Previously we were. Good catch, thanks!

ronen-fr · 2024-12-12T09:02:22Z

src/os/bluestore/BlueStore.cc

    next_lat_acc,
    c->store->cct->_conf->bluestore_log_omap_iterator_age);
-  return 0;
+  return more;


I'd rather not return bool as int.

ronen-fr · 2024-12-12T09:03:48Z

src/osd/PGLog.h

 	  }
 	  missing.add(oid, std::move(item));
-	} else if (p->key().substr(0, 4) == std::string("dup_")) {
+	} else if (key.substr(0, 4) == std::string("dup_")) {


starts_with ?

Refactored altogether with missing case.

ronen-fr · 2024-12-12T09:04:51Z

src/osd/OSD.cc

-      r = SnapMapper::convert_legacy(cct, store.get(), ch, hoid, max);
-      if (r < 0)
-	goto out;
+      derr << __func__ << " snap_mapper upgrade from pre-octopus"


@Matan-B - any problem with this?

Approved here - #60855

ronen-fr · 2024-12-12T09:09:45Z

src/tools/ceph_objectstore_tool.cc

      ceph::signedspan total_time;
-      string tail_key;
-      std::tie(coll, ghobj, first_seek_time, last_seek_time, total_time, tail_key) = *i;
+      std::tie(coll, ghobj, first_seek_time, last_seek_time, total_time) = *i;


structured binding instead?

rzarzynski · 2024-12-12T12:38:07Z

Dropping the legacy key conversion of SnapMapper is sent separately for review: #60855.

github-actions · 2025-01-13T14:15:29Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

When `get_omap_iterator()` is replaced with `omap_iterate()`, the CRC calculations will be happening in-place, with 0copy. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

`tail_key`, and even presence of some key designated to be a gaaurd, seems to an impementation detail of particular `ObjectStore`. I doubt this piece of information is useful enough to introduce a getter for it to the `ObjectStore` interface. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

Please note this also drops a BlueStore's perf counter: `l_bluestore_omap_seek_to_first_lat`. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

Please don't confuse with osdmaptool. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

rzarzynski · 2025-04-04T18:24:01Z

Squashed a fixup and rebased after trivial conflits due to the s/WITH_SEASTAR/WITH_CRIMSON renaming. No need to restart the ongoing QA, IMHO.

ljflores · 2025-04-07T22:05:29Z

Rados approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues70798

Introduced recently in ceph#60890. _NDEBUG is a typo for NDEBUG. Possibly confused with _DEBUG which is a Visual Studio concept. Signed-off-by: Matan Breizman <mbreizma@redhat.com>

rzarzynski requested review from a team as code owners November 29, 2024 20:37

github-actions bot added bluestore common core crimson tests labels Nov 29, 2024

rzarzynski mentioned this pull request Nov 29, 2024

os, osd: bring the lightweight OMAP iteration #60278

Merged

14 tasks

rzarzynski force-pushed the wip-os-fastomapiter-killomapiterator branch from 5d0ca38 to d9fcde2 Compare December 11, 2024 00:09

github-actions bot added the build/ops label Dec 11, 2024

rzarzynski changed the title ~~osd, os: drop the OmapIterator in favor of omap_iterate~~ osd, os: drop the OmapIterator in favor of omap_iterate. Remove ObjectMap Dec 11, 2024

rzarzynski requested review from Matan-B, aclamk, athanatos and ronen-fr December 11, 2024 00:13