Merge pull request #12568 from ifed01/wip-bluestore-onode-diet2

os/bluestore: reduce Onode in-memory footprint

Reviewed-by: Sage Weil <sage@redhat.com>

src/os/bluestore/BlueStore.cc

@@ -1291,7 +1291,6 @@ BlueStore::SharedBlob::SharedBlob(uint64_t i, Cache *c)
     bc(c)
 {
   assert(sbid > 0);
-  get_shared_blob_key(sbid, &key);
 }
 
 BlueStore::SharedBlob::~SharedBlob()
@@ -1332,7 +1331,6 @@ ostream& operator<<(ostream& out, const BlueStore::Blob& b)
     out << " spanning " << b.id;
   }
   out << " " << b.get_blob() << " " << b.get_ref_map()
-      << (b.is_dirty() ? " (dirty)" : " (clean)")
       << " " << *b.shared_blob
       << ")";
   return out;
@@ -1816,7 +1814,7 @@ bool BlueStore::ExtentMap::encode_some(uint32_t offset, uint32_t length,
     denc_varint(0, bound); // logical_offset
     denc_varint(0, bound); // len
     denc_varint(0, bound); // blob_offset
-    p->blob->bound_encode(bound, struct_v, false);
+    p->blob->bound_encode(bound, struct_v, p->blob->shared_blob->sbid, false);
   }
 
   {
@@ -1868,7 +1866,7 @@ bool BlueStore::ExtentMap::encode_some(uint32_t offset, uint32_t length,
       }
       pos = p->logical_end();
       if (include_blob) {
-	p->blob->encode(app, struct_v, false);
+	p->blob->encode(app, struct_v, p->blob->shared_blob->sbid, false);
       }
     }
   }
@@ -1928,9 +1926,10 @@ void BlueStore::ExtentMap::decode_some(bufferlist& bl)
 	assert(le->blob);
       } else {
 	Blob *b = new Blob();
-	b->decode(p, struct_v, false);
+        uint64_t sbid = 0;
+	b->decode(p, struct_v, &sbid, false);
 	blobs[n] = b;
-	onode->c->open_shared_blob(b);
+	onode->c->open_shared_blob(sbid, b);
 	le->assign_blob(b);
       }
       // we build ref_map dynamically for non-spanning blobs
@@ -1953,7 +1952,7 @@ void BlueStore::ExtentMap::bound_encode_spanning_blobs(size_t& p)
   denc_varint((uint32_t)0, key_size);
   p += spanning_blob_map.size() * key_size;
   for (const auto& i : spanning_blob_map) {
-    i.second->bound_encode(p, struct_v, true);
+    i.second->bound_encode(p, struct_v, i.second->shared_blob->sbid, true);
   }
 }
 
@@ -1965,7 +1964,7 @@ void BlueStore::ExtentMap::encode_spanning_blobs(
   denc_varint(spanning_blob_map.size(), p);
   for (auto& i : spanning_blob_map) {
     denc_varint(i.second->id, p);
-    i.second->encode(p, struct_v, true);
+    i.second->encode(p, struct_v, i.second->shared_blob->sbid, true);
   }
 }
 
@@ -1982,8 +1981,9 @@ void BlueStore::ExtentMap::decode_spanning_blobs(
     BlobRef b(new Blob());
     denc_varint(b->id, p);
     spanning_blob_map[b->id] = b;
-    b->decode(p, struct_v, true);
-    c->open_shared_blob(b);
+    uint64_t sbid = 0;
+    b->decode(p, struct_v, &sbid, true);
+    c->open_shared_blob(sbid, b);
   }
 }
 
@@ -2315,7 +2315,7 @@ BlueStore::Collection::Collection(BlueStore *ns, Cache *c, coll_t cid)
 {
 }
 
-void BlueStore::Collection::open_shared_blob(BlobRef b)
+void BlueStore::Collection::open_shared_blob(uint64_t sbid, BlobRef b)
 {
   assert(!b->shared_blob);
   const bluestore_blob_t& blob = b->get_blob();
@@ -2324,14 +2324,14 @@ void BlueStore::Collection::open_shared_blob(BlobRef b)
     return;
   }
 
-  b->shared_blob = shared_blob_set.lookup(blob.sbid);
+  b->shared_blob = shared_blob_set.lookup(sbid);
   if (b->shared_blob) {
-    dout(10) << __func__ << " sbid 0x" << std::hex << blob.sbid << std::dec
+    dout(10) << __func__ << " sbid 0x" << std::hex << sbid << std::dec
 	     << " had " << *b->shared_blob << dendl;
   } else {
-    b->shared_blob = new SharedBlob(blob.sbid, cache);
+    b->shared_blob = new SharedBlob(sbid, cache);
     shared_blob_set.add(b->shared_blob.get());
-    dout(10) << __func__ << " sbid 0x" << std::hex << blob.sbid << std::dec
+    dout(10) << __func__ << " sbid 0x" << std::hex << sbid << std::dec
 	     << " opened " << *b->shared_blob << dendl;
   }
 }
@@ -2340,10 +2340,12 @@ void BlueStore::Collection::load_shared_blob(SharedBlobRef sb)
 {
   assert(!sb->loaded);
   bufferlist v;
-  int r = store->db->get(PREFIX_SHARED_BLOB, sb->key, &v);
+  string key;
+  get_shared_blob_key(sb->sbid, &key);
+  int r = store->db->get(PREFIX_SHARED_BLOB, key, &v);
   if (r < 0) {
     derr << __func__ << " sbid 0x" << std::hex << sb->sbid << std::dec
-	 << " not found at key " << pretty_binary_string(sb->key) << dendl;
+	 << " not found at key " << pretty_binary_string(key) << dendl;
     assert(0 == "uh oh, missing shared_blob");
   }
 
@@ -2354,7 +2356,7 @@ void BlueStore::Collection::load_shared_blob(SharedBlobRef sb)
   sb->loaded = true;
 }
 
-void BlueStore::Collection::make_blob_shared(BlobRef b)
+void BlueStore::Collection::make_blob_shared(uint64_t sbid, BlobRef b)
 {
   dout(10) << __func__ << " " << *b << dendl;
   bluestore_blob_t& blob = b->dirty_blob();
@@ -2365,8 +2367,7 @@ void BlueStore::Collection::make_blob_shared(BlobRef b)
 
   // update shared blob
   b->shared_blob->loaded = true;  // we are new and therefore up to date
-  b->shared_blob->sbid = blob.sbid;
-  get_shared_blob_key(blob.sbid, &b->shared_blob->key);
+  b->shared_blob->sbid = sbid;
   shared_blob_set.add(b->shared_blob.get());
   for (auto p : blob.extents) {
     if (p.is_valid()) {
@@ -4436,18 +4437,18 @@ int BlueStore::fsck(bool deep)
 	  }
 	}
 	if (blob.is_shared()) {
-	  if (blob.sbid > blobid_max) {
+	  if (i.first->shared_blob->sbid > blobid_max) {
 	    derr << __func__ << " " << oid << " blob " << blob
-		 << " sbid " << blob.sbid << " > blobid_max "
+		 << " sbid " << i.first->shared_blob->sbid << " > blobid_max "
 		 << blobid_max << dendl;
 	    ++errors;
-	  } else if (blob.sbid == 0) {
+	  } else if (i.first->shared_blob->sbid == 0) {
             derr << __func__ << " " << oid << " blob " << blob
                  << " marked as shared but has uninitialized sbid"
                  << dendl;
             ++errors;
           }
-	  sb_info_t& sbi = sb_info[blob.sbid];
+	  sb_info_t& sbi = sb_info[i.first->shared_blob->sbid];
 	  sbi.sb = i.first->shared_blob;
 	  sbi.oids.push_back(oid);
 	  sbi.compressed = blob.is_compressed();
@@ -6414,16 +6415,18 @@ void BlueStore::_txc_write_nodes(TransContext *txc, KeyValueDB::Transaction t)
 
   // finalize shared_blobs
   for (auto sb : txc->shared_blobs) {
+    string key;
+    get_shared_blob_key(sb->sbid, &key);
     if (sb->shared_blob.empty()) {
       dout(20) << "  shared_blob 0x" << std::hex << sb->sbid << std::dec
 	       << " is empty" << dendl;
-      t->rmkey(PREFIX_SHARED_BLOB, sb->key);
+      t->rmkey(PREFIX_SHARED_BLOB, key);
     } else {
       bufferlist bl;
       ::encode(sb->shared_blob, bl);
       dout(20) << "  shared_blob 0x" << std::hex << sb->sbid << std::dec
 	       << " is " << bl.length() << dendl;
-      t->set(PREFIX_SHARED_BLOB, sb->key, bl);
+      t->set(PREFIX_SHARED_BLOB, key, bl);
     }
   }
 }
@@ -8676,8 +8679,7 @@ int BlueStore::_do_clone_range(
       const bluestore_blob_t& blob = e.blob->get_blob();
       // make sure it is shared
       if (!blob.is_shared()) {
-	e.blob->dirty_blob().sbid = _assign_blobid(txc);
-	c->make_blob_shared(e.blob);
+	c->make_blob_shared(_assign_blobid(txc), e.blob);
 	dirtied_oldo = true;  // fixme: overkill
       } else if (!e.blob->shared_blob->loaded) {
 	c->load_shared_blob(e.blob->shared_blob);

src/os/bluestore/BlueStore.h

@@ -334,7 +334,6 @@ class BlueStore : public ObjectStore,
 
     // these are defined/set if the blob is marked 'shared'
     uint64_t sbid = 0;          ///< shared blob id
-    string key;                 ///< key in kv store
     SharedBlobSet *parent_set = 0;  ///< containing SharedBlobSet
 
     BufferSpace bc;             ///< buffer cache
@@ -410,6 +409,8 @@ class BlueStore : public ObjectStore,
     }
   };
 
+//#define CACHE_BLOB_BL  // not sure if this is a win yet or not... :/
+
   /// in-memory blob metadata and associated cached buffers (if any)
   struct Blob {
     MEMPOOL_CLASS_HELPERS();
@@ -421,8 +422,9 @@ class BlueStore : public ObjectStore,
 
   private:
     mutable bluestore_blob_t blob;  ///< decoded blob metadata
-    mutable bool dirty = true;      ///< true if blob is newer than blob_bl
-    mutable bufferlist blob_bl;     ///< cached encoded blob
+#ifdef CACHE_BLOB_BL
+    mutable bufferlist blob_bl;     ///< cached encoded blob, blob is dirty if empty
+#endif
     /// refs from this shard.  ephemeral if id<0, persisted if spanning.
     bluestore_extent_ref_map_t ref_map;
 
@@ -452,24 +454,20 @@ class BlueStore : public ObjectStore,
     void dup(Blob& o) {
       o.shared_blob = shared_blob;
       o.blob = blob;
-      o.dirty = dirty;
+#ifdef CACHE_BLOB_BL
       o.blob_bl = blob_bl;
+#endif
     }
 
     const bluestore_blob_t& get_blob() const {
       return blob;
     }
     bluestore_blob_t& dirty_blob() {
-      if (!dirty) {
-	dirty = true;
-	blob_bl.clear();
-      }
+#ifdef CACHE_BLOB_BL
+      blob_bl.clear();
+#endif
       return blob;
     }
-    bool is_dirty() const {
-      return dirty;
-    }
-
     bool is_unreferenced(uint64_t offset, uint64_t length) const {
       return !ref_map.intersects(offset, length);
     }
@@ -496,14 +494,11 @@ class BlueStore : public ObjectStore,
 	delete this;
     }
 
-//#define CACHE_BLOB_BL  // not sure if this is a win yet or not... :/
 
 #ifdef CACHE_BLOB_BL
     void _encode() const {
-      if (dirty) {
-	blob_bl.clear();
+      if (blob_bl.length() == 0 ) {
 	::encode(blob, blob_bl);
-	dirty = false;
       } else {
 	assert(blob_bl.length());
       }
@@ -528,28 +523,36 @@ class BlueStore : public ObjectStore,
       const char *end = p.get_pos();
       blob_bl.clear();
       blob_bl.append(start, end - start);
-      dirty = false;
       if (include_ref_map) {
         ref_map.decode(p);
       }
     }
 #else
-    void bound_encode(size_t& p, uint64_t struct_v, bool include_ref_map) const {
+    void bound_encode(size_t& p, uint64_t struct_v, uint64_t sbid, bool include_ref_map) const {
       denc(blob, p, struct_v);
+      if (blob.is_shared()) {
+        denc(sbid, p);
+      }
       if (include_ref_map) {
         ref_map.bound_encode(p);
       }
     }
-    void encode(bufferlist::contiguous_appender& p, uint64_t struct_v,
+    void encode(bufferlist::contiguous_appender& p, uint64_t struct_v, uint64_t sbid,
 		bool include_ref_map) const {
       denc(blob, p, struct_v);
+      if (blob.is_shared()) {
+        denc(sbid, p);
+      }
       if (include_ref_map) {
         ref_map.encode(p);
       }
     }
-    void decode(bufferptr::iterator& p, uint64_t struct_v,
+    void decode(bufferptr::iterator& p, uint64_t struct_v, uint64_t* sbid,
 		bool include_ref_map) {
       denc(blob, p, struct_v);
+      if (blob.is_shared()) {
+        denc(*sbid, p);
+      }
       if (include_ref_map) {
         ref_map.decode(p);
       }
@@ -1111,9 +1114,9 @@ class BlueStore : public ObjectStore,
     //  open = SharedBlob is instantiated
     //  shared = blob_t shared flag is set; SharedBlob is hashed.
     //  loaded = SharedBlob::shared_blob_t is loaded from kv store
-    void open_shared_blob(BlobRef b);
+    void open_shared_blob(uint64_t sbid, BlobRef b);
     void load_shared_blob(SharedBlobRef sb);
-    void make_blob_shared(BlobRef b);
+    void make_blob_shared(uint64_t sbid, BlobRef b);
 
     BlobRef new_blob() {
       BlobRef b = new Blob;

src/os/bluestore/bluestore_types.cc

@@ -400,7 +400,6 @@ void bluestore_blob_t::dump(Formatter *f) const
     f->dump_object("extent", p);
   }
   f->close_section();
-  f->dump_unsigned("shared_blob_id", sbid);
   f->dump_unsigned("compressed_length_original", compressed_length_orig);
   f->dump_unsigned("compressed_length", compressed_length);
   f->dump_unsigned("flags", flags);
@@ -434,9 +433,6 @@ void bluestore_blob_t::generate_test_instances(list<bluestore_blob_t*>& ls)
 ostream& operator<<(ostream& out, const bluestore_blob_t& o)
 {
   out << "blob(" << o.extents;
-  if (o.sbid) {
-    out << " sbid 0x" << std::hex << o.sbid << std::dec;
-  }
   if (o.is_compressed()) {
     out << " clen 0x" << std::hex
 	<< o.compressed_length_orig

src/os/bluestore/bluestore_types.h

@@ -280,7 +280,6 @@ struct bluestore_blob_t {
   static string get_flags_string(unsigned flags);
 
   vector<bluestore_pextent_t> extents;///< raw data position on device
-  uint64_t sbid = 0;                  ///< shared blob id (if shared)
   uint32_t compressed_length_orig = 0;///< original length of compressed blob if any
   uint32_t compressed_length = 0;     ///< compressed length if any
   uint32_t flags = 0;                 ///< FLAG_*
@@ -301,7 +300,6 @@ struct bluestore_blob_t {
     assert(struct_v == 1);
     denc(extents, p);
     denc_varint(flags, p);
-    denc_varint(sbid, p);
     denc_varint_lowz(compressed_length_orig, p);
     denc_varint_lowz(compressed_length, p);
     denc(csum_type, p);
@@ -315,9 +313,6 @@ struct bluestore_blob_t {
     assert(struct_v == 1);
     denc(extents, p);
     denc_varint(flags, p);
-    if (is_shared()) {
-      denc_varint(sbid, p);
-    }
     if (is_compressed()) {
       denc_varint_lowz(compressed_length_orig, p);
       denc_varint_lowz(compressed_length, p);
@@ -338,9 +333,6 @@ struct bluestore_blob_t {
     assert(struct_v == 1);
     denc(extents, p);
     denc_varint(flags, p);
-    if (is_shared()) {
-      denc_varint(sbid, p);
-    }
     if (is_compressed()) {
       denc_varint_lowz(compressed_length_orig, p);
       denc_varint_lowz(compressed_length, p);

src/test/objectstore/test_bluestore_types.cc

@@ -688,7 +688,6 @@ TEST(Blob, put_ref)
     b.extents.push_back(bluestore_pextent_t(0x40118000, 0x7000));
     b.set_flag(bluestore_blob_t::FLAG_SHARED);
     b.init_csum(Checksummer::CSUM_CRC32C, 12, 0x1e000);
-    b.sbid = 0xcf92e;
 
     cout << "before: " << B << std::endl;
     vector<bluestore_pextent_t> r;