Merge pull request ceph#55196 from rzarzynski/wip-osd-ec-partial-reads

osd: EC Partial Stripe Reads (Retry of ceph#23138 and ceph#52746)

Reviewed-by: Samuel Just <[email protected]>

Author: rzarzynski

qa/standalone/erasure-code/test-erasure-eio.sh

@@ -178,9 +178,19 @@ function rados_put_get_data() {
         wait_for_clean || return 1
         # Won't check for eio on get here -- recovery above might have fixed it
     else
-        shard_id=$(expr $shard_id + 1)
-        inject_$inject ec data $poolname $objname $dir $shard_id || return 1
-        rados_get $dir $poolname $objname fail || return 1
+        local another_shard_id=$(expr $shard_id + 1)
+        inject_$inject ec data $poolname $objname $dir $another_shard_id || return 1
+        if [ $shard_id -eq 1 -a $another_shard_id -eq 2 ];
+        then
+            # we're reading 4 kb long object while the stripe size is 8 kb.
+            # as we do partial reads and this request can be satisfied
+            # from the undamaged shard 0, we expect a success.
+            rados_get $dir $poolname $objname || return 1
+        else
+            # both shards 0 and 1 are demaged. there is no way no serve
+            # the requests, regardless of partial reads
+            rados_get $dir $poolname $objname fail || return 1
+        fi
         rm $dir/ORIGINAL
     fi
 
@@ -238,9 +248,19 @@ function rados_get_data_bad_size() {
     rados_get $dir $poolname $objname || return 1
 
     # Leave objname and modify another shard
-    shard_id=$(expr $shard_id + 1)
-    set_size $objname $dir $shard_id $bytes $mode || return 1
-    rados_get $dir $poolname $objname fail || return 1
+    local another_shard_id=$(expr $shard_id + 1)
+    set_size $objname $dir $another_shard_id $bytes $mode || return 1
+    if [ $shard_id -eq 1 -a $another_shard_id -eq 2 ];
+    then
+	# we're reading 4 kb long object while the stripe size is 8 kb.
+	# as we do partial reads and this request can be satisfied
+	# from the undamaged shard 0, we expect a success.
+	rados_get $dir $poolname $objname || return 1
+    else
+	# both shards 0 and 1 are demaged. there is no way no serve
+	# the requests, regardless of partial reads
+	rados_get $dir $poolname $objname fail || return 1
+    fi
     rm $dir/ORIGINAL
 }
 

src/common/options/osd.yaml.in

@@ -1234,6 +1234,11 @@ options:
   level: advanced
   default: false
   with_legacy: true
+- name: osd_ec_partial_reads
+  type: bool
+  level: advanced
+  default: true
+  with_legacy: true
 - name: osd_recovery_delay_start
   type: float
   level: advanced

src/erasure-code/ErasureCode.cc

@@ -348,21 +348,35 @@ int ErasureCode::to_string(const std::string &name,
   return 0;
 }
 
-int ErasureCode::decode_concat(const map<int, bufferlist> &chunks,
+int ErasureCode::decode_concat(const set<int>& want_to_read,
+			       const map<int, bufferlist> &chunks,
 			       bufferlist *decoded)
 {
-  set<int> want_to_read;
-
-  for (unsigned int i = 0; i < get_data_chunk_count(); i++) {
-    want_to_read.insert(chunk_index(i));
-  }
   map<int, bufferlist> decoded_map;
   int r = _decode(want_to_read, chunks, &decoded_map);
   if (r == 0) {
     for (unsigned int i = 0; i < get_data_chunk_count(); i++) {
-      decoded->claim_append(decoded_map[chunk_index(i)]);
+      // XXX: the ErasureCodeInterface allows `decode()` to return
+      // *at least* `want_to_read chunks`; that is, they may more.
+      // Some implementations are consistently exact but jerasure
+      // is quirky: it outputs more only when deailing with degraded.
+      // The check below uniforms the behavior.
+      if (want_to_read.contains(chunk_index(i)) &&
+	  decoded_map.contains(chunk_index(i))) {
+        decoded->claim_append(decoded_map[chunk_index(i)]);
+      }
     }
   }
   return r;
 }
+
+int ErasureCode::decode_concat(const map<int, bufferlist> &chunks,
+			       bufferlist *decoded)
+{
+  set<int> want_to_read;
+  for (unsigned int i = 0; i < get_data_chunk_count(); i++) {
+    want_to_read.insert(chunk_index(i));
+  }
+  return decode_concat(want_to_read, chunks, decoded);
+}
 }

src/erasure-code/ErasureCode.h

@@ -112,8 +112,11 @@ namespace ceph {
 			 const std::string &default_value,
 			 std::ostream *ss);
 
+    int decode_concat(const std::set<int>& want_to_read,
+		      const std::map<int, bufferlist> &chunks,
+		      bufferlist *decoded) override;
     int decode_concat(const std::map<int, bufferlist> &chunks,
-			      bufferlist *decoded) override;
+		      bufferlist *decoded) override;
 
   protected:
     int parse(const ErasureCodeProfile &profile,

src/erasure-code/ErasureCodeInterface.h

@@ -453,12 +453,20 @@ namespace ceph {
      *
      * Returns 0 on success.
      *
-     * @param [in] chunks map chunk indexes to chunk data
-     * @param [out] decoded concatenante of the data chunks
+     * @param [in] want_to_read mapped std::set of chunks caller wants
+     *				concatenated to `decoded`. This works as
+     *				selectors for `chunks`
+     * @param [in] chunks set of chunks with data available for decoding
+     * @param [out] decoded must be non-null, chunks specified in `want_to_read`
+     * 			    will be concatenated into `decoded` in index order
      * @return **0** on success or a negative errno on error.
      */
+    virtual int decode_concat(const std::set<int>& want_to_read,
+			      const std::map<int, bufferlist> &chunks,
+			      bufferlist *decoded) = 0;
     virtual int decode_concat(const std::map<int, bufferlist> &chunks,
 			      bufferlist *decoded) = 0;
+
   };
 
   typedef std::shared_ptr<ErasureCodeInterface> ErasureCodeInterfaceRef;

src/erasure-code/clay/ErasureCodeClay.cc

@@ -306,7 +306,14 @@ int ErasureCodeClay::is_repair(const set<int> &want_to_read,
 
   if (includes(available_chunks.begin(), available_chunks.end(),
                want_to_read.begin(), want_to_read.end())) return 0;
+  // Oops, before the attempt to EC partial reads the fellowing
+  // condition was always true as `get_want_to_read_shards()` yields
+  // entire stripe. Unfortunately, we built upon this assumption and
+  // even `ECUtil::decode()` asserts on chunks being multiply of
+  // `chunk_size`.
+  // XXX: for now returning 0 and knocking the optimization out.
   if (want_to_read.size() > 1) return 0;
+  else return 0;
 
   int i = *want_to_read.begin();
   int lost_node_id = (i < k) ? i: i+nu;

src/osd/ECBackend.cc

@@ -195,8 +195,8 @@ struct RecoveryMessages {
     const map<pg_shard_t, vector<pair<int, int>>> &need,
     bool attrs)
   {
-    list<boost::tuple<uint64_t, uint64_t, uint32_t> > to_read;
-    to_read.push_back(boost::make_tuple(off, len, 0));
+    list<ECCommon::ec_align_t> to_read;
+    to_read.emplace_back(ECCommon::ec_align_t{off, len, 0});
     ceph_assert(!recovery_reads.count(hoid));
     want_to_read.insert(make_pair(hoid, std::move(_want_to_read)));
     recovery_reads.insert(
@@ -463,7 +463,8 @@ struct RecoveryReadCompleter : ECCommon::ReadCompleter {
   void finish_single_request(
     const hobject_t &hoid,
     ECCommon::read_result_t &res,
-    list<boost::tuple<uint64_t, uint64_t, uint32_t> >) override
+    list<ECCommon::ec_align_t>,
+    set<int> wanted_to_read) override
   {
     if (!(res.r == 0 && res.errors.empty())) {
       backend._failed_push(hoid, res);
@@ -1028,17 +1029,19 @@ void ECBackend::handle_sub_read(
       if ((op.subchunks.find(i->first)->second.size() == 1) && 
           (op.subchunks.find(i->first)->second.front().second == 
                                             ec_impl->get_sub_chunk_count())) {
-        dout(25) << __func__ << " case1: reading the complete chunk/shard." << dendl;
+        dout(20) << __func__ << " case1: reading the complete chunk/shard." << dendl;
         r = store->read(
 	  ch,
 	  ghobject_t(i->first, ghobject_t::NO_GEN, shard),
 	  j->get<0>(),
 	  j->get<1>(),
 	  bl, j->get<2>()); // Allow EIO return
       } else {
-        dout(25) << __func__ << " case2: going to do fragmented read." << dendl;
         int subchunk_size =
           sinfo.get_chunk_size() / ec_impl->get_sub_chunk_count();
+        dout(20) << __func__ << " case2: going to do fragmented read;"
+		 << " subchunk_size=" << subchunk_size
+		 << " chunk_size=" << sinfo.get_chunk_size() << dendl;
         bool error = false;
         for (int m = 0; m < (int)j->get<1>() && !error;
              m += sinfo.get_chunk_size()) {
@@ -1214,7 +1217,7 @@ void ECBackend::handle_sub_read_reply(
       dout(20) << __func__ << " to_read skipping" << dendl;
       continue;
     }
-    list<boost::tuple<uint64_t, uint64_t, uint32_t> >::const_iterator req_iter =
+    list<ec_align_t>::const_iterator req_iter =
       rop.to_read.find(i->first)->second.to_read.begin();
     list<
       boost::tuple<
@@ -1225,10 +1228,10 @@ void ECBackend::handle_sub_read_reply(
 	 ++j, ++req_iter, ++riter) {
       ceph_assert(req_iter != rop.to_read.find(i->first)->second.to_read.end());
       ceph_assert(riter != rop.complete[i->first].returned.end());
-      pair<uint64_t, uint64_t> adjusted =
-	sinfo.aligned_offset_len_to_chunk(
-	  make_pair(req_iter->get<0>(), req_iter->get<1>()));
-      ceph_assert(adjusted.first == j->first);
+      pair<uint64_t, uint64_t> aligned =
+	sinfo.chunk_aligned_offset_len_to_chunk(
+	  make_pair(req_iter->offset, req_iter->size));
+      ceph_assert(aligned.first == j->first);
       riter->get<2>()[from] = std::move(j->second);
     }
   }
@@ -1529,62 +1532,70 @@ int ECBackend::objects_read_sync(
   return -EOPNOTSUPP;
 }
 
+static bool should_partial_read(
+  const ECUtil::stripe_info_t& sinfo,
+  uint64_t off,
+  uint32_t len,
+  bool fast_read)
+{
+  // Don't partial read if we are doing a fast_read
+  if (fast_read) {
+    return false;
+  }
+  // Same stripe only
+  return sinfo.offset_length_is_same_stripe(off, len);
+}
+
 void ECBackend::objects_read_async(
   const hobject_t &hoid,
-  const list<pair<boost::tuple<uint64_t, uint64_t, uint32_t>,
-             pair<bufferlist*, Context*> > > &to_read,
+  const list<pair<ECCommon::ec_align_t,
+                  pair<bufferlist*, Context*>>> &to_read,
   Context *on_complete,
   bool fast_read)
 {
-  map<hobject_t,std::list<boost::tuple<uint64_t, uint64_t, uint32_t> > >
-    reads;
+  map<hobject_t,std::list<ec_align_t>> reads;
 
   uint32_t flags = 0;
   extent_set es;
-  for (list<pair<boost::tuple<uint64_t, uint64_t, uint32_t>,
-	 pair<bufferlist*, Context*> > >::const_iterator i =
-	 to_read.begin();
-       i != to_read.end();
-       ++i) {
-    pair<uint64_t, uint64_t> tmp =
-      sinfo.offset_len_to_stripe_bounds(
-	make_pair(i->first.get<0>(), i->first.get<1>()));
-
+  for (const auto& [read, ctx] : to_read) {
+    pair<uint64_t, uint64_t> tmp;
+    if (!cct->_conf->osd_ec_partial_reads ||
+	!should_partial_read(sinfo, read.offset, read.size, fast_read)) {
+      tmp = sinfo.offset_len_to_stripe_bounds(make_pair(read.offset, read.size));
+    } else {
+      tmp = sinfo.offset_len_to_chunk_bounds(make_pair(read.offset, read.size));
+    }
     es.union_insert(tmp.first, tmp.second);
-    flags |= i->first.get<2>();
+    flags |= read.flags;
   }
 
   if (!es.empty()) {
     auto &offsets = reads[hoid];
     for (auto j = es.begin();
 	 j != es.end();
 	 ++j) {
-      offsets.push_back(
-	boost::make_tuple(
-	  j.get_start(),
-	  j.get_len(),
-	  flags));
+      offsets.emplace_back(ec_align_t{j.get_start(), j.get_len(), flags});
     }
   }
 
   struct cb {
     ECBackend *ec;
     hobject_t hoid;
-    list<pair<boost::tuple<uint64_t, uint64_t, uint32_t>,
+    list<pair<ECCommon::ec_align_t,
 	      pair<bufferlist*, Context*> > > to_read;
     unique_ptr<Context> on_complete;
     cb(const cb&) = delete;
     cb(cb &&) = default;
     cb(ECBackend *ec,
        const hobject_t &hoid,
-       const list<pair<boost::tuple<uint64_t, uint64_t, uint32_t>,
+       const list<pair<ECCommon::ec_align_t,
                   pair<bufferlist*, Context*> > > &to_read,
        Context *on_complete)
       : ec(ec),
 	hoid(hoid),
 	to_read(to_read),
 	on_complete(on_complete) {}
-    void operator()(map<hobject_t,pair<int, extent_map> > &&results) {
+    void operator()(ECCommon::ec_extents_t &&results) {
       auto dpp = ec->get_parent()->get_dpp();
       ldpp_dout(dpp, 20) << "objects_read_async_cb: got: " << results
 			 << dendl;
@@ -1595,24 +1606,24 @@ void ECBackend::objects_read_async(
 
       int r = 0;
       for (auto &&read: to_read) {
-	if (got.first < 0) {
+	if (got.err < 0) {
 	  // error handling
 	  if (read.second.second) {
-	    read.second.second->complete(got.first);
+	    read.second.second->complete(got.err);
 	  }
 	  if (r == 0)
-	    r = got.first;
+	    r = got.err;
 	} else {
 	  ceph_assert(read.second.first);
-	  uint64_t offset = read.first.get<0>();
-	  uint64_t length = read.first.get<1>();
-	  auto range = got.second.get_containing_range(offset, length);
+	  uint64_t offset = read.first.offset;
+	  uint64_t length = read.first.size;
+	  auto range = got.emap.get_containing_range(offset, length);
 	  ceph_assert(range.first != range.second);
 	  ceph_assert(range.first.get_off() <= offset);
-          ldpp_dout(dpp, 30) << "offset: " << offset << dendl;
-          ldpp_dout(dpp, 30) << "range offset: " << range.first.get_off() << dendl;
-          ldpp_dout(dpp, 30) << "length: " << length << dendl;
-          ldpp_dout(dpp, 30) << "range length: " << range.first.get_len()  << dendl;
+          ldpp_dout(dpp, 20) << "offset: " << offset << dendl;
+          ldpp_dout(dpp, 20) << "range offset: " << range.first.get_off() << dendl;
+          ldpp_dout(dpp, 20) << "length: " << length << dendl;
+          ldpp_dout(dpp, 20) << "range length: " << range.first.get_len()  << dendl;
 	  ceph_assert(
 	    (offset + length) <=
 	    (range.first.get_off() + range.first.get_len()));
@@ -1642,7 +1653,7 @@ void ECBackend::objects_read_async(
     reads,
     fast_read,
     make_gen_lambda_context<
-      map<hobject_t,pair<int, extent_map> > &&, cb>(
+      ECCommon::ec_extents_t &&, cb>(
 	cb(this,
 	   hoid,
 	   to_read,
@@ -1651,10 +1662,10 @@ void ECBackend::objects_read_async(
 
 void ECBackend::objects_read_and_reconstruct(
   const map<hobject_t,
-    std::list<boost::tuple<uint64_t, uint64_t, uint32_t> >
+    std::list<ECBackend::ec_align_t>
   > &reads,
   bool fast_read,
-  GenContextURef<map<hobject_t,pair<int, extent_map> > &&> &&func)
+  GenContextURef<ECCommon::ec_extents_t &&> &&func)
 {
   return read_pipeline.objects_read_and_reconstruct(
     reads, fast_read, std::move(func));

src/osd/ECBackend.h

@@ -141,15 +141,14 @@ class ECBackend : public PGBackend, public ECCommon {
    * check_recovery_sources.
    */
   void objects_read_and_reconstruct(
-    const std::map<hobject_t, std::list<boost::tuple<uint64_t, uint64_t, uint32_t> >
-    > &reads,
+    const std::map<hobject_t, std::list<ECCommon::ec_align_t>> &reads,
     bool fast_read,
-    GenContextURef<std::map<hobject_t,std::pair<int, extent_map> > &&> &&func) override;
+    GenContextURef<ECCommon::ec_extents_t &&> &&func) override;
 
   void objects_read_async(
     const hobject_t &hoid,
-    const std::list<std::pair<boost::tuple<uint64_t, uint64_t, uint32_t>,
-		    std::pair<ceph::buffer::list*, Context*> > > &to_read,
+    const std::list<std::pair<ECCommon::ec_align_t,
+                              std::pair<ceph::buffer::list*, Context*>>> &to_read,
     Context *on_complete,
     bool fast_read = false) override;