layer: evict downloaded-but-not-initialized

[koivunej]

This issue is a catch-all for struct Layer situations where the internal state (LayerInner::inner) does not match filesystem (file present, not present).

"downloaded-but-not-initialized" happens when a call to LayerInner::get_or_maybe_download changes state such that an eviction is cancelled, but the call is cancelled before it reinitializes or downloads and initializes the internal state. Currently the workaround is to have Layer::keep_resident be async, to do initialization (repair) but not download.

As recently noticed, the Layer::keep_resident will need to wait for an ongoing download. This can mean 10min wait during an overloaded situation. Slow Layer::keep_resident slows down collecting resident layers (LayerManager::resident_layers) called from most importantly disk usage based eviction. This need to wait for a download is very much against original design goals, which only succeeded for Layer::evict_and_wait.

Solution alternatives

Keep only the fastest path spawn-less

Currently only downloading the layer involves spawning a new task. A spawn would make the make the "file is present locally" case initialization cancellation safe. Spawning however is difficult due to heavier_once_cell::OnceCell using a std::sync::Mutex internally, but also because we would have to do the stat call while holding the mutex.

Alternative to doing stat while holding the mutex is to have the spawn_blocking'd eviction wait for the semaphore with Handle::block_on, exposing it to wait on a download. The Handle::block_on wait on semaphore permit could be short-circuited by signalling when we've decided to download and not just reinitialize.

Additional state to track the last known filesystem status

During timeline layermap initialization, we know if the layer is evicted or present. When new layers are created locally, we know they are present because a blocking std::fs::rename from the temporary path succeeds. Seems obvious we could cache this knowledge, only later updated when eviction's std::fs::remove_file succeeds.

Caching the value will expose another problem I was hoping to avoid originally. If we end up in a state where a previous spawn_blocking eviction was "cancelled", we should fire up a new one from Layer::evict_and_wait. In the meantime the Layer::keep_resident should report status based on this cached value.

Race Layer::keep_resident to when download path is entered

Would a watch channel change the size of the Layer?

---

"downloaded-but-not-initialized" happens if there is a single cancelled request which ends up firing up the download, which is not cancellation safe. Could make it, or could make evictions work even if we don't know if we are initialized or not. Filesystem is the ground truth for this.

Originally posted by @koivunej in #6000 (comment)

[koivunej]

Found a good abstraction for this, fixing it will not be difficult.

[jcsp]

(noting for my benefit):

• Currently LayerManager::resident_layers is an async function that calls keep_resident on all the layers, rather than just yielding the layers directly.
• The purpose of that keep_resident is to handle a case where a layer might have been downloaded but then a future was cancelled during download, leaving the layer in the "downloaded but not initialized" state described in this ticket.
• Getting the layer out of that bad state is important during eviction (which uses resident_layers()), in order to avoid leaking disk space.

[koivunej]

Next steps:

• just do this; it has just been deprioritized by urgent work

[koivunej]

Other effects this bug causes:

• Layer::keep_resident called by LayerMap::resident_layers will join any wait to download a layer
• normally layer downloads are fast, but in an overloaded situation they are not because we are also waiting on concurrency limiter
  • earlier on Epic: improved eviction #5331 I witnessed the 10min hang while having 200 downloads ongoing
• on a broken by generation numbers resetting tenant for which remote layers are removed but listed on index_part.json, this causes very long collection times because there is an infinite loop retrying the initial logical size and compaction tries to access the layer contents, so there are many queued download attempts
  • struct Layer has an internal backoff mechanism which also plays a role