pageserver: issue concurrent IO on the read path

[VladLazar]

Problem

The read path does sequential IOs.

Summary of changes

Previously, the read path would wait for all IO in one layer visit to
complete before visiting the next layer (if a subsequent visit is
required). IO within one layer visit was also sequential.

With this PR we gain the ability to 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.

This is a slightly cleaned up version of the work done at the Lisbon hackathon.

Error handling

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

Feature Gating

One can configure this via the NEON_PAGESERVER_VALUE_RECONSTRUCT_IO_CONCURRENCY
env var. A config is possible as well, but it's more work and this is
enough for experimentation.

Checklist before requesting a review

• I have performed a self-review of my code.
• If it is a core feature, I have added thorough tests.
• Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
• If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

• Do not forget to reformat commit message to not include the above checklist

[github-actions]

7095 tests run: 6777 passed, 0 failed, 318 skipped (full report)

---

Flaky tests (6)

Postgres 17

• test_nbtree_pagesplit_cycleid: release-arm64

Postgres 16

• test_compute_pageserver_connection_stress: release-x86-64

Postgres 15

• test_pgdata_import_smoke[None-1024-RelBlockSize.MULTIPLE_RELATION_SEGMENTS]: release-arm64
• test_pgdata_import_smoke[8-1024-RelBlockSize.MULTIPLE_RELATION_SEGMENTS]: release-arm64

Postgres 14

• test_pgdata_import_smoke[8-1024-RelBlockSize.MULTIPLE_RELATION_SEGMENTS]: release-arm64
• test_pgdata_import_smoke[None-1024-RelBlockSize.MULTIPLE_RELATION_SEGMENTS]: release-arm64

Test coverage report is not available

_{The comment gets automatically updated with the latest test results
31fec1f at 2024-12-12T20:23:56.140Z :recycle:}

[erikgrinaker]

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.

Is this true? It really depends on how the IO futures are implemented, but in general, dropping a future should cancel the in-flight operation and stop polling it. Assuming they're implemented that way, it should be sufficient to ensure that the caller receives the error as soon as it happens and then drops the in-flight futures by returning the error. I don't think we need any synchronization beyond that, or am I missing something?

[erikgrinaker]

Still reviewing, just flushing some comments for now. All nits, take them or leave them.

[erikgrinaker]

I find the overall shape of this interaction a bit curious. Is there a compelling reason why we chose to use channels here instead of making update_key an async function and passing it an IO future for the value? That seems like a more natural approach, and would allow us to use standard async utilities to control the scheduling -- but maybe I'm missing some detail.

[VladLazar]

Is there a compelling reason why we chose to use channels here instead of making update_key an async function and passing it an IO future for the value?

update_key shouldn't wait on the completion of the IO, so I don't really understand why we would make it async.
The receiver is just pushed into VectoredValueReconstructState::on_disk_values and that vector is drained at the end of the read in VectoredValueReconstructState::collect_pending_ios.

I guess we could change VectoredValueReconstructState::on_disk_values to stash IO futures instead of oneshot channels, but I don't really see what that gets us.

[erikgrinaker]

update_key shouldn't wait on the completion of the IO, so I don't really understand why we would make it async.

I think I just intuitively considered it easier to understand updating the state once the read completed, which we could do by nesting futures, but I don't think it has any practical consequence. We'd probably run into some ownership issues as well.

I guess we could change VectoredValueReconstructState::on_disk_values to stash IO futures instead of oneshot channels, but I don't really see what that gets us.

I think there are a few benefits to consider:

• Spawning a task has a cost, and spawning many tasks puts pressure on the Tokio scheduler. I don't know if this matters considering we also do IO here, but it might at scale. Futures can be awaited on the current task without spawning new ones.

• Serial/parallel operation is trivial simply by awaiting the futures sequentially (serial) or using e.g. FuturesOrdered or FuturesUnordered to await them all concurrently on the same task (parallel) -- the latter also optimizes away polling costs.

• Futures are automatically cancelled when dropped, while spawned tasks need explicit cancellation (you alluded to this being a problem in the PR description).

• We avoid the additional channel synchronization and overhead. This probably doesn't matter considering we're doing IO.

[VladLazar]

I think I just intuitively considered it easier to understand updating the state once the read completed, which we could do by nesting futures, but I don't think it has any practical consequence. We'd probably run into some ownership issues as well.

Yeah, I agree. The issue with that is that we don't get concurrency. The gist here is that we move on to the next layer after issuing IO without waiting on completion.

I think there are a few benefits to consider

That's fair. I'll give it a go when I get the time.

[erikgrinaker]

Yeah, I agree. The issue with that is that we don't get concurrency. The gist here is that we move on to the next layer after issuing IO without waiting on completion.

I meant that these would just wrap the IO future, but not await it straight away -- we'd await the futures concurrently. The state would update as each read completed. But as I said, this might cause ownership issues (we might get away with it if we're not spawning tasks, but could get ugly either way).

[VladLazar]

I gave this a go. It's doable but quite tricky. The difficulty comes from the fact that we need to fan out futures from the IO future itself (which reads data for multiple keys). This would be quite inefficient currently because we have to iterate over all VectoredBlobsBuff::blobs for each blob.

I think we should defer this to when we pick this back up and benchmark it. The task and future approaches should be benchmarked separately. With the task approach we actually perform IO while the traversal is happening, but with the futures approach all IO would be done at the end.

[erikgrinaker]

Yeah, benchmarks seem reasonable. I'm a bit worried that putting this amount of pressure on the Tokio scheduler could cause latency issues, probably more than the gains we get from executing IOs earlier, but we can find out with appropriate benchmarks.

[VladLazar]

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.

Is this true? It really depends on how the IO futures are implemented, but in general, dropping a future should cancel the in-flight operation and stop polling it. Assuming they're implemented that way, it should be sufficient to ensure that the caller receives the error as soon as it happens and then drops the in-flight futures by returning the error. I don't think we need any synchronization beyond that, or am I missing something?

I missed this question.

All "IO futures" are collected in VectoredValueReconstructState::on_disk_values. With this PR, the read path does not wait for the outcome of one IO before issuing the next one. Hence, at the end of the layer traversal, we will have created "IO futures" for all required values. Each "IO future" is independent. If the first one fails, the others are still present.

Let's also consider what happens when all the "IO futures" after the failed one are not complete. We bail out in collect_pending_ios, but the tasks for all the incomplete IOs still run since we don't call abort on the JoinHandle or have cancellation wired in.

[erikgrinaker]

I've done another pass to check that there aren't any issues with ordering or races, and I can't see any -- even though we dispatch IOs concurrently, we always access the results in a predetermined order.

I think this should be good to go, once we resolve the tasks vs. futures discussion above.