fs: metadata_log: applied numerous suggestions from code review

CMakeLists.txt

@@ -490,7 +490,6 @@ add_library (seastar STATIC
  include/seastar/fs/block_device.hh
  include/seastar/fs/file.hh
  include/seastar/fs/overloaded.hh
- include/seastar/fs/path.hh
  include/seastar/fs/temporary_file.hh
  include/seastar/http/api_docs.hh
  include/seastar/http/common.hh
@@ -611,6 +610,7 @@ add_library (seastar STATIC
  src/fs/metadata_log_bootstrap.hh
  src/fs/metadata_log_operations/create_file.hh
  src/fs/metadata_to_disk_buffer.hh
+ src/fs/path.hh
  src/fs/range.hh
  src/fs/to_disk_buffer.hh
  src/fs/units.hh

src/fs/cluster.hh

@@ -21,8 +21,8 @@
 
 #pragma once
 
-#include "bitwise.hh"
-#include "units.hh"
+#include "fs/bitwise.hh"
+#include "fs/units.hh"
 
 namespace seastar::fs {
 

src/fs/inode.hh

@@ -21,8 +21,8 @@
 
 #pragma once
 
-#include "bitwise.hh"
-#include "units.hh"
+#include "fs/bitwise.hh"
+#include "fs/units.hh"
 
 #include <cstdint>
 #include <optional>

src/fs/inode_info.hh

@@ -21,12 +21,12 @@
 
 #pragma once
 
-#include "inode.hh"
+#include "fs/inode.hh"
+#include "fs/units.hh"
+#include "fs/unix_metadata.hh"
 #include "seastar/core/sstring.hh"
 #include "seastar/core/temporary_buffer.hh"
 #include "seastar/fs/overloaded.hh"
-#include "units.hh"
-#include "unix_metadata.hh"
 
 #include <map>
 #include <variant>
@@ -131,6 +131,22 @@ struct inode_info {
  };
 
  std::variant<directory, file> contents;
+
+ constexpr bool is_directory() const noexcept { return std::holds_alternative<directory>(contents); }
+
+ // These are noexcept because invalid access is a bug not an error
+ constexpr directory& get_directory() & noexcept { return std::get<directory>(contents); }
+ constexpr const directory& get_directory() const & noexcept { return std::get<directory>(contents); }
+ constexpr directory&& get_directory() && noexcept { return std::move(std::get<directory>(contents)); }
+ constexpr const directory&& get_directory() const && noexcept { return std::move(std::get<directory>(contents)); }
+
+ constexpr bool is_file() const noexcept { return std::holds_alternative<file>(contents); }
+
+ // These are noexcept because invalid access is a bug not an error
+ constexpr file& get_file() & noexcept { return std::get<file>(contents); }
+ constexpr const file& get_file() const & noexcept { return std::get<file>(contents); }
+ constexpr file&& get_file() && noexcept { return std::move(std::get<file>(contents)); }
+ constexpr const file&& get_file() const && noexcept { return std::move(std::get<file>(contents)); }
 };
 
 } // namespace seastar::fs

src/fs/metadata_disk_entries.hh

@@ -21,9 +21,9 @@
 
 #pragma once
 
-#include "cluster.hh"
-#include "inode.hh"
-#include "unix_metadata.hh"
+#include "fs/cluster.hh"
+#include "fs/inode.hh"
+#include "fs/unix_metadata.hh"
 
 namespace seastar::fs {
 

src/fs/metadata_log.cc

@@ -28,6 +28,7 @@
 #include "fs/metadata_log_bootstrap.hh"
 #include "fs/metadata_log_operations/create_file.hh"
 #include "fs/metadata_to_disk_buffer.hh"
+#include "fs/path.hh"
 #include "fs/unix_metadata.hh"
 #include "seastar/core/aligned_buffer.hh"
 #include "seastar/core/do_with.hh"
@@ -36,9 +37,7 @@
 #include "seastar/core/future.hh"
 #include "seastar/core/shared_mutex.hh"
 #include "seastar/fs/overloaded.hh"
-#include "seastar/fs/path.hh"
 
-#include <bits/stdint-uintn.h>
 #include <boost/crc.hpp>
 #include <boost/range/irange.hpp>
 #include <chrono>
@@ -83,6 +82,12 @@ void metadata_log::memory_only_create_inode(inode_t inode, bool is_directory, un
  });
 }
 
+void metadata_log::memory_only_update_metadata(inode_t inode, unix_metadata metadata) {
+ auto it = _inodes.find(inode);
+ assert(it != _inodes.end());
+ it->second.metadata = std::move(metadata);
+}
+
 void metadata_log::memory_only_delete_inode(inode_t inode) {
  auto it = _inodes.find(inode);
  assert(it != _inodes.end());
@@ -109,8 +114,8 @@ void metadata_log::memory_only_small_write(inode_t inode, disk_offset_t offset,
 
  auto it = _inodes.find(inode);
  assert(it != _inodes.end());
- assert(std::holds_alternative<inode_info::file>(it->second.contents));
- write_update(std::get<inode_info::file>(it->second.contents), std::move(data_vec));
+ assert(it->second.is_file());
+ write_update(it->second.get_file(), std::move(data_vec));
 }
 
 void metadata_log::memory_only_update_mtime(inode_t inode, decltype(unix_metadata::mtime_ns) mtime_ns) {
@@ -122,8 +127,8 @@ void metadata_log::memory_only_update_mtime(inode_t inode, decltype(unix_metadat
 void metadata_log::memory_only_truncate(inode_t inode, disk_offset_t size) {
  auto it = _inodes.find(inode);
  assert(it != _inodes.end());
- assert(std::holds_alternative<inode_info::file>(it->second.contents));
- auto& file = std::get<inode_info::file>(it->second.contents);
+ assert(it->second.is_file());
+ auto& file = it->second.get_file();
 
  auto file_size = file.size();
  if (size > file_size) {
@@ -144,7 +149,7 @@ void metadata_log::memory_only_add_dir_entry(inode_info::directory& dir, inode_t
  auto it = _inodes.find(entry_inode);
  assert(it != _inodes.end());
  // Directory may only be linked once (to avoid creating cycles)
- assert(not std::holds_alternative<inode_info::directory>(it->second.contents) or it->second.directories_containing_file == 0);
+ assert(not it->second.is_directory() or it->second.directories_containing_file == 0);
 
  bool inserted = dir.entries.emplace(std::move(entry_name), entry_inode).second;
  assert(inserted);
@@ -183,6 +188,7 @@ future<> metadata_log::flush_curr_cluster_and_change_it_to_new_one() {
  auto next_cluster = _cluster_allocator.alloc();
  if (not next_cluster) {
  // Here metadata log dies, we cannot even flush current cluster because from there we won't be able to recover
+ // TODO: ^ add protection from it and take it into account during compaction
  return make_exception_future(no_more_space_exception());
  }
 
@@ -250,8 +256,8 @@ std::variant<inode_t, metadata_log::path_lookup_error> metadata_log::path_lookup
  } else {
  auto dir_it = _inodes.find(components_stack.back());
  assert(dir_it != _inodes.end() and "inode comes from some previous lookup (or is a root directory) hence dir_it has to be valid");
- assert(std::holds_alternative<inode_info::directory>(dir_it->second.contents) and "every previous component is a directory and it was checked when they were processed");
- auto& curr_dir = std::get<inode_info::directory>(dir_it->second.contents);
+ assert(dir_it->second.is_directory() and "every previous component is a directory and it was checked when they were processed");
+ auto& curr_dir = dir_it->second.get_directory();
 
  auto it = curr_dir.entries.find(component);
  if (it == curr_dir.entries.end()) {
@@ -262,7 +268,7 @@ std::variant<inode_t, metadata_log::path_lookup_error> metadata_log::path_lookup
  if (check_if_dir) {
  auto entry_it = _inodes.find(entry_inode);
  assert(entry_it != _inodes.end() and "dir entries have to exist");
- if (not std::holds_alternative<inode_info::directory>(entry_it->second.contents)) {
+ if (not entry_it->second.is_directory()) {
  return path_lookup_error::NOT_DIR;
  }
  }

src/fs/metadata_log.hh

@@ -139,6 +139,8 @@ public:
 private:
  void memory_only_create_inode(inode_t inode, bool is_directory, unix_metadata metadata);
 
+ void memory_only_update_metadata(inode_t inode, unix_metadata metadata);
+
  void memory_only_delete_inode(inode_t inode);
 
  void memory_only_small_write(inode_t inode, disk_offset_t offset, temporary_buffer<uint8_t> data);
@@ -207,7 +209,7 @@ public:
  if (it == _inodes.end()) {
  return now(); // Directory disappeared
  }
- if (not std::holds_alternative<inode_info::directory>(it->second.contents)) {
+ if (not it->second.is_directory()) {
  return make_exception_future(path_component_not_directory());
  }
 
@@ -216,10 +218,10 @@ public:
  if (it == _inodes.end()) {
  return make_ready_future<stop_iteration>(stop_iteration::yes); // Directory disappeared
  }
- if (not std::holds_alternative<inode_info::directory>(it->second.contents)) {
- return make_ready_future<stop_iteration>(stop_iteration::yes); // Directory became a file (should not happen, but safe-check)
+ if (not it->second.is_directory()) {
+ return make_ready_future<stop_iteration>(stop_iteration::yes); // Directory became a file
  }
- auto& dir = std::get<inode_info::directory>(it->second.contents);
+ auto& dir = it->second.get_directory();
 
  auto entry_it = dir.entries.upper_bound(prev_entry);
  if (entry_it == dir.entries.end()) {

src/fs/metadata_log_bootstrap.hh

@@ -26,13 +26,13 @@
 #include "fs/inode_info.hh"
 #include "fs/metadata_disk_entries.hh"
 #include "fs/units.hh"
-#include "metadata_log.hh"
+#include "fs/metadata_log.hh"
 #include "seastar/core/do_with.hh"
 #include "seastar/core/future-util.hh"
 #include "seastar/core/future.hh"
 #include "seastar/core/sstring.hh"
 #include "seastar/core/temporary_buffer.hh"
-#include <bits/stdint-uintn.h>
+
 #include <boost/crc.hpp>
 #include <cstddef>
 #include <cstring>
@@ -61,7 +61,7 @@ public:
  return false;
  }
 
- memcpy(destination, _data + _pos, size);
+ std::memcpy(destination, _data + _pos, size);
  _pos += size;
  return true;
  }
@@ -111,7 +111,7 @@ class metadata_log_bootstrap {
  metadata_log& _metadata_log;
  cluster_range _available_clusters;
  std::unordered_set<cluster_id_t> _taken_clusters;
- std::optional<inode_t> _next_cluster;
+ std::optional<cluster_id_t> _next_cluster;
  temporary_buffer<uint8_t> _curr_cluster_data;
  data_reader _curr_cluster;
  data_reader _curr_checkpoint;
@@ -135,7 +135,7 @@ class metadata_log_bootstrap {
  }).then([this, &last_cluster] {
  // Initialize _curr_cluster_buff
  _metadata_log._curr_cluster_buff = make_lw_shared<metadata_to_disk_buffer>(_metadata_log._cluster_size, _metadata_log._alignment, 0);
- _metadata_log._curr_cluster_buff->reset_from_bootstraped_cluster(cluster_id_to_offset(last_cluster, _metadata_log._cluster_size), _curr_cluster_data.get(), _curr_cluster.curr_pos(), metadata_log::align_after_flush);
+ _metadata_log._curr_cluster_buff->reset_from_bootstrapped_cluster(cluster_id_to_offset(last_cluster, _metadata_log._cluster_size), _curr_cluster_data.get(), _curr_cluster.curr_pos(), metadata_log::align_after_flush);
  });
  }).then([this, fs_shards_pool_size, fs_shard_id] {
  // Initialize _cluser_allocator
@@ -268,6 +268,10 @@ class metadata_log_bootstrap {
  return invalid_entry_exception();
  }
 
+ if (_next_cluster.has_value()) {
+ return invalid_entry_exception(); // Only one NEXT_METADATA_CLUSTER may appear in one cluster
+ }
+
  _next_cluster = (cluster_id_t)entry.cluster_id;
  return now();
  }
@@ -292,7 +296,7 @@ class metadata_log_bootstrap {
  return invalid_entry_exception();
  }
 
- _metadata_log._inodes[entry.inode].metadata = ondisk_metadata_to_metadata(entry.metadata);
+ _metadata_log.memory_only_update_metadata(entry.inode, ondisk_metadata_to_metadata(entry.metadata));
  return now();
  }
 
@@ -307,7 +311,7 @@ class metadata_log_bootstrap {
  return invalid_entry_exception(); // Only unlinked inodes may be deleted
  }
 
- if (std::holds_alternative<inode_info::directory>(inode_info.contents) and not std::get<inode_info::directory>(inode_info.contents).entries.empty()) {
+ if (inode_info.is_directory() and not inode_info.get_directory().entries.empty()) {
  return invalid_entry_exception(); // Only empty directories may be deleted
  }
 
@@ -321,34 +325,37 @@ class metadata_log_bootstrap {
  return invalid_entry_exception();
  }
 
- auto data_opt = _curr_checkpoint.read_tmp_buff(entry.length);
- if (not data_opt) {
+ if (not _metadata_log._inodes[entry.inode].is_file()) {
  return invalid_entry_exception();
  }
- temporary_buffer<uint8_t>& data = *data_opt;
 
- if (not std::holds_alternative<inode_info::file>(_metadata_log._inodes[entry.inode].contents)) {
+ auto data_opt = _curr_checkpoint.read_tmp_buff(entry.length);
+ if (not data_opt) {
  return invalid_entry_exception();
  }
+ temporary_buffer<uint8_t>& data = *data_opt;
 
  _metadata_log.memory_only_small_write(entry.inode, entry.offset, std::move(data));
  _metadata_log.memory_only_update_mtime(entry.inode, entry.mtime_ns);
  return now();
  }
 
  future<> bootstrap_medium_write() {
+ // TODO: update _taken_clusters
  // TODO: will be very similar to SMALL_WRITE
  assert(false && "Not implemented");
  return now();
  }
 
  future<> bootstrap_large_write() {
+ // TODO: update _taken_clusters
  // TODO: will be very similar to SMALL_WRITE
  assert(false && "Not implemented");
  return now();
  }
 
  future<> bootstrap_large_write_without_mtime() {
+ // TODO: update _taken_clusters
  // TODO: will be very similar to SMALL_WRITE
  assert(false && "Not implemented");
  return now();
@@ -360,7 +367,7 @@ class metadata_log_bootstrap {
  return invalid_entry_exception();
  }
 
- if (not std::holds_alternative<inode_info::file>(_metadata_log._inodes[entry.inode].contents)) {
+ if (not _metadata_log._inodes[entry.inode].is_file()) {
  return invalid_entry_exception();
  }
 
@@ -391,14 +398,14 @@ class metadata_log_bootstrap {
  }
 
  // Only files may be linked as not to create cycles
- if (not std::holds_alternative<inode_info::file>(_metadata_log._inodes[entry.entry_inode].contents)) {
+ if (not _metadata_log._inodes[entry.entry_inode].is_file()) {
  return invalid_entry_exception();
  }
 
- if (not std::holds_alternative<inode_info::directory>(_metadata_log._inodes[entry.dir_inode].contents)) {
+ if (not _metadata_log._inodes[entry.dir_inode].is_directory()) {
  return invalid_entry_exception();
  }
- auto& dir = std::get<inode_info::directory>(_metadata_log._inodes[entry.dir_inode].contents);
+ auto& dir = _metadata_log._inodes[entry.dir_inode].get_directory();
 
  if (dir.entries.count(dir_entry_name) != 0) {
  return invalid_entry_exception();
@@ -420,10 +427,10 @@ class metadata_log_bootstrap {
  return invalid_entry_exception();
  }
 
- if (not std::holds_alternative<inode_info::directory>(_metadata_log._inodes[entry.dir_inode].contents)) {
+ if (not _metadata_log._inodes[entry.dir_inode].is_directory()) {
  return invalid_entry_exception();
  }
- auto& dir = std::get<inode_info::directory>(_metadata_log._inodes[entry.dir_inode].contents);
+ auto& dir = _metadata_log._inodes[entry.dir_inode].get_directory();
 
  if (dir.entries.count(dir_entry_name) != 0) {
  return invalid_entry_exception();
@@ -446,20 +453,16 @@ class metadata_log_bootstrap {
  return invalid_entry_exception();
  }
 
- if (not std::holds_alternative<inode_info::directory>(_metadata_log._inodes[entry.dir_inode].contents)) {
+ if (not _metadata_log._inodes[entry.dir_inode].is_directory()) {
  return invalid_entry_exception();
  }
- auto& dir = std::get<inode_info::directory>(_metadata_log._inodes[entry.dir_inode].contents);
+ auto& dir = _metadata_log._inodes[entry.dir_inode].get_directory();
 
  auto it = dir.entries.find(dir_entry_name);
  if (it == dir.entries.end()) {
  return invalid_entry_exception();
  }
 
- if (_metadata_log._inodes.at(it->second).directories_containing_file == 0) {
- return invalid_entry_exception();
- }
-
  _metadata_log.memory_only_delete_dir_entry(dir, std::move(dir_entry_name));
  // TODO: Maybe mtime_ns for modifying directory?
  return now();