[Buffer Manager] Add StorageRegion class to read and write pages to disk #2621

nikriek · 2023-10-17T12:47:27Z

This PR depends on #2618.

We add the StorageRegion class that allows reading and writing pages to disk using standard posix IO.

…:nikriek/hyrise into nikriek/buffer-manager/persistent-region

…thub.com/hyrise/hyrise into nikriek/buffer-manager/persistent-region

src/lib/storage/buffer/persistence_manager.hpp

src/test/lib/storage/buffer/persistence_manager_test.cpp

…sistent-region

Bouncner · 2023-11-06T21:05:18Z

src/lib/storage/buffer/page_id.hpp

@@ -95,7 +95,7 @@ struct PageID {

 static_assert(sizeof(PageID) == 8, "PageID must be 64 bit");

-std::ostream& operator<<(std::ostream& os, const PageID& page_id) {
+inline std::ostream& operator<<(std::ostream& os, const PageID& page_id) {


Why inline?

Bouncner · 2023-11-06T21:15:54Z

src/lib/storage/buffer/storage_region.cpp

@@ -0,0 +1,166 @@
+#include "storage_region.hpp"
+#ifdef __linux__


Bouncner · 2023-11-06T21:16:04Z

src/lib/storage/buffer/storage_region.cpp

+
+namespace hyrise {
+
+inline void DebugAssertPageAlignment(const std::byte* data) {


Why inline?

Bouncner · 2023-11-06T21:28:35Z

src/lib/storage/buffer/storage_region.cpp

+
+inline void DebugAssertPageAlignment(const std::byte* data) {
+  DebugAssert(reinterpret_cast<std::uintptr_t>(data) % 512 == 0,
+              "Data buffer is not properly aligned to 512 byte boundary as required for direct IO: " +


Can it be "inproperly aligned to 512 byte"?

Bouncner · 2023-11-06T22:04:24Z

src/lib/storage/buffer/storage_region.cpp

+static StorageRegion::Mode find_mode_or_fail(const std::filesystem::path& file_name) {
+  if (std::filesystem::is_directory(file_name)) {
+    return StorageRegion::Mode::FILE_PER_PAGE_SIZE;
+  } else if (std::filesystem::is_block_file(file_name)) {


I guess tidy will complain to avoid else after return.

Two stand-alone if's with a trailing fail?

Bouncner · 2023-11-08T22:10:30Z

src/lib/storage/buffer/storage_region.cpp

+  if (fcntl(fd, F_NOCACHE, 1) == -1) {
+    const auto error = errno;
+    close(fd);
+    Fail("Error while setting F_NOCACHE on __APPLE__: " + strerror(error));


on macOS? Looks like a placeholder that wasn't replaced.

Bouncner · 2023-11-08T22:14:35Z

src/lib/storage/buffer/storage_region.cpp

+  const auto handle = _file_handles[static_cast<uint64_t>(page_id.size_type())];
+  const auto byte_count = page_id.byte_count();
+  const auto offset = handle.offset + byte_count * page_id.index();
+  DebugAssertPageAlignment(data);


Doesn't really matter, but I would check for asserts on the input parameters in the beginning of the method.

Bouncner · 2023-11-08T22:17:30Z

src/lib/storage/buffer/storage_region.cpp

+std::array<StorageRegion::FileHandle, PAGE_SIZE_TYPES_COUNT> StorageRegion::_open_file_handles_in_directory(
+    const std::filesystem::path& path) {
+  DebugAssert(std::filesystem::is_directory(path), "StorageRegion path must be a directory");
+  auto array = std::array<StorageRegion::FileHandle, PAGE_SIZE_TYPES_COUNT>{};


auto file_handles = ...?

array can be more descriptive, see Martin's comment

Bouncner · 2023-11-08T22:35:45Z

src/lib/storage/buffer/storage_region.cpp

+                                   std::to_string(page_size_type_index) + ".bin");
+    array[page_size_type_index] = {_open_file_descriptor(file_name), file_name};
+    // We initialize the file with 32 MiB.
+    std::filesystem::resize_file(file_name, 1UL << 25);


When/how is this ever resized? Let's say I run SF 1000.

So we always use something like 300 MB for the StorageRegion?

When/how is this ever resized? Let's say I run SF 1000.

if you use pwrite to write data to a position beyond the current end of the file, the OS will extend the file size to include the new data.

Bouncner · 2023-11-08T22:37:21Z

src/lib/storage/buffer/storage_region.cpp

+
+  const auto fd = _open_file_descriptor(path);
+
+  // ioctl with the BLKGETSIZE64 allows us to get the size of the block device on Linux


Trailing periods.

mweisgut · 2023-11-08T20:01:12Z

src/lib/storage/buffer/storage_region.cpp

+int StorageRegion::_open_file_descriptor(const std::filesystem::path& file_name) {
+  // Partially taken from
+  // DuckDB: https://github.com/duckdb/duckdb/blob/60ed227816669be497fa4ba53e593d3899479c43/src/common/local_file_system.cpp
+  // LevelDB: https://github.com/google/leveldb/commit/296de8d5b8e4e57bd1e46c981114dfbe58a8c4fa


Rather use a permalink to the file instead of the commit?

mweisgut · 2023-11-08T20:01:36Z

src/lib/storage/buffer/storage_region.cpp

+  // LevelDB: https://github.com/google/leveldb/commit/296de8d5b8e4e57bd1e46c981114dfbe58a8c4fa
+
+  // On Linux, we use O_DIRECT to bypass the page cache. This is not supported on Mac,
+  // but we use the fcntl(F_NOCACHE) call below. Other nan that, files are opened in


Suggested change

// but we use the fcntl(F_NOCACHE) call below. Other nan that, files are opened in

// but we use the fcntl(F_NOCACHE) call below. Other than that, files are opened in

mweisgut · 2023-11-08T20:51:33Z

src/lib/storage/buffer/storage_region.hpp

+namespace hyrise {
+
+/**
+ * StorageRegion manages read and write access of pages to an SSD. StorageRegion can either use a block device directly by assigning equal-sized regions


Suggested change

* StorageRegion manages read and write access of pages to an SSD. StorageRegion can either use a block device directly by assigning equal-sized regions

* StorageRegion manages read and write access of pages to a storage device. StorageRegion can either use a block device directly by assigning equal-sized regions

mweisgut · 2023-11-17T14:28:53Z

src/lib/storage/buffer/storage_region.cpp

+std::array<StorageRegion::FileHandle, PAGE_SIZE_TYPES_COUNT> StorageRegion::_open_file_handles_in_directory(
+    const std::filesystem::path& path) {
+  DebugAssert(std::filesystem::is_directory(path), "StorageRegion path must be a directory");
+  auto array = std::array<StorageRegion::FileHandle, PAGE_SIZE_TYPES_COUNT>{};


array can be more descriptive, see Martin's comment

mweisgut · 2023-11-17T14:39:00Z

src/lib/storage/buffer/storage_region.cpp

+
+std::array<StorageRegion::FileHandle, PAGE_SIZE_TYPES_COUNT> StorageRegion::_open_file_handles_block(
+    const std::filesystem::path& path) {
+  DebugAssert(std::filesystem::is_block_file(path), "StorageRegion path must be a directory");


Suggested change

DebugAssert(std::filesystem::is_block_file(path), "StorageRegion path must be a directory");

DebugAssert(std::filesystem::is_block_file(path), "StorageRegion path must be a block device");

mweisgut · 2023-11-17T15:36:18Z

src/lib/storage/buffer/storage_region.hpp

+  std::atomic_uint64_t _total_bytes_written = 0;
+  std::atomic_uint64_t _total_bytes_read = 0;
+
+  // Open file haswndles for the given path. If the path is a directory,


Similar to Martin's comment below, this comment sounds like multiple types of paths are possible here.

mweisgut · 2023-11-17T15:48:35Z

src/test/lib/storage/buffer/storage_region_test.cpp

+}
+
+TEST_F(StorageRegionTest, WriteFailsWithUnalignedData) {
+  if constexpr (!HYRISE_DEBUG) {


A comment here about why this test case is only executed in Debug build would help.

mweisgut · 2023-11-17T15:48:41Z

src/test/lib/storage/buffer/storage_region_test.cpp

+}
+
+TEST_F(StorageRegionTest, ReadFailsWithUnalignedData) {
+  if constexpr (!HYRISE_DEBUG) {


A comment here about why this test case is only executed in debug build would help.

mweisgut · 2023-11-17T15:51:29Z

src/test/lib/storage/buffer/storage_region_test.cpp

+  std::memset(write_page.data.data(), 0x1, page_id.byte_count());
+  storage_region->write_page(page_id, write_page.data.data());
+  storage_region->read_page(page_id, read_page1.data.data());
+  EXPECT_EQ(std::memcmp(read_page1.data.data(), read_page1.data.data(), page_id.byte_count()), 0);


rhs and lhs are the buffer pointers. Better compare write_page and read_page1?

mweisgut · 2023-11-17T15:54:39Z

src/test/lib/storage/buffer/storage_region_test.cpp

+  auto read_page2 = Page{};
+  std::memset(write_page.data.data(), 0x2, page_id.byte_count());
+  storage_region->write_page(page_id, write_page.data.data());
+  EXPECT_NE(std::memcmp(read_page1.data.data(), read_page2.data.data(), page_id.byte_count()), 0);


read_page2 is default initialized here. read_page(page_id, read_page2) is missing.

nikriek and others added 8 commits October 12, 2023 11:34

Add frame and page id

07f5d3b

Add some more debug asserts

965e2ca

Add new line chars

fe677fe

Fix

c6398ed

Fix spaceship operator for now

55d535d

Add persistence manager

aaa0313

Merge branch 'master' into nikriek/buffer-manager/persistent-region

f97d8dc

Update naming and tests

eb6161f

nikriek mentioned this pull request Oct 17, 2023

[Buffer Manager] Add Persistence Manager class to read and write pages to disk #2619

Closed

Merge branch 'master' into nikriek/buffer-manager/persistent-region

2d4fb88

nikriek added the FullCI Run all CI tests (slow, but required for merge) label Oct 17, 2023

nikriek added 9 commits October 17, 2023 16:26

Use list directory function

6b53e05

Merge branch 'nikriek/buffer-manager/persistent-region' of github.com…

6165932

…:nikriek/hyrise into nikriek/buffer-manager/persistent-region

Remove comma

9db5b6a

Merge branch 'nikriek/buffer-manager/persistent-region' of https://gi…

638390f

…thub.com/hyrise/hyrise into nikriek/buffer-manager/persistent-region

Fix tests

843d539

Remove cout and add more tests

2888800

Add more comments

47a1944

Some minor fix

d2f5018

Some fixes for persistence manager

a426139

mweisgut reviewed Oct 26, 2023

View reviewed changes

nikriek added 3 commits October 27, 2023 13:30

Rename

32b07ac

Snapshot

577adc4

Merge remote-tracking branch 'origin' into nikriek/buffer-manager/per…

e84ab16

…sistent-region

nikriek changed the title ~~[Buffer Manager] Add Persistence Manager class to read and write pages to disk~~ [Buffer Manager] Add StorageRegion class to read and write pages to disk Nov 2, 2023

nikriek added 2 commits November 3, 2023 08:49

Adapt to merge

ccab151

Remove buffer manager artifct

971a72b

nikriek marked this pull request as ready for review November 3, 2023 07:53

nikriek added 2 commits November 5, 2023 16:14

Improve tests

e8b8e3e

Fix comments and stack issue

5d6ac42

nikriek requested a review from mweisgut November 5, 2023 21:03

nikriek added 5 commits November 6, 2023 08:50

Empty-Commit

7cf875b

Try fix lint issues

8234550

Fix order

927cbe3

Fix order

f566323

Skip some tests in debug mode

c4f3815

Bouncner reviewed Nov 8, 2023

View reviewed changes

mweisgut reviewed Nov 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Buffer Manager] Add StorageRegion class to read and write pages to disk #2621

[Buffer Manager] Add StorageRegion class to read and write pages to disk #2621

nikriek commented Oct 17, 2023 •

edited

Bouncner Nov 6, 2023

Bouncner Nov 6, 2023

Bouncner Nov 6, 2023

Bouncner Nov 6, 2023

Bouncner Nov 6, 2023

Bouncner Nov 8, 2023

Bouncner Nov 8, 2023

Bouncner Nov 8, 2023

mweisgut Nov 17, 2023

Bouncner Nov 8, 2023

Bouncner Nov 8, 2023

mweisgut Nov 17, 2023

Bouncner Nov 8, 2023

mweisgut Nov 8, 2023

mweisgut Nov 8, 2023

mweisgut Nov 8, 2023

mweisgut Nov 17, 2023

mweisgut Nov 17, 2023

mweisgut Nov 17, 2023

mweisgut Nov 17, 2023

mweisgut Nov 17, 2023

mweisgut Nov 17, 2023

mweisgut Nov 17, 2023

		@@ -0,0 +1,166 @@
		#include "storage_region.hpp"
		#ifdef __linux__


		namespace hyrise {

		inline void DebugAssertPageAlignment(const std::byte* data) {


		const auto fd = _open_file_descriptor(path);

		// ioctl with the BLKGETSIZE64 allows us to get the size of the block device on Linux

	// but we use the fcntl(F_NOCACHE) call below. Other nan that, files are opened in
	// but we use the fcntl(F_NOCACHE) call below. Other than that, files are opened in

	* StorageRegion manages read and write access of pages to an SSD. StorageRegion can either use a block device directly by assigning equal-sized regions
	* StorageRegion manages read and write access of pages to a storage device. StorageRegion can either use a block device directly by assigning equal-sized regions

	DebugAssert(std::filesystem::is_block_file(path), "StorageRegion path must be a directory");
	DebugAssert(std::filesystem::is_block_file(path), "StorageRegion path must be a block device");

[Buffer Manager] Add StorageRegion class to read and write pages to disk #2621

Are you sure you want to change the base?

[Buffer Manager] Add StorageRegion class to read and write pages to disk #2621

Conversation

nikriek commented Oct 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikriek commented Oct 17, 2023 •

edited