Merge pull request ceph#55008 from NitzanMordhai/wip-nitzan-clear-data-digest-for-crc-check

osd: full-object read CRC mismatch due to 'truncate' modifying oi.size w/o clearing 'data_digest'

Author: NitzanMordhai

src/osd/PrimaryLogPG.cc

@@ -6746,6 +6746,8 @@ int PrimaryLogPG::do_osd_ops(OpContext *ctx, vector<OSDOp>& ops)
               truncate_update_size_and_usage(ctx->delta_stats,
                                              oi,
                                              op.extent.truncate_size);
+	      //truncate modify oi.size, need clear old data_digest and DIGEST flag
+	      oi.clear_data_digest();
 	    }
 	  } else {
 	    dout(10) << " truncate_seq " << op.extent.truncate_seq << " > current " << seq
@@ -6764,10 +6766,16 @@ int PrimaryLogPG::do_osd_ops(OpContext *ctx, vector<OSDOp>& ops)
 
 	if (op.extent.length == 0) {
 	  if (op.extent.offset > oi.size) {
-	    t->truncate(
-	      soid, op.extent.offset);
-            truncate_update_size_and_usage(ctx->delta_stats, oi,
-                                           op.extent.offset);
+	    if (seq && (seq > op.extent.truncate_seq)) {
+	      //do nothing
+	      //write arrived after truncate, we should not truncate to offset
+	    } else {
+	      t->truncate(
+	        soid, op.extent.offset);
+	      truncate_update_size_and_usage(ctx->delta_stats, oi,
+	                                     op.extent.offset);
+	      oi.clear_data_digest();
+	    }
 	  } else {
 	    t->nop(soid);
 	  }

src/test/librados/io_cxx.cc

@@ -495,6 +495,18 @@ TEST_F(LibRadosIoPP, XattrListPP) {
   }
 }
 
+TEST_F(LibRadosIoPP, CrcZeroWrite) {
+  char buf[128];
+  bufferlist bl;
+
+  ASSERT_EQ(0, ioctx.write("foo", bl, 0, 0));
+  ASSERT_EQ(0, ioctx.write("foo", bl, 0, sizeof(buf)));
+
+  ObjectReadOperation read;
+  read.read(0, bl.length(), NULL, NULL);
+  ASSERT_EQ(0, ioctx.operate("foo", &read, &bl));
+}
+
 TEST_F(LibRadosIoECPP, SimpleWritePP) {
   SKIP_IF_CRIMSON();
   char buf[128];
@@ -865,6 +877,22 @@ TEST_F(LibRadosIoECPP, RmXattrPP) {
   ASSERT_EQ(-ENOENT, ioctx.rmxattr("foo_rmxattr", attr2));
 }
 
+TEST_F(LibRadosIoECPP, CrcZeroWrite) {
+  SKIP_IF_CRIMSON();
+  set_allow_ec_overwrites(pool_name, true);
+  char buf[128];
+  memset(buf, 0xcc, sizeof(buf));
+  bufferlist bl;
+
+  ASSERT_EQ(0, ioctx.write("foo", bl, 0, 0));
+  ASSERT_EQ(0, ioctx.write("foo", bl, 0, sizeof(buf)));
+
+  ObjectReadOperation read;
+  read.read(0, bl.length(), NULL, NULL);
+  ASSERT_EQ(0, ioctx.operate("foo", &read, &bl));
+  recreate_pool();
+}
+
 TEST_F(LibRadosIoECPP, XattrListPP) {
   SKIP_IF_CRIMSON();
   char buf[128];

src/test/librados/test_cxx.cc

@@ -121,6 +121,21 @@ std::string create_one_ec_pool_pp(const std::string &pool_name, Rados &cluster)
   return "";
 }
 
+std::string set_allow_ec_overwrites_pp(const std::string &pool_name, Rados &cluster, bool allow)
+{
+  std::ostringstream oss;
+  bufferlist inbl;
+  int ret = cluster.mon_command(
+    "{\"prefix\": \"osd pool set\", \"pool\": \"" + pool_name + "\", \"var\": \"allow_ec_overwrites\", \"val\": \"" + (allow ? "true" : "false") + "\"}",
+    inbl, NULL, NULL);
+  if (ret) {
+    cluster.shutdown();
+    oss << "mon_command osd pool set pool:" << pool_name << " pool_type:erasure allow_ec_overwrites true failed with error " << ret;
+    return oss.str();
+  }
+  return "";
+}
+
 std::string connect_cluster_pp(librados::Rados &cluster)
 {
   return connect_cluster_pp(cluster, {});

src/test/librados/test_cxx.h

@@ -12,6 +12,8 @@ std::string create_one_pool_pp(const std::string &pool_name,
 			       const std::map<std::string, std::string> &config);
 std::string create_one_ec_pool_pp(const std::string &pool_name,
 			    librados::Rados &cluster);
+std::string set_allow_ec_overwrites_pp(const std::string &pool_name,
+				       librados::Rados &cluster, bool allow);
 std::string connect_cluster_pp(librados::Rados &cluster);
 std::string connect_cluster_pp(librados::Rados &cluster,
 			       const std::map<std::string, std::string> &config);

src/test/librados/testcase_cxx.cc

@@ -193,7 +193,6 @@ Rados RadosTestPP::s_cluster;
 void RadosTestPP::SetUpTestCase()
 {
   init_rand();
-
   auto pool_prefix = fmt::format("{}_", ::testing::UnitTest::GetInstance()->current_test_case()->name());
   pool_name = get_temp_pool_name(pool_prefix);
   ASSERT_EQ("", create_one_pool_pp(pool_name, s_cluster));
@@ -405,3 +404,15 @@ void RadosTestECPP::TearDown()
   ioctx.close();
 }
 
+void RadosTestECPP::recreate_pool()
+{
+  SKIP_IF_CRIMSON();
+  ASSERT_EQ(0, destroy_one_ec_pool_pp(pool_name, s_cluster));
+  ASSERT_EQ("", create_one_ec_pool_pp(pool_name, s_cluster));
+  SetUp();
+}
+
+void RadosTestECPP::set_allow_ec_overwrites(std::string pool, bool allow)
+{
+  ASSERT_EQ("", set_allow_ec_overwrites_pp(pool, cluster, allow));
+}

src/test/librados/testcase_cxx.h

@@ -117,6 +117,8 @@ class RadosTestECPP : public RadosTestPP {
 protected:
   static void SetUpTestCase();
   static void TearDownTestCase();
+  void recreate_pool();
+  void set_allow_ec_overwrites(std::string pool, bool allow=true);
   static librados::Rados s_cluster;
   static std::string pool_name;
 

src/test/pybind/test_rados.py

@@ -312,8 +312,12 @@ def test_cmpext(self):
     def test_list_objects_empty(self):
         eq(list(self.ioctx.list_objects()), [])
 
-    def test_list_objects(self):
+    def test_read_crc(self):
         self.ioctx.write('a', b'')
+        self.ioctx.write('a', b'', 5)
+        self.ioctx.read('a')
+
+    def test_list_objects(self):
         self.ioctx.write('b', b'foo')
         self.ioctx.write_full('c', b'bar')
         self.ioctx.append('d', b'jazz')