From 22c384c2e9c079c2fa23b64c11f423269c4bc7e2 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Thu, 16 Jul 2020 20:23:57 +0300 Subject: [PATCH 1/4] commitlog: allocate_segment_ex: filename capture is unused Signed-off-by: Benny Halevy --- db/commitlog/commitlog.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/commitlog/commitlog.cc b/db/commitlog/commitlog.cc index 89a1aab924..b290606ea6 100644 --- a/db/commitlog/commitlog.cc +++ b/db/commitlog/commitlog.cc @@ -1356,7 +1356,7 @@ future db::commitlog::segment_manager: } else { fut = f.truncate(max_size); } - return fut.then([this, d, f, filename] () mutable { + return fut.then([this, d, f] () mutable { auto s = make_shared(shared_from_this(), d, std::move(f)); return make_ready_future(s); }); From 54c5583b8dfb4790fe2b72386db6920ed828b9a4 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Thu, 16 Jul 2020 19:50:53 +0300 Subject: [PATCH 2/4] commitlog: allocate_segment_ex, segment: pass descriptor by value Besdies being more robust than passing const descriptor& to continuations, this helps simplify making allocate_segment_ex's continuations nothrow_move_constructible, that is need for using seastar::with_file_close_on_failure(). Signed-off-by: Benny Halevy --- db/commitlog/commitlog.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/db/commitlog/commitlog.cc b/db/commitlog/commitlog.cc index b290606ea6..f554650c64 100644 --- a/db/commitlog/commitlog.cc +++ b/db/commitlog/commitlog.cc @@ -305,7 +305,7 @@ public: future new_segment(); future active_segment(db::timeout_clock::time_point timeout); future allocate_segment(); - future allocate_segment_ex(const descriptor&, sstring filename, open_flags); + future allocate_segment_ex(descriptor, sstring filename, open_flags); sstring filename(const descriptor& d) const { return cfg.commit_log_location + "/" + d.filename(); @@ -503,7 +503,7 @@ public: // TODO : tune initial / default size static constexpr size_t default_size = align_up(128 * 1024, alignment); - segment(::shared_ptr m, const descriptor& d, file && f) + segment(::shared_ptr m, descriptor d, file && f) : _segment_manager(std::move(m)), _desc(std::move(d)), _file(std::move(f)), _file_name(_segment_manager->cfg.commit_log_location + "/" + _desc.filename()), _sync_time( clock_type::now()), _pending_ops(true) // want exception propagation @@ -1287,7 +1287,7 @@ static auto close_on_failure(future file_fut, Func func) { }); } -future db::commitlog::segment_manager::allocate_segment_ex(const descriptor& d, sstring filename, open_flags flags) { +future db::commitlog::segment_manager::allocate_segment_ex(descriptor d, sstring filename, open_flags flags) { file_open_options opt; opt.extent_allocation_size_hint = max_size; auto fut = do_io_check(commit_error_handler, [this, filename, flags, opt] { @@ -1357,7 +1357,7 @@ future db::commitlog::segment_manager: fut = f.truncate(max_size); } return fut.then([this, d, f] () mutable { - auto s = make_shared(shared_from_this(), d, std::move(f)); + auto s = make_shared(shared_from_this(), std::move(d), std::move(f)); return make_ready_future(s); }); }); @@ -1387,12 +1387,12 @@ future db::commitlog::segment_manager: // that recycled the file we could potentially have // out-of-order files. (Sort does not help). clogger.debug("Using recycled segment file {} -> {}", src, dst); - return rename_file(std::move(src), dst).then([this, d, dst, flags] { - return allocate_segment_ex(d, dst, flags); + return rename_file(std::move(src), dst).then([this, d = std::move(d), dst = std::move(dst), flags] () mutable { + return allocate_segment_ex(std::move(d), std::move(dst), flags); }); } - return allocate_segment_ex(d, dst, flags|open_flags::create); + return allocate_segment_ex(std::move(d), std::move(dst), flags|open_flags::create); } future db::commitlog::segment_manager::new_segment() { From 742298fa2a6560c60725f04ab2f5a40ee2f063d9 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Thu, 16 Jul 2020 20:20:40 +0300 Subject: [PATCH 3/4] commitlog: descriptor: make nothrow move constructible inherit from sstring nothrow move constructor. Signed-off-by: Benny Halevy --- db/commitlog/commitlog.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/commitlog/commitlog.hh b/db/commitlog/commitlog.hh index b07b1c401e..6da07f2973 100644 --- a/db/commitlog/commitlog.hh +++ b/db/commitlog/commitlog.hh @@ -150,7 +150,7 @@ public: static const std::string FILENAME_PREFIX; static const std::string FILENAME_EXTENSION; - descriptor(descriptor&&) = default; + descriptor(descriptor&&) noexcept = default; descriptor(const descriptor&) = default; descriptor(segment_id_type i, const std::string& fname_prefix, uint32_t v = 1, sstring = {}); descriptor(replay_position p, const std::string& fname_prefix = FILENAME_PREFIX); From 3ab1d9fe1d04e1b0063f14656bf5db94af9259b6 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Thu, 16 Jul 2020 18:33:49 +0300 Subject: [PATCH 4/4] commitlog: use seastar::with_file_close_on_failure `close_on_failure` was committed to seastar so use the library version. Signed-off-by: Benny Halevy --- db/commitlog/commitlog.cc | 40 ++++++--------------------------------- 1 file changed, 6 insertions(+), 34 deletions(-) diff --git a/db/commitlog/commitlog.cc b/db/commitlog/commitlog.cc index f554650c64..56a4f6db8f 100644 --- a/db/commitlog/commitlog.cc +++ b/db/commitlog/commitlog.cc @@ -1259,43 +1259,15 @@ void db::commitlog::segment_manager::flush_segments(bool force) { } } -/// \brief Helper for ensuring a file is closed if an exception is thrown. -/// -/// The file provided by the file_fut future is passed to func. -/// * If func throws an exception E, the file is closed and we return -/// a failed future with E. -/// * If func returns a value V, the file is not closed and we return -/// a future with V. -/// Note that when an exception is not thrown, it is the -/// responsibility of func to make sure the file will be closed. It -/// can close the file itself, return it, or store it somewhere. -/// -/// \tparam Func The type of function this wraps -/// \param file_fut A future that produces a file -/// \param func A function that uses a file -/// \return A future that passes the file produced by file_fut to func -/// and closes it if func fails -template -static auto close_on_failure(future file_fut, Func func) { - return file_fut.then([func = std::move(func)](file f) { - return futurize_invoke(func, f).handle_exception([f] (std::exception_ptr e) mutable { - return f.close().then_wrapped([f, e = std::move(e)] (future<> x) { - using futurator = futurize>; - return futurator::make_exception_future(e); - }); - }); - }); -} - future db::commitlog::segment_manager::allocate_segment_ex(descriptor d, sstring filename, open_flags flags) { file_open_options opt; opt.extent_allocation_size_hint = max_size; - auto fut = do_io_check(commit_error_handler, [this, filename, flags, opt] { + auto fut = do_io_check(commit_error_handler, [this, filename, flags, opt] () mutable { auto fut = open_file_dma(filename, flags, opt); if (cfg.extensions && !cfg.extensions->commitlog_file_extensions().empty()) { for (auto * ext : cfg.extensions->commitlog_file_extensions()) { - fut = close_on_failure(std::move(fut), [ext, filename, flags](file f) { - return ext->wrap_file(filename, f, flags).then([f](file nf) mutable { + fut = with_file_close_on_failure(std::move(fut), [ext, filename = std::move(filename), flags](file f) mutable { + return ext->wrap_file(std::move(filename), f, flags).then([f](file nf) mutable { return nf ? nf : std::move(f); }); }); @@ -1304,7 +1276,7 @@ future db::commitlog::segment_manager: return fut; }); - return close_on_failure(std::move(fut), [this, d, filename, flags] (file f) { + return with_file_close_on_failure(std::move(fut), [this, d = std::move(d), filename = std::move(filename), flags] (file f) mutable { f = make_checked_file(commit_error_handler, f); // xfs doesn't like files extended betond eof, so enlarge the file auto fut = make_ready_future<>(); @@ -1345,7 +1317,7 @@ future db::commitlog::segment_manager: v.emplace_back(iovec{ buf.get_write(), s}); m += s; } - return f.dma_write(max_size - rem, std::move(v), service::get_local_commitlog_priority()).then([&rem](size_t s) { + return f.dma_write(max_size - rem, std::move(v), service::get_local_commitlog_priority()).then([&rem](size_t s) mutable { rem -= s; return stop_iteration::no; }); @@ -1356,7 +1328,7 @@ future db::commitlog::segment_manager: } else { fut = f.truncate(max_size); } - return fut.then([this, d, f] () mutable { + return fut.then([this, d = std::move(d), f = std::move(f)] () mutable { auto s = make_shared(shared_from_this(), std::move(d), std::move(f)); return make_ready_future(s); });