pageserver: concurrent IO on the get page read path

[VladLazar]

Problem

The read path does sequential IOs.

We should issue IO concurrently within one layer visit and to move on to the next layer without waiting for IOs
from the previous visit to complete.

Tasks & Milestones

• hackathon prototype: [HACKATHON]: superscalar page_service / get_vectored #9002
• initial PR pageserver: issue concurrent IO on the read path #9353
• test in staging & preprod, together with direct IO
  • remaining direct IO impact will be on code that is sequential at a higher level (collect_keyspace is a good example)
  • only part on the user data path that isn't concurrent are the Timeline::get calls made by basebackup, use "show callers" to get an idea of it; speculation is we can get away with taking the direct IO latency hit for these
• move IoConcurrency plumbing to RequestContext & integrate with TenantHarness #10460
• mini retroactive RFC

Impl

Impl

Preview Give feedback

1. initial logical size calculation: always poll to completion #10471
   +0
2. pageserver: issue concurrent IO on the read path #9353
   +0
   
3. basebackup: use Timeline::get for get_rel instead of get_rel_page_at_lsn #10476
   +0
4. WIP: concurrent IO benchmarking #10466
   +0
5. https://github.com/neondatabase/infra/pull/2836

[problame]

Status update:

• impl is in pageserver: issue concurrent IO on the read path #9353
• Vlad started, I took over
• today, I merged in fix(page_service pipelining): tenant cannot shut down because gate kept open while flushing responses #10386
  because that PR is going to get merged and deployed first; conflicts around protocol version v3 (super high confidence the merge was correct, and @skyzh 's feat(pageserver): support inherited sparse keyspace #10313 )
• got rid of the barrier stage, as discussed with Vlad before the holidays
• that appears to have fixed another mysterious hang bug (?)
• removed todo() and deprecation warning ; just rename it to serial() and remove the warning?

Next steps:

• clean up deep_layers_with_delta.py script / make it reusable and use in batching benchmark
• clean up ad-hoc changes made to benchmarks (new params are ok, removing param space not)
• convince ourselves that the current design works fine with limited slots, because the background task advances the io futures ( => don't need DRAFT: remove slots concepts, allocate for each IO instead tokio-epoll-uring#63 )
• review interaction with sparse keyspace
  • sparked by the minor merge conflicts with feat(pageserver): support inherited sparse keyspace #10313
  • but really, I don't think either vlad or me thought about this at all
• review teardown / VirtualFile lifecycle
  • basically: because the VirtualFile is now wrapped in an Arc<> that lives inside the spawned ios, thus potentially longer than the get_values_reconstruct_data call that spawned the io, can it happen that compaction deletes the file from the filesystem while the Arc still exists?
• // TODO: IO futures are not failable - we could expect
  • at a minimum, understand how exactly we're behaving if we bail on the second of three on_disk_values
• figure out how to monitor impact in staging/prod wrt compaction debt
  • maybe just do a big ingest before and after enabling it?

[problame]

Had a call with Vlad:

convince ourselves that the current design works fine with limited slots, because the background task advances the io futures ( => don't need neondatabase/tokio-epoll-uring#63 )

We don't need it because each of the IO futures only uses one tokio-epoll-uring slot at a time, i.e., there's never a "hold tokio-epoll-uring slot and try to acquire another one".

However, that's totally non-obvious and thus brittle.
Should fix that after we have removed the Serial implementation and transition away from opaque IO futures to a well-defined operation that's like "do one vectored read against this VirtualFile and collect the results into this oneshot".

review interaction with sparse keyspace

We convinced ourselves that conurrent IO and sparse keyspace are orthogonal.

The intersection between the two is only in the traversal code around update_key.

neon/pageserver/src/tenant/storage_layer.rs

Lines 415 to 456 in 8c5005f

	/// Update the state collected for a given key.
	/// Returns true if this was the last value needed for the key and false otherwise.
	///
	/// If the key is done after the update, mark it as such.
	///
	/// If the key is in the sparse keyspace (i.e., aux files), we do not track them in
	/// `key_done`.
	pub(crate) fn update_key(
	&mut self,
	key: &Key,
	lsn: Lsn,
	completes: bool,
	value: tokio::sync::oneshot::Receiver<Result<OnDiskValue, std::io::Error>>,
	) -> ValueReconstructSituation {
	let state = self.keys.entry(*key).or_default();
	let is_sparse_key = key.is_sparse();
	match state.situation {
	ValueReconstructSituation::Complete => {
	if is_sparse_key {
	// Sparse keyspace might be visited multiple times because
	// we don't track unmapped keyspaces.
	return ValueReconstructSituation::Complete;
	} else {
	unreachable!()
	}
	}
	ValueReconstructSituation::Continue => {
	state.on_disk_values.push((lsn, value));
	}
	}
	if completes && state.situation == ValueReconstructSituation::Continue {
	state.situation = ValueReconstructSituation::Complete;
	if !is_sparse_key {
	self.keys_done.add_key(*key);
	}
	}
	state.situation
	}

Before, it was called after IO completed and used the Value to decide whether to mark state as complete + addd do keys_done yes/no .

Now, instead of a read Value, we have argument completes.

Looking at the code, it's equivalent.

review teardown / VirtualFile lifecycle

The current code extends the lifetime of the "raw" file abstractions too much, i.e.

• Delta and Image layer's VirtualFile is extended beyond the lifetime of the DownloadedLayer
  • This can cause eviction to happen, local file to be removed, and thus spawned IO to fail.
  • This is a bug and needs to be fixed before merging.
• InMemoryLayer's EphemeralFile lifetime is extended
  • No bug potential because the EphemeralFile::drop is what does the deletion from disk.

Decision: hold DownloadedLayer and InMemoryLayer alive in the spawned IO futures, instead of the "raw" file abstractions VirtualFile / EphemeralFile. That fixes the bug AND expresses more clearly to future readers of the code the implications of the spawned io futures.

// TODO: IO futures are not failable - we could expect

Didn't get to this. Will DM with Vlad about it.

---

The other issues will be addressed post-merge when we experiment in staging:

• new automated benchmarks utilizing L0 layer stack
• slots performance implications
• figure out how to monitor impact in staging/prod wrt compaction debt

[problame]

// TODO: IO futures are not failable - we could expect

The TODO was just an observation while writing the code.

Regarding error handling in general, what we want as part of the first cut of this PR is that when get_vectored_impl returns, even with an error, all IOs are done.
For now we can assume get_vectored_impl is polled to completion.

The above desired behavior is not the case right now.
We do poll to completion, but, collect_pending_ios will wrap both mpsc RecvError and IO errors into a PagestreamError::Other and bail.

IO errors were wrapped into PagestreamError::Other even before the current state of the concurrent IO PR, so continuing to do that is fine.

But, we really want to wait for the IOs to drain.

Ideas (I will think of something):

• posion pill message to cancel all preceding IOs?
• cancellation token / dropguard in the ValuesReconstructData?