Merge pull request #556 from ceph/wip-user-version

make ceph_test_rados / RadosModel validate the versions exposed by librados

Reviewed-by: Greg Farnum <greg@inktank.com>
Reviewed-by: Samuel Just <sam.just@inktank.com>

7 files changed, 60 insertions, 13 deletions

diff --git a/PendingReleaseNotes b/PendingReleaseNotes
index ccbe0596b70..6609acbd5a8 100644
--- a/PendingReleaseNotes
+++ b/PendingReleaseNotes
@@ -17,4 +17,12 @@ v0.68
   is probably how it should have been acting from the beginning. Users are
   unlikely to notice but it could result in lower performance in some
   circumstances. Those who care should switch to using the async interfaces,
-  which let you specify safety semantics precisely.
\ No newline at end of file
+  which let you specify safety semantics precisely.
+
+* The C++ librados AioComplete::get_version() method was incorrectly
+  returning an int (usually 32-bits).  To avoid breaking library
+  compatibility, a get_version64() method is added that returns the
+  full-width value.  The old method is deprecated and will be removed
+  in a future release.  Users of the C++ librados API that make use of
+  the get_version() method should modify their code to avoid getting a
+  value that is truncated from 64 to to 32 bits.
\ No newline at end of file
diff --git a/src/include/rados/librados.hpp b/src/include/rados/librados.hpp
index cf193b36d72..bc0bcc95ceb 100644
--- a/src/include/rados/librados.hpp
+++ b/src/include/rados/librados.hpp
@@ -96,7 +96,8 @@ namespace librados
     bool is_complete_and_cb();
     bool is_safe_and_cb();
     int get_return_value();
-    int get_version();
+    int get_version();  ///< DEPRECATED get_version() only returns 32-bits
+    uint64_t get_version64();
     void release();
     AioCompletionImpl *pc;
   };
diff --git a/src/librados/librados.cc b/src/librados/librados.cc
index f704412559f..12372d960b1 100644
--- a/src/librados/librados.cc
+++ b/src/librados/librados.cc
@@ -592,6 +592,12 @@ int librados::AioCompletion::AioCompletion::get_version()
   return c->get_version();
 }
 
+uint64_t librados::AioCompletion::AioCompletion::get_version64()
+{
+  AioCompletionImpl *c = (AioCompletionImpl *)pc;
+  return c->get_version();
+}
+
 void librados::AioCompletion::AioCompletion::release()
 {
   AioCompletionImpl *c = (AioCompletionImpl *)pc;
diff --git a/src/messages/MOSDOpReply.h b/src/messages/MOSDOpReply.h
index 2defeecc1e0..3005cfc3e8f 100644
--- a/src/messages/MOSDOpReply.h
+++ b/src/messages/MOSDOpReply.h
@@ -228,9 +228,10 @@ public:
       if (header.version >= 5) {
 	::decode(replay_version, p);
 	::decode(user_version, p);
-      } else
+      } else {
 	replay_version = bad_replay_version;
 	user_version = replay_version.version;
+      }
     }
   }
 
diff --git a/src/osd/ReplicatedPG.cc b/src/osd/ReplicatedPG.cc
index a04ab485e7e..86d2db51cdb 100644
--- a/src/osd/ReplicatedPG.cc
+++ b/src/osd/ReplicatedPG.cc
@@ -1024,21 +1024,22 @@ void ReplicatedPG::do_op(OpRequestRef op)
   }
   ctx->reply->set_result(result);
 
-  if (result >= 0) {
-    ctx->reply->set_reply_versions(ctx->at_version, ctx->user_at_version);
-  } else if (result == -ENOENT) {
-    ctx->reply->set_enoent_reply_versions(info.last_update, ctx->user_at_version);
-  }
-
   // read or error?
   if (ctx->op_t.empty() || result < 0) {
+    MOSDOpReply *reply = ctx->reply;
+    ctx->reply = NULL;
+
     if (result >= 0) {
       log_op_stats(ctx);
       publish_stats_to_osd();
+
+      // on read, return the current object version
+      reply->set_reply_versions(eversion_t(), ctx->obs->oi.user_version);
+    } else if (result == -ENOENT) {
+      // on ENOENT, set a floor for what the next user version will be. 
+      reply->set_enoent_reply_versions(info.last_update, ctx->user_at_version);
     }
     
-    MOSDOpReply *reply = ctx->reply;
-    ctx->reply = NULL;
     reply->add_flags(CEPH_OSD_FLAG_ACK | CEPH_OSD_FLAG_ONDISK);
     osd->send_message_osd_client(reply, m->get_connection());
     delete ctx;
@@ -1046,6 +1047,8 @@ void ReplicatedPG::do_op(OpRequestRef op)
     return;
   }
 
+  ctx->reply->set_reply_versions(ctx->at_version, ctx->user_at_version);
+
   assert(op->may_write());
 
   // trim log?
diff --git a/src/test/osd/Object.h b/src/test/osd/Object.h
index 39acf1e2175..09f0a5f2e4c 100644
--- a/src/test/osd/Object.h
+++ b/src/test/osd/Object.h
@@ -240,9 +240,9 @@ public:
 class ObjectDesc {
 public:
   ObjectDesc(ContentsGenerator *cont_gen) : 
-    exists(false), tmap(false), layers(), cont_gen(cont_gen) {};
+    exists(false), tmap(false), version(0), layers(), cont_gen(cont_gen) {};
   ObjectDesc(const ContDesc &init, ContentsGenerator *cont_gen) : 
-    exists(false), tmap(false), layers(), cont_gen(cont_gen) {
+    exists(false), tmap(false), version(0), layers(), cont_gen(cont_gen) {
     layers.push_front(init);
   };
 
@@ -314,6 +314,7 @@ public:
   bool exists;
   bool tmap;
   bufferlist tmap_contents;
+  uint64_t version;
 private:
   list<ContDesc> layers;
   ContentsGenerator *cont_gen;
diff --git a/src/test/osd/RadosModel.h b/src/test/osd/RadosModel.h
index b022d24dc91..b2325341ca0 100644
--- a/src/test/osd/RadosModel.h
+++ b/src/test/osd/RadosModel.h
@@ -396,6 +396,21 @@ public:
     pool_obj_cont[current_snap].insert(pair<string,ObjectDesc>(oid, new_obj));
   }
 
+  void update_object_version(const string &oid, uint64_t version)
+  {
+    for (map<int, map<string,ObjectDesc> >::reverse_iterator i = 
+	   pool_obj_cont.rbegin();
+	 i != pool_obj_cont.rend();
+	 ++i) {
+      map<string,ObjectDesc>::iterator j = i->second.find(oid);
+      if (j != i->second.end()) {
+	j->second.version = version;
+	cout << __func__ << " oid " << oid << " is version " << version << std::endl;
+	break;
+      }
+    }
+  }
+
   void remove_object(const string &oid)
   {
     assert(!get_watch_context(oid));
@@ -624,6 +639,7 @@ public:
       assert(0);
     }
     done = true;
+    context->update_object_version(oid, comp->get_version());
     context->oid_in_use.erase(oid);
     context->oid_not_in_use.insert(oid);
     context->kick();
@@ -714,6 +730,7 @@ public:
       assert(0);
     }
     done = true;
+    context->update_object_version(oid, comp->get_version());
     context->oid_in_use.erase(oid);
     context->oid_not_in_use.insert(oid);
     context->kick();
@@ -825,6 +842,7 @@ public:
     assert(!done);
     waiting_on--;
     if (waiting_on == 0) {
+      uint64_t version = 0;
       for (set<librados::AioCompletion *>::iterator i = waiting.begin();
 	   i != waiting.end();
 	   ) {
@@ -833,10 +851,13 @@ public:
 	  cerr << "Error: oid " << oid << " write returned error code "
 	       << err << std::endl;
 	}
+	if ((*i)->get_version64() > version)
+	  version = (*i)->get_version64();
 	(*i)->release();
 	waiting.erase(i++);
       }
       
+      context->update_object_version(oid, version);
       context->oid_in_use.erase(oid);
       context->oid_not_in_use.insert(oid);
       context->kick();
@@ -1027,6 +1048,7 @@ public:
     context->oid_in_use.erase(oid);
     context->oid_not_in_use.insert(oid);
     assert(completion->is_complete());
+    uint64_t version = completion->get_version64();
     if (int err = completion->get_return_value()) {
       if (!(err == -ENOENT && old_value.deleted())) {
 	cerr << "Error: oid " << oid << " read returned error code "
@@ -1075,6 +1097,11 @@ public:
 	     << " and old is " << old_value.attrs.size() << std::endl;
 	assert(xattrs.size() == old_value.attrs.size());
       }
+      if (version != old_value.version) {
+	cerr << "oid: " << oid << " version is " << version
+	     << " and expected " << old_value.version << std::endl;
+	assert(version == old_value.version);
+      }
       for (map<string, bufferlist>::iterator omap_iter = omap.begin();
 	   omap_iter != omap.end();
 	   ++omap_iter) {