Dead Page Reference Count Bug Fix #181
Conversation
bhaskarbora2
changed the title
| @@ -39,6 +39,8 @@ struct PageToRecycle { | |||
| // | |||
| i32 depth; | |||
|
|
|||
| slot_offset_type offset_as_unique_identifier; | |||
There was a problem hiding this comment.
This is a bit verbose... maybe we can use the same convention as PageAllocator, which calls this user_slot?
See: https://github.com/mathworks/llfs/blob/main/src/llfs/page_allocator_events.hpp#L44
| //+++++++++++-+-+--+----- --- -- - - - - | ||
|
|
||
| static PageCount default_max_buffered_page_count(const PageRecyclerOptions& options); | ||
|
|
||
| static u64 calculate_log_size(const PageRecyclerOptions& options, | ||
| Optional<PageCount> max_buffered_page_count = None); | ||
|
|
||
| static u64 calculate_log_size_no_padding(const PageRecyclerOptions& options, |
There was a problem hiding this comment.
Would you please add doc comments for this new function? Thanks!
There was a problem hiding this comment.
General comment: on functions that return Status or StatusOr, usually it's best to declare as noexcept. My advice would be to do this on functions whose signature we modify, or newly added functions; as opposed to just going through and changing a bunch of existing signatures.
| @@ -55,13 +55,21 @@ class PageRecycler | |||
| CountMetric<u64> page_drop_error_count{0}; | |||
| }; | |||
|
|
|||
| struct GlobalMetrics { | |||
| CountMetric<u32> page_id_deletion_reissue_count{0}; | |||
There was a problem hiding this comment.
Why not u64 here? Not that we expect a lot of these, but why not?
| // recycle_pages. | ||
| // | ||
| StatusOr<slot_offset_type> recycle_pages_depth_n(const Slice<const PageId>& page_ids_in, | ||
| batt::Grant* grant, const i32 depth); |
There was a problem hiding this comment.
Now that the grant param is required for this overload, consider changing the type to batt::Grant&.
|
|
||
| // This tracks the page index within recycle_pages request. | ||
| // | ||
| u16 page_index; |
There was a problem hiding this comment.
I think this should be u32 at least; it's reasonable to expect that we may trim a log multiple MB at a time, in which case we could easily have more than 64k pages.
| @@ -132,12 +142,14 @@ struct PackedPageToRecycle { | |||
|
|
|||
| little_page_id_int page_id; | |||
| little_u64 batch_slot; | |||
| u64 offset_as_unique_identifier; | |||
There was a problem hiding this comment.
Please use little_u64 here.
| little_i32 depth; | ||
| u16 page_index; |
There was a problem hiding this comment.
This should be little_u32.
| @@ -111,13 +113,28 @@ Optional<SlotRange> PageRecyclerRecoveryVisitor::latest_info_refresh_slot() cons | |||
| return this->latest_info_refresh_slot_; | |||
| } | |||
|
|
|||
| //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - | |||
| // | |||
| slot_offset_type PageRecyclerRecoveryVisitor::volume_trim_offset() const | |||
There was a problem hiding this comment.
nit: Maybe have the member function name match the member data name? volume_trim_slot()?
| // Save the largest unique offset identifier. Note that if offsets are same then we need to only | ||
| // update the largest page_index. | ||
| // | ||
| if (this->volume_trim_slot_ < recovered.offset_as_unique_identifier) { |
There was a problem hiding this comment.
We should always use llfs::slot_less_than (and friends; see https://github.com/mathworks/llfs/blob/main/src/llfs/slot.hpp#L36) to compare two slot offset values, instead of the built-in comparison operators.
| @@ -45,6 +45,9 @@ class PageRecyclerRecoveryVisitor | |||
|
|
|||
| Optional<SlotRange> latest_info_refresh_slot() const; | |||
|
|
|||
| slot_offset_type volume_trim_offset() const; | |||
There was a problem hiding this comment.
Great! Consider grouping these together as a single struct. The advantages would be:
- Code that calls this API gets a very strong "hint" that the two always need to be called together (and the pair needs to travel around together)
- Can define custom operators for that type, to re-use the
slot_less_thanand lexicographical ordering that is coded inline in a couple places currently.
| @@ -29,6 +29,8 @@ namespace llfs { | |||
| , latest_batch_{} | |||
| , recycler_uuid_{boost::uuids::random_generator{}()} | |||
| , latest_info_refresh_slot_{} | |||
| , volume_trim_slot_{0} | |||
There was a problem hiding this comment.
Consider having these be Optional so that we can detect that case where no slot/index information could be recovered from the log. It's unlikely the slow will over overflow (being 64-bits), but making this Optional would also have the benefit of handling integer wrap (like slot_less_than).
This MR is to release the fix for ref count bug in page recycler's dead page recycling flow. This issue crops up when volume trimmer and page recycler retries a dead page recycling task for a page after recovery. The solution is to track largest_slot_offset in recycler which will be checked at the time of receiving any new dead_page recycling request from volume trimmer. Any request having lower or equal slot_offset compare to largest-slot-offset will be ignored.
The MR also adds in a histogram plot to show where all we are hitting the bug over the entire seed range. This is done through an existing page ref count test.
#13