From 57718e510c453dd7121a8693bb4cf2913688534f Mon Sep 17 00:00:00 2001 From: Neha Ojha Date: Tue, 5 Feb 2019 19:23:21 -0800 Subject: [PATCH] osd/PGLog: should not rollback further than deleted object version When a deleted object becomes a divergent entry in the pg log, we should not be able to rollback to a version of the deleted object that doesn't exist. To avoid this, we need to preserve the original crt of the pg log, before we update it in rewind_from_head() and use that to decide whether we can rollback or not in _merge_object_divergent_entries(). Fixes: http://tracker.ceph.com/issues/36739 Signed-off-by: Neha Ojha (cherry picked from commit de18c592259816ad013ac82983cdec71bbfa51a0) Conflicts: src/osd/PGLog.h : Resolved for ceph_assert --- src/osd/PGLog.cc | 9 ++++++++- src/osd/PGLog.h | 13 +++++++++++-- src/test/osd/TestPGLog.cc | 2 ++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/osd/PGLog.cc b/src/osd/PGLog.cc index 8a6648c33eec5..88fc1a2bcfc5d 100644 --- a/src/osd/PGLog.cc +++ b/src/osd/PGLog.cc @@ -264,6 +264,7 @@ void PGLog::proc_replica_log( divergent, oinfo, olog.get_can_rollback_to(), + olog.get_can_rollback_to(), omissing, 0, this); @@ -306,7 +307,11 @@ void PGLog::rewind_divergent_log(eversion_t newhead, dout(10) << "rewind_divergent_log truncate divergent future " << newhead << dendl; - + // We need to preserve the original crt before it gets updated in rewind_from_head(). + // Later, in merge_object_divergent_entries(), we use it to check whether we can rollback + // a divergent entry or not. + eversion_t original_crt = log.get_can_rollback_to(); + dout(20) << __func__ << " original_crt = " << original_crt << dendl; if (info.last_complete > newhead) info.last_complete = newhead; @@ -324,6 +329,7 @@ void PGLog::rewind_divergent_log(eversion_t newhead, divergent, info, log.get_can_rollback_to(), + original_crt, missing, rollbacker, this); @@ -445,6 +451,7 @@ void PGLog::merge_log(pg_info_t &oinfo, pg_log_t &olog, pg_shard_t fromosd, divergent, info, log.get_can_rollback_to(), + log.get_can_rollback_to(), missing, rollbacker, this); diff --git a/src/osd/PGLog.h b/src/osd/PGLog.h index 2c7a1c992a850..dc282b183f284 100644 --- a/src/osd/PGLog.h +++ b/src/osd/PGLog.h @@ -844,6 +844,7 @@ protected: const mempool::osd_pglog::list &orig_entries, ///< [in] entries for hoid to merge const pg_info_t &info, ///< [in] info for merging entries eversion_t olog_can_rollback_to, ///< [in] rollback boundary + eversion_t original_can_rollback_to, ///< [in] original rollback boundary missing_type &missing, ///< [in,out] missing to adjust, use LogEntryHandler *rollbacker, ///< [in] optional rollbacker object const DoutPrefixProvider *dpp ///< [in] logging provider @@ -907,6 +908,8 @@ protected: const bool object_not_in_store = !missing.is_missing(hoid) && entries.rbegin()->is_delete(); + ldpp_dout(dpp, 10) << __func__ << ": hoid " << " object_not_in_store: " + << object_not_in_store << dendl; ldpp_dout(dpp, 10) << __func__ << ": hoid " << hoid << " prior_version: " << prior_version << " first_divergent_update: " << first_divergent_update @@ -1009,7 +1012,10 @@ protected: for (list::const_reverse_iterator i = entries.rbegin(); i != entries.rend(); ++i) { - if (!i->can_rollback() || i->version <= olog_can_rollback_to) { + /// Use original_can_rollback_to instead of olog_can_rollback_to to check + // if we can rollback or not. This is to ensure that we don't try to rollback + // to an object that has been deleted and doesn't exist. + if (!i->can_rollback() || i->version <= original_can_rollback_to) { ldpp_dout(dpp, 10) << __func__ << ": hoid " << hoid << " cannot rollback " << *i << dendl; can_rollback = false; @@ -1022,7 +1028,7 @@ protected: for (list::const_reverse_iterator i = entries.rbegin(); i != entries.rend(); ++i) { - assert(i->can_rollback() && i->version > olog_can_rollback_to); + assert(i->can_rollback() && i->version > original_can_rollback_to); ldpp_dout(dpp, 10) << __func__ << ": hoid " << hoid << " rolling back " << *i << dendl; if (rollbacker) @@ -1059,6 +1065,7 @@ protected: mempool::osd_pglog::list &entries, ///< [in] entries to merge const pg_info_t &oinfo, ///< [in] info for merging entries eversion_t olog_can_rollback_to, ///< [in] rollback boundary + eversion_t original_can_rollback_to, ///< [in] original rollback boundary missing_type &omissing, ///< [in,out] missing to adjust, use LogEntryHandler *rollbacker, ///< [in] optional rollbacker object const DoutPrefixProvider *dpp ///< [in] logging provider @@ -1074,6 +1081,7 @@ protected: i->second, oinfo, olog_can_rollback_to, + original_can_rollback_to, omissing, rollbacker, dpp); @@ -1097,6 +1105,7 @@ protected: entries, info, log.get_can_rollback_to(), + log.get_can_rollback_to(), missing, rollbacker, this); diff --git a/src/test/osd/TestPGLog.cc b/src/test/osd/TestPGLog.cc index cdae317ae86fc..9076784f416e3 100644 --- a/src/test/osd/TestPGLog.cc +++ b/src/test/osd/TestPGLog.cc @@ -2938,6 +2938,7 @@ TEST_F(PGLogTest, _merge_object_divergent_entries) { _merge_object_divergent_entries(log, hoid, orig_entries, oinfo, log.get_can_rollback_to(), + log.get_can_rollback_to(), missing, &rollbacker, this); // No core dump @@ -2964,6 +2965,7 @@ TEST_F(PGLogTest, _merge_object_divergent_entries) { _merge_object_divergent_entries(log, hoid, orig_entries, oinfo, log.get_can_rollback_to(), + log.get_can_rollback_to(), missing, &rollbacker, this); // No core dump -- 2.39.5