ThinProvision: skip all zero buffers #318
Conversation
| return result; | ||
| } | ||
|
|
||
| static std::vector< std::pair< int, int > > compute_range_intervals(const uint8_t* buf, size_t page_size, |
There was a problem hiding this comment.
for later extension on subranges
shosseinimotlagh
changed the base branch from
thin_provisioning
to
stable/v3.x
shosseinimotlagh
changed the title
| @@ -18,7 +18,6 @@ AlignOperands: false | |||
| AlignTrailingComments: true | |||
| AllowShortBlocksOnASingleLine: true | |||
| AllowShortIfStatementsOnASingleLine: true | |||
| AllowShortBlocksOnASingleLine: true | |||
There was a problem hiding this comment.
verbose (line 19)
shosseinimotlagh
changed the base branch from
stable/v3.x
to
thin_provisioning
src/homeblks/volume/volume.cpp
Outdated
| std::vector< bool > empty_blocks; | ||
|
|
||
| auto is_buf_empty = [](const uint8_t* buf, size_t size) -> bool { | ||
| return buf[0] == 0 && !std::memcmp(buf, buf + 1, size - 1); |
There was a problem hiding this comment.
as we discussed, you are going to change it with the intel crc api to detect zerod crc then do a bitwise compare, right?
There was a problem hiding this comment.
affirmative. I will make this change and will update this pr
| @@ -142,6 +142,8 @@ table Generic { | |||
| // percentage of cache used to create indx mempool. It should be more than 100 to | |||
| // take into account some floating buffers in writeback cache. | |||
| indx_mempool_percent : uint32 = 110; | |||
|
|
|||
| boot_thin_provisioning: bool = false; | |||
There was a problem hiding this comment.
What is the deployment plan to enable this feature in production if this is defaulted to false?
| @@ -335,7 +335,105 @@ indx_tbl* Volume::recover_indx_tbl(btree_super_block& sb, btree_cp_sb& cp_info) | |||
| return static_cast< indx_tbl* >(tbl); | |||
| } | |||
|
|
|||
| static std::vector< std::pair< int, int > > get_true_intervals(const std::vector< bool >& empty_blocks) { | |||
There was a problem hiding this comment.
I don't see any place using this api as well as copmute_range_intervals, can we remove them unless there is a plan to use them in the near future?
| return is_iovec_zero(iface_req->iovecs); | ||
| } | ||
|
|
||
| static std::string print_ranges(lba_t start_lba, const std::vector< std::pair< int, int > >& intervals) { |
There was a problem hiding this comment.
this one also can be removed if not going to be used
There was a problem hiding this comment.
Can you put these three APIs that are currently not used together and make an #if 0 around them, so that it would be easier for others to read and understand this thin-provisioning code? Also put comment that these apis are for future usage if we support paritial zero detection.
| @@ -2758,6 +2795,9 @@ int main(int argc, char* argv[]) { | |||
| gcfg.app_mem_size_in_gb = SISL_OPTIONS["app_mem_size_in_gb"].as< uint32_t >(); | |||
| gcfg.vol_copy_file_path = SISL_OPTIONS["vol_copy_file_path"].as< std::string >(); | |||
| const auto io_size_in_kb = SISL_OPTIONS["io_size"].as< uint32_t >(); | |||
| gcfg.p_zero_buffer = SISL_OPTIONS["p_zero_buffer"].as< uint32_t >(); | |||
There was a problem hiding this comment.
as we discussed, would suggest to add a basic zero write/read test case:
- populate real data from LBA [0, 2MB]
- write all zeros to [128K, 128K + 32K]
- read LBA from [128K, 128K + 32K], verify all zeros are returned.
Another basic test:
Similar as above one, only skip the 1st step, e.g. write zeros to LBAs that has never been written before.
This is to verify that new writes and/or overwrite with all zeros both works.
There was a problem hiding this comment.
Sounds good to me. I am adding it to the PR
Add thin flag Add baseline functions for thin pro
No description provided.