src/: remove all direct comparisons to get_max()

get_max() now returns a special singleton type from which hobject_t's
can be assigned and constructed, but which cannot be directly compared.

This patch also cleans up all such uses to use is_max() instead.

This should prevent some issues like 16113 by preventing us from
checking for max-ness by comparing against a sentinel value.  The more
complete fix will be to make all fields of hobject_t private and enforce
a canonical max() representation that way.  That patch will be hard to
backport, however, so we'll settle for this for now.

Fixes: http://tracker.ceph.com/issues/16113
Signed-off-by: Samuel Just <sjust@redhat.com>

diff --git a/src/common/hobject.h b/src/common/hobject.h
index d295cf7b60ba1613386e637c9df42b89f9045533..69bb61116025da03f7c50c12cfa4f912b13c5b69 100644 (file)
--- a/src/common/hobject.h
+++ b/src/common/hobject.h
@@ -52,6 +52,8 @@ public:
 private:
   string key;
 
+  class hobject_t_max {};
+
 public:
   const string &get_key() const {
     return key;
@@ -92,6 +94,24 @@ public:
     build_hash_cache();
   }
 
+  hobject_t(const hobject_t &rhs) = default;
+  hobject_t(hobject_t &&rhs) = default;
+  hobject_t(hobject_t_max &&singleon) : hobject_t() {
+    max = true;
+  }
+  hobject_t &operator=(const hobject_t &rhs) = default;
+  hobject_t &operator=(hobject_t &&rhs) = default;
+  hobject_t &operator=(hobject_t_max &&singleton) {
+    *this = hobject_t();
+    max = true;
+    return *this;
+  }
+
+  // maximum sorted value.
+  static hobject_t_max get_max() {
+    return hobject_t_max();
+  }
+
   hobject_t(object_t oid, const string& key, snapid_t snap, uint32_t hash,
            int64_t pool, string nspace)
     : oid(oid), snap(snap), hash(hash), max(false),
@@ -158,12 +178,6 @@ public:
     set_hash(std::hash<sobject_t>()(o));
   }
 
-  // maximum sorted value.
-  static hobject_t get_max() {
-    hobject_t h;
-    h.max = true;
-    return h;
-  }
   bool is_max() const {
     return max;
   }
@@ -320,6 +334,32 @@ ostream& operator<<(ostream& out, const hobject_t& o);
 
 WRITE_EQ_OPERATORS_7(hobject_t, hash, oid, get_key(), snap, pool, max, nspace)
 
+template <typename T>
+struct always_false {
+  using value = std::false_type;
+};
+
+template <typename T>
+inline bool operator==(const hobject_t &lhs, const T&) {
+  static_assert(always_false<T>::value::value, "Do not compare to get_max()");
+  return lhs.is_max();
+}
+template <typename T>
+inline bool operator==(const T&, const hobject_t &rhs) {
+  static_assert(always_false<T>::value::value, "Do not compare to get_max()");
+  return rhs.is_max();
+}
+template <typename T>
+inline bool operator!=(const hobject_t &lhs, const T&) {
+  static_assert(always_false<T>::value::value, "Do not compare to get_max()");
+  return !lhs.is_max();
+}
+template <typename T>
+inline bool operator!=(const T&, const hobject_t &rhs) {
+  static_assert(always_false<T>::value::value, "Do not compare to get_max()");
+  return !rhs.is_max();
+}
+
 extern int cmp_nibblewise(const hobject_t& l, const hobject_t& r);
 extern int cmp_bitwise(const hobject_t& l, const hobject_t& r);
 static inline int cmp(const hobject_t& l, const hobject_t& r, bool sort_bitwise) {
@@ -328,6 +368,38 @@ static inline int cmp(const hobject_t& l, const hobject_t& r, bool sort_bitwise)
   else
     return cmp_nibblewise(l, r);
 }
+template <typename T>
+static inline int cmp(const hobject_t &l, const T&, bool sort_bitwise) {
+  static_assert(always_false<T>::value::value, "Do not compare to get_max()");
+  return l.is_max() ? 0 : -1;
+}
+template <typename T>
+static inline int cmp(const T&, const hobject_t&r, bool sort_bitwise) {
+  static_assert(always_false<T>::value::value, "Do not compare to get_max()");
+  return r.is_max() ? 0 : 1;
+}
+template <typename T>
+static inline int cmp_nibblewise(const hobject_t &l, const T&, bool sort_bitwise) {
+  static_assert(always_false<T>::value::value, "Do not compare to get_max()");
+  return l.is_max() ? 0 : -1;
+}
+template <typename T>
+static inline int cmp_nibblewise(const T&, const hobject_t&r, bool sort_bitwise) {
+  static_assert(always_false<T>::value::value, "Do not compare to get_max()");
+  return r.is_max() ? 0 : 1;
+}
+template <typename T>
+static inline int cmp_bitwise(const hobject_t &l, const T&, bool sort_bitwise) {
+  static_assert(always_false<T>::value::value, "Do not compare to get_max()");
+  return l.is_max() ? 0 : -1;
+}
+template <typename T>
+static inline int cmp_bitwise(const T&, const hobject_t&r, bool sort_bitwise) {
+  static_assert(always_false<T>::value::value, "Do not compare to get_max()");
+  return r.is_max() ? 0 : 1;
+}
+
+
 
 // these are convenient
 static inline hobject_t MAX_HOBJ(const hobject_t& l, const hobject_t& r, bool bitwise) {

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 2f1092ee6182a2d8b3a88abe709ec4af9893c963..1ecaa5dd51f45ab85877c5bec621d512f9239c65 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -3605,7 +3605,7 @@ int BlueStore::collection_list(
     pnext = &static_next;
 
   if (start == ghobject_t::get_max() ||
-      start.hobj == hobject_t::get_max()) {
+      start.hobj.is_max()) {
     goto out;
   }
   get_coll_key_range(c->cid, c->cnode.bits, &temp_start_key, &temp_end_key,

diff --git a/src/osd/PG.cc b/src/osd/PG.cc
index 426a0f41044612d525b9c4c68bf03f54a97bc251..66a23e234e399dd8a9f542d8e792b3d7480ed57e 100644 (file)
--- a/src/osd/PG.cc
+++ b/src/osd/PG.cc
@@ -538,7 +538,7 @@ bool PG::MissingLoc::add_source_info(
               << dendl;
       continue;
     }
-    if (oinfo.last_backfill != hobject_t::get_max() &&
+    if (!oinfo.last_backfill.is_max() &&
        oinfo.last_backfill_bitwise != sort_bitwise) {
       dout(10) << "search_for_missing " << soid << " " << need
               << " also missing on osd." << fromosd
@@ -1798,7 +1798,7 @@ void PG::activate(ObjectStore::Transaction& t,
       } else {
        assert(peer_missing.count(*i));
        missing_loc.add_active_missing(peer_missing[*i]);
-        if (!peer_missing[*i].have_missing() && peer_info[*i].last_backfill == hobject_t::get_max())
+        if (!peer_missing[*i].have_missing() && peer_info[*i].last_backfill.is_max())
           complete_shards.insert(*i);
       }
     }
@@ -4246,7 +4246,7 @@ void PG::chunky_scrub(ThreadPool::TPHandle &handle)
          break;
        }
 
-       if (cmp(scrubber.end, hobject_t::get_max(), get_sort_bitwise()) < 0) {
+       if (!(scrubber.end.is_max())) {
           scrubber.state = PG::Scrubber::NEW_CHUNK;
          requeue_scrub();
           done = true;

diff --git a/src/osd/PG.h b/src/osd/PG.h
index 1c596fe4d136e3daaea07cb3bf8887ade5084173..3d28dd327bfe7fd6e59514bcb52fedf859419565 100644 (file)
--- a/src/osd/PG.h
+++ b/src/osd/PG.h
@@ -452,7 +452,7 @@ public:
       needs_recovery_map[hoid] = *item;
       auto mliter =
        missing_loc.insert(make_pair(hoid, set<pg_shard_t>())).first;
-      assert(info.last_backfill == hobject_t::get_max());
+      assert(info.last_backfill.is_max());
       assert(info.last_update >= item->need);
       if (!missing.is_missing(hoid))
        mliter->second.insert(self);

diff --git a/src/osd/ReplicatedPG.cc b/src/osd/ReplicatedPG.cc
index d60d665780cf5978b96678e86dd4e37fb4fbb5b8..368a00ac4302e2aec66835f2e25d8339fe6ea9f2 100644 (file)
--- a/src/osd/ReplicatedPG.cc
+++ b/src/osd/ReplicatedPG.cc
@@ -933,7 +933,7 @@ void ReplicatedPG::do_pg_op(OpRequestRef op)
         dout(10) << " pgnls lower_bound " << lower_bound
                 << " pg_end " << pg_end << dendl;
        if (get_sort_bitwise() &&
-           ((lower_bound != hobject_t::get_max() &&
+           ((!lower_bound.is_max() &&
              cmp_bitwise(lower_bound, pg_end) >= 0) ||
             (lower_bound != hobject_t() &&
              cmp_bitwise(lower_bound, pg_start) < 0))) {

diff --git a/src/osd/osd_types.cc b/src/osd/osd_types.cc
index 9ccdf672db2a0eb6e980ab1e7eeed05fedf09357..3871fccffb868424654fb3ff44a911404841ffc3 100644 (file)
--- a/src/osd/osd_types.cc
+++ b/src/osd/osd_types.cc
@@ -2757,7 +2757,7 @@ void pg_info_t::encode(bufferlist &bl) const
   ::encode(last_update, bl);
   ::encode(last_complete, bl);
   ::encode(log_tail, bl);
-  if (last_backfill_bitwise && last_backfill != last_backfill.get_max()) {
+  if (last_backfill_bitwise && !last_backfill.is_max()) {
     ::encode(hobject_t(), bl);
   } else {
     ::encode(last_backfill, bl);