From 48ca01c3ab2b0376fff03d3003d562baba5566f1 Mon Sep 17 00:00:00 2001
From: Calle Wilund <calle@scylladb.com>
Date: Mon, 15 Mar 2021 09:32:01 +0000
Subject: [PATCH] commitlog: Make pre-allocation drop O_DSYNC while pre-filling

Refs #7794

Iff we need to pre-fill segment file ni O_DSYNC mode, we should
drop this for the pre-fill, to avoid issuing flushes until the file
is filled. Done by temporarily closing, re-opening in "normal" mode,
filling, then re-opening.

v2:
* More comment
v3:
* Add missing flush
v4:
* comment
v5:
* Split coroutine and fix into separate patches
---
 db/commitlog/commitlog.cc    |  9 +++++++++
 test/boost/commitlog_test.cc | 29 +++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/db/commitlog/commitlog.cc b/db/commitlog/commitlog.cc
index cefa615b45..4f6c45932a 100644
--- a/db/commitlog/commitlog.cc
+++ b/db/commitlog/commitlog.cc
@@ -1418,6 +1418,13 @@ future<db::commitlog::segment_manager::sseg_ptr> db::commitlog::segment_manager:
 
                 clogger.trace("Pre-writing {} of {} KB to segment {}", (max_size - existing_size)/1024, max_size/1024, filename);
 
+                // re-open without o_dsync for pre-alloc. The reason/rationale
+                // being that we want automatic (meta)data sync from O_DSYNC for when
+                // we do actual CL flushes, but here it would just result in
+                // data being committed before we've reached eof/finished writing.
+                // Again an argument for sendfile-like constructs I guess...
+                co_await f.close();
+                f = co_await open_file_dma(filename, flags & open_flags(~int(open_flags::dsync)), opt);
                 co_await f.allocate(existing_size, max_size - existing_size);
 
                 static constexpr size_t buf_size = 4 * segment::alignment;
@@ -1443,6 +1450,8 @@ future<db::commitlog::segment_manager::sseg_ptr> db::commitlog::segment_manager:
 
                 // sync metadata (size/written)
                 co_await f.flush();
+                co_await f.close();
+                f = co_await open_file_dma(filename, flags, opt);
             }
         } else {
             co_await f.truncate(max_size);
diff --git a/test/boost/commitlog_test.cc b/test/boost/commitlog_test.cc
index 5523d96944..5069c36ed6 100644
--- a/test/boost/commitlog_test.cc
+++ b/test/boost/commitlog_test.cc
@@ -30,6 +30,7 @@
 #include <set>
 
 #include <seastar/testing/test_case.hh>
+#include <seastar/core/coroutine.hh>
 #include <seastar/core/future-util.hh>
 #include <seastar/core/do_with.hh>
 #include <seastar/core/scollectd_api.hh>
@@ -719,3 +720,31 @@ SEASTAR_TEST_CASE(test_commitlog_add_entries) {
     });
 }
 
+SEASTAR_TEST_CASE(test_commitlog_new_segment_odsync){
+    commitlog::config cfg;
+    cfg.commitlog_segment_size_in_mb = 1;
+    cfg.use_o_dsync = true;
+
+    return cl_test(cfg, [](commitlog& log) -> future<> {
+        auto uuid = utils::UUID_gen::get_time_UUID();
+        rp_set set;
+        while (set.size() <= 1) {
+            sstring tmp = "hej bubba cow";
+            rp_handle h = co_await log.add_mutation(uuid, tmp.size(), db::commitlog::force_sync::no, [tmp](db::commitlog::output& dst) {
+                dst.write(tmp.data(), tmp.size());
+            });
+            set.put(std::move(h));
+        }
+
+        auto names = log.get_active_segment_names();
+        BOOST_REQUIRE(names.size() > 1);
+
+        // check that all the segments are pre-allocated.
+        for (auto& name : names) {
+            auto f = co_await seastar::open_file_dma(name, seastar::open_flags::ro);
+            auto s = co_await f.size();
+            co_await f.close();
+            BOOST_CHECK_EQUAL(s, 1024u*1024u);
+        }
+    });
+}