Merge pull request ceph#61314 from aclamk/wip-aclamk-bluefs-truncate-fix

os/bluestore: Fix BlueFS::truncate()

Author: aclamk

src/os/bluestore/BlueFS.cc

@@ -1706,7 +1706,8 @@ int BlueFS::_replay(bool noop, bool to_stdout)
 		   << " fnode=" << fnode
 		   << " delta=" << delta
 		   << dendl;
-	      ceph_assert(delta.offset == fnode.allocated);
+	      // be leanient, if there is no extents just produce error message
+	      ceph_assert(delta.offset == fnode.allocated || delta.extents.empty());
 	    }
 	    if (cct->_conf->bluefs_log_replay_check_allocations) {
               int r = _check_allocations(fnode,
@@ -3830,6 +3831,7 @@ int BlueFS::truncate(FileWriter *h, uint64_t offset)/*_WF_L*/
       fnode.size = offset;
       fnode.allocated = new_allocated;
       fnode.reset_delta();
+      fnode.recalc_allocated();
       log.t.op_file_update(fnode);
       // sad, but is_dirty must be set to signal flushing of the log
       h->file->is_dirty = true;

src/os/bluestore/bluefs_types.cc

@@ -154,7 +154,9 @@ mempool::bluefs::vector<bluefs_extent_t>::iterator bluefs_fnode_t::seek(
     assert(it != extents_index.begin());
     --it;
     assert(offset >= *it);
-    p += it - extents_index.begin();
+    uint32_t skip = it - extents_index.begin();
+    ceph_assert(skip <= extents.size());
+    p += skip;
     offset -= *it;
   }
 

src/os/bluestore/bluefs_types.h

@@ -89,6 +89,7 @@ struct bluefs_fnode_t {
   void recalc_allocated() {
     allocated = 0;
     extents_index.reserve(extents.size());
+    extents_index.clear();
     for (auto& p : extents) {
       extents_index.emplace_back(allocated);
       allocated += p.length;

src/test/objectstore/test_bluefs.cc

@@ -1608,6 +1608,91 @@ TEST(BlueFS, test_log_runway_advance_seq) {
   fs.compact_log();
 }
 
+TEST(BlueFS, test_69481_truncate_corrupts_log) {
+  uint64_t size = 1048576 * 128;
+  TempBdev bdev{size};
+  BlueFS fs(g_ceph_context);
+  ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false));
+  uuid_d fsid;
+  ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false }));
+  ASSERT_EQ(0, fs.mount());
+  ASSERT_EQ(0, fs.maybe_verify_layout({ BlueFS::BDEV_DB, false, false }));
+
+  BlueFS::FileWriter *f = nullptr;
+  BlueFS::FileWriter *a = nullptr;
+  ASSERT_EQ(0, fs.mkdir("dir"));
+  ASSERT_EQ(0, fs.open_for_write("dir", "test-file", &f, false));
+  ASSERT_EQ(0, fs.open_for_write("dir", "just-allocate", &a, false));
+
+  // create 4 distinct extents in file f
+  // a is here only to prevent f from merging extents together
+  fs.preallocate(f->file, 0, 0x10000);
+  fs.preallocate(a->file, 0, 0x10000);
+  fs.preallocate(f->file, 0, 0x20000);
+  fs.preallocate(a->file, 0, 0x20000);
+  fs.preallocate(f->file, 0, 0x30000);
+  fs.preallocate(a->file, 0, 0x30000);
+  fs.preallocate(f->file, 0, 0x40000);
+  fs.preallocate(a->file, 0, 0x40000);
+  fs.close_writer(a);
+
+  fs.truncate(f, 0);
+  fs.fsync(f);
+
+  bufferlist bl;
+  bl.append(std::string(" ", 0x15678));
+  f->append(bl);
+  fs.truncate(f, 0x15678);
+  fs.fsync(f);
+  fs.close_writer(f);
+
+  fs.umount();
+  // remount to verify
+  ASSERT_EQ(0, fs.mount());
+  fs.umount();
+}
+
+TEST(BlueFS, test_69481_truncate_asserts) {
+  uint64_t size = 1048576 * 128;
+  TempBdev bdev{size};
+  BlueFS fs(g_ceph_context);
+  ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false));
+  uuid_d fsid;
+  ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false }));
+  ASSERT_EQ(0, fs.mount());
+  ASSERT_EQ(0, fs.maybe_verify_layout({ BlueFS::BDEV_DB, false, false }));
+
+  BlueFS::FileWriter *f = nullptr;
+  BlueFS::FileWriter *a = nullptr;
+  ASSERT_EQ(0, fs.mkdir("dir"));
+  ASSERT_EQ(0, fs.open_for_write("dir", "test-file", &f, false));
+  ASSERT_EQ(0, fs.open_for_write("dir", "just-allocate", &a, false));
+
+  // create 4 distinct extents in file f
+  // a is here only to prevent f from merging extents together
+  fs.preallocate(f->file, 0, 0x10000);
+  fs.preallocate(a->file, 0, 0x10000);
+  fs.preallocate(f->file, 0, 0x20000);
+  fs.preallocate(a->file, 0, 0x20000);
+  fs.preallocate(f->file, 0, 0x30000);
+  fs.preallocate(a->file, 0, 0x30000);
+  fs.preallocate(f->file, 0, 0x40000);
+  fs.preallocate(a->file, 0, 0x40000);
+  fs.close_writer(a);
+
+  fs.truncate(f, 0);
+  fs.fsync(f);
+
+  bufferlist bl;
+  bl.append(std::string(" ", 0x35678));
+  f->append(bl);
+  fs.truncate(f, 0x35678);
+  fs.fsync(f);
+  fs.close_writer(f);
+
+  fs.umount();
+}
+
 int main(int argc, char **argv) {
   auto args = argv_to_vec(argc, argv);
   map<string,string> defaults = {