Reduce layer map locking on the read path #6833
Assignees
Labels
c/storage/pageserver
Component: storage: pageserver
t/bug
Issue Type: Bug
triaged
bugs that were already triaged
Comments
VladLazar
added
t/bug
|
Just realized while looking at some logs (slack thread), that compaction now needs to wait for on-demand downloads. This is a regression introduced with the Since no one has noticed this in quite some time, I don't think this is critical, but we should get it eventually fixed, so a slight boost to this issue. What needs to be done:
|
This was referenced Mar 23, 2024
VladLazar
added a commit
that referenced
this issue
Mar 26, 2024
## Problem We currently hold the layer map read lock while doing IO on the read path. This is not required for correctness. ## Summary of changes Drop the layer map lock after figuring out which layer we wish to read from. Why is this correct: * `Layer` models the lifecycle of an on disk layer. In the event the layer is removed from local disk, it will be on demand downloaded * `InMemoryLayer` holds the `EphemeralFile` which wraps the on disk file. As long as the `InMemoryLayer` is in scope, it's safe to read from it. Related #6833
|
Update:
|
VladLazar
added a commit
that referenced
this issue
Apr 2, 2024
## Problem The vectored read path holds the layer map lock while visiting a timeline. ## Summary of changes * Rework the fringe order to hold `Layer` on `Arc<InMemoryLayer>` handles instead of descriptions that are resolved by the layer map at the time of read. Note that previously `get_values_reconstruct_data` was implemented for the layer description which already knew the lsn range for the read. Now it is implemented on the new `ReadableLayer` handle and needs to get the lsn range as an argument. * Drop the layer map lock after updating the fringe. Related #6833
|
Both PRs merged. Releasing on the week of 2024-04-08 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently, the read path (
Timeline::getandTimeline::get_vectoredwhen it merges) hold the layer manager read lock while accumulating the reconstruct data (includes on-demand downloads and disk IO). The non-vectored read path grabs the lock here.Locking for this long is not required. Instead, the read path could hold on to a
storage_layer::Layer(introduced in #4938) and only hold the lock while querying the layer map. The fix for the vectored read path will be similar, but slightly more involved. The search fringe must be updated to holdstorage_layer::Layerinstead of layer descriptions and the in-memory handles will need some thought as well.The text was updated successfully, but these errors were encountered: