From a32820d15fd5d2f817bc5fc94b731f4e7adafbf3 Mon Sep 17 00:00:00 2001 From: Samuel Just Date: Thu, 2 Jun 2016 17:13:09 -0700 Subject: [PATCH] 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 (cherry picked from commit 7c5f2acaa57bd6caaf4b13b48154df3ad6fbe84d) --- src/common/hobject.h | 86 ++++++++++++++++++++++++++++++++--- src/os/bluestore/BlueStore.cc | 2 +- src/osd/PG.cc | 6 +-- src/osd/PG.h | 2 +- src/osd/ReplicatedPG.cc | 2 +- src/osd/osd_types.cc | 2 +- 6 files changed, 86 insertions(+), 14 deletions(-) diff --git a/src/common/hobject.h b/src/common/hobject.h index 0cc96fb3670..69bb6111602 100644 --- 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,7 +94,25 @@ public: build_hash_cache(); } - hobject_t(object_t oid, const string& key, snapid_t snap, uint64_t hash, + 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), pool(pool), nspace(nspace), @@ -158,12 +178,6 @@ public: set_hash(std::hash()(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 +struct always_false { + using value = std::false_type; +}; + +template +inline bool operator==(const hobject_t &lhs, const T&) { + static_assert(always_false::value::value, "Do not compare to get_max()"); + return lhs.is_max(); +} +template +inline bool operator==(const T&, const hobject_t &rhs) { + static_assert(always_false::value::value, "Do not compare to get_max()"); + return rhs.is_max(); +} +template +inline bool operator!=(const hobject_t &lhs, const T&) { + static_assert(always_false::value::value, "Do not compare to get_max()"); + return !lhs.is_max(); +} +template +inline bool operator!=(const T&, const hobject_t &rhs) { + static_assert(always_false::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 +static inline int cmp(const hobject_t &l, const T&, bool sort_bitwise) { + static_assert(always_false::value::value, "Do not compare to get_max()"); + return l.is_max() ? 0 : -1; +} +template +static inline int cmp(const T&, const hobject_t&r, bool sort_bitwise) { + static_assert(always_false::value::value, "Do not compare to get_max()"); + return r.is_max() ? 0 : 1; +} +template +static inline int cmp_nibblewise(const hobject_t &l, const T&, bool sort_bitwise) { + static_assert(always_false::value::value, "Do not compare to get_max()"); + return l.is_max() ? 0 : -1; +} +template +static inline int cmp_nibblewise(const T&, const hobject_t&r, bool sort_bitwise) { + static_assert(always_false::value::value, "Do not compare to get_max()"); + return r.is_max() ? 0 : 1; +} +template +static inline int cmp_bitwise(const hobject_t &l, const T&, bool sort_bitwise) { + static_assert(always_false::value::value, "Do not compare to get_max()"); + return l.is_max() ? 0 : -1; +} +template +static inline int cmp_bitwise(const T&, const hobject_t&r, bool sort_bitwise) { + static_assert(always_false::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 553bf108fb5..f8e8c73233a 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -3018,7 +3018,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 e06c560cabc..51b5c1387bf 100644 --- a/src/osd/PG.cc +++ b/src/osd/PG.cc @@ -532,7 +532,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); } } @@ -4281,7 +4281,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 50a8d72f06f..2121c67cd57 100644 --- a/src/osd/PG.h +++ b/src/osd/PG.h @@ -457,7 +457,7 @@ public: needs_recovery_map[hoid] = *item; auto mliter = missing_loc.insert(make_pair(hoid, set())).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 be65e6ed714..eeb9a6c5b8f 100644 --- a/src/osd/ReplicatedPG.cc +++ b/src/osd/ReplicatedPG.cc @@ -953,7 +953,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 c5f790653fd..7ff99cbd23b 100644 --- a/src/osd/osd_types.cc +++ b/src/osd/osd_types.cc @@ -2761,7 +2761,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); -- 2.47.3