Merge 'commitlog: Fix position adjustment in large allocation' from Calle Wilund

Refs SCYLLADB-1815
Refs SCYLLADB-1757

(Together with #29753, should be fix for the latter)

Typo/copy-paste error in previous fix for position checks vs. file max when calculating available allocation space.

Should be backported to same branches as original (broken) fix, #29753

Closes scylladb/scylladb#30179

* github.com:scylladb/scylladb:
  commitlog_test: Add test for segment size overflow
  commitlog: Fix position adjustment in large allocation

db/commitlog/commitlog.cc

@@ -340,7 +340,9 @@ public:
 
     future<> oversized_allocation(entry_writer&, db::timeout_clock::time_point timeout);
 
-    replay_position min_position();
+    replay_position min_position() const;
+    replay_position current_position() const;
+    size_t sector_overhead(segment_id_type, size_t) const;
 
     template<typename T>
     struct byte_flow {
@@ -1508,7 +1510,7 @@ public:
     }
 };
 
-db::replay_position db::commitlog::segment_manager::min_position() {
+db::replay_position db::commitlog::segment_manager::min_position() const {
     if (_segments.empty()) {
         return {_ids, 0};
     } else {
@@ -1516,6 +1518,24 @@ db::replay_position db::commitlog::segment_manager::min_position() {
     }
 }
 
+db::replay_position db::commitlog::segment_manager::current_position() const {
+    if (_segments.empty()) {
+        return {_ids, 0};
+    } else {
+        auto s = _segments.front();
+        return {s->_desc.id, s->position()};
+    }
+}
+
+size_t db::commitlog::segment_manager::sector_overhead(segment_id_type id, size_t size) const {
+    for (auto& s : _segments) {
+        if (s->_desc.id == id) {
+            return s->sector_overhead(size);
+        }
+    }
+    return 0;
+}
+
 template<typename T, typename R>
 requires std::derived_from<T, db::commitlog::entry_writer> && std::same_as<R, decltype(std::declval<T>().result())>
 future<R> db::commitlog::segment_manager::allocate_when_possible(T writer, db::timeout_clock::time_point timeout) {
@@ -1777,7 +1797,7 @@ future<> db::commitlog::segment_manager::oversized_allocation(entry_writer& writ
                 }
                 // bytes not counting overhead
                 auto pos = s->position();
-                auto max = std::max<size_t>(pos, max_size);
+                auto max = std::min<size_t>(pos, max_size);
                 auto buf_rem = std::min(max_size - max, s->_buffer_ostream.size());
 
                 size_t avail;
@@ -1791,8 +1811,8 @@ future<> db::commitlog::segment_manager::oversized_allocation(entry_writer& writ
                 } else {
                     co_await s->cycle();
                     auto pos = s->position();
-                    auto max = std::max<size_t>(pos, max_size);
-                    auto file_rem = max - pos;
+                    auto max = std::min<size_t>(pos, max_size);
+                    auto file_rem = max_size - max;
 
                     if (file_rem < align) {
                         co_await s->close();
@@ -3967,6 +3987,14 @@ db::replay_position db::commitlog::min_position() const {
     return _segment_manager->min_position();
 }
 
+db::replay_position db::commitlog::current_position() const {
+    return _segment_manager->current_position();
+}
+
+size_t db::commitlog::sector_overhead(segment_id_type id, size_t size) const {
+    return _segment_manager->sector_overhead(id, size);
+}
+
 void db::commitlog::update_max_data_lifetime(std::optional<uint64_t> commitlog_data_max_lifetime_in_seconds) {
     _segment_manager->cfg.commitlog_data_max_lifetime_in_seconds = commitlog_data_max_lifetime_in_seconds;
 }

db/commitlog/commitlog.hh

@@ -386,6 +386,12 @@ public:
     // be replayed on the next reboot.
     replay_position min_position() const;
 
+    // For testing only. Returns the active segments current position
+    // (ignoring chunk overhead etc)
+    replay_position current_position() const;
+    // Gets sector overhead for size in given segment
+    size_t sector_overhead(segment_id_type, size_t) const;
+
     // (Re-)set data mix lifetime.
     void update_max_data_lifetime(std::optional<uint64_t> commitlog_data_max_lifetime_in_seconds);
 

test/boost/commitlog_test.cc

@@ -2144,6 +2144,52 @@ SEASTAR_TEST_CASE(test_oversized_with_terminate_in_buffer_wait) {
     }, exts.get());
 }
 
+
+SEASTAR_TEST_CASE(test_oversized_at_segment_boundary) {
+    co_await test_oversized(1, 1, [&](commitlog& log) -> future<> {
+        // Add an entry that is exactly segment_size - chunk+entry overhead.
+        // provoking segment overflow.
+        auto uuid = make_table_id();
+        auto max_size = log.max_record_size();
+        auto buf = fragmented_temporary_buffer::allocate_to_fit(max_size);
+        auto h = co_await log.add_mutation(uuid, buf.size_bytes(), db::commitlog::force_sync::no, [&](db::commitlog::output& dst) {
+            for (auto& tmp : buf) {
+                dst.write(tmp.get(), tmp.size());
+            }
+        });
+
+        co_await log.sync_all_segments();
+        auto pos = log.current_position();
+
+        BOOST_CHECK_EQUAL(h.rp().id, pos.id);
+        BOOST_CHECK_LT(h.rp().pos, pos.pos);
+
+        // TODO: maybe export this so we don't know it here as well...
+        auto overhead = 2 * sizeof(uint32_t);
+        auto base_size = 1024*1024 - pos.pos - overhead;
+        auto sector_overhead = log.sector_overhead(pos.id, base_size);
+
+        BOOST_CHECK_GT(sector_overhead, 0);
+
+        // With bug, we get to write this in same segment as above, even though we overshoot position now
+        buf = fragmented_temporary_buffer::allocate_to_fit(base_size - sector_overhead);
+        auto h2 = co_await log.add_mutation(uuid, buf.size_bytes(), db::commitlog::force_sync::no, [&](db::commitlog::output& dst) {
+            for (auto& tmp : buf) {
+                dst.write(tmp.get(), tmp.size());
+            }
+        });
+
+        // Should have gone to new segment.
+        BOOST_CHECK_NE(h2.rp().id, pos.id);
+
+        h.release();
+        h2.release();
+
+        // Now, if bugged, oversized test will overflow size calc and crash.
+        // With test, it will not.
+    });
+}
+
 // tests #20862 - buffer usage counter not being updated correctly
 SEASTAR_TEST_CASE(test_commitlog_buffer_size_counter) {
     commitlog::config cfg;