Merge pull request ceph#64917 from aainscow/ec_fixpack3_pr

EC fixpack 3

Author: ljflores

src/crimson/osd/recovery_backend.cc

@@ -270,8 +270,9 @@ RecoveryBackend::scan_for_backfill_primary(
 	bool added_default = false;
 	for (auto & shard: backfill_targets) {
 	  if (shard_versions.contains(shard.shard)) {
-	    version = shard_versions.at(shard.shard);
-	    version_map->emplace(object, std::make_pair(shard.shard, version));
+	    auto shard_version = shard_versions.at(shard.shard);
+	    version_map->emplace(object, std::make_pair(shard.shard,
+							shard_version));
 	  } else if (!added_default) {
 	    version_map->emplace(object, std::make_pair(shard_id_t::NO_SHARD,
 							version));

src/osd/ECBackend.cc

@@ -707,13 +707,10 @@ void ECBackend::handle_sub_read_reply(
     rop.debug_log.emplace_back(ECUtil::ERROR, op.from, complete.buffers_read);
     complete.buffers_read.erase_shard(from.shard);
     complete.processed_read_requests.erase(from.shard);
-    // If we are doing redundant reads, then we must take care that any failed
-    // reads are not replaced with a zero buffer. When fast_reads are disabled,
-    // the send_all_remaining_reads() call will replace the zeros_for_decode
-    // based on the recovery read.
-    if (rop.do_redundant_reads) {
-      rop.to_read.at(hoid).zeros_for_decode.erase(from.shard);
-    }
+    // If there was an error for non-zero data on this shard, then we must also
+    // ignore all zeros, or minimum_to_decode may conclude that it has enough
+    // shards available.
+    rop.to_read.at(hoid).zeros_for_decode.erase(from.shard);
     dout(20) << __func__ << " shard=" << from << " error=" << err << dendl;
   }
 

src/osd/ECCommon.cc

@@ -620,7 +620,7 @@ struct ClientReadCompleter final : ECCommon::ReadCompleter {
     extent_map result;
     if (res.r == 0) {
       ceph_assert(res.errors.empty());
-      dout(20) << __func__ << ": before decode: "
+      dout(30) << __func__ << ": before decode: "
                << res.buffers_read.debug_string(2048, 0)
                << dendl;
       /* Decode any missing buffers */
@@ -630,7 +630,7 @@ struct ClientReadCompleter final : ECCommon::ReadCompleter {
                                   req.object_size,
                                   read_pipeline.get_parent()->get_dpp());
       ceph_assert( r == 0 );
-      dout(20) << __func__ << ": after decode: "
+      dout(30) << __func__ << ": after decode: "
                << res.buffers_read.debug_string(2048, 0)
                << dendl;
 
@@ -1286,7 +1286,7 @@ void ECCommon::RecoveryBackend::handle_recovery_read_complete(
   op.returned_data.emplace(std::move(res.buffers_read));
   uint64_t aligned_size = ECUtil::align_next(op.obc->obs.oi.size);
 
-  dout(20) << __func__ << " before decode: oid=" << op.hoid << " EC_DEBUG_BUFFERS: "
+  dout(30) << __func__ << " before decode: oid=" << op.hoid << " EC_DEBUG_BUFFERS: "
          << op.returned_data->debug_string(2048, 0)
          << dendl;
 
@@ -1299,7 +1299,7 @@ void ECCommon::RecoveryBackend::handle_recovery_read_complete(
   op.returned_data->erase_after_ro_offset(aligned_size);
 
   dout(20) << __func__ << ": oid=" << op.hoid << dendl;
-  dout(20) << __func__ << " after decode: oid=" << op.hoid << " EC_DEBUG_BUFFERS: "
+  dout(30) << __func__ << " after decode: oid=" << op.hoid << " EC_DEBUG_BUFFERS: "
            << op.returned_data->debug_string(2048, 0)
            << dendl;
 

src/osd/ECTransaction.cc

@@ -38,7 +38,7 @@ void debug(const hobject_t &oid, const std::string &str,
            const ECUtil::shard_extent_map_t &map, DoutPrefixProvider *dpp) {
   ldpp_dout(dpp, 20)
     << " generate_transactions: " << "oid: " << oid << " " << str << " " << map << dendl;
-  ldpp_dout(dpp, 20)
+  ldpp_dout(dpp, 30)
     << "EC_DEBUG_BUFFERS: " << map.debug_string(2048, 0) << dendl;
 }
 
@@ -213,6 +213,10 @@ ECTransaction::WritePlanObj::WritePlanObj(
 
         if (pdw_write_mode != 0) {
           do_parity_delta_write = (pdw_write_mode == 2);
+        } else if (pdw_read_shards.size() >= sinfo.get_k()) {
+          // Even if recovery required for a convention RMW, PDW is not more
+          // efficient.
+          do_parity_delta_write = false;
         } else if (!shard_id_set::difference(pdw_read_shards, readable_shards).empty()) {
           // Some kind of reconstruct would be needed for PDW, so don't bother.
           do_parity_delta_write = false;
@@ -588,7 +592,7 @@ ECTransaction::Generate::Generate(PGTransaction &t,
     shard_written(shard_id_t(0));
   }
 
-  written_and_present_shards();
+  written_shards();
 
   if (!op.attr_updates.empty()) {
     attr_updates();
@@ -855,7 +859,7 @@ void ECTransaction::Generate::appends_and_clone_ranges() {
   }
 }
 
-void ECTransaction::Generate::written_and_present_shards() {
+void ECTransaction::Generate::written_shards() {
   if (entry) {
     if (!rollback_extents.empty()) {
       entry->mod_desc.rollback_extents(
@@ -869,14 +873,6 @@ void ECTransaction::Generate::written_and_present_shards() {
       entry->written_shards.clear();
       written_shards_final = true;
     }
-    // Calculate set of present shards
-    for (auto &&[shard, t]: transactions) {
-      entry->present_shards.insert(shard);
-    }
-    if (entry->present_shards.size() == sinfo.get_k_plus_m()) {
-      // More efficient to encode an empty set for all shards
-      entry->present_shards.clear();
-    }
 
     // Update shard_versions in object_info to record which shards are being
     // written
@@ -932,7 +928,6 @@ void ECTransaction::Generate::written_and_present_shards() {
       }
       ldpp_dout(dpp, 20) << __func__ << " shard_info: oid=" << oid
                          << " version=" << entry->version
-                         << " present=" << entry->present_shards
                          << " written=" << entry->written_shards
                          << " shard_versions=" << oi.shard_versions << dendl;
     }

src/osd/ECTransaction.h

@@ -106,7 +106,7 @@ class Generate {
   void truncate();
   void overlay_writes();
   void appends_and_clone_ranges();
-  void written_and_present_shards();
+  void written_shards();
   void attr_updates();
 
  public:

src/osd/ECUtil.cc

@@ -674,7 +674,11 @@ int shard_extent_map_t::decode(const ErasureCodeInterfaceRef &ec_impl,
        */
       decode_set.insert(decode_for_parity_shards);
       need_set.insert(decode_for_parity_shards);
-      extent_set decode_for_parity = get_extent_superset();
+      extent_set decode_for_parity;
+
+      for (auto shard : encode_set) {
+        decode_for_parity.insert(want.at(shard));
+      }
 
       for (auto shard : decode_for_parity_shards) {
         extent_set parity_pad;
@@ -983,25 +987,47 @@ bufferlist shard_extent_map_t::get_ro_buffer() const {
   return get_ro_buffer(ro_start, ro_end - ro_start);
 }
 
+bool get_int_from_bufferlist(bufferlist bl, int offset, uint32_t *value) {
+  *value = 0;
+
+  if (bl.length() < offset + sizeof(uint32_t)) {
+    return false;
+  }
+
+  auto bl_iter = bl.begin();
+  bl_iter += offset;
+  unsigned b = 0;
+  for (; b < sizeof(uint32_t); ++b, ++bl_iter) {
+    ceph_assert(bl_iter != bl.end());
+    *value |= (((unsigned int)bl_iter.get_current_ptr().c_str()[0]) << b);
+  }
+  return true;
+}
+
 std::string shard_extent_map_t::debug_string(uint64_t interval, uint64_t offset) const {
   std::stringstream str;
   str << "shard_extent_map_t: " << *this << " bufs: [";
 
   bool s_comma = false;
   for (auto &&[shard, emap] : get_extent_maps()) {
-    if (s_comma) str << ", ";
+    if (s_comma) {
+      str << ", ";
+    }
     s_comma = true;
     str << shard << ": [";
 
     bool comma = false;
     for (auto &&extent : emap) {
       bufferlist bl = extent.get_val();
-      char *buf = bl.c_str();
-      for (uint64_t i = 0; i < extent.get_len(); i += interval) {
-        int *seed = (int*)&buf[i + offset];
-        if (comma) str << ", ";
-        str << (i + extent.get_off()) << ":" << std::to_string(*seed);
+      uint32_t seed;
+      int i = 0;
+      while (get_int_from_bufferlist(bl, i + offset, &seed)) {
+        if (comma) {
+          str << ", ";
+        }
+        str << (i + extent.get_off()) << ":" << seed;
         comma = true;
+        i += interval;
       }
     }
     str << "]";

src/osd/PGBackend.cc

@@ -420,22 +420,27 @@ void PGBackend::partial_write(
   if (entry.written_shards.empty() && info->partial_writes_last_complete.empty()) {
     return;
   }
+  const pg_pool_t &pool = get_parent()->get_pool();
+  if (pool.is_nonprimary_shard(get_parent()->whoami_shard().shard)) {
+    // Don't update pwlc on nonprimary shards because they only
+    // observe writes that update their shard
+    return;
+  }
   auto dpp = get_parent()->get_dpp();
   ldpp_dout(dpp, 20) << __func__ << " version=" << entry.version
 		     << " written_shards=" << entry.written_shards
-		     << " present_shards=" << entry.present_shards
-		     << " pwlc=" << info->partial_writes_last_complete
+		     << " pwlc=e" << info->partial_writes_last_complete_epoch
+		     << ":" << info->partial_writes_last_complete
 		     << " previous_version=" << previous_version
 		     << dendl;
-  const pg_pool_t &pool = get_parent()->get_pool();
   for (shard_id_t shard : pool.nonprimary_shards) {
     auto pwlc_iter = info->partial_writes_last_complete.find(shard);
     if (!entry.is_written_shard(shard)) {
       if (pwlc_iter == info->partial_writes_last_complete.end()) {
 	// 1st partial write since all logs were updated
 	info->partial_writes_last_complete[shard] =
 	  std::pair(previous_version, entry.version);
-
+        info->partial_writes_last_complete_epoch = get_osdmap_epoch();
 	continue;
       }
       auto &&[old_v,  new_v] = pwlc_iter->second;
@@ -445,7 +450,7 @@ void PGBackend::partial_write(
 	  // invalid
 	  ldpp_dout(dpp, 20) << __func__ << " pwlc invalid " << shard
 			   << dendl;
-	} else if (old_v.version >= entry.version.version) {
+	} else if (old_v >= entry.version) {
 	  // Abnormal case - consider_adjusting_pwlc may advance pwlc
 	  // during peering because all shards have updates but these
 	  // have not been marked complete. At the end of peering
@@ -456,10 +461,12 @@ void PGBackend::partial_write(
 	} else {
 	  old_v = previous_version;
 	  new_v = entry.version;
+	  info->partial_writes_last_complete_epoch = get_osdmap_epoch();
 	}
       } else if (new_v == previous_version) {
 	// Subsequent partial write, contiguous versions
 	new_v = entry.version;
+	info->partial_writes_last_complete_epoch = get_osdmap_epoch();
       } else {
 	// Subsequent partial write, discontiguous versions
 	ldpp_dout(dpp, 20) << __func__ << " cannot update shard " << shard
@@ -472,17 +479,19 @@ void PGBackend::partial_write(
 	// shard is backfilling or in async recovery, pwlc is invalid
 	ldpp_dout(dpp, 20) << __func__ << " pwlc invalid " << shard
 			   << dendl;
-      } else if (old_v.version >= entry.version.version) {
+      } else if (old_v >= entry.version) {
 	// Abnormal case - see above
 	ldpp_dout(dpp, 20) << __func__ << " pwlc is ahead of entry " << shard
 			   << dendl;
       } else {
 	old_v = new_v = entry.version;
+	info->partial_writes_last_complete_epoch = get_osdmap_epoch();
       }
     }
   }
-  ldpp_dout(dpp, 20) << __func__ << " after pwlc="
-		     << info->partial_writes_last_complete << dendl;
+  ldpp_dout(dpp, 20) << __func__ << " after pwlc=e"
+		     << info->partial_writes_last_complete_epoch
+		     << ":" << info->partial_writes_last_complete << dendl;
 }
 
 void PGBackend::remove(