kv: avoid memcpy around key() in OMAP iterator of KeyValueDB

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
(cherry picked from commit a7c81953d07ab78dfb7c5ccacca07e972a06394c)

Conflicts:
    src/os/bluestore/BlueStore.cc
      trivial conflict with Onode::finish_write() that doesn't
      exist in squid.

diff --git a/src/kv/KeyValueDB.h b/src/kv/KeyValueDB.h
index ba3686fe4c815b0d7a1bb437410822b1213044f0..d02ba7c434a9f268e4de85cbb1e2fa14dcafd8b0 100644 (file)
--- a/src/kv/KeyValueDB.h
+++ b/src/kv/KeyValueDB.h
@@ -206,6 +206,9 @@ public:
       return "";
     }
     virtual ceph::buffer::list value() = 0;
+    // When valid() returns true, value returned as string-view
+    // is guaranteed to be valid until iterator is moved to another
+    // position; that is until call to next() / seek_to_first() / etc.
     virtual std::string_view value_as_sv() = 0;
     virtual int status() = 0;
     virtual ~SimplestIteratorImpl() {}
@@ -216,7 +219,12 @@ public:
     virtual ~IteratorImpl() {}
     virtual int seek_to_last() = 0;
     virtual int prev() = 0;
+    // When valid() returns true, key returned as string-view
+    // is guaranteed to be valid until iterator is moved to another
+    // position; that is until call to next() / seek_to_first() / etc.
+    virtual std::string_view key_as_sv() = 0;
     virtual std::pair<std::string, std::string> raw_key() = 0;
+    virtual std::pair<std::string_view, std::string_view> raw_key_as_sv() = 0;
     virtual ceph::buffer::ptr value_as_ptr() {
       ceph::buffer::list bl = value();
       if (bl.length() == 1) {
@@ -243,7 +251,9 @@ public:
     virtual int next() = 0;
     virtual int prev() = 0;
     virtual std::string key() = 0;
+    virtual std::string_view key_as_sv() = 0;
     virtual std::pair<std::string,std::string> raw_key() = 0;
+    virtual std::pair<std::string_view, std::string_view> raw_key_as_sv() = 0;
     virtual bool raw_key_is_prefixed(const std::string &prefix) = 0;
     virtual ceph::buffer::list value() = 0;
     virtual ceph::buffer::ptr value_as_ptr() {
@@ -312,9 +322,15 @@ private:
     std::string key() override {
       return generic_iter->key();
     }
+    std::string_view key_as_sv() override {
+      return generic_iter->key_as_sv();
+    }
     std::pair<std::string, std::string> raw_key() override {
       return generic_iter->raw_key();
     }
+    std::pair<std::string_view, std::string_view> raw_key_as_sv() override {
+      return generic_iter->raw_key_as_sv();
+    }
     ceph::buffer::list value() override {
       return generic_iter->value();
     }

diff --git a/src/kv/RocksDBStore.cc b/src/kv/RocksDBStore.cc
index e553e199c877bd6a5e977aefdd406cf50eb2ecae..b92f76ea7f4c73b68846e55dd80978b2d8a6d6b2 100644 (file)
--- a/src/kv/RocksDBStore.cc
+++ b/src/kv/RocksDBStore.cc
@@ -6,6 +6,7 @@
 #include <memory>
 #include <set>
 #include <string>
+#include <string_view>
 #include <errno.h>
 #include <unistd.h>
 #include <sys/types.h>
@@ -47,6 +48,7 @@ using std::ostream;
 using std::pair;
 using std::set;
 using std::string;
+using std::string_view;
 using std::unique_ptr;
 using std::vector;
 
@@ -1974,7 +1976,7 @@ int RocksDBStore::split_key(rocksdb::Slice in, string *prefix, string *key)
 
   // Find separator inside Slice
   char* separator = (char*) memchr(in.data(), 0, in.size());
-  if (separator == NULL)
+  if (separator == nullptr)
      return -EINVAL;
   prefix_len = size_t(separator - in.data());
   if (prefix_len >= in.size())
@@ -1988,6 +1990,27 @@ int RocksDBStore::split_key(rocksdb::Slice in, string *prefix, string *key)
   return 0;
 }
 
+// TODO: deduplicate the code, preferrably by removing the string variant
+int RocksDBStore::split_key(rocksdb::Slice in, string_view *prefix, string_view *key)
+{
+  size_t prefix_len = 0;
+
+  // Find separator inside Slice
+  char* separator = (char*) memchr(in.data(), 0, in.size());
+  if (separator == nullptr)
+     return -EINVAL;
+  prefix_len = size_t(separator - in.data());
+  if (prefix_len >= in.size())
+    return -EINVAL;
+
+  // Fetch prefix and/or key directly from Slice
+  if (prefix)
+    *prefix = string_view(in.data(), prefix_len);
+  if (key)
+    *key = string_view(separator + 1, in.size() - prefix_len - 1);
+  return 0;
+}
+
 void RocksDBStore::compact()
 {
   dout(2) << __func__ << " starting" << dendl;
@@ -2208,7 +2231,13 @@ int RocksDBStore::RocksDBWholeSpaceIteratorImpl::prev()
 string RocksDBStore::RocksDBWholeSpaceIteratorImpl::key()
 {
   string out_key;
-  split_key(dbiter->key(), 0, &out_key);
+  split_key(dbiter->key(), nullptr, &out_key);
+  return out_key;
+}
+string_view RocksDBStore::RocksDBWholeSpaceIteratorImpl::key_as_sv()
+{
+  string_view out_key;
+  split_key(dbiter->key(), nullptr, &out_key);
   return out_key;
 }
 pair<string,string> RocksDBStore::RocksDBWholeSpaceIteratorImpl::raw_key()
@@ -2217,6 +2246,12 @@ pair<string,string> RocksDBStore::RocksDBWholeSpaceIteratorImpl::raw_key()
   split_key(dbiter->key(), &prefix, &key);
   return make_pair(prefix, key);
 }
+pair<string_view,string_view> RocksDBStore::RocksDBWholeSpaceIteratorImpl::raw_key_as_sv()
+{
+  string_view prefix, key;
+  split_key(dbiter->key(), &prefix, &key);
+  return make_pair(prefix, key);
+}
 
 bool RocksDBStore::RocksDBWholeSpaceIteratorImpl::raw_key_is_prefixed(const string &prefix) {
   // Look for "prefix\0" right in rocksb::Slice
@@ -2336,9 +2371,15 @@ public:
   string key() override {
     return dbiter->key().ToString();
   }
+  string_view key_as_sv() override {
+    return dbiter->key().ToStringView();
+  }
   std::pair<std::string, std::string> raw_key() override {
     return make_pair(prefix, key());
   }
+  std::pair<std::string_view, std::string_view> raw_key_as_sv() override {
+    return make_pair(prefix, dbiter->key().ToStringView());
+  }
   bufferlist value() override {
     return to_bufferlist(dbiter->value());
   }
@@ -2660,6 +2701,15 @@ public:
     }
   }
 
+  std::string_view key_as_sv() override
+  {
+    if (smaller == on_main) {
+      return main->key_as_sv();
+    } else {
+      return current_shard->second->key_as_sv();
+    }
+  }
+
   std::pair<std::string,std::string> raw_key() override
   {
     if (smaller == on_main) {
@@ -2669,6 +2719,15 @@ public:
     }
   }
 
+  std::pair<std::string_view,std::string_view> raw_key_as_sv() override
+  {
+    if (smaller == on_main) {
+      return main->raw_key_as_sv();
+    } else {
+      return { current_shard->first, current_shard->second->key_as_sv() };
+    }
+  }
+
   bool raw_key_is_prefixed(const std::string &prefix) override
   {
     if (smaller == on_main) {
@@ -3018,9 +3077,15 @@ public:
   string key() override {
     return iters[0]->key().ToString();
   }
+  string_view key_as_sv() override {
+    return iters[0]->key().ToStringView();
+  }
   std::pair<std::string, std::string> raw_key() override {
     return make_pair(prefix, key());
   }
+  std::pair<std::string_view, std::string_view> raw_key_as_sv() override {
+    return make_pair(prefix, iters[0]->key().ToStringView());
+  }
   bufferlist value() override {
     return to_bufferlist(iters[0]->value());
   }

diff --git a/src/kv/RocksDBStore.h b/src/kv/RocksDBStore.h
index 24330a1c78d836a0331e39a4bd259addacfc9bd8..b372e31d120b1aa71d34efe67d7f62579fb7bddc 100644 (file)
--- a/src/kv/RocksDBStore.h
+++ b/src/kv/RocksDBStore.h
@@ -380,7 +380,9 @@ public:
     int next() override;
     int prev() override;
     std::string key() override;
+    std::string_view key_as_sv() override;
     std::pair<std::string,std::string> raw_key() override;
+    std::pair<std::string_view,std::string_view> raw_key_as_sv() override;
     bool raw_key_is_prefixed(const std::string &prefix) override;
     ceph::bufferlist value() override;
     ceph::bufferptr value_as_ptr() override;
@@ -414,6 +416,7 @@ public:
   }
 
   static int split_key(rocksdb::Slice in, std::string *prefix, std::string *key);
+  static int split_key(rocksdb::Slice in, std::string_view *prefix, std::string_view *key);
 
   static std::string past_prefix(const std::string &prefix);
 

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 0fd122cdce1761690bd90847cc7617d7982b5194..1c482c505f1b8a3aa1f3e50538fd2fa2fdee0db6 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -4818,6 +4818,19 @@ void BlueStore::Onode::decode_omap_key(const string& key, string *user_key)
   *user_key = key.substr(pos);
 }
 
+void BlueStore::Onode::decode_omap_key(const std::string_view& key, std::string_view *user_key)
+{
+  size_t pos = sizeof(uint64_t) + 1;
+  if (!onode.is_pgmeta_omap()) {
+    if (onode.is_perpg_omap()) {
+      pos += sizeof(uint64_t) + sizeof(uint32_t);
+    } else if (onode.is_perpool_omap()) {
+      pos += sizeof(uint64_t);
+    }
+  }
+  *user_key = key.substr(pos);
+}
+
 // =======================================================
 // WriteContext
  
@@ -13521,12 +13534,12 @@ int BlueStore::omap_iterate(
   ceph::timespan next_lat_acc{0};
   o->get_omap_tail(&tail);
   while (it->valid()) {
-    std::string user_key;
-    if (const auto& db_key = it->raw_key().second; db_key >= tail) {
+    const auto& db_key = it->raw_key_as_sv().second;
+    if (db_key >= tail) {
       break;
-    } else {
-      o->decode_omap_key(db_key, &user_key);
     }
+    std::string_view user_key;
+    o->decode_omap_key(db_key, &user_key);
     omap_iter_ret_t ret = f(user_key, it->value_as_sv());
     if (ret == omap_iter_ret_t::STOP) {
       break;

diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index fe2257d375a1b9f79072f058a119181269d173d7..c92ce9448c0ddefa46dfc77656b9f92a4e20984e 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -1429,6 +1429,7 @@ public:
 
     void rewrite_omap_key(const std::string& old, std::string *out);
     void decode_omap_key(const std::string& key, std::string *user_key);
+    void decode_omap_key(const std::string_view& key, std::string_view *user_key);
 
 private:
     void _decode(const ceph::buffer::list& v);

diff --git a/src/test/ObjectMap/KeyValueDBMemory.cc b/src/test/ObjectMap/KeyValueDBMemory.cc
index 1dd9070d028769ea6415d894826e5d92285a2dd4..cfe25930d6ac5d862ebf1363c3e85d5fb098d48f 100644 (file)
--- a/src/test/ObjectMap/KeyValueDBMemory.cc
+++ b/src/test/ObjectMap/KeyValueDBMemory.cc
@@ -132,12 +132,26 @@ public:
       return "";
   }
 
+  string_view key_as_sv() override {
+    if (valid())
+      return (*it).first.second;
+    else
+      return "";
+  }
+
   pair<string,string> raw_key() override {
     if (valid())
       return (*it).first;
     else
       return make_pair("", "");
   }
+
+  pair<string_view,string_view> raw_key_as_sv() override {
+    if (valid())
+      return (*it).first;
+    else
+      return make_pair("", "");
+  }
   
   bool raw_key_is_prefixed(const string &prefix) override {
     return prefix == (*it).first.first;