From c073be6f8d1da126931c38f1065894d3426840b2 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 19 Jun 2024 00:26:02 +0200 Subject: [PATCH] fix tests, remove traces from docs, remove imprecise cache lookup code --- docs/pageserver-pagecache.md | 1 - docs/settings.md | 2 +- pageserver/src/metrics.rs | 2 +- pageserver/src/page_cache.rs | 29 ++++++----------------------- test_runner/fixtures/metrics.py | 2 -- 5 files changed, 8 insertions(+), 28 deletions(-) diff --git a/docs/pageserver-pagecache.md b/docs/pageserver-pagecache.md index d9b120bbb9..d022742dff 100644 --- a/docs/pageserver-pagecache.md +++ b/docs/pageserver-pagecache.md @@ -5,4 +5,3 @@ TODO: - shared across tenants - store pages from layer files - store pages from "in-memory layer" -- store materialized pages diff --git a/docs/settings.md b/docs/settings.md index 817f97d8ba..12a6a4c171 100644 --- a/docs/settings.md +++ b/docs/settings.md @@ -134,7 +134,7 @@ depends on that, so if you change it, bad things will happen. #### page_cache_size -Size of the page cache, to hold materialized page versions. Unit is +Size of the page cache. Unit is number of 8 kB blocks. The default is 8192, which means 64 MB. #### max_file_descriptors diff --git a/pageserver/src/metrics.rs b/pageserver/src/metrics.rs index 30b32c9d09..2992fef561 100644 --- a/pageserver/src/metrics.rs +++ b/pageserver/src/metrics.rs @@ -2860,7 +2860,7 @@ pub fn preinitialize_metrics() { // FIXME(4813): make it so that we have no top level metrics as this fn will easily fall out of // order: // - global metrics reside in a Lazy - // - access via crate::metrics::PS_METRICS.materialized_page_cache_hit.inc() + // - access via crate::metrics::PS_METRICS.some_metric.inc() // - could move the statics into TimelineMetrics::new()? // counters diff --git a/pageserver/src/page_cache.rs b/pageserver/src/page_cache.rs index db2b902efa..f386c825b8 100644 --- a/pageserver/src/page_cache.rs +++ b/pageserver/src/page_cache.rs @@ -17,7 +17,6 @@ //! //! Two types of pages are supported: //! -//! * **Materialized pages**, filled & used by page reconstruction //! * **Immutable File pages**, filled & used by [`crate::tenant::block_io`] and [`crate::tenant::ephemeral_file`]. //! //! Note that [`crate::tenant::ephemeral_file::EphemeralFile`] is generally mutable, but, it's append-only. @@ -28,9 +27,6 @@ //! Page cache maps from a cache key to a buffer slot. //! The cache key uniquely identifies the piece of data that is being cached. //! -//! The cache key for **materialized pages** is [`TenantShardId`], [`TimelineId`], [`Key`], and [`Lsn`]. -//! Use [`PageCache::memorize_materialized_page`] and [`PageCache::lookup_materialized_page`] for fill & access. -//! //! The cache key for **immutable file** pages is [`FileId`] and a block number. //! Users of page cache that wish to page-cache an arbitrary (immutable!) on-disk file do the following: //! * Have a mechanism to deterministically associate the on-disk file with a [`FileId`]. @@ -337,9 +333,8 @@ impl PageCache { blkno: u32, ctx: &RequestContext, ) -> anyhow::Result { - let mut cache_key = CacheKey::ImmutableFilePage { file_id, blkno }; - - self.lock_for_read(&mut cache_key, ctx).await + self.lock_for_read(&(CacheKey::ImmutableFilePage { file_id, blkno }), ctx) + .await } // @@ -373,19 +368,11 @@ impl PageCache { /// Look up a page in the cache. /// - /// If the search criteria is not exact, *cache_key is updated with the key - /// for exact key of the returned page. (For materialized pages, that means - /// that the LSN in 'cache_key' is updated with the LSN of the returned page - /// version.) - /// - /// If no page is found, returns None and *cache_key is left unmodified. - /// async fn try_lock_for_read( &self, - cache_key: &mut CacheKey, + cache_key: &CacheKey, permit: &mut Option, ) -> Option { - let cache_key_orig = cache_key.clone(); if let Some(slot_idx) = self.search_mapping(cache_key) { // The page was found in the mapping. Lock the slot, and re-check // that it's still what we expected (because we released the mapping @@ -398,9 +385,6 @@ impl PageCache { _permit: inner.coalesce_readers_permit(permit.take().unwrap()), slot_guard: inner, }); - } else { - // search_mapping might have modified the search key; restore it. - *cache_key = cache_key_orig; } } None @@ -437,7 +421,7 @@ impl PageCache { /// async fn lock_for_read( &self, - cache_key: &mut CacheKey, + cache_key: &CacheKey, ctx: &RequestContext, ) -> anyhow::Result { let mut permit = Some(self.try_get_pinned_slot_permit().await?); @@ -514,15 +498,14 @@ impl PageCache { /// Search for a page in the cache using the given search key. /// - /// Returns the slot index, if any. If the search criteria is not exact, - /// *cache_key is updated with the actual key of the found page. + /// Returns the slot index, if any. /// /// NOTE: We don't hold any lock on the mapping on return, so the slot might /// get recycled for an unrelated page immediately after this function /// returns. The caller is responsible for re-checking that the slot still /// contains the page with the same key before using it. /// - fn search_mapping(&self, cache_key: &mut CacheKey) -> Option { + fn search_mapping(&self, cache_key: &CacheKey) -> Option { match cache_key { CacheKey::ImmutableFilePage { file_id, blkno } => { let map = self.immutable_page_map.read().unwrap(); diff --git a/test_runner/fixtures/metrics.py b/test_runner/fixtures/metrics.py index 8b8075f8c1..e01bb6da51 100644 --- a/test_runner/fixtures/metrics.py +++ b/test_runner/fixtures/metrics.py @@ -118,8 +118,6 @@ PAGESERVER_GLOBAL_METRICS: Tuple[str, ...] = ( "libmetrics_launch_timestamp", "libmetrics_build_info", "libmetrics_tracing_event_count_total", - "pageserver_materialized_cache_hits_total", - "pageserver_materialized_cache_hits_direct_total", "pageserver_page_cache_read_hits_total", "pageserver_page_cache_read_accesses_total", "pageserver_page_cache_size_current_bytes",