osd/PG: clean up fastinfo key when last_update does not increase

src/crimson/osd/pg.cc

@@ -233,6 +233,44 @@ void PG::on_activate_complete()
   }
 }
 
+void PG::prepare_write(pg_info_t &info,
+		       pg_info_t &last_written_info,
+		       PastIntervals &past_intervals,
+		       PGLog &pglog,
+		       bool dirty_info,
+		       bool dirty_big_info,
+		       bool need_write_epoch,
+		       ceph::os::Transaction &t)
+{
+  std::map<string,bufferlist> km;
+  std::string key_to_remove;
+  if (dirty_big_info || dirty_info) {
+    int ret = prepare_info_keymap(
+      shard_services.get_cct(),
+      &km,
+      &key_to_remove,
+      get_osdmap_epoch(),
+      info,
+      last_written_info,
+      past_intervals,
+      dirty_big_info,
+      need_write_epoch,
+      true,
+      nullptr,
+      this);
+    ceph_assert(ret == 0);
+  }
+  pglog.write_log_and_missing(
+    t, &km, coll, pgmeta_oid,
+    peering_state.get_pool().info.require_rollback());
+  if (!km.empty()) {
+    t.omap_setkeys(coll, pgmeta_oid, km);
+  }
+  if (!key_to_remove.empty()) {
+    t.omap_rmkey(coll, pgmeta_oid, key_to_remove);
+  }
+}
+
 void PG::log_state_enter(const char *state) {
   logger().info("Entering state: {}", state);
 }

src/crimson/osd/pg.h

@@ -119,29 +119,7 @@ class PG : public boost::intrusive_ref_counter<
     bool dirty_info,
     bool dirty_big_info,
     bool need_write_epoch,
-    ceph::os::Transaction &t) final {
-    std::map<string,bufferlist> km;
-    if (dirty_big_info || dirty_info) {
-      int ret = prepare_info_keymap(
-	shard_services.get_cct(),
-	&km,
-	get_osdmap_epoch(),
-	info,
-	last_written_info,
-	past_intervals,
-	dirty_big_info,
-	need_write_epoch,
-	true,
-	nullptr,
-	this);
-      ceph_assert(ret == 0);
-    }
-    pglog.write_log_and_missing(
-      t, &km, coll, pgmeta_oid,
-      peering_state.get_pool().info.require_rollback());
-    if (!km.empty())
-      t.omap_setkeys(coll, pgmeta_oid, km);
-  }
+    ceph::os::Transaction &t) final;
 
   void on_info_history_change() final {
     // Not needed yet -- mainly for scrub scheduling

src/os/FuseStore.cc

@@ -1008,9 +1008,7 @@ static int os_unlink(const char *path)
   switch (f) {
   case FN_OBJECT_OMAP_VAL:
     {
-      set<string> keys;
-      keys.insert(key);
-      t.omap_rmkeys(cid, oid, keys);
+      t.omap_rmkey(cid, oid, key);
     }
     break;
 

src/os/Transaction.h

@@ -1119,6 +1119,22 @@ class Transaction {
     data.ops = data.ops + 1;
   }
 
+  /// Remove key from oid omap
+  void omap_rmkey(
+    const coll_t &cid,             ///< [in] Collection containing oid
+    const ghobject_t &oid,  ///< [in] Object from which to remove the omap
+    const std::string& key ///< [in] Keys to clear
+    ) {
+    Op* _op = _get_next_op();
+    _op->op = OP_OMAP_RMKEYS;
+    _op->cid = _get_coll_id(cid);
+    _op->oid = _get_object_id(oid);
+    using ceph::encode;
+    encode((uint32_t)1, data_bl);
+    encode(key, data_bl);
+    data.ops = data.ops + 1;
+  }
+
   /// Remove keys from oid omap
   void omap_rmkeys(
     const coll_t &cid,             ///< [in] Collection containing oid

src/osd/OSD.cc

@@ -6001,11 +6001,9 @@ void TestOpsSocketHook::test_ops(OSDService *service, ObjectStore *store,
         ss << "ok";
     } else if (command == "rmomapkey") {
       string key;
-      set<string> keys;
       cmd_getval(service->cct, cmdmap, "key", key);
 
-      keys.insert(key);
-      t.omap_rmkeys(coll_t(pgid), ghobject_t(obj), keys);
+      t.omap_rmkey(coll_t(pgid), ghobject_t(obj), key);
       r = store->queue_transaction(service->meta_ch, std::move(t));
       if (r < 0)
         ss << "error=" << r;

src/osd/PG.cc

@@ -949,10 +949,12 @@ void PG::prepare_write(
   info.stats.stats.add(unstable_stats);
   unstable_stats.clear();
   map<string,bufferlist> km;
+  string key_to_remove;
   if (dirty_big_info || dirty_info) {
     int ret = prepare_info_keymap(
       cct,
       &km,
+      &key_to_remove,
       get_osdmap_epoch(),
       info,
       last_written_info,
@@ -968,6 +970,8 @@ void PG::prepare_write(
     t, &km, coll, pgmeta_oid, pool.info.require_rollback());
   if (!km.empty())
     t.omap_setkeys(coll, pgmeta_oid, km);
+  if (!key_to_remove.empty())
+    t.omap_rmkey(coll, pgmeta_oid, key_to_remove);
 }
 
 #pragma GCC diagnostic ignored "-Wpragmas"

src/osd/osd_types.cc

@@ -6840,6 +6840,7 @@ void OSDOp::clear_data(vector<OSDOp>& ops)
 int prepare_info_keymap(
   CephContext* cct,
   map<string,bufferlist> *km,
+  string *key_to_remove,
   epoch_t epoch,
   pg_info_t &info,
   pg_info_t &last_written_info,
@@ -6885,6 +6886,10 @@ int prepare_info_keymap(
       }
       *_dout << dendl;
     }
+  } else if (info.last_update <= last_written_info.last_update) {
+    // clean up any potentially stale fastinfo key resulting from last_update
+    // not moving forwards (e.g., a backwards jump during peering)
+    *key_to_remove = fastinfo_key;
   }
 
   last_written_info = info;

src/osd/osd_types.h

@@ -6229,6 +6229,7 @@ static const __u8 pg_compat_struct_v = 10;
 int prepare_info_keymap(
   CephContext* cct,
   map<string,bufferlist> *km,
+  string *key_to_remove,
   epoch_t epoch,
   pg_info_t &info,
   pg_info_t &last_written_info,

src/test/objectstore/ObjectStoreTransactionBenchmark.cc

@@ -41,7 +41,7 @@ class Transaction {
       count++;
     }
   };
-  static Tick write_ticks, setattr_ticks, omap_setkeys_ticks, omap_rmkeys_ticks;
+  static Tick write_ticks, setattr_ticks, omap_setkeys_ticks, omap_rmkey_ticks;
   static Tick encode_ticks, decode_ticks, iterate_ticks;
 
   void write(coll_t cid, const ghobject_t& oid, uint64_t off, uint64_t len,
@@ -63,11 +63,11 @@ class Transaction {
     t.omap_setkeys(cid, oid, attrset);
     omap_setkeys_ticks.add(Cycles::rdtsc() - start_time);
   }
-  void omap_rmkeys(coll_t cid, const ghobject_t &oid,
-                   const set<string> &keys) {
+  void omap_rmkey(coll_t cid, const ghobject_t &oid,
+                   const string &key) {
     uint64_t start_time = Cycles::rdtsc();
-    t.omap_rmkeys(cid, oid, keys);
-    omap_rmkeys_ticks.add(Cycles::rdtsc() - start_time);
+    t.omap_rmkey(cid, oid, key);
+    omap_rmkey_ticks.add(Cycles::rdtsc() - start_time);
   }
 
   void apply_encode_decode() {
@@ -130,7 +130,7 @@ class Transaction {
     cerr << " write op: " << Cycles::to_microseconds(write_ticks.ticks) << "us count: " << write_ticks.count << std::endl;
     cerr << " setattr op: " << Cycles::to_microseconds(setattr_ticks.ticks) << "us count: " << setattr_ticks.count << std::endl;
     cerr << " omap_setkeys op: " << Cycles::to_microseconds(Transaction::omap_setkeys_ticks.ticks) << "us count: " << Transaction::omap_setkeys_ticks.count << std::endl;
-    cerr << " omap_rmkeys op: " << Cycles::to_microseconds(Transaction::omap_rmkeys_ticks.ticks) << "us count: " << Transaction::omap_rmkeys_ticks.count << std::endl;
+    cerr << " omap_rmkey op: " << Cycles::to_microseconds(Transaction::omap_rmkey_ticks.ticks) << "us count: " << Transaction::omap_rmkey_ticks.count << std::endl;
     cerr << " encode op: " << Cycles::to_microseconds(Transaction::encode_ticks.ticks) << "us count: " << Transaction::encode_ticks.count << std::endl;
     cerr << " decode op: " << Cycles::to_microseconds(Transaction::decode_ticks.ticks) << "us count: " << Transaction::decode_ticks.count << std::endl;
     cerr << " iterate op: " << Cycles::to_microseconds(Transaction::iterate_ticks.ticks) << "us count: " << Transaction::iterate_ticks.count << std::endl;
@@ -207,15 +207,13 @@ class PerfCase {
         Transaction t;
         map<string, bufferlist> pglog_attrset;
         map<string, bufferlist> info_attrset;
-        set<string> keys;
-        keys.insert(pglog_attr);
         pglog_attrset[pglog_attr] = data[pglog_attr];
         info_attrset[info_epoch_attr] = data[info_epoch_attr];
         info_attrset[info_info_attr] = data[info_info_attr];
         start_time = Cycles::rdtsc();
         t.omap_setkeys(meta_cid, pglog_oid, pglog_attrset);
         t.omap_setkeys(meta_cid, info_oid, info_attrset);
-        t.omap_rmkeys(meta_cid, pglog_oid, keys);
+        t.omap_rmkey(meta_cid, pglog_oid, pglog_attr);
         t.apply_encode_decode();
         t.apply_iterate();
         ticks += Cycles::rdtsc() - start_time;
@@ -233,7 +231,7 @@ const coll_t PerfCase::meta_cid;
 const coll_t PerfCase::cid;
 const ghobject_t PerfCase::pglog_oid(hobject_t(sobject_t(object_t("cid_pglog"), 0)));
 const ghobject_t PerfCase::info_oid(hobject_t(sobject_t(object_t("infos"), 0)));
-Transaction::Tick Transaction::write_ticks, Transaction::setattr_ticks, Transaction::omap_setkeys_ticks, Transaction::omap_rmkeys_ticks;
+Transaction::Tick Transaction::write_ticks, Transaction::setattr_ticks, Transaction::omap_setkeys_ticks, Transaction::omap_rmkey_ticks;
 Transaction::Tick Transaction::encode_ticks, Transaction::decode_ticks, Transaction::iterate_ticks;
 
 void usage(const string &name) {

src/test/objectstore/store_test.cc

@@ -5099,9 +5099,7 @@ TEST_P(StoreTest, OMapTest) {
     }
 
     string to_remove = attrs.begin()->first;
-    set<string> keys_to_remove;
-    keys_to_remove.insert(to_remove);
-    t.omap_rmkeys(cid, hoid, keys_to_remove);
+    t.omap_rmkey(cid, hoid, to_remove);
     r = queue_transaction(store, ch, std::move(t));
     ASSERT_EQ(r, 0);
 

src/tools/ceph_objectstore_tool.cc

@@ -577,16 +577,21 @@ int write_info(ObjectStore::Transaction &t, epoch_t epoch, pg_info_t &info,
   coll_t coll(info.pgid);
   ghobject_t pgmeta_oid(info.pgid.make_pgmeta_oid());
   map<string,bufferlist> km;
+  string key_to_remove;
   pg_info_t last_written_info;
   int ret = prepare_info_keymap(
     g_ceph_context,
-    &km, epoch,
+    &km, &key_to_remove,
+    epoch,
     info,
     last_written_info,
     past_intervals,
     true, true, false);
   if (ret) cerr << "Failed to write info" << std::endl;
   t.omap_setkeys(coll, pgmeta_oid, km);
+  if (!key_to_remove.empty()) {
+    t.omap_rmkey(coll, pgmeta_oid, key_to_remove);
+  }
   return ret;
 }
 
@@ -1972,9 +1977,7 @@ int ObjectStoreTool::do_import(ObjectStore *store, OSDSuperblock& sb,
     cerr << "done, clearing removal flag" << std::endl;
 
   if (!dry_run) {
-    set<string> remove;
-    remove.insert("_remove");
-    t.omap_rmkeys(coll, pgid.make_pgmeta_oid(), remove);
+    t.omap_rmkey(coll, pgid.make_pgmeta_oid(), "_remove");
     wait_until_done(&t, [&] {
       store->queue_transaction(ch, std::move(t));
       // make sure we flush onreadable items before mapper/driver are destroyed.
@@ -2395,17 +2398,14 @@ int do_rm_omap(ObjectStore *store, coll_t coll,
 {
   ObjectStore::Transaction tran;
   ObjectStore::Transaction *t = &tran;
-  set<string> keys;
-
-  keys.insert(key);
 
   if (debug)
     cerr << "Rm_omap " << ghobj << std::endl;
 
   if (dry_run)
     return 0;
 
-  t->omap_rmkeys(coll, ghobj, keys);
+  t->omap_rmkey(coll, ghobj, key);
 
   auto ch = store->open_collection(coll);
   store->queue_transaction(ch, std::move(*t));