From 9633f4a6a7069b051c44c94cab0c469f42f8d007 Mon Sep 17 00:00:00 2001 From: Xiaoxi Chen Date: Wed, 19 Jun 2024 12:05:10 +0800 Subject: [PATCH 1/4] Check buffer alignmen. This is the fix for https://github.com/eBay/HomeStore/issues/365 The root cause is we passed un-aligned memory address to pwrite. The alignment is not only on size but also on start_pos, previously we only check size, not the memory address. Signed-off-by: Xiaoxi Chen --- src/lib/common/homestore_utils.cpp | 7 +++++++ src/lib/common/homestore_utils.hpp | 1 + src/lib/meta/meta_blk_service.cpp | 8 +++++--- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/lib/common/homestore_utils.cpp b/src/lib/common/homestore_utils.cpp index 34c814fb9..a3eacbfdc 100644 --- a/src/lib/common/homestore_utils.cpp +++ b/src/lib/common/homestore_utils.cpp @@ -49,6 +49,13 @@ bool hs_utils::mod_aligned_sz(size_t size_to_check, size_t align_sz) { return !(size_to_check & static_cast< size_t >(align_sz - 1)); // return true if it is aligned. } +bool hs_utils::is_ptr_aligned(void* ptr, std::size_t alignment) { + // Cast the pointer to uintptr_t, which is an integer type capable of holding a pointer + auto intptr = reinterpret_cast(ptr); + // Check if the pointer is a multiple of the alignment + return (intptr % alignment) == 0; +} + sisl::byte_view hs_utils::create_byte_view(const uint64_t size, const bool is_aligned_needed, const sisl::buftag tag, const size_t alignment) { return (is_aligned_needed) ? sisl::byte_view{static_cast< uint32_t >(aligned_size(size, alignment)), diff --git a/src/lib/common/homestore_utils.hpp b/src/lib/common/homestore_utils.hpp index 7de531806..2ee51b03d 100644 --- a/src/lib/common/homestore_utils.hpp +++ b/src/lib/common/homestore_utils.hpp @@ -34,6 +34,7 @@ class hs_utils { static void iobuf_free(uint8_t* const ptr, const sisl::buftag tag, const size_t size); static uint64_t aligned_size(const size_t size, const size_t alignment); static bool mod_aligned_sz(const size_t size_to_check, const size_t align_sz); + static bool is_ptr_aligned(void* ptr, std::size_t alignment); static sisl::byte_view create_byte_view(const uint64_t size, const bool is_aligned_needed, const sisl::buftag tag, const size_t alignment); static sisl::io_blob create_io_blob(const uint64_t size, const bool is_aligned_needed, const sisl::buftag tag, diff --git a/src/lib/meta/meta_blk_service.cpp b/src/lib/meta/meta_blk_service.cpp index fb2792173..acc48ccf3 100644 --- a/src/lib/meta/meta_blk_service.cpp +++ b/src/lib/meta/meta_blk_service.cpp @@ -464,8 +464,8 @@ void MetaBlkService::write_ovf_blk_to_disk(meta_blk_ovf_hdr* ovf_hdr, const uint uint8_t* write_context_data = (const_cast< uint8_t* >(context_data) + offset); size_t write_size = ovf_hdr->h.context_sz; uint8_t* context_data_aligned{nullptr}; - if (!hs_utils::mod_aligned_sz(write_size, align_sz)) { - HS_LOG_EVERY_N(WARN, metablk, 50, "[type={}] Unaligned address found for input context_data.", type); + if (!hs_utils::is_ptr_aligned(write_context_data, align_sz) || !hs_utils::mod_aligned_sz(write_size, align_sz)) { + HS_LOG_EVERY_N(WARN, metablk, 50, "[type={}] Unaligned address found for input context_data, ptr {}, size {}, align {} ", type, (void *)write_context_data, write_size, align_sz); const size_t aligned_write_size = uint64_cast(sisl::round_up(write_size, align_sz)); context_data_aligned = hs_utils::iobuf_alloc(aligned_write_size, sisl::buftag::metablk, align_size()); std::memcpy(context_data_aligned, write_context_data, write_size); @@ -506,6 +506,8 @@ void MetaBlkService::write_ovf_blk_to_disk(meta_blk_ovf_hdr* ovf_hdr, const uint size_written += (ovf_hdr->h.context_sz - size_written); } + // check if the buffer is aligned + HS_REL_ASSERT(hs_utils::is_ptr_aligned(cur_ptr, align_sz) || hs_utils::mod_aligned_sz(cur_size, align_sz), "Unaligned address found for input context_data, ptr {}, size {}, align {} ", (void *)cur_ptr, cur_size, align_sz); auto error = m_sb_vdev->sync_write(r_cast< const char* >(cur_ptr), cur_size, data_bid[i]); if (error.value()) { // the offset and buffer length is printed in the error messages of iomgr. @@ -609,7 +611,7 @@ void MetaBlkService::write_meta_blk_ovf(BlkId& out_obid, const uint8_t* context_ context_data_blkids.clear(); alloc_meta_blks(sisl::round_up(sz, block_size()), context_data_blkids); - HS_LOG(DEBUG, metablk, "Start to allocate nblks(data): {}, mstore used size: {}", context_data_blkids.size(), + HS_LOG(DEBUG, metablk, "Context data size {}, rounded up to {}, block_size {}, allocated {} blkIDs, mstore used size: {}", sz, sisl::round_up(sz, block_size()), block_size(), context_data_blkids.size(), m_sb_vdev->used_size()); // return the 1st ovf header blk id to caller; From 0104a4be1d07d4324fc2cd652a18144f436a4485 Mon Sep 17 00:00:00 2001 From: Xiaoxi Chen Date: Thu, 20 Jun 2024 00:20:15 +0800 Subject: [PATCH 2/4] Restartbility fix in UT. We introduce restart for random_load_test that restarts HS every 8K IO. Previously the UT is not support restart within single test, as during restart all previous saved cookies are invalid, need to get from recovery. This commit fix the issue. Signed-off-by: Xiaoxi Chen --- src/tests/test_meta_blk_mgr.cpp | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/tests/test_meta_blk_mgr.cpp b/src/tests/test_meta_blk_mgr.cpp index 89372995a..8fc688dc4 100644 --- a/src/tests/test_meta_blk_mgr.cpp +++ b/src/tests/test_meta_blk_mgr.cpp @@ -266,7 +266,7 @@ class VMetaBlkMgrTest : public ::testing::Test { if (!done_read) { done_read = true; m_mbm->read_sub_sb(mtype); - const auto read_buf_str = m_cb_blks[mblk->hdr.h.bid.to_integer()]; + const auto read_buf_str = m_cb_blks[mblk->hdr.h.bid.to_integer()].str; const std::string write_buf_str = m_write_sbs[mblk->hdr.h.bid.to_integer()].str; const auto ret = read_buf_str.compare(write_buf_str); if (mblk->hdr.h.compressed == false) { @@ -327,7 +327,7 @@ class VMetaBlkMgrTest : public ::testing::Test { { std::unique_lock< std::mutex > lg{m_mtx}; - const auto read_buf_str = m_cb_blks[mblk->hdr.h.bid.to_integer()]; + const auto read_buf_str = m_cb_blks[mblk->hdr.h.bid.to_integer()].str; const std::string write_buf_str{str}; const auto ret = read_buf_str.compare(write_buf_str); HS_DBG_ASSERT(ret == 0, "Context data mismatch: Saved: {}, read: {}.", write_buf_str, read_buf_str); @@ -422,8 +422,8 @@ class VMetaBlkMgrTest : public ::testing::Test { HS_DBG_ASSERT(it_cb != std::cend(m_cb_blks), "Saved bid during write not found in recover callback."); // the saved buf should be equal to the buf received in the recover callback; - const int ret = it->second.str.compare(it_cb->second); - HS_DBG_ASSERT(ret == 0, "Context data mismatch: Saved: {}, callback: {}.", it->second.str, it_cb->second); + const int ret = it->second.str.compare(it_cb->second.str); + HS_DBG_ASSERT(ret == 0, "Context data mismatch: Saved: {}, callback: {}.", it->second.str, it_cb->second.str); } } @@ -497,6 +497,7 @@ class VMetaBlkMgrTest : public ::testing::Test { void recover_with_on_complete() { // restart will cause recovery and callbacks will be triggered + ++m_restart_cnt; m_cb_blks.clear(); restart_homestore(); } @@ -504,6 +505,8 @@ class VMetaBlkMgrTest : public ::testing::Test { void validate() { // verify received blks via callbaks are all good; verify_cb_blks(); + m_write_sbs.swap(m_cb_blks); + m_cb_blks.clear(); } meta_op_type get_op() { @@ -537,7 +540,7 @@ class VMetaBlkMgrTest : public ::testing::Test { } } - uint64_t total_op_cnt() const { return m_update_cnt + m_wrt_cnt + m_rm_cnt; } + uint64_t total_op_cnt() const { return m_update_cnt + m_wrt_cnt + m_rm_cnt + m_restart_cnt; } uint32_t write_ratio() const { if (m_wrt_cnt == 0) return 0; @@ -579,24 +582,28 @@ class VMetaBlkMgrTest : public ::testing::Test { m_update_cnt = 0; m_rm_cnt = 0; m_total_wrt_sz = 0; + m_restart_cnt = 0; } void register_client() { m_mbm = &(meta_service()); m_total_wrt_sz = m_mbm->used_size(); - HS_REL_ASSERT_EQ(m_mbm->total_size() - m_total_wrt_sz, m_mbm->available_blks() * m_mbm->block_size()); - m_mbm->deregister_handler(mtype); m_mbm->register_handler( mtype, [this](meta_blk* mblk, sisl::byte_view buf, size_t size) { if (mblk) { std::unique_lock< std::mutex > lg{m_mtx}; - m_cb_blks[mblk->hdr.h.bid.to_integer()] = md5_sum(r_cast< const char* >(buf.bytes()), size); + m_cb_blks[mblk->hdr.h.bid.to_integer()] = sb_info_t { + mblk, md5_sum(r_cast< const char* >(buf.bytes()), size)}; } }, - [this](bool success) { HS_DBG_ASSERT_EQ(success, true); }); + [this](bool success) { + HS_DBG_ASSERT_EQ(success, true); + m_total_wrt_sz = m_mbm->used_size(); + HS_REL_ASSERT_EQ(m_mbm->total_size() - m_total_wrt_sz, m_mbm->available_blks() * m_mbm->block_size()); + }); } void register_client_inlcuding_dependencies() { @@ -682,7 +689,8 @@ class VMetaBlkMgrTest : public ::testing::Test { [this](meta_blk* mblk, sisl::byte_view buf, size_t size) { if (mblk) { std::unique_lock< std::mutex > lg{m_mtx}; - m_cb_blks[mblk->hdr.h.bid.to_integer()] = std::string{r_cast< const char* >(buf.bytes()), size}; + m_cb_blks[mblk->hdr.h.bid.to_integer()] = sb_info_t { + mblk, md5_sum(r_cast< const char* >(buf.bytes()), size)}; } }, [this](bool success) { HS_DBG_ASSERT_EQ(success, true); }); @@ -702,10 +710,11 @@ class VMetaBlkMgrTest : public ::testing::Test { uint64_t m_wrt_cnt{0}; uint64_t m_update_cnt{0}; uint64_t m_rm_cnt{0}; + uint64_t m_restart_cnt{0}; uint64_t m_total_wrt_sz{0}; MetaBlkService* m_mbm{nullptr}; std::map< uint64_t, sb_info_t > m_write_sbs; // during write, save blkid to buf map; - std::map< uint64_t, std::string > m_cb_blks; // during recover, save blkid to buf map; + std::map< uint64_t, sb_info_t > m_cb_blks; // during recover, save blkid to buf map; std::mutex m_mtx; #ifdef _PRERELEASE flip::FlipClient m_fc{HomeStoreFlip::instance()}; From a045654d78ea48301ebc2c17c261bebbe6d81583 Mon Sep 17 00:00:00 2001 From: Xiaoxi Chen Date: Thu, 20 Jun 2024 00:41:14 -0700 Subject: [PATCH 3/4] update conan Signed-off-by: Xiaoxi Chen --- conanfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conanfile.py b/conanfile.py index d01d5305e..6787c8b9c 100644 --- a/conanfile.py +++ b/conanfile.py @@ -5,7 +5,7 @@ class HomestoreConan(ConanFile): name = "homestore" - version = "6.4.19" + version = "6.4.20" homepage = "https://github.com/eBay/Homestore" description = "HomeStore Storage Engine" topics = ("ebay", "nublox") From 3f86b334e9f760cb3a66d49e077bce26447547b1 Mon Sep 17 00:00:00 2001 From: Xiaoxi Chen Date: Thu, 20 Jun 2024 23:09:28 +0800 Subject: [PATCH 4/4] Logging at the end of the test. Signed-off-by: Xiaoxi Chen --- src/tests/test_meta_blk_mgr.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/tests/test_meta_blk_mgr.cpp b/src/tests/test_meta_blk_mgr.cpp index 8fc688dc4..6a924d327 100644 --- a/src/tests/test_meta_blk_mgr.cpp +++ b/src/tests/test_meta_blk_mgr.cpp @@ -403,7 +403,7 @@ class VMetaBlkMgrTest : public ::testing::Test { iomanager.iobuf_free(buf); } else { if (unaligned_addr) { - delete[](buf - unaligned_shift); + delete[] (buf - unaligned_shift); } else { delete[] buf; } @@ -423,7 +423,8 @@ class VMetaBlkMgrTest : public ::testing::Test { // the saved buf should be equal to the buf received in the recover callback; const int ret = it->second.str.compare(it_cb->second.str); - HS_DBG_ASSERT(ret == 0, "Context data mismatch: Saved: {}, callback: {}.", it->second.str, it_cb->second.str); + HS_DBG_ASSERT(ret == 0, "Context data mismatch: Saved: {}, callback: {}.", it->second.str, + it_cb->second.str); } } @@ -475,6 +476,8 @@ class VMetaBlkMgrTest : public ::testing::Test { break; } } + LOGINFO("rand load test finished, total ops: {}, write ops: {}, remove ops:{}, update ops: {}, restart: {}", + total_op_cnt(), m_wrt_cnt, m_rm_cnt, m_update_cnt, m_restart_cnt); } bool do_overflow() const { @@ -595,8 +598,8 @@ class VMetaBlkMgrTest : public ::testing::Test { [this](meta_blk* mblk, sisl::byte_view buf, size_t size) { if (mblk) { std::unique_lock< std::mutex > lg{m_mtx}; - m_cb_blks[mblk->hdr.h.bid.to_integer()] = sb_info_t { - mblk, md5_sum(r_cast< const char* >(buf.bytes()), size)}; + m_cb_blks[mblk->hdr.h.bid.to_integer()] = + sb_info_t{mblk, md5_sum(r_cast< const char* >(buf.bytes()), size)}; } }, [this](bool success) { @@ -689,8 +692,8 @@ class VMetaBlkMgrTest : public ::testing::Test { [this](meta_blk* mblk, sisl::byte_view buf, size_t size) { if (mblk) { std::unique_lock< std::mutex > lg{m_mtx}; - m_cb_blks[mblk->hdr.h.bid.to_integer()] = sb_info_t { - mblk, md5_sum(r_cast< const char* >(buf.bytes()), size)}; + m_cb_blks[mblk->hdr.h.bid.to_integer()] = + sb_info_t{mblk, md5_sum(r_cast< const char* >(buf.bytes()), size)}; } }, [this](bool success) { HS_DBG_ASSERT_EQ(success, true); }); @@ -714,7 +717,7 @@ class VMetaBlkMgrTest : public ::testing::Test { uint64_t m_total_wrt_sz{0}; MetaBlkService* m_mbm{nullptr}; std::map< uint64_t, sb_info_t > m_write_sbs; // during write, save blkid to buf map; - std::map< uint64_t, sb_info_t > m_cb_blks; // during recover, save blkid to buf map; + std::map< uint64_t, sb_info_t > m_cb_blks; // during recover, save blkid to buf map; std::mutex m_mtx; #ifdef _PRERELEASE flip::FlipClient m_fc{HomeStoreFlip::instance()};