Dead Page Reference Count Bug Fix #181

bhaskarbora2 · 2024-12-18T16:55:36Z

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

tonyastolfi · 2025-01-28T19:15:12Z

src/llfs/page_recycler_events.hpp

@@ -39,6 +39,8 @@ struct PageToRecycle {
  //
  i32 depth;

+  slot_offset_type offset_as_unique_identifier;


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

sure.. this is all changed now. As discussed will use volume_trim_slot instead.

src/llfs/page_allocator_state.hpp

src/llfs/page_recycler.hpp

tonyastolfi · 2025-01-28T19:34:03Z

src/llfs/page_recycler.hpp

  //+++++++++++-+-+--+----- --- -- -  -  -   -

  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,


Would you please add doc comments for this new function? Thanks!

src/llfs/page_recycler.hpp

tonyastolfi

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.

tonyastolfi · 2025-02-04T19:13:38Z

src/llfs/page_recycler.hpp

@@ -55,13 +55,21 @@ class PageRecycler
    CountMetric<u64> page_drop_error_count{0};
  };

+  struct GlobalMetrics {
+    CountMetric<u32> page_id_deletion_reissue_count{0};


Why not u64 here? Not that we expect a lot of these, but why not?

My reasoning was this. Say we do recovery every second and we dedup all these requests then we could do that for next ~50,000 years with a 32-bit counter. Why allocate more space when we are never going to hit the limit.

tonyastolfi · 2025-02-04T19:17:28Z

src/llfs/page_recycler.hpp

+  // recycle_pages.
+  //
+  StatusOr<slot_offset_type> recycle_pages_depth_n(const Slice<const PageId>& page_ids_in,
+                                                   batt::Grant* grant, const i32 depth);


Now that the grant param is required for this overload, consider changing the type to batt::Grant&.

tonyastolfi

Here's what I have so far... I need to review the page_recycler.cpp changes in more detail. Will post another review later.

tonyastolfi · 2025-02-04T19:27:14Z

src/llfs/page_recycler_events.hpp

+
+  // This tracks the page index within recycle_pages request.
+  //
+  u16 page_index;


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.

tonyastolfi · 2025-02-04T19:29:33Z

src/llfs/page_recycler_events.hpp

@@ -132,12 +142,14 @@ struct PackedPageToRecycle {

  little_page_id_int page_id;
  little_u64 batch_slot;
+  u64 offset_as_unique_identifier;


Please use little_u64 here.

sure... this part of the code is all changed now.

tonyastolfi · 2025-02-04T19:35:30Z

src/llfs/page_recycler_events.hpp

  little_i32 depth;
+  u16 page_index;


This should be little_u32.

tonyastolfi · 2025-02-04T19:39:54Z

src/llfs/page_recycler_recovery_visitor.cpp

@@ -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


nit: Maybe have the member function name match the member data name? volume_trim_slot()?

sure...thats a good point.

tonyastolfi · 2025-02-04T19:41:14Z

src/llfs/page_recycler_recovery_visitor.cpp

+  // 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) {


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.

sure.. I will use slot_less_than.

tonyastolfi · 2025-02-04T19:51:29Z

src/llfs/page_recycler_recovery_visitor.hpp

@@ -45,6 +45,9 @@ class PageRecyclerRecoveryVisitor

  Optional<SlotRange> latest_info_refresh_slot() const;

+  slot_offset_type volume_trim_offset() const;


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_than and lexicographical ordering that is coded inline in a couple places currently.

Will do the change as discussed...

tonyastolfi · 2025-02-04T19:53:03Z

src/llfs/page_recycler_recovery_visitor.cpp

@@ -29,6 +29,8 @@ namespace llfs {
    , latest_batch_{}
    , recycler_uuid_{boost::uuids::random_generator{}()}
    , latest_info_refresh_slot_{}
+    , volume_trim_slot_{0}


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).

First time when the DB is coming up we will never find a slot/index during recovery. We take them as 0/0. I am not sure if optional will be useful here. Let me know if I missed anything?

Bhaskar Bora added 6 commits October 21, 2024 14:21

save debug changes

8fd6fb0

ref count changes

f94af8d

Merge branch 'main' of https://github.com/mathworks/llfs

b990c00

Merge branch 'main' into ref_count_bug_fix

e9a43b7

removed death-test

f6d4360

added a histogram plot for the bug hits

315d8ba

bhaskarbora2 requested review from tonyastolfi and gabrielbornstein December 18, 2024 16:55

bhaskarbora2 self-assigned this Dec 18, 2024

bhaskarbora2 changed the title ~~Page Reference Count Bug Fix~~ Dead Page Reference Count Bug Fix Dec 18, 2024

Bhaskar Bora added 17 commits January 7, 2025 15:04

temp fix for the test failure

8f92645

page_tracer test case fix

4b68e88

fixed llfs crash-recovery test case

34e1b71

updated the test page_count

550ca3c

updated test cases

cdf7ea7

added support for recycler calling recycle_pages with depth>0

1f2d280

minor style changes...

6216416

Merge branch 'main' of https://github.com/mathworks/llfs

0c5f7d8

Merge branch 'main' into ref_count_bug_fix

908043c

submodule update

20cf66a

added support for partial dead page processing

18020d7

removed attachment-index display code

f9a97fc

fixed depth related check

ad2980a

updated page_index logic

985bd25

updated variable name

2785b7f

update to avoid clearing the entire log

35c646d

changed some test paramaters

5d350d1

tonyastolfi reviewed Feb 3, 2025

View reviewed changes

Bhaskar Bora added 2 commits February 3, 2025 17:11

:review comment changes

be0412d

update some comments and log messages

3548923

Bhaskar Bora added 5 commits February 3, 2025 17:57

updated some log messages

e560bc6

simplified check function

f8815b1

made recycle_pages more modular

dae089d

removed a typo

56187de

updated variable name

6d70676

tonyastolfi reviewed Feb 4, 2025

View reviewed changes

Bhaskar Bora added 2 commits February 5, 2025 10:40

part of review comment changes

5164285

part-2 of review comments

f9b088b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dead Page Reference Count Bug Fix #181

Dead Page Reference Count Bug Fix #181

bhaskarbora2 commented Dec 18, 2024 •

edited

Loading

tonyastolfi Jan 28, 2025

bhaskarbora2 Feb 5, 2025

tonyastolfi Jan 28, 2025

bhaskarbora2 Feb 4, 2025

tonyastolfi left a comment •

edited

Loading

tonyastolfi Feb 4, 2025

bhaskarbora2 Feb 5, 2025

tonyastolfi Feb 4, 2025

tonyastolfi left a comment

tonyastolfi Feb 4, 2025

bhaskarbora2 Feb 5, 2025

tonyastolfi Feb 4, 2025

bhaskarbora2 Feb 5, 2025

tonyastolfi Feb 4, 2025

bhaskarbora2 Feb 5, 2025

tonyastolfi Feb 4, 2025

bhaskarbora2 Feb 5, 2025

tonyastolfi Feb 4, 2025

bhaskarbora2 Feb 5, 2025

tonyastolfi Feb 4, 2025

bhaskarbora2 Feb 5, 2025

tonyastolfi Feb 4, 2025

bhaskarbora2 Feb 5, 2025

		@@ -45,6 +45,9 @@ class PageRecyclerRecoveryVisitor

		Optional<SlotRange> latest_info_refresh_slot() const;

		slot_offset_type volume_trim_offset() const;

Dead Page Reference Count Bug Fix #181

Are you sure you want to change the base?

Dead Page Reference Count Bug Fix #181

Conversation

bhaskarbora2 commented Dec 18, 2024 • edited Loading

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

tonyastolfi left a comment • edited Loading

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

tonyastolfi left a 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

bhaskarbora2 commented Dec 18, 2024 •

edited

Loading

tonyastolfi left a comment •

edited

Loading