From bde1694cb9208f6dffa02842683e9baeb88fe1d8 Mon Sep 17 00:00:00 2001 From: Vlad Lazar Date: Thu, 10 Oct 2024 18:15:27 +0200 Subject: [PATCH] pageserver: remove unused on_key_error method It's obvious the method is unused, but let's break down error handling of the read path. Before this patch set, all IO was done sequentially for a given read. If one IO failed, then the error would stop the processing of the read path. Now that we are doing IO concurrently when serving a read request it's not trivial to implement the same error handling approach. As of this commit, one IO failure does not stop any other IO requests. When awaiting for the IOs to complete, we stop waiting on the first failure, but we do not signal any other pending IOs to complete and they will just fail silently. Long term, we need a better approach for this. Two broad ideas: 1. Introduce some synchronization between pending IO tasks such that new IOs are not issued after the first failure 2. Cancel any pending IOs when the first error is discovered --- pageserver/src/tenant/storage_layer.rs | 10 ---------- pageserver/src/tenant/storage_layer/image_layer.rs | 4 ---- 2 files changed, 14 deletions(-) diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index f52eee9904..5534239614 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -232,16 +232,6 @@ impl ValuesReconstructState { self.io_concurrency.spawn_io(fut); } - /// Associate a key with the error which it encountered and mark it as done - pub(crate) fn on_key_error(&mut self, key: Key, err: PageReconstructError) { - let previous = self.keys.insert(key, Err(err)); - if let Some(Ok(state)) = previous { - if state.situation == ValueReconstructSituation::Continue { - self.keys_done.add_key(key); - } - } - } - pub(crate) fn on_layer_visited(&mut self, layer: &ReadableLayer) { self.layers_visited += 1; if let ReadableLayer::PersistentLayer(layer) = layer { diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index db070d277b..bf3e43c5d5 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -622,9 +622,6 @@ impl ImageLayerInner { } } - // Why did on_key_error go away? - // There was a good reason for it ... - let read_from = self.file.clone(); let read_ctx = ctx.attached_child(); reconstruct_state.spawn_io(async move { @@ -657,7 +654,6 @@ impl ImageLayerInner { } Err(err) => { for (_, sender) in senders { - // TODO: when can this fail? let _ = sender .send(Err(std::io::Error::new(err.kind(), "vec read failed"))); }