From: Samuel Just <sjust@redhat.com>
Date: Wed, 23 Nov 2016 19:23:54 +0000 (-0800)
Subject: PGTransaction,ReplicatedPG: clarify handling of noop operations
X-Git-Tag: v11.1.0~58^2~10
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=d6417f4c2de4fec16afe515d662a88ca66136b2a;p=ceph-ci.git

PGTransaction,ReplicatedPG: clarify handling of noop operations

The offending transaction was [call rbd.copyup,delete] on a non-existent
object.  PGTransaction incorrectly ended up with Create and delete_first
causing a transaction beginning with trying to collection_move_rename a
non-existent head object.  In fact, if we delete an object which the
transaction currently claims to be creating, the transaction should show
as empty (for that object).  Rather than going through the normal write
pipeline for that case, let's just record a 0 error code in the log and
call it a day.  That way, the transaction generating code only needs to
worry about about updates to actual objects.

Signed-off-by: Samuel Just <sjust@redhat.com>
---

diff --git a/src/osd/PGTransaction.h b/src/osd/PGTransaction.h
index a8c0608cc76..7ef85abaeab 100644
--- a/src/osd/PGTransaction.h
+++ b/src/osd/PGTransaction.h
@@ -92,6 +92,9 @@ public:
     bool is_fresh_object() const {
       return boost::get<Init::None>(&init_type) == nullptr;
     }
+    bool is_rename() const {
+      return boost::get<Init::Rename>(&init_type) != nullptr;
+    }
     bool has_source(hobject_t *source = nullptr) const {
       return match(
 	init_type,
@@ -295,14 +298,19 @@ public:
     op.init_type = ObjectOperation::Init::Rename{source};
   }
 
-  /// Remove
+  /// Remove -- must not be called on rename target
   void remove(
     const hobject_t &hoid          ///< [in] obj to remove
     ) {
     auto &op = get_object_op_for_modify(hoid);
-    assert(!op.updated_snaps);
-    op = ObjectOperation();
-    op.delete_first = true;
+    if (!op.is_fresh_object()) {
+      assert(!op.updated_snaps);
+      op = ObjectOperation();
+      op.delete_first = true;
+    } else {
+      assert(!op.is_rename());
+      op_map.erase(hoid); // make it a noop if it's a fresh object
+    }
   }
 
   void update_snaps(
diff --git a/src/osd/ReplicatedPG.cc b/src/osd/ReplicatedPG.cc
index b69e90349d7..3143d4e8dc4 100644
--- a/src/osd/ReplicatedPG.cc
+++ b/src/osd/ReplicatedPG.cc
@@ -3220,8 +3220,10 @@ void ReplicatedPG::execute_ctx(OpContext *ctx)
   }
 
   if (ctx->update_log_only) {
+    if (result >= 0)
+      do_osd_op_effects(ctx, m->get_connection());
+
     dout(20) << __func__ << " update_log_only -- result=" << result << dendl;
-    assert(result < 0);
     // save just what we need from ctx
     MOSDOpReply *reply = ctx->reply;
     ctx->reply = nullptr;
@@ -6737,9 +6739,13 @@ int ReplicatedPG::prepare_transaction(OpContext *ctx)
     return result;
   }
 
-  // read-op?  done?
+  // read-op?  write-op noop? done?
   if (ctx->op_t->empty() && !ctx->modify) {
     unstable_stats.add(ctx->delta_stats);
+    if (ctx->op->may_write() &&
+	get_osdmap()->test_flag(CEPH_OSDMAP_REQUIRE_KRAKEN)) {
+      ctx->update_log_only = true;
+    }
     return result;
   }