pageserver: more elegant cancellation for remote operations

[jcsp]

• Timeouts should be applied after accquiring the semaphore that limits concurrent remote operations. Applying a timeout elsewhere results in loss of fairness if operations are timing out waiting for the semaphore and thereby losing their place in the queue.
• Remote operations should return an error type that distinguishes actual errors from shutdown, and callers should thereby avoid logging (+metric-counting) shutdown as if it were an error.
• download_retry should understand cancellation errors from the inner function, and then we can avoid passing another cancellation token explicitly to download_retry: it can infer shutdown just from the result of the wrapped function.

[jcsp]

Related PR for adding cancellation tokens to the remote storage interface: #4781

[koivunej]

#6618 takes care of extra CancellationToken cloning. I looked into implementing this and it would turn:

let remote_storage_operation = GenericRemoteStorage::something(million, args, cancel).await;

into

// this is callled from within backoff::retry

let new_nested_token = cancel.child_token();
let remote_storage_operation = GenericRemoteStorage::something(million, args, &new_nested_token);
let remote_storage_operation = std::pin::pin!(remote_storage_operation);

tokio::select! {
    res = &mut remote_storage_operation => res,
    _ = tokio::time::sleep(UPLOAD_TIMEOUT) => {
        new_nested_token.cancel();
        remote_storage_operation.await
  },
}

Leaving us at +1 cancellation token creation after #6618 removes 1-2 clones, becoming a zero sum. Looked that there is no real way to solve this at LocalFS level by spawning so that we could get the gracefulness and have only LocalFS pay for it. This would result in very much the same problem it could currently have.

The best alternative is to just add warning logging to LocalFS that if you get a test failure because of a cancellation (and too fast retry), it would pop out right away as something to consider instead of a very hard to reproduce problem.

Or do nothing and hope no one ever debugs test failure like this. Seems the dropguard logging would be easy to add.

[jcsp]

The best alternative is to just add warning logging to LocalFS that if you get a test failure because of a cancellation (and too fast retry), it would pop out right away as something to consider instead of a very hard to reproduce problem.

Sounds good, I agree with not letting LocalFS guide our design. It just has to work, not be graceful.

Leaving us at +1 cancellation token creation after #6618 removes 1-2 clones, becoming a zero sum.

Yep. I like getting rid of the spurious clone in #6618. If the net result is ~similar overhead then we're good, it's fine to pay some clone-level efficiency tax for cancellation, when the operation is a big remote HTTP request.

[jcsp]

This week:

• John & Joonas to sync on the scope

[jcsp]

PR in flight: #6697