From: Adam Emerson <aemerson@redhat.com>
Date: Wed, 9 Aug 2023 00:38:24 +0000 (-0400)
Subject: neorados: Fix `Op::cmpext` error and unfound behavior
X-Git-Tag: v19.3.0~349^2~20
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=826e18552412a99550d8186d3cc763e1e60ef645;p=ceph.git

neorados: Fix `Op::cmpext` error and unfound behavior

A failure will produce the output `osd_errc::cmpext_mismatch`. On
failure, the `unmatch` out parameter, if provided, will be set to the
index of the first nonmatching character; On success, it will be set
to `-1`.

Signed-off-by: Adam Emerson <aemerson@redhat.com>
---

diff --git a/src/include/neorados/RADOS.hpp b/src/include/neorados/RADOS.hpp
index 67a01b43148..e890d7a9361 100644
--- a/src/include/neorados/RADOS.hpp
+++ b/src/include/neorados/RADOS.hpp
@@ -302,7 +302,8 @@ public:
   void set_fadvise_dontneed();
   void set_fadvise_nocache();
 
-  void cmpext(uint64_t off, ceph::buffer::list&& cmp_bl, std::size_t* s);
+  void cmpext(uint64_t off, ceph::buffer::list&& cmp_bl,
+	      uint64_t* unmatch = nullptr);
   void cmpxattr(std::string_view name, cmpxattr_op op,
 		const ceph::buffer::list& val);
   void cmpxattr(std::string_view name, cmpxattr_op op, std::uint64_t val);
@@ -536,12 +537,14 @@ public:
     return std::move(*this);
   }
 
-  ReadOp& cmpext(uint64_t off, ceph::buffer::list&& cmp_bl, std::size_t* s) & {
-    Op::cmpext(off, std::move(cmp_bl), s);
+  ReadOp& cmpext(uint64_t off, ceph::buffer::list&& cmp_bl,
+		 uint64_t* unmatch = nullptr) & {
+    Op::cmpext(off, std::move(cmp_bl), unmatch);
     return *this;
   }
-  ReadOp&& cmpext(uint64_t off, ceph::buffer::list&& cmp_bl, std::size_t* s) && {
-    Op::cmpext(off, std::move(cmp_bl), s);
+  ReadOp&& cmpext(uint64_t off, ceph::buffer::list&& cmp_bl,
+		  uint64_t* unmatch = nullptr) && {
+    Op::cmpext(off, std::move(cmp_bl), unmatch);
     return std::move(*this);
   }
 
@@ -918,12 +921,14 @@ public:
     return std::move(*this);
   }
 
-  WriteOp& cmpext(uint64_t off, ceph::buffer::list&& cmp_bl, std::size_t* s) & {
-    Op::cmpext(off, std::move(cmp_bl), s);
+  WriteOp& cmpext(uint64_t off, ceph::buffer::list&& cmp_bl,
+		  uint64_t* unmatch = nullptr) & {
+    Op::cmpext(off, std::move(cmp_bl), unmatch);
     return *this;
   }
-  WriteOp&& cmpext(uint64_t off, ceph::buffer::list&& cmp_bl, std::size_t* s) && {
-    Op::cmpext(off, std::move(cmp_bl), s);
+  WriteOp&& cmpext(uint64_t off, ceph::buffer::list&& cmp_bl,
+		   uint64_t* unmatch = nullptr) && {
+    Op::cmpext(off, std::move(cmp_bl), unmatch);
     return std::move(*this);
   }
 
diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc
index 86650869616..428f0fbdf58 100644
--- a/src/librbd/io/ObjectRequest.cc
+++ b/src/librbd/io/ObjectRequest.cc
@@ -698,7 +698,7 @@ void ObjectWriteSameRequest<I>::add_write_ops(neorados::WriteOp* wr) {
 
 template <typename I>
 void ObjectCompareAndWriteRequest<I>::add_write_ops(neorados::WriteOp* wr) {
-  wr->cmpext(this->m_object_off, bufferlist{m_cmp_bl}, nullptr);
+  wr->cmpext(this->m_object_off, bufferlist{m_cmp_bl}, &m_mismatch_object_offset);
 
   if (this->m_full_object) {
     wr->write_full(bufferlist{m_write_bl});
@@ -710,13 +710,14 @@ void ObjectCompareAndWriteRequest<I>::add_write_ops(neorados::WriteOp* wr) {
 
 template <typename I>
 int ObjectCompareAndWriteRequest<I>::filter_write_result(int r) const {
-  if (r <= -MAX_ERRNO) {
+  // Error code value for cmpext mismatch. Works for both neorados and
+  // mock image, which seems to be short-circuiting on nonexistence.
+  if (r == -MAX_ERRNO) {
     I *image_ctx = this->m_ictx;
 
     // object extent compare mismatch
-    uint64_t offset = -MAX_ERRNO - r;
     auto [image_extents, _] = io::util::object_to_area_extents(
-        image_ctx, this->m_object_no, {{offset, this->m_object_len}});
+        image_ctx, this->m_object_no, {{m_mismatch_object_offset, this->m_object_len}});
     ceph_assert(image_extents.size() == 1);
 
     if (m_mismatch_offset) {
diff --git a/src/librbd/io/ObjectRequest.h b/src/librbd/io/ObjectRequest.h
index caf644023be..d9815860f78 100644
--- a/src/librbd/io/ObjectRequest.h
+++ b/src/librbd/io/ObjectRequest.h
@@ -440,6 +440,7 @@ private:
   ceph::bufferlist m_cmp_bl;
   ceph::bufferlist m_write_bl;
   uint64_t *m_mismatch_offset;
+  uint64_t m_mismatch_object_offset;
   int m_op_flags;
 };
 
diff --git a/src/neorados/RADOS.cc b/src/neorados/RADOS.cc
index 026f77043dc..47b1c0d4882 100644
--- a/src/neorados/RADOS.cc
+++ b/src/neorados/RADOS.cc
@@ -420,9 +420,9 @@ void Op::set_fadvise_nocache() {
     CEPH_OSD_OP_FLAG_FADVISE_NOCACHE);
 }
 
-void Op::cmpext(uint64_t off, bufferlist&& cmp_bl, std::size_t* s) {
+void Op::cmpext(uint64_t off, bufferlist&& cmp_bl, uint64_t* unmatch) {
   reinterpret_cast<OpImpl*>(&impl)->op.cmpext(off, std::move(cmp_bl), nullptr,
-					      s);
+					      unmatch);
 }
 void Op::cmpxattr(std::string_view name, cmpxattr_op op, const bufferlist& val) {
   reinterpret_cast<OpImpl*>(&impl)->
diff --git a/src/osd/error_code.cc b/src/osd/error_code.cc
index 97f0012fdba..927e4592b12 100644
--- a/src/osd/error_code.cc
+++ b/src/osd/error_code.cc
@@ -53,6 +53,8 @@ const char* osd_error_category::message(int ev, char* buf,
     return "ORDERSNAP flag set; writer has old snapc";
   case osd_errc::blocklisted:
     return "Blocklisted";
+  case osd_errc::cmpext_mismatch:
+    return "CmpExt mismatch";
   }
 
   if (len) {
@@ -72,6 +74,8 @@ std::string osd_error_category::message(int ev) const {
     return "ORDERSNAP flag set; writer has old snapc";
   case osd_errc::blocklisted:
     return "Blocklisted";
+  case osd_errc::cmpext_mismatch:
+    return "CmpExt mismatch";
   }
 
   return cpp_strerror(ev);
@@ -79,7 +83,8 @@ std::string osd_error_category::message(int ev) const {
 
 boost::system::error_condition osd_error_category::default_error_condition(int ev) const noexcept {
   if (ev == static_cast<int>(osd_errc::old_snapc) ||
-      ev == static_cast<int>(osd_errc::blocklisted))
+      ev == static_cast<int>(osd_errc::blocklisted) ||
+      ev == static_cast<int>(osd_errc::cmpext_mismatch))
     return { ev, *this };
   else
     return { ev, boost::system::generic_category() };
@@ -91,6 +96,8 @@ bool osd_error_category::equivalent(int ev, const boost::system::error_condition
       return c == boost::system::errc::invalid_argument;
   case osd_errc::blocklisted:
       return c == boost::system::errc::operation_not_permitted;
+  case osd_errc::cmpext_mismatch:
+      return c == boost::system::errc::operation_canceled;
   }
   return default_error_condition(ev) == c;
 }
diff --git a/src/osd/error_code.h b/src/osd/error_code.h
index d36e79db4a8..762e03a4643 100644
--- a/src/osd/error_code.h
+++ b/src/osd/error_code.h
@@ -19,6 +19,8 @@
 
 #include "include/rados.h"
 
+#include "include/err.h"
+
 const boost::system::error_category& osd_category() noexcept;
 
 // Since the OSD mostly uses POSIX error codes plus a couple
@@ -27,7 +29,8 @@ const boost::system::error_category& osd_category() noexcept;
 
 enum class osd_errc {
   old_snapc = 85,  /* ORDERSNAP flag set; writer has old snapc*/
-  blocklisted = 108 /* blocklisted */
+  blocklisted = 108, /* blocklisted */
+  cmpext_mismatch = MAX_ERRNO /* cmpext failed */
 };
 
 namespace boost::system {
diff --git a/src/osdc/Objecter.cc b/src/osdc/Objecter.cc
index a74d21b0f15..9c2daed3ba6 100644
--- a/src/osdc/Objecter.cc
+++ b/src/osdc/Objecter.cc
@@ -3586,7 +3586,7 @@ void Objecter::handle_osd_op_reply(MOSDOpReply *m)
 	if (*pe) {
 	  **pe = e.code();
 	}
-	if (*pr) {
+	if (*pr && **pr == 0) {
 	  **pr = ceph::from_error_code(e.code());
 	}
       } catch (const std::exception& e) {
@@ -3596,7 +3596,7 @@ void Objecter::handle_osd_op_reply(MOSDOpReply *m)
 	if (*pe) {
 	  **pe = osdc_errc::handler_failed;
 	}
-	if (*pr) {
+	if (*pr && **pr == 0) {
 	  **pr = -EIO;
 	}
       }
@@ -3633,6 +3633,8 @@ void Objecter::handle_osd_op_reply(MOSDOpReply *m)
   if (Op::has_completion(onfinish)) {
     if (rc == 0 && handler_error) {
       Op::complete(std::move(onfinish), handler_error, -EIO, service.get_executor());
+    } else if (handler_error) {
+      Op::complete(std::move(onfinish), handler_error, rc, service.get_executor());
     } else {
       Op::complete(std::move(onfinish), osdcode(rc), rc, service.get_executor());
     }
diff --git a/src/osdc/Objecter.h b/src/osdc/Objecter.h
index 870083a29b6..512a4435ecc 100644
--- a/src/osdc/Objecter.h
+++ b/src/osdc/Objecter.h
@@ -64,6 +64,7 @@
 #include "msg/Dispatcher.h"
 
 #include "osd/OSDMap.h"
+#include "osd/error_code.h"
 
 class Context;
 class Messenger;
@@ -434,19 +435,41 @@ struct ObjectOperation {
   struct CB_ObjectOperation_cmpext {
     int* prval = nullptr;
     boost::system::error_code* ec = nullptr;
-    std::size_t* s = nullptr;
+    uint64_t* mismatch_offset = nullptr;
     explicit CB_ObjectOperation_cmpext(int *prval)
       : prval(prval) {}
-    CB_ObjectOperation_cmpext(boost::system::error_code* ec, std::size_t* s)
-      : ec(ec), s(s) {}
+    CB_ObjectOperation_cmpext(boost::system::error_code* ec,
+			      uint64_t* mismatch_offset)
+      : ec(ec), mismatch_offset(mismatch_offset) {}
 
-    void operator()(boost::system::error_code ec, int r, const ceph::buffer::list&) {
+    void operator()(boost::system::error_code ec, int r,
+		    const ceph::buffer::list&) {
       if (prval)
         *prval = r;
-      if (this->ec)
-	*this->ec = ec;
-      if (s)
-	*s = static_cast<std::size_t>(-(MAX_ERRNO - r));
+
+      if (r <= -MAX_ERRNO) {
+	if (this->ec) {
+	  *this->ec = make_error_code(osd_errc::cmpext_mismatch);
+	}
+	if (mismatch_offset) {
+	  *mismatch_offset = -MAX_ERRNO - r;
+	}
+	throw boost::system::system_error(osd_errc::cmpext_mismatch);
+      } else if (r < 0) {
+	if (this->ec) {
+	  *this->ec = ec;
+	}
+	if (mismatch_offset) {
+	  *mismatch_offset = -1;
+	}
+      } else {
+	if (this->ec) {
+	  this->ec->clear();
+	}
+	if (mismatch_offset) {
+	  *mismatch_offset = -1;
+	}
+      }
     }
   };
 
@@ -457,9 +480,9 @@ struct ObjectOperation {
   }
 
   void cmpext(uint64_t off, ceph::buffer::list&& cmp_bl, boost::system::error_code* ec,
-	      std::size_t* s) {
+	      uint64_t* mismatch_offset) {
     add_data(CEPH_OSD_OP_CMPEXT, off, cmp_bl.length(), cmp_bl);
-    set_handler(CB_ObjectOperation_cmpext(ec, s));
+    set_handler(CB_ObjectOperation_cmpext(ec, mismatch_offset));
     out_ec.back() = ec;
   }
 
diff --git a/src/test/librados_test_stub/NeoradosTestStub.cc b/src/test/librados_test_stub/NeoradosTestStub.cc
index 8e2a864f9b8..1eb0d627ebe 100644
--- a/src/test/librados_test_stub/NeoradosTestStub.cc
+++ b/src/test/librados_test_stub/NeoradosTestStub.cc
@@ -113,11 +113,19 @@ librados::AioCompletionImpl* create_aio_completion(
   return impl;
 }
 
-int save_operation_size(int result, size_t* pval) {
-  if (pval != NULL) {
-    *pval = result;
+int save_operation_size(int result, uint64_t* pval) {
+  int our_r = result;
+  if (result <= -MAX_ERRNO) {
+    if (pval != NULL) {
+      *pval = -MAX_ERRNO - result;
+    }
+    our_r = -MAX_ERRNO;
+  } else {
+    if (pval != NULL) {
+      *pval = -1;
+    }
   }
-  return result;
+  return our_r;
 }
 
 int save_operation_ec(int result, boost::system::error_code* ec) {
@@ -323,7 +331,7 @@ void Op::assert_version(uint64_t ver) {
           &librados::TestIoCtxImpl::assert_version, _1, _2, ver));
 }
 
-void Op::cmpext(uint64_t off, ceph::buffer::list&& cmp_bl, std::size_t* s) {
+void Op::cmpext(uint64_t off, ceph::buffer::list&& cmp_bl, uint64_t* s) {
   auto o = *reinterpret_cast<librados::TestObjectOperationImpl**>(&impl);
   librados::ObjectOperationTestImpl op = std::bind(
     &librados::TestIoCtxImpl::cmpext, _1, _2, off, cmp_bl, _4);