Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed the number of chunks issue #415

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fixed the number of chunks issue #415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a3f9b8
Fixed the number of chunks issue
hkadayam May 11, 2024
a556f1e
Addressed review comments
hkadayam May 15, 2024
30cfed2
Merge branch 'master' into bitset_fix
hkadayam May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

class HomestoreConan(ConanFile):
name = "homestore"
version = "6.4.7"
version = "6.4.8"

homepage = "https://github.com/eBay/Homestore"
description = "HomeStore Storage Engine"
Expand Down
6 changes: 4 additions & 2 deletions src/lib/device/physical_dev.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,10 @@ void PhysicalDev::submit_batch() { m_drive_iface->submit_batch(); }

//////////////////////////// Chunk Creation/Load related methods /////////////////////////////////////////
void PhysicalDev::format_chunks() {
m_chunk_info_slots = std::make_unique< sisl::Bitset >(hs_super_blk::chunk_info_bitmap_size(m_dev_info));
m_chunk_info_slots = std::make_unique< sisl::Bitset >(std::max(1u, hs_super_blk::max_chunks_in_pdev(m_dev_info)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add alignment_size = 4096

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand the comment, why would I need to alignment_size here

Copy link
Contributor

@xiaoxichen xiaoxichen May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you specified alignment when initializing the bitset , and use same alignment configuration for serialize(), the serialize() is free, otherwise it is a copy.
c.f https://github.com/eBay/sisl/blob/03282536e309252d85319bebf4b227be0be6f9fe/include/sisl/fds/bitset.hpp#L647

Copy link
Contributor

@JacksonYao287 JacksonYao287 May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now , when we call max_chunks_in_pdev, we calculate the max_chunks_num using the following code.

(dinfo.dev_size - 1) / min_chunk_size + 1;

however , we have superblk and vdevinfo in the starting part of a pdev, which will also take some space of a pdev. so should we take into account this when calculating?

auto bitmap_mem = m_chunk_info_slots->serialize(m_pdev_info.dev_attr.align_size);
Copy link
Contributor

@xiaoxichen xiaoxichen May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_chunk_info_slots->serialized_size(4096);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are asking the bitset library to serialize based on pdev align size. So I don't see anything wrong here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in chunk_info_bitmap_size we round up to 4K instead of pdev align size. see the example here #415 (comment) we might hit the assert.

HS_REL_ASSERT_LE(bitmap_mem->size(), hs_super_blk::chunk_info_bitmap_size(m_dev_info),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see chunk_info_bitmap_size round up to 4K, and didnt properly handle the boundary when max_chunks_in_pdev % 8 != 0

however the bitset->serilize(align_size == 512) added up sizeof(bitset_serialized) and align to 512.

    static uint64_t chunk_info_bitmap_size(const dev_info& dinfo) {
        // Chunk bitmap area has bitmap of max_chunks rounded off to 4k page
        return sisl::round_up(std::max(1u, hs_super_blk::max_chunks_in_pdev(dinfo) / 8), 4096);
    }

           const uint64_t total_words{bitset_serialized::total_words(num_bits)};
           const uint64_t total_bytes{sizeof(bitset_serialized) + sizeof(word_t) * total_words};
           const uint64_t size{(alignment_size > 0) ? round_up(total_bytes, alignment_size) : total_bytes};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so if hs_super_blk::max_chunks_in_pdev(m_dev_info) == 4096 * 8, we get
hs_super_blk::chunk_info_bitmap_size ==4096
and
m_chunk_info_slots->serialize = round_up (4096 + sizeof(bitset_serialized), 512) = 4608

Copy link
Contributor

@xiaoxichen xiaoxichen May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe change chunk_info_bitmap_size to

static uint64_t chunk_info_bitmap_size(const dev_info& dinfo) {
        // Chunk bitmap area has bitmap of max_chunks rounded off to 4k page
        return sisl::round_up(bitset_serialized::nbytes(hs_super_blk::max_chunks_in_pdev(dinfo)), 4096));
}

"Chunk info serialized bitmap mismatch with expected size");
write_super_block(bitmap_mem->cbytes(), bitmap_mem->size(), hs_super_blk::chunk_sb_offset());
}

Expand All @@ -234,7 +236,7 @@ std::vector< shared< Chunk > > PhysicalDev::create_chunks(const std::vector< uin

try {
while (chunks_remaining > 0) {
auto b = m_chunk_info_slots->get_next_contiguous_n_reset_bits(0u, chunks_remaining);
auto b = m_chunk_info_slots->get_next_contiguous_n_reset_bits(0u, std::nullopt, 1u, chunks_remaining);
if (b.nbits == 0) { throw std::out_of_range("System has no room for additional chunk"); }

auto buf = hs_utils::iobuf_alloc(chunk_info::size * b.nbits, sisl::buftag::superblk,
Expand Down
Loading