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.

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

cst/cache: tracker based trim #18786

Merged

Conversation

abhijat
Copy link
Contributor

@abhijat abhijat commented Jun 5, 2024

This changeset extends the access time tracker to hold size as well as atime of cache entries. The keys for the tracker are changed from hash of paths to paths themselves.

The entries in the cache are used directly to trim, by sorting the paths based on atime and then removing the least recently used entries. This avoids the need to make directory listing or file stat calls during the trim.

A background task is added to periodically sync the tracker with on-disk data.

FIXES: #18759

New metrics:

  • in_mem_trims - number of tracker based trims
  • tracker_syncs - number of times tracker was synced with disk
  • tracker_size - size of tracker (count of entries - one per file - in cache)

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@abhijat abhijat marked this pull request as draft June 5, 2024 15:44
@abhijat abhijat force-pushed the enhancement/18759/tracker-based-trim branch from 261d8b8 to 2ae21b5 Compare June 6, 2024 05:00
@abhijat abhijat changed the title DNM: tracker based cache trim cst/cache: tracker based trim Jun 6, 2024
@abhijat abhijat force-pushed the enhancement/18759/tracker-based-trim branch 12 times, most recently from 403a63e to 66e146d Compare June 6, 2024 14:45
@abhijat
Copy link
Contributor Author

abhijat commented Jun 6, 2024

  • metrics for in-memory trim
  • investigate skipping fast trim altogether
  • ducktape test based on metrics from 1
  • cleanup for size accounting in trim

});
});
});
_tracker_sync_timer.arm(tracker_sync_period);
Copy link
Contributor

Choose a reason for hiding this comment

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

So the sync will be done every 6 hours in a non-blocking manner.
Maybe we should sync early if the tracker is empty (this could happen when we upgrade) or when we're low on space but tracker says that we should have space? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sync is based on access_time_tracker::trim which is one-way right now, it will only ever remove items from the cache and never add new items, which originally made sense because it was expected to remove manually deleted files from the tracker.

Right now the only way to add entries to the tracker is to issue a get on the cache.

To populate an empty tracker, maybe the sync (and tracker's trim method) should have an optional flag to enable it to add items in addition to removing them, IE when the flag argument is off by default but when it is on, if the item is present in incoming set but not in tracker, then it is added. If the item is not in the incoming set but present in tracker, then it is removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we're low on space but tracker says that we should have space

Would this happen because of extra tmp files and the index and tx files which are present on disk but not in the tracker?

Even if we sync the tracker state with disk in an additive manner, we would probably skip adding these files to the tracker, so the size difference might still be there after the sync.

Copy link
Contributor

Choose a reason for hiding this comment

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

To populate an empty tracker, maybe the sync (and tracker's trim method) should have an optional flag to enable it to add items in addition to removing them

maybe the tracker could have a field which is populated when the tracker is loaded, if the tracker was upgraded from v1 to v2 and lost all data then we should set the flag. And if the flag is set we could start the sync early.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this happen because of extra tmp files and the index and tx files which are present on disk but not in the tracker?

because of this or because redpanda added some files to the cache and crashed before the tracker was saved to disk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On startup in cleanup_at_start we sync the tracker. I added a flag there to do add entries to tracker if they are found in directory but missing in the data structure (except index, tx and tmp files). I think this should take care of both scenarios:

  1. empty tracker due to upgrade - the sync is done after read from disk so we will populate the tracker during sync with on-disk data
  2. tracker not written during shutdown - the sync will write missing entries into the tracker on startup

@abhijat abhijat force-pushed the enhancement/18759/tracker-based-trim branch 8 times, most recently from 0ffdab3 to bea55ac Compare June 11, 2024 10:23
@abhijat abhijat marked this pull request as ready for review June 11, 2024 10:24
@abhijat abhijat force-pushed the enhancement/18759/tracker-based-trim branch 2 times, most recently from 92fc40c to a0c3319 Compare June 11, 2024 13:08
@abhijat abhijat requested a review from Lazin June 11, 2024 15:36
abhijat added 5 commits June 12, 2024 10:59
* The access time tracker now also tracks file sizes.
* The key to the tracker map is now the full file path instead of a hash
* The serialized form of tracker now contains a version to distinguish
  between the old and new data structures.
* The recursive directory walker uses tracker entries to avoid file stat
  operations.
When trimming a first attempt is made to get LRU entries from the access
tracker. If this does not free up enough space, then we proceed with
the existing trim based on directory walk.
The tracker is synced periodically with on-disk data using a directory
walk. This is necessary because once we switch to trimming using the
tracker, the data in tracker and on-disk may drift because of tmp
files, aborted downloads or manual deletion of files. A periodic sync
brings the tracker in sync with the metadata on disk.
@abhijat abhijat force-pushed the enhancement/18759/tracker-based-trim branch from a0c3319 to c17d79d Compare June 12, 2024 05:30
@vbotbuildovich
Copy link
Collaborator

new failures in https://buildkite.com/redpanda/redpanda/builds/50148#01900b33-65e0-45e5-ac78-fda6b6125a41:

"rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.ABS"

@abhijat
Copy link
Contributor Author

abhijat commented Jun 12, 2024

/ci-repeat

@abhijat abhijat merged commit 20a24ca into redpanda-data:dev Jun 12, 2024
19 checks passed
@abhijat
Copy link
Contributor Author

abhijat commented Jul 18, 2024

/backport v24.1.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Perform cache trim using in memory data
3 participants