osd: Corrent accounting and return codes for Direct Reads

We will never return -EAGAIN from ECBackend. If ECBackend returns EAGAIN, this causes the PrimaryLogPG code to drop the op. This is for historical reasons, but hard to refactor out.
Instead, the PrimaryLogPG code has been refactored to work out that EAGAIN is required much earlier in the processing, where EAGAIN will be returned to the client.

Here we also correct accounting in do_read and sparse_read so that we can correctly track the number of bytes read from direct reads.

Signed-off-by: Alex Ainscow <aainscow@uk.ibm.com>
Signed-off-by: Jon Bailey <jonathan.bailey1@ibm.com>

diff --git a/src/osd/ECBackend.cc b/src/osd/ECBackend.cc
index a9f3ce4af195ecf0a44cdeb771d5514230eb8e32..00b0811e58cee9dbd508e38573318dfd41bd3e7d 100644 (file)
--- a/src/osd/ECBackend.cc
+++ b/src/osd/ECBackend.cc
@@ -1043,12 +1043,12 @@ int ECBackend::objects_read_local(
     bufferlist *bl) {
 
   if (!sinfo.supports_direct_reads()) {
-    return -EOPNOTSUPP;
+    return -EOPNOTSUPP; // For exec calls
   }
 
-  if (get_parent()->get_local_missing().is_missing(hoid)) {
-    return -EIO;  // Permission denied (cos its missing)
-  }
+  // Cannot return EAGAIN here: the op would get dropped.  This check must have
+  // been done earlier.
+  ceph_assert(!get_parent()->get_local_missing().is_missing(hoid));
 
   auto [shard_offset, shard_len] = extent_to_shard_extent(off, len);
 
@@ -1096,13 +1096,13 @@ int ECBackend::objects_readv_sync(const hobject_t &hoid,
      std::map<uint64_t, uint64_t>& m,
      uint32_t op_flags,
      ceph::buffer::list *bl) {
-  if (get_parent()->get_local_missing().is_missing(hoid)) {
-    return -EACCES;  // Permission denied (cos its missing)
-  }
 
-  // Not using extent set, since we need the one used by readv.
+  // Cannot return EAGAIN here: the op would get dropped.  This check must have
+  // been done earlier.
+  ceph_assert(!get_parent()->get_local_missing().is_missing(hoid));
 
   auto shard = get_parent()->whoami_shard().shard;
+  // Not using extent_set, since we need the one used by readv.
   interval_set im(std::move(m));
   m.clear(); // Make m safe to write to again.
   auto r = switcher->store->readv(switcher->ch, ghobject_t(hoid, ghobject_t::NO_GEN, shard), im, *bl, op_flags);

diff --git a/src/osd/PrimaryLogPG.cc b/src/osd/PrimaryLogPG.cc
index 3531fb08cd902b6a8a46542689cdec89824da173..24deee836d42c64ba4ec4ec48bde7665d952194d 100644 (file)
--- a/src/osd/PrimaryLogPG.cc
+++ b/src/osd/PrimaryLogPG.cc
@@ -2230,6 +2230,14 @@ void PrimaryLogPG::do_op_impl(OpRequestRef op)
     return;
   }
 
+  // Missing direct read (EC version)
+  if (m->has_flag(CEPH_OSD_FLAG_EC_DIRECT_READ) &&
+      get_local_missing().is_missing(head)) {
+    dout(20) << __func__ << ": oid=" << head << " missing in direct read" << dendl;
+    osd->reply_op_error(op, -EAGAIN);
+    return;
+  }
+
   if (write_ordered) {
     // degraded object?
     if (is_degraded_or_backfilling_object(head)) {
@@ -5958,6 +5966,8 @@ int PrimaryLogPG::do_read(OpContext *ctx, OSDOp& osd_op) {
 
   // read into a buffer
   int result = 0;
+  uint64_t bytes_read = 0;  // Track actual bytes read for statistics
+  
   if (trimmed_read && op.extent.length == 0) {
     // read size was trimmed to zero and it is expected to do nothing
     // a read operation of 0 bytes does *not* do nothing, this is why
@@ -5974,8 +5984,17 @@ int PrimaryLogPG::do_read(OpContext *ctx, OSDOp& osd_op) {
       maybe_crc = oi.data_digest;
 
     if (ctx->op->ec_direct_read()) {
-      result = pgbackend->objects_read_local(
+      int r = pgbackend->objects_read_local(
         soid, op.extent.offset, op.extent.length, op.flags, &osd_op.outdata);
+      if (r >= 0) {
+        bytes_read = r;
+        // Don't update op.extent.length - causes issues with recursive
+        // calls from operations like CHECKSUM
+      } else if (r == -EAGAIN) {
+        result = -EAGAIN;
+      } else {
+        result = r;
+      }
       dout(20) << " EC local read for " << soid << " result=" << result << dendl;
     } else if (ctx->op->ec_sync_read()) {
       result = pgbackend->objects_read_sync(
@@ -5983,17 +6002,19 @@ int PrimaryLogPG::do_read(OpContext *ctx, OSDOp& osd_op) {
         oi.size, ctx->op->coro_handles);
       dout(20) << " EC sync read for " << soid << " result=" << result << dendl;
     } else {
-    ctx->pending_async_reads.push_back(
-      make_pair(
-        boost::make_tuple(op.extent.offset, op.extent.length, op.flags),
-        make_pair(&osd_op.outdata,
-                 new FillInVerifyExtent(&op.extent.length, &osd_op.rval,
-                                        &osd_op.outdata, maybe_crc, oi.size,
-                                        osd, soid, op.flags))));
-    dout(10) << " async_read noted for " << soid << dendl;
+      ctx->pending_async_reads.push_back(
+        make_pair(
+          boost::make_tuple(op.extent.offset, op.extent.length, op.flags),
+          make_pair(&osd_op.outdata,
+                   new FillInVerifyExtent(&op.extent.length, &osd_op.rval,
+                                          &osd_op.outdata, maybe_crc, oi.size,
+                                          osd, soid, op.flags))));
+      dout(10) << " async_read noted for " << soid << dendl;
 
-    ctx->op_finishers[ctx->current_osd_subop_num].reset(
+      ctx->op_finishers[ctx->current_osd_subop_num].reset(
       new ReadFinisher(osd_op));
+      // For async reads, op.extent.length will be updated by FillInVerifyExtent
+      bytes_read = op.extent.length;
     }
   } else {
     int r = pgbackend->objects_read_sync(
@@ -6014,9 +6035,10 @@ int PrimaryLogPG::do_read(OpContext *ctx, OSDOp& osd_op) {
     if (r == -EIO) {
       r = rep_repair_primary_object(soid, ctx);
     }
-    if (r >= 0)
+    if (r >= 0) {
       op.extent.length = r;
-    else if (r == -EAGAIN) {
+      bytes_read = r;
+    } else if (r == -EAGAIN) {
       result = -EAGAIN;
     } else {
       result = r;
@@ -6026,7 +6048,7 @@ int PrimaryLogPG::do_read(OpContext *ctx, OSDOp& osd_op) {
             << " bytes from obj " << soid << dendl;
   }
   if (result >= 0) {
-    ctx->delta_stats.num_rd_kb += shift_round_up(op.extent.length, 10);
+    ctx->delta_stats.num_rd_kb += shift_round_up(bytes_read, 10);
     ctx->delta_stats.num_rd++;
   }
   return result;
@@ -6040,6 +6062,7 @@ int PrimaryLogPG::do_sparse_read(OpContext *ctx, OSDOp& osd_op) {
   uint64_t size = oi.size;
   uint64_t offset = op.extent.offset;
   uint64_t length = op.extent.length;
+  uint64_t bytes_read = 0;  // Track actual bytes read for statistics
 
   // are we beyond truncate_size?
   if ((oi.truncate_seq < op.extent.truncate_seq) &&
@@ -6063,13 +6086,15 @@ int PrimaryLogPG::do_sparse_read(OpContext *ctx, OSDOp& osd_op) {
         make_pair(
           boost::make_tuple(offset, length, op.flags),
           make_pair(
-           &osd_op.outdata,
-           new ToSparseReadResult(&osd_op.rval, &osd_op.outdata, offset,
-                                  &op.extent.length))));
+     &osd_op.outdata,
+     new ToSparseReadResult(&osd_op.rval, &osd_op.outdata, offset,
+                  &op.extent.length))));
       dout(10) << " async_read (was sparse_read) noted for " << soid << dendl;
 
       ctx->op_finishers[ctx->current_osd_subop_num].reset(
         new ReadFinisher(osd_op));
+      // For async reads, op.extent.length will be updated by ToSparseReadResult
+      bytes_read = length;
     } else {
       dout(10) << " sparse read ended up empty for " << soid << dendl;
       map<uint64_t, uint64_t> extents;
@@ -6082,8 +6107,8 @@ int PrimaryLogPG::do_sparse_read(OpContext *ctx, OSDOp& osd_op) {
     map<uint64_t, uint64_t> m;
     auto [shard_offset, shard_length] = pgbackend->extent_to_shard_extent(offset, length);
     int r = osd->store->fiemap(ch, ghobject_t(soid, ghobject_t::NO_GEN,
-                                             info.pgid.shard),
-                              shard_offset, shard_length, m);
+                             info.pgid.shard),
+              shard_offset, shard_length, m);
     if (r < 0)  {
       return r;
     }
@@ -6110,13 +6135,18 @@ int PrimaryLogPG::do_sparse_read(OpContext *ctx, OSDOp& osd_op) {
           << " != expected 0x" << oi.data_digest
           << std::dec << " on " << soid;
         r = rep_repair_primary_object(soid, ctx);
-       if (r < 0) {
-         return r;
-       }
+        if (r < 0) {
+          return r;
+        }
       }
     }
 
-    op.extent.length = r;
+    bytes_read = r;
+    // Only set op.extent.length for non-EC-direct-read to avoid issues
+    // with recursive calls from operations like CHECKSUM
+    if (!ctx->op->ec_direct_read()) {
+      op.extent.length = r;
+    }
 
     encode(m, osd_op.outdata); // re-encode since it might be modified
     ::encode_destructively(data_bl, osd_op.outdata);
@@ -6125,7 +6155,7 @@ int PrimaryLogPG::do_sparse_read(OpContext *ctx, OSDOp& osd_op) {
              << " bytes from object " << soid << dendl;
   }
 
-  ctx->delta_stats.num_rd_kb += shift_round_up(op.extent.length, 10);
+  ctx->delta_stats.num_rd_kb += shift_round_up(bytes_read, 10);
   ctx->delta_stats.num_rd++;
   return 0;
 }