Merge pull request ceph#61990 from aclamk/wip-aclamk-write-v2-dirty-reshard

os/bluestore: write_v2 resharding misplaced

Author: aclamk

src/os/bluestore/BlueStore.cc

@@ -4178,6 +4178,29 @@ void BlueStore::ExtentMap::init_shards(bool loaded, bool dirty)
   }
 }
 
+std::pair<uint32_t, uint32_t> BlueStore::ExtentMap::fault_range_ex(
+  KeyValueDB *db,
+  uint32_t offset,
+  uint32_t length)
+{
+  dout(30) << __func__ << " 0x" << std::hex << offset << "~" << length
+	   << std::dec << dendl;
+  if (shards.size() == 0) {
+    // no sharding yet; everyting is loaded
+    return {0, OBJECT_MAX_SIZE};
+  }
+  auto start = seek_shard(offset);
+  auto last = seek_shard(offset + length);
+  maybe_load_shard(db, start, last);
+  uint32_t left_bound = shards[start].shard_info->offset;
+  uint32_t right_bound =  (size_t)last + 1 < shards.size() ?
+                          shards[last + 1].shard_info->offset : OBJECT_MAX_SIZE;
+  dout(20) << __func__ << " start=" << start << " last=" << last
+    << " -> 0x" << std::hex << left_bound << "~" << right_bound
+    << std::dec << dendl;
+  return {left_bound, right_bound};
+}
+
 void BlueStore::ExtentMap::fault_range(
   KeyValueDB *db,
   uint32_t offset,
@@ -4191,6 +4214,14 @@ void BlueStore::ExtentMap::fault_range(
   }
   auto start = seek_shard(offset);
   auto last = seek_shard(offset + length);
+  maybe_load_shard(db, start, last);
+}
+
+void BlueStore::ExtentMap::maybe_load_shard(
+  KeyValueDB *db,
+  int start,
+  int last)
+{
   ceph_assert(last >= start);
   ceph_assert(start >= 0);
 
@@ -4216,9 +4247,10 @@ void BlueStore::ExtentMap::fault_range(
       );
       p->extents = decode_some(v);
       p->loaded = true;
-      dout(20) << __func__ << " open shard 0x" << std::hex
-	       << p->shard_info->offset
-	       << " for range 0x" << offset << "~" << length << std::dec
+      uint32_t shard_end =
+        (size_t)start + 1 < shards.size() ? (p + 1)->shard_info->offset : OBJECT_MAX_SIZE;
+      dout(20) << __func__ << " open shard for range 0x"
+               << std::hex << p->shard_info->offset << "~" << shard_end << std::dec
 	       << " (" << v.length() << " bytes)" << dendl;
       ceph_assert(p->dirty == false);
       ceph_assert(v.length() == p->shard_info->bytes);
@@ -17621,11 +17653,16 @@ int BlueStore::_do_write_v2(
   if (bl.length() != length) {
     bl.splice(length, bl.length() - length);
   }
+  BlueStore::Writer wr(this, txc, &wctx, o);
   uint64_t start = p2align(offset, min_alloc_size);
   uint64_t end = p2roundup(offset + length, min_alloc_size);
-  o->extent_map.fault_range(db, start, end - start);
-  BlueStore::Writer wr(this, txc, &wctx, o);
+  wr.left_affected_range = start;
+  wr.right_affected_range = end;
+  std::tie(wr.left_shard_bound, wr.right_shard_bound) =
+    o->extent_map.fault_range_ex(db, start, end - start);
   wr.do_write(offset, bl);
+  o->extent_map.dirty_range(wr.left_affected_range, wr.right_affected_range - wr.left_affected_range);
+  o->extent_map.maybe_reshard(wr.left_affected_range, wr.right_affected_range);
   return r;
 }
 

src/os/bluestore/BlueStore.h

@@ -1149,6 +1149,16 @@ class BlueStore : public ObjectStore,
     /// ensure that a range of the map is loaded
     void fault_range(KeyValueDB *db,
 		     uint32_t offset, uint32_t length);
+    /// ensure that a range of the map is loaded
+    /// return range that is encompassed by affected shards
+    std::pair<uint32_t, uint32_t> fault_range_ex(
+      KeyValueDB *db,
+      uint32_t offset,
+      uint32_t length);
+    void maybe_load_shard(
+      KeyValueDB *db,
+      int begin_shard,
+      int end_shard);
 
     /// ensure a range of the map is marked dirty
     void dirty_range(uint32_t offset, uint32_t length);

src/os/bluestore/Writer.cc

@@ -282,7 +282,6 @@ inline BlueStore::extent_map_t::iterator BlueStore::Writer::_find_mutable_blob_r
       if (it->blob_end() <= mapmust_end) continue;
     }
     return it;
-    break;
   };
   return map.end();
 }
@@ -547,6 +546,68 @@ BlueStore::BlobRef BlueStore::Writer::_blob_create_full(
   return blob;
 }
 
+inline void BlueStore::Writer::_place_extent_in_blob(
+  Extent* ex,
+  uint32_t map_begin,
+  uint32_t map_end,
+  uint32_t in_blob_offset)
+{
+  if (ex->logical_end() <= map_begin) {
+    // we are adding to right side of the target
+    if (ex->logical_end() == map_begin) {
+      // we can just expand existing Extent
+      ex->length += map_end - map_begin;
+      dout(20) << __func__ << " expanded extent " << ex->print(pp_mode) << dendl;
+      left_affected_range = std::min(left_affected_range, ex->logical_offset);
+    } else {
+      // disjointed, new extent needed
+      Extent *le = new Extent(
+        map_begin, in_blob_offset, map_end - map_begin, ex->blob);
+      dout(20) << __func__ << " new extent " << le->print(pp_mode) << dendl;
+      onode->extent_map.extent_map.insert(*le);
+      left_affected_range = std::min(left_affected_range, le->logical_offset);
+    }
+  } else if (ex->logical_offset >= map_end) {
+    // we are adding to left side of target
+    if (ex->logical_offset == map_end) {
+      // we can just expand existing Extent
+      //ceph_assert(ref_end == want_subau_end);
+      ex->logical_offset -= (map_end - map_begin);
+      ex->blob_offset -= (map_end - map_begin);
+      ex->length += (map_end - map_begin);
+      dout(20) << __func__ << " expanded extent " << ex->print(pp_mode) << dendl;
+      right_affected_range = std::max(right_affected_range, ex->logical_end());
+    } else {
+      // disjointed, new extent needed
+      Extent *le = new Extent(
+        map_begin, in_blob_offset, map_end - map_begin, ex->blob);
+      dout(20) << __func__ << " new extent " << le->print(pp_mode) << dendl;
+      onode->extent_map.extent_map.insert(*le);
+      right_affected_range = std::max(right_affected_range, le->logical_end());
+    }
+  }
+}
+
+// Iterator it can be invalidated.
+void BlueStore::Writer::_maybe_meld_with_prev_extent(exmp_it it)
+{
+  if (it == onode->extent_map.extent_map.end())   return; // can't merge with non-existent
+  if (it == onode->extent_map.extent_map.begin()) return; // can't merge when there is only 1 extent
+  if (it->logical_end() >= right_shard_bound)     return; // not allowed to escape shard range
+  auto it_p = it;
+  --it_p;
+  if (it_p->logical_offset < left_shard_bound) return; // we could jump here behind our inserted range
+  if (it_p->blob == it->blob &&
+      it_p->logical_end() == it->logical_offset &&
+      it_p->blob_offset + it_p->length == it->blob_offset) // this one is specifc, currently is always true
+  {
+    it_p->length += it->length;
+    onode->extent_map.rm(it);
+    left_affected_range = std::min(left_affected_range, it_p->logical_offset);
+    right_affected_range = std::max(right_affected_range, it_p->logical_end());
+  }
+}
+
 /**
  * Note from developer
  * This module tries to keep naming convention:
@@ -732,6 +793,7 @@ void BlueStore::Writer::_try_reuse_allocated_l(
   blob_data_t& bd)            // modified when consumed
 {
   uint32_t search_stop = p2align(logical_offset, (uint32_t)wctx->target_blob_size);
+  search_stop = std::max(left_shard_bound, search_stop);
   uint32_t au_size = bstore->min_alloc_size;
   uint32_t block_size = bstore->block_size;
   ceph_assert(!bd.is_compressed());
@@ -781,11 +843,7 @@ void BlueStore::Writer::_try_reuse_allocated_l(
     uint32_t ref_end = std::min(ref_end_offset, want_subau_end);
     //fixme/improve - need something without stupid extras - that is without coll
     b->get_ref(onode->c, in_blob_offset, ref_end - want_subau_begin);
-    Extent *le = new Extent(
-      want_subau_begin, in_blob_offset, ref_end - want_subau_begin, it->blob);
-    dout(20) << __func__ << " new extent " << le->print(pp_mode) << dendl;
-    emap.extent_map.insert(*le);
-
+    _place_extent_in_blob(&*it, want_subau_begin, ref_end, in_blob_offset);
     logical_offset += data_size;
     break;
   }
@@ -812,6 +870,7 @@ void BlueStore::Writer::_try_reuse_allocated_r(
   uint32_t block_size = bstore->block_size;
   uint32_t blob_size = wctx->target_blob_size;
   uint32_t search_end = p2roundup(end_offset, blob_size);
+  search_end = std::min(right_shard_bound, search_end);
   ceph_assert(!bd.is_compressed());
   ceph_assert(p2phase<uint32_t>(end_offset, au_size) != 0);
   BlueStore::ExtentMap& emap = onode->extent_map;
@@ -854,11 +913,7 @@ void BlueStore::Writer::_try_reuse_allocated_r(
     uint32_t ref_end = std::min(ref_end_offset, want_subau_end);
     //fixme/improve - need something without stupid extras - that is without coll
     b->get_ref(onode->c, in_blob_offset, ref_end - want_subau_begin);
-    Extent *le = new Extent(
-      want_subau_begin, in_blob_offset, ref_end - want_subau_begin, it->blob);
-    dout(20) << __func__ << " new extent " << le->print(pp_mode) << dendl;
-    emap.extent_map.insert(*le);
-
+    _place_extent_in_blob(&*it, want_subau_begin, ref_end, in_blob_offset);
     end_offset -= data_size;
     break;
   }
@@ -1023,14 +1078,11 @@ void BlueStore::Writer::_do_put_blobs(
       uint32_t ref_end = std::min(ref_end_offset, data_end_offset);
       //fixme/improve - need something without stupid extras - that is without coll
       left_b->blob->get_ref(coll, in_blob_offset, ref_end - logical_offset);
-      Extent *le = new Extent(
-        logical_offset, in_blob_offset, ref_end - logical_offset, left_b->blob);
-      dout(20) << __func__ << " new extent " << le->print(pp_mode) << dendl;
-      emap.insert(*le);
+      _place_extent_in_blob(&*left_b, logical_offset, ref_end, in_blob_offset);
+      bstore->logger->inc(l_bluestore_write_small);
+      bstore->logger->inc(l_bluestore_write_small_bytes, ref_end - logical_offset);
       logical_offset = ref_end;
       ++bd_it;
-      bstore->logger->inc(l_bluestore_write_small);
-      bstore->logger->inc(l_bluestore_write_small_bytes, le->length);
     } else {
       // it is still possible to use first bd and put it into
       // blob after punch_hole
@@ -1062,13 +1114,10 @@ void BlueStore::Writer::_do_put_blobs(
         uint32_t ref_end = std::min(ref_end_offset, data_begin_offset + back_it->disk_data.length());
         //fixme - need something without stupid extras
         right_b->blob->get_ref(coll, in_blob_offset, ref_end - data_begin_offset);
-        Extent *le = new Extent(
-          data_begin_offset, in_blob_offset, ref_end - data_begin_offset, right_b->blob);
-        dout(20) << __func__ << " new extent " << le->print(pp_mode) << dendl;
-        emap.insert(*le);
+        _place_extent_in_blob(&*right_b, data_begin_offset, ref_end, in_blob_offset);
         bd.erase(back_it); //TODO - or other way of limiting end
         bstore->logger->inc(l_bluestore_write_small);
-        bstore->logger->inc(l_bluestore_write_small_bytes, le->length);
+        bstore->logger->inc(l_bluestore_write_small_bytes, ref_end - data_begin_offset);
       }
     }
   }
@@ -1379,9 +1428,13 @@ void BlueStore::Writer::do_write(
   _collect_released_allocated();
   // update statfs
   txc->statfs_delta += statfs_delta;
-  onode->extent_map.compress_extent_map(location, data_end - location);
-  onode->extent_map.dirty_range(location, data_end-location);
-  onode->extent_map.maybe_reshard(location, data_end);
+  // Note: compress extent is mostly not needed; _try_reuse_allocated_* joins extents if possible;
+  // same is done after _blob_put_data_subau_allocate (allocate more to existing blob).
+  // New blobs cannot be joined with existing ones.
+  // The only case that adjecent extents can be meld together is on the boundary.
+  // More specific, since we are operating left side first, right side later
+  // the only non-joined extent is between after_punch_it - 1 and after_punch_it.
+  _maybe_meld_with_prev_extent(after_punch_it);
   dout(25) << "result: " << std::endl << onode->print(pp_mode) << dendl;
 }
 

src/os/bluestore/Writer.h

@@ -50,7 +50,8 @@ class BlueStore::Writer {
     virtual bufferlist read(uint32_t object_offset, uint32_t object_length) = 0;
   };
   Writer(BlueStore* bstore, TransContext* txc, WriteContext* wctx, OnodeRef o)
-    :bstore(bstore), txc(txc), wctx(wctx), onode(o) {
+    :left_shard_bound(0), right_shard_bound(OBJECT_MAX_SIZE)
+    , bstore(bstore), txc(txc), wctx(wctx), onode(o) {
       pp_mode = debug_level_to_pp_mode(bstore->cct);
     }
 public:
@@ -67,7 +68,10 @@ class BlueStore::Writer {
   read_divertor* test_read_divertor = nullptr;
   std::vector<BlobRef> pruned_blobs;
   volatile_statfs statfs_delta;
-
+  uint32_t left_shard_bound;  // if sharding is in effect,
+  uint32_t right_shard_bound; // do not cross this line
+  uint32_t left_affected_range;
+  uint32_t right_affected_range;
 private:
   BlueStore* bstore;
   TransContext* txc;
@@ -140,8 +144,15 @@ class BlueStore::Writer {
   void _align_to_disk_block(
     uint32_t& location,
     uint32_t& ref_end,
-    blob_vec& blobs
-  );
+    blob_vec& blobs);
+
+  void _place_extent_in_blob(
+    Extent* target,
+    uint32_t map_begin,
+    uint32_t map_end,
+    uint32_t in_blob_offset);
+
+  void _maybe_meld_with_prev_extent(exmp_it after_punch_it);
 
   inline void _blob_put_data_subau(
     Blob* blob,

src/os/bluestore/bluestore_types.h

@@ -1004,10 +1004,14 @@ struct bluestore_blob_t {
   void add_tail(uint32_t new_len) {
     ceph_assert(!has_unused());
     ceph_assert(new_len > logical_length);
-    extents.emplace_back(
-      bluestore_pextent_t(
-        bluestore_pextent_t::INVALID_OFFSET,
-        new_len - logical_length));
+    if (extents.size() == 0 || extents.back().is_valid()) {
+      extents.emplace_back(
+        bluestore_pextent_t(
+          bluestore_pextent_t::INVALID_OFFSET,
+          new_len - logical_length));
+    } else {
+      extents.back().length += new_len - logical_length;
+    }
     logical_length = new_len;
     if (has_csum()) {
       ceph::buffer::ptr t;