From 94888d3b31d9c0f4bbdea309f23f759ec6caa5ba Mon Sep 17 00:00:00 2001 From: Rob Johnson Date: Fri, 31 Jul 2026 12:11:33 -0700 Subject: [PATCH 1/8] cache layer of new writeback scheme Signed-off-by: Rob Johnson --- src/blob.c | 6 +- src/cache.h | 139 ++++++++--- src/clockcache.c | 445 ++++++++++++++++++++++------------ src/shard_log.c | 6 +- tests/functional/cache_test.c | 4 +- 5 files changed, 403 insertions(+), 197 deletions(-) diff --git a/src/blob.c b/src/blob.c index f8c0c375..77c01f3d 100644 --- a/src/blob.c +++ b/src/blob.c @@ -326,7 +326,11 @@ blob_sync(cache *cc, slice sblob) if (!SUCCESS(rc)) { break; } - cache_page_writeback(cc, itor.page, FALSE, PAGE_TYPE_BLOB); + cache_writeback_request wb_req; + platform_status wb_rc = + cache_writeback_page(cc, itor.page, PAGE_TYPE_BLOB, &wb_req); + // The iterator holds a read reference only, never a claim or lock. + platform_assert_status_ok(wb_rc); blob_page_iterator_advance_page(&itor); } diff --git a/src/cache.h b/src/cache.h index 598df0f5..74da6326 100644 --- a/src/cache.h +++ b/src/cache.h @@ -128,13 +128,54 @@ typedef page_handle *(*page_get_fn)(cache *cc, bool32 blocking, page_type type); typedef bool32 (*page_try_claim_fn)(cache *cc, page_handle *page); -typedef void (*page_writeback_fn)(cache *cc, - page_handle *page, - bool32 is_blocking, - page_type type); -typedef void (*extent_writeback_fn)(cache *cc, - uint64 addr, - uint64 *pages_outstanding); + +/* + * ---- Writeback requests ---- + * + * A writeback request is the receipt for one issued writeback: it names what + * was written and identifies the page's dirty interval at the moment the write + * was handed to the I/O layer. Present it to cache_writeback_get_status() to + * learn whether that write has completed. + * + * The interval identity is what makes a request meaningful, so it is produced + * by the call that issues the write -- where it can be read atomically with the + * CC_WRITEBACK transition -- rather than by a free-standing query. + * + * A request holds only the address, so it survives eviction of the page it + * names and the caller need not hold a reference: a page that is no longer + * resident was necessarily written back before it was evicted. + * + * gen == 0 means "nothing to wait for": the page was already clean when the + * writeback was requested. + */ +typedef struct cache_writeback_request { + uint64 addr; // page addr, or the base addr of an extent + uint64 gen; // dirty interval the write covers; 0 == nothing issued + bool32 is_extent; // whether addr names an extent rather than one page +} cache_writeback_request; + +typedef enum cache_writeback_status { + /* The write is still outstanding. Poll cache_cleanup() and re-check. */ + CACHE_WRITEBACK_PENDING, + /* The write completed: the contents as of the request reached the device. + * Note this is completion, NOT durability -- see cache_durable_barrier(). */ + CACHE_WRITEBACK_COMPLETE, + /* The write completed, but the page has since been dirtied again. The + * request is satisfied; callers that do not expect concurrent writers to + * their pages should treat this as a bug in their own locking. */ + CACHE_WRITEBACK_REDIRTIED, +} cache_writeback_status; + +typedef platform_status (*page_writeback_fn)(cache *cc, + page_handle *page, + page_type type, + cache_writeback_request *req); +typedef platform_status (*extent_writeback_fn)(cache *cc, + uint64 addr, + page_type type, + cache_writeback_request *req); +typedef cache_writeback_status ( + *writeback_get_status_fn)(cache *cc, const cache_writeback_request *req); typedef void (*page_prefetch_fn)(cache *cc, uint64 addr, page_type type); typedef int (*evict_fn)(cache *cc, bool32 ignore_pinned); typedef bool32 (*page_addr_pred_fn)(cache *cc, uint64 addr); @@ -174,6 +215,7 @@ typedef struct cache_ops { page_generic_fn page_unpin; page_writeback_fn page_writeback; extent_writeback_fn extent_writeback; + writeback_get_status_fn writeback_get_status; cache_generic_void_fn flush; cache_generic_status_fn writeback_dirty; cache_generic_status_fn durable_barrier; @@ -463,54 +505,75 @@ cache_unpin(cache *cc, page_handle *page) /* *----------------------------------------------------------------------------- - * cache_page_writeback + * cache_writeback_page * - * Issues writeback of the page to disk. This does NOT make the page durable; - * it only hands the write to the I/O layer (no device-cache flush). + * Asynchronously issues writeback of the page and fills in *req, the receipt + * for that write. This does NOT make the page durable; it only hands the write + * to the I/O layer (no device-cache flush). Use cache_writeback_get_status() to + * learn when it completes and cache_durable_barrier() to make it durable. * - * With is_blocking == FALSE the writeback is issued asynchronously and this - * returns without waiting for completion; there is no per-call way to observe - * when it finishes. With is_blocking == TRUE the page is written synchronously - * and the write has completed by the time this returns. + * Does not block. If a writeback of this page is already in flight -- issued by + * the pressure cleaner, say -- nothing further is issued and *req names that + * in-flight interval, so the caller waits on it exactly as it would on its own + * write. If the page is already clean, req->gen is 0: nothing to wait for. + * + * Returns STATUS_BUSY if the page is dirty but not writeback-able (locked or + * claimed). Callers must treat that as a failure to make the page durable: no + * write was issued and *req cannot report one. *----------------------------------------------------------------------------- */ -static inline void -cache_page_writeback(cache *cc, - page_handle *page, - bool32 is_blocking, - page_type type) +static inline platform_status +cache_writeback_page(cache *cc, + page_handle *page, + page_type type, + cache_writeback_request *req) { - return cc->ops->page_writeback(cc, page, is_blocking, type); + return cc->ops->page_writeback(cc, page, type, req); } /* *----------------------------------------------------------------------------- - * cache_extent_writeback - * - * Asynchronously issues writeback of the extent beginning at addr. This does - * NOT make the extent durable; it only hands the writes to the I/O layer (no - * device-cache flush) and returns without waiting for completion. + * cache_writeback_extent * - * *pages_outstanding is immediately incremented by the number of pages - * issued for writeback (the non-clean pages of the extent); as writebacks - * complete, *pages_outstanding is decremented atomically. This counter is the - * only way to observe when the issued writebacks finish. + * As cache_writeback_page(), but for every page of the extent beginning at + * addr, coalesced into as few larger I/Os as the extent's residency allows. + * One request covers the whole extent: req->gen is the newest dirty interval + * among its pages, which is safe to compare every page against. * - * Assumes pages_outstanding is an aligned uint64, so (on x86) the caller - * can access it and observe its value atomically. + * Pages of the extent that are clean or not resident need no write and are + * skipped. Returns STATUS_BUSY if any page is dirty but not writeback-able; as + * with cache_writeback_page(), the caller must treat that as a failure, since + * that page's contents will not reach the device. * - * TODO: What happens if two callers call cache_extent_writeback on the same + * TODO: What happens if two callers call cache_writeback_extent on the same * extent? + *----------------------------------------------------------------------------- + */ +static inline platform_status +cache_writeback_extent(cache *cc, + uint64 addr, + page_type type, + cache_writeback_request *req) +{ + return cc->ops->extent_writeback(cc, addr, type, req); +} + +/* + *----------------------------------------------------------------------------- + * cache_writeback_get_status * - * All pages in the extent must be clean or cleanable. - * The page may not be in writeback, loading, or locked, or claimed, otherwise - * undefined behavior will occur. + * Whether the write named by *req has completed. Never blocks; a caller + * waiting for completion loops on this and cache_cleanup(), which reaps I/O + * completions on the calling thread and is therefore what makes the loop + * progress rather than merely spin. + * + * Completion is not durability: follow with cache_durable_barrier(). *----------------------------------------------------------------------------- */ -static inline void -cache_extent_writeback(cache *cc, uint64 addr, uint64 *pages_outstanding) +static inline cache_writeback_status +cache_writeback_get_status(cache *cc, const cache_writeback_request *req) { - cc->ops->extent_writeback(cc, addr, pages_outstanding); + return cc->ops->writeback_get_status(cc, req); } /* diff --git a/src/clockcache.c b/src/clockcache.c index bd59647b..bbb1c9b6 100644 --- a/src/clockcache.c +++ b/src/clockcache.c @@ -254,6 +254,20 @@ clockcache_dirty_begin(clockcache *cc, uint32 entry_number) * before CC_WRITEBACK is cleared: the intermediate CC_CLEAN|CC_WRITEBACK state * is not cleanable, so no thread can start a duplicate writeback in the gap. */ +/* + * The dirty interval the entry is currently in, or 0 if it is clean. + * + * Meaningful as a writeback receipt only when read while CC_WRITEBACK is set, + * which excludes the writer and so pins the interval; see + * clockcache_page_writeback(). + */ +static inline uint64 +clockcache_get_writeback_generation(clockcache *cc, uint32 entry_number) +{ + clockcache_entry *entry = clockcache_get_entry(cc, entry_number); + return __atomic_load_n(&entry->dirty_generation, __ATOMIC_RELAXED); +} + static void clockcache_dirty_complete_writeback(clockcache *cc, uint32 entry_number) { @@ -887,7 +901,6 @@ clockcache_try_set_writeback(clockcache *cc, typedef struct async_io_state { clockcache *cc; - uint64 *outstanding_pages; io_async_state_buffer iostate; } async_io_state; @@ -936,10 +949,6 @@ clockcache_write_callback(void *wbs) clockcache_dirty_complete_writeback(cc, entry_number); } - if (state->outstanding_pages) { - __sync_fetch_and_sub(state->outstanding_pages, count); - } - io_async_state_deinit(state->iostate); platform_free(PROCESS_PRIVATE_HEAP_ID, state); } @@ -1052,9 +1061,8 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool32 is_urgent) goto close_log; } - state->cc = cc; - state->outstanding_pages = NULL; - platform_status rc = io_async_state_init(state->iostate, + state->cc = cc; + platform_status rc = io_async_state_init(state->iostate, cc->io, io_async_pwritev, first_addr, @@ -1206,6 +1214,12 @@ clockcache_writeback_dirty(clockcache *cc) * clockcache_try_evict * * Attempts to evict the page if it is evictable + * + * Evictability requires CC_EVICTABLE_STATUS, i.e. CC_CLEAN and nothing + * else, and CC_CLEAN is set only by clockcache_dirty_complete_writeback(). + * So a page cannot leave the cache before its writeback has completed -- + * an invariant clockcache_page_writeback_get_status() relies on to treat a + * non-resident page as written. *---------------------------------------------------------------------- */ static void @@ -2396,20 +2410,17 @@ clockcache_unpin(clockcache *cc, page_handle *page) *----------------------------------------------------------------------------- * clockcache_page_writeback -- * - * Issues writeback of the page. This does not make the page durable; it - * only hands the write to the I/O layer. + * Issues writeback of the page and fills in *req. This does not make the + * page durable; it only hands the write to the I/O layer. * - * With is_blocking == FALSE the writeback is issued asynchronously and - * this returns without waiting for completion. With is_blocking == TRUE - * the page is written synchronously and has completed by the time this - * returns. + * Never blocks. See cache_writeback_page() for the contract. *----------------------------------------------------------------------------- */ -void -clockcache_page_writeback(clockcache *cc, - page_handle *page, - bool32 is_blocking, - page_type type) +platform_status +clockcache_page_writeback(clockcache *cc, + page_handle *page, + page_type type, + cache_writeback_request *req) { uint32 entry_number = clockcache_page_to_entry_number(cc, page); async_io_state *state; @@ -2417,122 +2428,123 @@ clockcache_page_writeback(clockcache *cc, const threadid tid = platform_get_tid(); platform_status status; - while (!clockcache_try_set_writeback(cc, entry_number, TRUE)) { + req->addr = addr; + req->gen = 0; + req->is_extent = FALSE; + + if (!clockcache_try_set_writeback(cc, entry_number, TRUE)) { if (clockcache_test_flag(cc, entry_number, CC_CLEAN)) { - return; + // Nothing to write: gen 0 tells the caller there is nothing to wait + // for. + return STATUS_OK; } - /* * A pressure cleaner or a checkpoint fence may have begun writeback - * after the caller released its claim. Wait for that writeback rather - * than treating a perfectly valid concurrent flush as an assertion. + * after the caller released its claim. Report that in-flight interval + * rather than blocking on it: the caller waits on the returned request + * exactly as it would on a write we issued ourselves. Blocking here + * would serialize a caller that is issuing writeback for many pages. */ if (clockcache_test_flag(cc, entry_number, CC_WRITEBACK)) { - clockcache_wait(cc); - continue; + req->gen = clockcache_get_writeback_generation(cc, entry_number); + return STATUS_OK; } - - platform_assert(0, - "page_writeback requires a cleanable page: entry=%u " - "status=%u\n", - entry_number, - clockcache_get_status(cc, entry_number)); + // Dirty but locked or claimed: no write can be issued for it. + return STATUS_BUSY; } + /* + * Read the generation only now that CC_WRITEBACK is set. That excludes the + * writer (see clockcache_get_write), so the interval cannot advance under + * us and the request provably names the interval this write covers. + */ + req->gen = clockcache_get_writeback_generation(cc, entry_number); + debug_assert(req->gen != 0, "a cleanable page must have a dirty interval"); + if (cc->cfg->use_stats) { cc->stats[tid].page_writes[type]++; cc->stats[tid].syncs_issued++; } - if (!is_blocking) { - state = TYPED_MALLOC(PROCESS_PRIVATE_HEAP_ID, state); - if (state == NULL) { - platform_error_log( - "clockcache_page_writeback: async_io_state allocation " - "failed for addr %lu, entry %u, type %u\n", - addr, - entry_number, - type); - } - platform_assert(state); - state->cc = cc; - state->outstanding_pages = NULL; - status = io_async_state_init(state->iostate, - cc->io, - io_async_pwritev, - addr, - clockcache_write_callback, - state); - if (!SUCCESS(status)) { - platform_error_log( - "clockcache_page_writeback: io_async_state_init failed " - "for addr %lu, entry %u, type %u: %s\n", - addr, - entry_number, - type, - platform_status_to_string(status)); - } - platform_assert_status_ok(status); - status = io_async_state_append_page(state->iostate, page->data); - if (!SUCCESS(status)) { - platform_error_log( - "clockcache_page_writeback: io_async_state_append_page " - "failed for addr %lu, entry %u, type %u: %s\n", - addr, - entry_number, - type, - platform_status_to_string(status)); - } - platform_assert_status_ok(status); - io_async_run(state->iostate); - } else { - status = io_write(cc->io, page->data, clockcache_page_size(cc), addr); - if (!SUCCESS(status)) { - platform_error_log( - "clockcache_page_writeback: io_write failed for addr " - "%lu, entry %u, type %u: %s\n", - addr, - entry_number, - type, - platform_status_to_string(status)); - } - platform_assert_status_ok(status); - clockcache_log(addr, - entry_number, - "page_writeback write entry %u addr %lu\n", - entry_number, - addr); - clockcache_dirty_complete_writeback(cc, entry_number); + state = TYPED_MALLOC(PROCESS_PRIVATE_HEAP_ID, state); + if (state == NULL) { + platform_error_log("clockcache_page_writeback: async_io_state allocation " + "failed for addr %lu, entry %u, type %u\n", + addr, + entry_number, + type); + clockcache_abort_writeback_range( + cc, addr, addr + clockcache_page_size(cc)); + return STATUS_NO_MEMORY; + } + state->cc = cc; + status = io_async_state_init(state->iostate, + cc->io, + io_async_pwritev, + addr, + clockcache_write_callback, + state); + if (!SUCCESS(status)) { + platform_error_log("clockcache_page_writeback: io_async_state_init " + "failed for addr %lu, entry %u, type %u: %s\n", + addr, + entry_number, + type, + platform_status_to_string(status)); } + platform_assert_status_ok(status); + status = io_async_state_append_page(state->iostate, page->data); + if (!SUCCESS(status)) { + platform_error_log("clockcache_page_writeback: " + "io_async_state_append_page failed for addr %lu, " + "entry %u, type %u: %s\n", + addr, + entry_number, + type, + platform_status_to_string(status)); + } + platform_assert_status_ok(status); + io_async_run(state->iostate); + return STATUS_OK; } /* *----------------------------------------------------------------------------- * clockcache_extent_writeback -- * - * Asynchronously issues writeback of the extent. This does not make the - * extent durable; it only hands the writes to the I/O layer and returns - * without waiting for completion. + * Asynchronously issues writeback of the extent and fills in *req. This + * does not make the extent durable; it only hands the writes to the I/O + * layer and returns without waiting for completion. * - * Adds the number of pages issued writeback to the counter pointed to - * by pages_outstanding. When the writes complete, a callback subtracts - * them off, so that the caller may track how many pages are in - * writeback. + * Resident dirty pages are coalesced into as few I/Os as their contiguity + * allows. Pages that are clean or not resident need no write and are + * skipped. A page that is dirty but not writeback-able fails the whole + * request: its contents will not reach the device, so reporting success + * would let a caller believe the extent had been written when it had not. * - * Assumes all pages in the extent are clean or cleanable + * req->gen is the newest dirty interval among the extent's pages, so it is + * safe to compare every page of the extent against; see + * clockcache_writeback_get_status(). *----------------------------------------------------------------------------- */ -void -clockcache_extent_writeback(clockcache *cc, - uint64 addr, - uint64 *pages_outstanding) +platform_status +clockcache_extent_writeback(clockcache *cc, + uint64 addr, + page_type type, + cache_writeback_request *req) { async_io_state *state = NULL; uint64 i; uint32 entry_number; - uint64 req_count = 0; uint64 req_addr; uint64 page_addr; + const threadid tid = platform_get_tid(); + uint64 max_gen = 0; + platform_status result = STATUS_OK; + + req->addr = addr; + req->gen = 0; + req->is_extent = TRUE; for (i = 0; i < cc->cfg->pages_per_extent; i++) { page_addr = addr + clockcache_multiply_by_page_size(cc, i); @@ -2540,6 +2552,19 @@ clockcache_extent_writeback(clockcache *cc, if (entry_number != CC_UNMAPPED_ENTRY && clockcache_try_set_writeback(cc, entry_number, TRUE)) { + /* + * As in clockcache_page_writeback(), read the generation only under + * CC_WRITEBACK, where the writer is excluded and the interval cannot + * advance under us. + */ + uint64 gen = clockcache_get_writeback_generation(cc, entry_number); + max_gen = MAX(max_gen, gen); + + if (cc->cfg->use_stats) { + cc->stats[tid].page_writes[type]++; + cc->stats[tid].syncs_issued++; + } + if (state == NULL) { req_addr = page_addr; state = TYPED_MALLOC(PROCESS_PRIVATE_HEAP_ID, state); @@ -2552,9 +2577,8 @@ clockcache_extent_writeback(clockcache *cc, entry_number); } platform_assert(state); - state->cc = cc; - state->outstanding_pages = pages_outstanding; - platform_status rc = io_async_state_init(state->iostate, + state->cc = cc; + platform_status rc = io_async_state_init(state->iostate, cc->io, io_async_pwritev, req_addr, @@ -2583,23 +2607,122 @@ clockcache_extent_writeback(clockcache *cc, platform_status_to_string(rc)); } platform_assert_status_ok(rc); - req_count++; } else { - // ALEX: There is maybe a race with eviction with this assertion - debug_assert(entry_number == CC_UNMAPPED_ENTRY - || clockcache_test_flag(cc, entry_number, CC_CLEAN)); + if (entry_number != CC_UNMAPPED_ENTRY + && !clockcache_test_flag(cc, entry_number, CC_CLEAN)) + { + /* + * Dirty and not cleanable. If a writeback is already in flight we + * can still wait on it; otherwise the page is locked or claimed and + * no write will be issued for it, which the caller has to hear + * about. + */ + if (clockcache_test_flag(cc, entry_number, CC_WRITEBACK)) { + max_gen = + MAX(max_gen, + clockcache_get_writeback_generation(cc, entry_number)); + } else { + result = STATUS_BUSY; + } + } + /* Contiguity is broken here, so close out the pending I/O. */ if (state != NULL) { - __sync_fetch_and_add(pages_outstanding, req_count); io_async_run(state->iostate); - state = NULL; - req_count = 0; + state = NULL; } } } if (state != NULL) { - __sync_fetch_and_add(pages_outstanding, req_count); io_async_run(state->iostate); } + + /* + * Publish the generation even on failure: writes were issued for the pages + * we did claim, and the caller must be able to wait them out before it + * reuses or frees the extent. + */ + req->gen = max_gen; + return result; +} + +/* + *----------------------------------------------------------------------------- + * clockcache_page_writeback_get_status -- + * + * Whether the write covering interval `gen` of the page at `addr` has + * completed. + * + * The flag is tested before the generation is read, and that order is + * load-bearing. Completion runs gen := 0, then set CC_CLEAN, then clear + * CC_WRITEBACK (see clockcache_dirty_complete_writeback), so observing + * CC_WRITEBACK clear implies the generation was already 0 at that instant. + * Reading the generation afterwards and finding it back at `gen` therefore + * proves the page was cleaned and dirtied again -- whereas reading the + * generation first would report that same pair for a write that had merely + * just completed, a false report of a concurrent writer. + *----------------------------------------------------------------------------- + */ +static cache_writeback_status +clockcache_page_writeback_get_status(clockcache *cc, uint64 addr, uint64 gen) +{ + if (gen == 0) { + // No write was issued for this page: nothing to wait for. + return CACHE_WRITEBACK_COMPLETE; + } + + uint32 entry_number = clockcache_lookup(cc, addr); + if (entry_number == CC_UNMAPPED_ENTRY) { + /* + * Not resident, so the contents reached the device before the page left + * the cache: eviction requires CC_EVICTABLE_STATUS, which is CC_CLEAN, + * and only clockcache_dirty_complete_writeback() sets CC_CLEAN. + */ + return CACHE_WRITEBACK_COMPLETE; + } + + bool32 in_writeback = clockcache_test_flag(cc, entry_number, CC_WRITEBACK); + uint64 cur = clockcache_get_writeback_generation(cc, entry_number); + + if (clockcache_get_entry(cc, entry_number)->page.disk_addr != addr) { + // The entry was rebound under us, so it was evicted -- and hence written + // back -- and the flag and generation we just read belong to some other + // page. + return CACHE_WRITEBACK_COMPLETE; + } + if (cur == 0) { + return CACHE_WRITEBACK_COMPLETE; + } + if (cur != gen) { + debug_assert( + cur > gen, "dirty generation went backwards for addr %lu", addr); + return CACHE_WRITEBACK_REDIRTIED; + } + return in_writeback ? CACHE_WRITEBACK_PENDING : CACHE_WRITEBACK_REDIRTIED; +} + +cache_writeback_status +clockcache_writeback_get_status(clockcache *cc, + const cache_writeback_request *req) +{ + uint64 num_pages = req->is_extent ? cc->cfg->pages_per_extent : 1; + bool32 redirtied = FALSE; + + for (uint64 i = 0; i < num_pages; i++) { + uint64 addr = req->addr + clockcache_multiply_by_page_size(cc, i); + switch (clockcache_page_writeback_get_status(cc, addr, req->gen)) { + case CACHE_WRITEBACK_PENDING: + // One outstanding page is enough; no need to look at the rest. + return CACHE_WRITEBACK_PENDING; + case CACHE_WRITEBACK_REDIRTIED: + // Keep going: a later page may still be pending, which the caller + // has to keep waiting for. + redirtied = TRUE; + break; + case CACHE_WRITEBACK_COMPLETE: + break; + } + } + return redirtied ? CACHE_WRITEBACK_REDIRTIED : CACHE_WRITEBACK_COMPLETE; } /* @@ -3312,23 +3435,32 @@ clockcache_get_async_state_result_virtual(void *payload) return state->__async_result; } -void -clockcache_page_writeback_virtual(cache *c, - page_handle *page, - bool32 is_blocking, - page_type type) +platform_status +clockcache_page_writeback_virtual(cache *c, + page_handle *page, + page_type type, + cache_writeback_request *req) { clockcache *cc = (clockcache *)c; - clockcache_page_writeback(cc, page, is_blocking, type); + return clockcache_page_writeback(cc, page, type, req); } -void -clockcache_extent_writeback_virtual(cache *c, - uint64 addr, - uint64 *pages_outstanding) +platform_status +clockcache_extent_writeback_virtual(cache *c, + uint64 addr, + page_type type, + cache_writeback_request *req) +{ + clockcache *cc = (clockcache *)c; + return clockcache_extent_writeback(cc, addr, type, req); +} + +cache_writeback_status +clockcache_writeback_get_status_virtual(cache *c, + const cache_writeback_request *req) { clockcache *cc = (clockcache *)c; - clockcache_extent_writeback(cc, addr, pages_outstanding); + return clockcache_writeback_get_status(cc, req); } void @@ -3473,36 +3605,37 @@ static cache_ops clockcache_ops = { .page_get_async = clockcache_get_async_virtual, .page_get_async_result = clockcache_get_async_state_result_virtual, - .page_unget = clockcache_unget_virtual, - .page_try_claim = clockcache_try_claim_virtual, - .page_unclaim = clockcache_unclaim_virtual, - .page_lock = clockcache_lock_virtual, - .page_unlock = clockcache_unlock_virtual, - .page_prefetch = clockcache_prefetch_virtual, - .page_prefetch_page = clockcache_prefetch_page_virtual, - .page_pin = clockcache_pin_virtual, - .page_unpin = clockcache_unpin_virtual, - .page_writeback = clockcache_page_writeback_virtual, - .extent_writeback = clockcache_extent_writeback_virtual, - .flush = clockcache_flush_virtual, - .writeback_dirty = clockcache_writeback_dirty_virtual, - .durable_barrier = clockcache_durable_barrier_virtual, - .evict = clockcache_evict_all_virtual, - .cleanup = clockcache_wait_virtual, - .in_use = clockcache_in_use_virtual, - .assert_ungot = clockcache_assert_ungot_virtual, - .assert_free = clockcache_assert_no_locks_held_virtual, - .print = clockcache_print_virtual, - .print_stats = clockcache_print_stats_virtual, - .io_stats = clockcache_io_stats_virtual, - .reset_stats = clockcache_reset_stats_virtual, - .validate_page = clockcache_validate_page_virtual, - .count_dirty = clockcache_count_dirty_virtual, - .page_get_read_ref = clockcache_get_read_ref_virtual, - .cache_present = clockcache_present_virtual, - .enable_sync_get = clockcache_enable_sync_get_virtual, - .get_allocator = clockcache_get_allocator_virtual, - .get_config = clockcache_get_config_virtual, + .page_unget = clockcache_unget_virtual, + .page_try_claim = clockcache_try_claim_virtual, + .page_unclaim = clockcache_unclaim_virtual, + .page_lock = clockcache_lock_virtual, + .page_unlock = clockcache_unlock_virtual, + .page_prefetch = clockcache_prefetch_virtual, + .page_prefetch_page = clockcache_prefetch_page_virtual, + .page_pin = clockcache_pin_virtual, + .page_unpin = clockcache_unpin_virtual, + .page_writeback = clockcache_page_writeback_virtual, + .extent_writeback = clockcache_extent_writeback_virtual, + .writeback_get_status = clockcache_writeback_get_status_virtual, + .flush = clockcache_flush_virtual, + .writeback_dirty = clockcache_writeback_dirty_virtual, + .durable_barrier = clockcache_durable_barrier_virtual, + .evict = clockcache_evict_all_virtual, + .cleanup = clockcache_wait_virtual, + .in_use = clockcache_in_use_virtual, + .assert_ungot = clockcache_assert_ungot_virtual, + .assert_free = clockcache_assert_no_locks_held_virtual, + .print = clockcache_print_virtual, + .print_stats = clockcache_print_stats_virtual, + .io_stats = clockcache_io_stats_virtual, + .reset_stats = clockcache_reset_stats_virtual, + .validate_page = clockcache_validate_page_virtual, + .count_dirty = clockcache_count_dirty_virtual, + .page_get_read_ref = clockcache_get_read_ref_virtual, + .cache_present = clockcache_present_virtual, + .enable_sync_get = clockcache_enable_sync_get_virtual, + .get_allocator = clockcache_get_allocator_virtual, + .get_config = clockcache_get_config_virtual, }; /* diff --git a/src/shard_log.c b/src/shard_log.c index 3f6934d9..3c8086ea 100644 --- a/src/shard_log.c +++ b/src/shard_log.c @@ -244,7 +244,11 @@ shard_log_write(log_handle *logh, cache_unlock(cc, page); cache_unclaim(cc, page); - cache_page_writeback(cc, page, FALSE, PAGE_TYPE_LOG); + cache_writeback_request wb_req; + platform_status wb_rc = + cache_writeback_page(cc, page, PAGE_TYPE_LOG, &wb_req); + // This is the thread's own append page: nothing else can have it locked. + platform_assert_status_ok(wb_rc); cache_unget(cc, page); if (get_new_page_for_thread(log, thread_data, &page)) { diff --git a/tests/functional/cache_test.c b/tests/functional/cache_test.c index e774f90a..4cc79dcf 100644 --- a/tests/functional/cache_test.c +++ b/tests/functional/cache_test.c @@ -892,7 +892,9 @@ cache_test_hammer_thread(void *arg) memset(page->data, (uint8)i, ctxt->page_size); cache_unlock(ctxt->cc, page); cache_unclaim(ctxt->cc, page); - cache_page_writeback(ctxt->cc, page, FALSE, PAGE_TYPE_MISC); + cache_writeback_request wb_req; + // Other threads race us for these pages, so STATUS_BUSY is expected. + cache_writeback_page(ctxt->cc, page, PAGE_TYPE_MISC, &wb_req); } cache_unget(ctxt->cc, page); i++; From bc9c1c1571c0b383a40399991838d8c4b8a8523b Mon Sep 17 00:00:00 2001 From: Rob Johnson Date: Fri, 31 Jul 2026 12:39:14 -0700 Subject: [PATCH 2/8] Fix writeback races Signed-off-by: Rob Johnson --- src/blob.c | 5 +- src/cache.h | 26 +++- src/clockcache.c | 235 ++++++++++++++++++++++------------ src/shard_log.c | 5 +- tests/functional/cache_test.c | 3 +- 5 files changed, 178 insertions(+), 96 deletions(-) diff --git a/src/blob.c b/src/blob.c index 77c01f3d..b4069c98 100644 --- a/src/blob.c +++ b/src/blob.c @@ -326,9 +326,8 @@ blob_sync(cache *cc, slice sblob) if (!SUCCESS(rc)) { break; } - cache_writeback_request wb_req; - platform_status wb_rc = - cache_writeback_page(cc, itor.page, PAGE_TYPE_BLOB, &wb_req); + platform_status wb_rc = + cache_writeback_page(cc, itor.page, PAGE_TYPE_BLOB, NULL); // The iterator holds a read reference only, never a claim or lock. platform_assert_status_ok(wb_rc); blob_page_iterator_advance_page(&itor); diff --git a/src/cache.h b/src/cache.h index 74da6326..1ddf091b 100644 --- a/src/cache.h +++ b/src/cache.h @@ -147,6 +147,9 @@ typedef bool32 (*page_try_claim_fn)(cache *cc, page_handle *page); * * gen == 0 means "nothing to wait for": the page was already clean when the * writeback was requested. + * + * A caller that only wants the write issued, and will never ask whether it + * completed, may pass NULL instead of a request. */ typedef struct cache_writeback_request { uint64 addr; // page addr, or the base addr of an extent @@ -507,10 +510,14 @@ cache_unpin(cache *cc, page_handle *page) *----------------------------------------------------------------------------- * cache_writeback_page * - * Asynchronously issues writeback of the page and fills in *req, the receipt - * for that write. This does NOT make the page durable; it only hands the write - * to the I/O layer (no device-cache flush). Use cache_writeback_get_status() to - * learn when it completes and cache_durable_barrier() to make it durable. + * Asynchronously issues writeback of the page and, if req is non-NULL, fills + * in *req, the receipt for that write. This does NOT make the page durable; it + * only hands the write to the I/O layer (no device-cache flush). Use + * cache_writeback_get_status() to learn when it completes and + * cache_durable_barrier() to make it durable. + * + * req may be NULL if the caller will never ask whether the write completed. + * The returned status is still worth checking even then; see below. * * Does not block. If a writeback of this page is already in flight -- issued by * the pressure cleaner, say -- nothing further is issued and *req names that @@ -528,7 +535,10 @@ cache_writeback_page(cache *cc, page_type type, cache_writeback_request *req) { - return cc->ops->page_writeback(cc, page, type, req); + // Absorb the optional request here so that every cache implementation may + // assume it was given somewhere to write the receipt. + cache_writeback_request scratch; + return cc->ops->page_writeback(cc, page, type, req ? req : &scratch); } /* @@ -538,7 +548,8 @@ cache_writeback_page(cache *cc, * As cache_writeback_page(), but for every page of the extent beginning at * addr, coalesced into as few larger I/Os as the extent's residency allows. * One request covers the whole extent: req->gen is the newest dirty interval - * among its pages, which is safe to compare every page against. + * among its pages, which is safe to compare every page against. As above, req + * may be NULL. * * Pages of the extent that are clean or not resident need no write and are * skipped. Returns STATUS_BUSY if any page is dirty but not writeback-able; as @@ -555,7 +566,8 @@ cache_writeback_extent(cache *cc, page_type type, cache_writeback_request *req) { - return cc->ops->extent_writeback(cc, addr, type, req); + cache_writeback_request scratch; + return cc->ops->extent_writeback(cc, addr, type, req ? req : &scratch); } /* diff --git a/src/clockcache.c b/src/clockcache.c index bbb1c9b6..c316c802 100644 --- a/src/clockcache.c +++ b/src/clockcache.c @@ -259,10 +259,10 @@ clockcache_dirty_begin(clockcache *cc, uint32 entry_number) * * Meaningful as a writeback receipt only when read while CC_WRITEBACK is set, * which excludes the writer and so pins the interval; see - * clockcache_page_writeback(). + * clockcache_writeback_page(). */ static inline uint64 -clockcache_get_writeback_generation(clockcache *cc, uint32 entry_number) +clockcache_writeback_get_generation(clockcache *cc, uint32 entry_number) { clockcache_entry *entry = clockcache_get_entry(cc, entry_number); return __atomic_load_n(&entry->dirty_generation, __ATOMIC_RELAXED); @@ -1218,7 +1218,7 @@ clockcache_writeback_dirty(clockcache *cc) * Evictability requires CC_EVICTABLE_STATUS, i.e. CC_CLEAN and nothing * else, and CC_CLEAN is set only by clockcache_dirty_complete_writeback(). * So a page cannot leave the cache before its writeback has completed -- - * an invariant clockcache_page_writeback_get_status() relies on to treat a + * an invariant clockcache_writeback_get_page_status() relies on to treat a * non-resident page as written. *---------------------------------------------------------------------- */ @@ -2408,7 +2408,93 @@ clockcache_unpin(clockcache *cc, page_handle *page) /* *----------------------------------------------------------------------------- - * clockcache_page_writeback -- + * clockcache_writeback_claim_page -- + * + * Try to take responsibility for writing back one resident page, and say + * what stands in the way if we cannot. *gen is set for CLAIMED and + * INFLIGHT (the interval the write covers) and to 0 for NOT_NEEDED; it is + * left untouched for UNAVAILABLE. + * + * Shared by the page and extent writeback paths so that the decision -- + * and in particular the single-snapshot rule below -- lives in one place. + *----------------------------------------------------------------------------- + */ +typedef enum clockcache_writeback_claim { + CC_WRITEBACK_CLAIMED, // we set CC_WRITEBACK; caller must issue the I/O + CC_WRITEBACK_NOT_NEEDED, // already clean: nothing to write + CC_WRITEBACK_INFLIGHT, // someone else's write already covers it + CC_WRITEBACK_UNAVAILABLE, // dirty, but locked or claimed +} clockcache_writeback_claim; + +static clockcache_writeback_claim +clockcache_writeback_claim_page(clockcache *cc, + uint32 entry_number, + uint64 *gen) +{ + while (TRUE) { + if (clockcache_try_set_writeback(cc, entry_number, TRUE)) { + /* + * Read the generation only now that CC_WRITEBACK is set. That excludes + * the writer (see clockcache_get_write), so the interval cannot + * advance under us and the receipt provably names the interval this + * write covers. + */ + *gen = clockcache_writeback_get_generation(cc, entry_number); + debug_assert(*gen != 0, "a cleanable page must have a dirty interval"); + return CC_WRITEBACK_CLAIMED; + } + + /* + * Decide from a single snapshot of the status word. Testing the flags one + * at a time lets a concurrent writeback complete in between -- dirty when + * we test CC_CLEAN, no longer in writeback when we test CC_WRITEBACK -- + * and report a failure for a page whose contents did in fact reach the + * device. + */ + entry_status status = clockcache_get_status(cc, entry_number); + + if (status & CC_CLEAN) { + // Includes the transient CC_CLEAN|CC_WRITEBACK state that + // clockcache_dirty_complete_writeback passes through: the write has + // completed by the time CC_CLEAN is set. + *gen = 0; + return CC_WRITEBACK_NOT_NEEDED; + } + if (status & CC_WRITEBACK) { + /* + * A pressure cleaner or a checkpoint fence got here first. Report that + * in-flight interval rather than blocking on it: the caller waits on + * the receipt exactly as it would on a write we issued ourselves, and + * blocking would serialize a caller issuing writeback for many pages. + * This may read 0 if the write has completed since the snapshot, which + * correctly says there is nothing left to wait for. + */ + *gen = clockcache_writeback_get_generation(cc, entry_number); + return CC_WRITEBACK_INFLIGHT; + } + if (status & (CC_WRITELOCKED | CC_CLAIMED | CC_FREE | CC_LOADING)) { + debug_assert(!(status & (CC_FREE | CC_LOADING)), + "writeback of a free or loading page: entry=%u status=%u", + entry_number, + status); + return CC_WRITEBACK_UNAVAILABLE; + } + + /* + * Dirty, unlocked and not in writeback. The tests above cover every + * status bit but CC_ACCESSED, so the snapshot is CC_CLEANABLE1_STATUS or + * CC_CLEANABLE2_STATUS -- both of which try_set_writeback() accepts. The + * status therefore changed between the failed compare-and-swap and the + * snapshot, so retry rather than report a failure we already know to be + * stale. (That exhaustiveness is what keeps this from spinning on a + * status the cascade forgot to handle.) + */ + } +} + +/* + *----------------------------------------------------------------------------- + * clockcache_writeback_page -- * * Issues writeback of the page and fills in *req. This does not make the * page durable; it only hands the write to the I/O layer. @@ -2417,7 +2503,7 @@ clockcache_unpin(clockcache *cc, page_handle *page) *----------------------------------------------------------------------------- */ platform_status -clockcache_page_writeback(clockcache *cc, +clockcache_writeback_page(clockcache *cc, page_handle *page, page_type type, cache_writeback_request *req) @@ -2432,35 +2518,18 @@ clockcache_page_writeback(clockcache *cc, req->gen = 0; req->is_extent = FALSE; - if (!clockcache_try_set_writeback(cc, entry_number, TRUE)) { - if (clockcache_test_flag(cc, entry_number, CC_CLEAN)) { - // Nothing to write: gen 0 tells the caller there is nothing to wait - // for. - return STATUS_OK; - } - /* - * A pressure cleaner or a checkpoint fence may have begun writeback - * after the caller released its claim. Report that in-flight interval - * rather than blocking on it: the caller waits on the returned request - * exactly as it would on a write we issued ourselves. Blocking here - * would serialize a caller that is issuing writeback for many pages. - */ - if (clockcache_test_flag(cc, entry_number, CC_WRITEBACK)) { - req->gen = clockcache_get_writeback_generation(cc, entry_number); + switch (clockcache_writeback_claim_page(cc, entry_number, &req->gen)) { + case CC_WRITEBACK_NOT_NEEDED: + case CC_WRITEBACK_INFLIGHT: + // Nothing for us to issue; req->gen says what to wait on, if + // anything. return STATUS_OK; - } - // Dirty but locked or claimed: no write can be issued for it. - return STATUS_BUSY; + case CC_WRITEBACK_UNAVAILABLE: + return STATUS_BUSY; + case CC_WRITEBACK_CLAIMED: + break; } - /* - * Read the generation only now that CC_WRITEBACK is set. That excludes the - * writer (see clockcache_get_write), so the interval cannot advance under - * us and the request provably names the interval this write covers. - */ - req->gen = clockcache_get_writeback_generation(cc, entry_number); - debug_assert(req->gen != 0, "a cleanable page must have a dirty interval"); - if (cc->cfg->use_stats) { cc->stats[tid].page_writes[type]++; cc->stats[tid].syncs_issued++; @@ -2468,7 +2537,7 @@ clockcache_page_writeback(clockcache *cc, state = TYPED_MALLOC(PROCESS_PRIVATE_HEAP_ID, state); if (state == NULL) { - platform_error_log("clockcache_page_writeback: async_io_state allocation " + platform_error_log("clockcache_writeback_page: async_io_state allocation " "failed for addr %lu, entry %u, type %u\n", addr, entry_number, @@ -2485,7 +2554,7 @@ clockcache_page_writeback(clockcache *cc, clockcache_write_callback, state); if (!SUCCESS(status)) { - platform_error_log("clockcache_page_writeback: io_async_state_init " + platform_error_log("clockcache_writeback_page: io_async_state_init " "failed for addr %lu, entry %u, type %u: %s\n", addr, entry_number, @@ -2495,7 +2564,7 @@ clockcache_page_writeback(clockcache *cc, platform_assert_status_ok(status); status = io_async_state_append_page(state->iostate, page->data); if (!SUCCESS(status)) { - platform_error_log("clockcache_page_writeback: " + platform_error_log("clockcache_writeback_page: " "io_async_state_append_page failed for addr %lu, " "entry %u, type %u: %s\n", addr, @@ -2510,7 +2579,7 @@ clockcache_page_writeback(clockcache *cc, /* *----------------------------------------------------------------------------- - * clockcache_extent_writeback -- + * clockcache_writeback_extent -- * * Asynchronously issues writeback of the extent and fills in *req. This * does not make the extent durable; it only hands the writes to the I/O @@ -2528,7 +2597,7 @@ clockcache_page_writeback(clockcache *cc, *----------------------------------------------------------------------------- */ platform_status -clockcache_extent_writeback(clockcache *cc, +clockcache_writeback_extent(clockcache *cc, uint64 addr, page_type type, cache_writeback_request *req) @@ -2549,17 +2618,24 @@ clockcache_extent_writeback(clockcache *cc, for (i = 0; i < cc->cfg->pages_per_extent; i++) { page_addr = addr + clockcache_multiply_by_page_size(cc, i); entry_number = clockcache_lookup(cc, page_addr); - if (entry_number != CC_UNMAPPED_ENTRY - && clockcache_try_set_writeback(cc, entry_number, TRUE)) - { - /* - * As in clockcache_page_writeback(), read the generation only under - * CC_WRITEBACK, where the writer is excluded and the interval cannot - * advance under us. - */ - uint64 gen = clockcache_get_writeback_generation(cc, entry_number); - max_gen = MAX(max_gen, gen); + /* + * A page that is not resident needs no write: eviction cannot happen + * before writeback completes (see clockcache_try_evict). + */ + uint64 gen = 0; + clockcache_writeback_claim claim = CC_WRITEBACK_NOT_NEEDED; + if (entry_number != CC_UNMAPPED_ENTRY) { + claim = clockcache_writeback_claim_page(cc, entry_number, &gen); + } + max_gen = MAX(max_gen, gen); + if (claim == CC_WRITEBACK_UNAVAILABLE) { + // No write will be issued for this page, so the extent will not be + // fully written. The caller has to hear about it. + result = STATUS_BUSY; + } + + if (claim == CC_WRITEBACK_CLAIMED) { if (cc->cfg->use_stats) { cc->stats[tid].page_writes[type]++; cc->stats[tid].syncs_issued++; @@ -2569,14 +2645,22 @@ clockcache_extent_writeback(clockcache *cc, req_addr = page_addr; state = TYPED_MALLOC(PROCESS_PRIVATE_HEAP_ID, state); if (state == NULL) { - platform_error_log("clockcache_extent_writeback: async_io_state " + platform_error_log("clockcache_writeback_extent: async_io_state " "allocation failed for extent addr %lu, " "page addr %lu, entry %u\n", addr, page_addr, entry_number); + /* + * We are at the head of a fresh run, so the only claim to undo + * is the one just taken; earlier runs are already in flight and + * the caller waits them out via req->gen. + */ + clockcache_abort_writeback_range( + cc, page_addr, page_addr + clockcache_page_size(cc)); + req->gen = max_gen; + return STATUS_NO_MEMORY; } - platform_assert(state); state->cc = cc; platform_status rc = io_async_state_init(state->iostate, cc->io, @@ -2585,7 +2669,7 @@ clockcache_extent_writeback(clockcache *cc, clockcache_write_callback, state); if (!SUCCESS(rc)) { - platform_error_log("clockcache_extent_writeback: " + platform_error_log("clockcache_writeback_extent: " "io_async_state_init failed for extent addr " "%lu, req addr %lu, entry %u: %s\n", addr, @@ -2598,7 +2682,7 @@ clockcache_extent_writeback(clockcache *cc, platform_status rc = io_async_state_append_page( state->iostate, clockcache_get_entry(cc, entry_number)->page.data); if (!SUCCESS(rc)) { - platform_error_log("clockcache_extent_writeback: " + platform_error_log("clockcache_writeback_extent: " "io_async_state_append_page failed for extent " "addr %lu, page addr %lu, entry %u: %s\n", addr, @@ -2608,24 +2692,8 @@ clockcache_extent_writeback(clockcache *cc, } platform_assert_status_ok(rc); } else { - if (entry_number != CC_UNMAPPED_ENTRY - && !clockcache_test_flag(cc, entry_number, CC_CLEAN)) - { - /* - * Dirty and not cleanable. If a writeback is already in flight we - * can still wait on it; otherwise the page is locked or claimed and - * no write will be issued for it, which the caller has to hear - * about. - */ - if (clockcache_test_flag(cc, entry_number, CC_WRITEBACK)) { - max_gen = - MAX(max_gen, - clockcache_get_writeback_generation(cc, entry_number)); - } else { - result = STATUS_BUSY; - } - } - /* Contiguity is broken here, so close out the pending I/O. */ + /* We are not writing this page, so contiguity breaks here: close out + * whatever run of pages we had accumulated. */ if (state != NULL) { io_async_run(state->iostate); state = NULL; @@ -2647,7 +2715,7 @@ clockcache_extent_writeback(clockcache *cc, /* *----------------------------------------------------------------------------- - * clockcache_page_writeback_get_status -- + * clockcache_writeback_get_page_status -- * * Whether the write covering interval `gen` of the page at `addr` has * completed. @@ -2663,7 +2731,7 @@ clockcache_extent_writeback(clockcache *cc, *----------------------------------------------------------------------------- */ static cache_writeback_status -clockcache_page_writeback_get_status(clockcache *cc, uint64 addr, uint64 gen) +clockcache_writeback_get_page_status(clockcache *cc, uint64 addr, uint64 gen) { if (gen == 0) { // No write was issued for this page: nothing to wait for. @@ -2681,7 +2749,7 @@ clockcache_page_writeback_get_status(clockcache *cc, uint64 addr, uint64 gen) } bool32 in_writeback = clockcache_test_flag(cc, entry_number, CC_WRITEBACK); - uint64 cur = clockcache_get_writeback_generation(cc, entry_number); + uint64 cur = clockcache_writeback_get_generation(cc, entry_number); if (clockcache_get_entry(cc, entry_number)->page.disk_addr != addr) { // The entry was rebound under us, so it was evicted -- and hence written @@ -2692,11 +2760,16 @@ clockcache_page_writeback_get_status(clockcache *cc, uint64 addr, uint64 gen) if (cur == 0) { return CACHE_WRITEBACK_COMPLETE; } - if (cur != gen) { - debug_assert( - cur > gen, "dirty generation went backwards for addr %lu", addr); + if (cur > gen) { + // Drained, then dirtied again in a later window. return CACHE_WRITEBACK_REDIRTIED; } + /* + * cur <= gen: the page may still be in the interval we wrote. Note that + * cur < gen is legitimate for an extent request, whose gen is the newest + * interval among its pages -- a writeback_dirty() landing while the extent + * was being filled leaves its pages spread over two windows. + */ return in_writeback ? CACHE_WRITEBACK_PENDING : CACHE_WRITEBACK_REDIRTIED; } @@ -2709,7 +2782,7 @@ clockcache_writeback_get_status(clockcache *cc, for (uint64 i = 0; i < num_pages; i++) { uint64 addr = req->addr + clockcache_multiply_by_page_size(cc, i); - switch (clockcache_page_writeback_get_status(cc, addr, req->gen)) { + switch (clockcache_writeback_get_page_status(cc, addr, req->gen)) { case CACHE_WRITEBACK_PENDING: // One outstanding page is enough; no need to look at the rest. return CACHE_WRITEBACK_PENDING; @@ -3436,23 +3509,23 @@ clockcache_get_async_state_result_virtual(void *payload) } platform_status -clockcache_page_writeback_virtual(cache *c, +clockcache_writeback_page_virtual(cache *c, page_handle *page, page_type type, cache_writeback_request *req) { clockcache *cc = (clockcache *)c; - return clockcache_page_writeback(cc, page, type, req); + return clockcache_writeback_page(cc, page, type, req); } platform_status -clockcache_extent_writeback_virtual(cache *c, +clockcache_writeback_extent_virtual(cache *c, uint64 addr, page_type type, cache_writeback_request *req) { clockcache *cc = (clockcache *)c; - return clockcache_extent_writeback(cc, addr, type, req); + return clockcache_writeback_extent(cc, addr, type, req); } cache_writeback_status @@ -3614,8 +3687,8 @@ static cache_ops clockcache_ops = { .page_prefetch_page = clockcache_prefetch_page_virtual, .page_pin = clockcache_pin_virtual, .page_unpin = clockcache_unpin_virtual, - .page_writeback = clockcache_page_writeback_virtual, - .extent_writeback = clockcache_extent_writeback_virtual, + .page_writeback = clockcache_writeback_page_virtual, + .extent_writeback = clockcache_writeback_extent_virtual, .writeback_get_status = clockcache_writeback_get_status_virtual, .flush = clockcache_flush_virtual, .writeback_dirty = clockcache_writeback_dirty_virtual, diff --git a/src/shard_log.c b/src/shard_log.c index 3c8086ea..31358d28 100644 --- a/src/shard_log.c +++ b/src/shard_log.c @@ -244,9 +244,8 @@ shard_log_write(log_handle *logh, cache_unlock(cc, page); cache_unclaim(cc, page); - cache_writeback_request wb_req; - platform_status wb_rc = - cache_writeback_page(cc, page, PAGE_TYPE_LOG, &wb_req); + platform_status wb_rc = + cache_writeback_page(cc, page, PAGE_TYPE_LOG, NULL); // This is the thread's own append page: nothing else can have it locked. platform_assert_status_ok(wb_rc); cache_unget(cc, page); diff --git a/tests/functional/cache_test.c b/tests/functional/cache_test.c index 4cc79dcf..280600cc 100644 --- a/tests/functional/cache_test.c +++ b/tests/functional/cache_test.c @@ -892,9 +892,8 @@ cache_test_hammer_thread(void *arg) memset(page->data, (uint8)i, ctxt->page_size); cache_unlock(ctxt->cc, page); cache_unclaim(ctxt->cc, page); - cache_writeback_request wb_req; // Other threads race us for these pages, so STATUS_BUSY is expected. - cache_writeback_page(ctxt->cc, page, PAGE_TYPE_MISC, &wb_req); + cache_writeback_page(ctxt->cc, page, PAGE_TYPE_MISC, NULL); } cache_unget(ctxt->cc, page); i++; From d997a56530b0d26d3bb616f5425cbf89145c0a18 Mon Sep 17 00:00:00 2001 From: Rob Johnson Date: Fri, 31 Jul 2026 13:12:35 -0700 Subject: [PATCH 3/8] fix some error handling in writeback Signed-off-by: Rob Johnson --- src/cache.h | 5 +++-- src/clockcache.c | 46 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/cache.h b/src/cache.h index 1ddf091b..8dfaf8b6 100644 --- a/src/cache.h +++ b/src/cache.h @@ -556,8 +556,9 @@ cache_writeback_page(cache *cc, * with cache_writeback_page(), the caller must treat that as a failure, since * that page's contents will not reach the device. * - * TODO: What happens if two callers call cache_writeback_extent on the same - * extent? + * Concurrent callers on the same extent are safe: only one can win a page's + * writeback, and the other's request names that same in-flight interval, so + * both observe its completion. *----------------------------------------------------------------------------- */ static inline platform_status diff --git a/src/clockcache.c b/src/clockcache.c index c316c802..70bbfd19 100644 --- a/src/clockcache.c +++ b/src/clockcache.c @@ -953,6 +953,26 @@ clockcache_write_callback(void *wbs) platform_free(PROCESS_PRIVATE_HEAP_ID, state); } +/* + * Retracts an in-progress writeback claim on I/O-layer setup failure (see + * clockcache_batch_start_writeback()). This leaves the affected pages dirty and + * eligible for writeback again, which is what lets + * clockcache_writeback_dirty() (and hence a checkpoint) recover from a + * transient allocation failure rather than fail outright. + * + * KNOWN GAP: if another thread already holds a CC_WRITEBACK_INFLIGHT receipt + * naming one of these pages -- i.e. it called cache_writeback_page() or + * cache_writeback_extent() on a page this function has claimed, before this + * function ran -- that thread's next status poll will read the page as + * REDIRTIED, which it cannot tell apart from a completed write followed by a + * legitimate re-dirty. That is a silent durability violation for that thread. + * clockcache_writeback_page() and clockcache_writeback_extent() close this for + * their own claims by crashing here instead of retracting; this call site does + * not, because retraction here is relied upon (see above). Closing it properly + * needs a distinguishable failure state that a waiter can observe and react + * to, which the cache does not have: it is built around I/O never failing, and + * fixing that is a larger, separate project. + */ static void clockcache_abort_writeback_range(clockcache *cc, uint64 first_addr, @@ -2542,10 +2562,17 @@ clockcache_writeback_page(clockcache *cc, addr, entry_number, type); - clockcache_abort_writeback_range( - cc, addr, addr + clockcache_page_size(cc)); - return STATUS_NO_MEMORY; } + /* + * Crash rather than retract the claim (as the two failure branches below + * already do). Retracting would clear CC_WRITEBACK while another thread may + * already hold a CC_WRITEBACK_INFLIGHT receipt naming this page's + * generation; its next status poll would then read the page as + * REDIRTIED -- indistinguishable from a completed write followed by a + * legitimate re-dirty. That would be a silent durability violation, which + * is worse than a crash. + */ + platform_assert(state != NULL); state->cc = cc; status = io_async_state_init(state->iostate, cc->io, @@ -2651,16 +2678,11 @@ clockcache_writeback_extent(clockcache *cc, addr, page_addr, entry_number); - /* - * We are at the head of a fresh run, so the only claim to undo - * is the one just taken; earlier runs are already in flight and - * the caller waits them out via req->gen. - */ - clockcache_abort_writeback_range( - cc, page_addr, page_addr + clockcache_page_size(cc)); - req->gen = max_gen; - return STATUS_NO_MEMORY; } + // See clockcache_writeback_page(): crash rather than retract this + // claim, which would let a concurrent INFLIGHT observer read the + // page as falsely complete. + platform_assert(state != NULL); state->cc = cc; platform_status rc = io_async_state_init(state->iostate, cc->io, From 1d4dd95900debafc69588ced4dc01e3ae9b35fd7 Mon Sep 17 00:00:00 2001 From: Rob Johnson Date: Fri, 31 Jul 2026 13:29:11 -0700 Subject: [PATCH 4/8] definitely die on io error in writeback Signed-off-by: Rob Johnson --- src/clockcache.c | 74 ++++++++++++++---------------------------------- 1 file changed, 21 insertions(+), 53 deletions(-) diff --git a/src/clockcache.c b/src/clockcache.c index 70bbfd19..6d53cfd9 100644 --- a/src/clockcache.c +++ b/src/clockcache.c @@ -862,6 +862,21 @@ clockcache_ok_to_writeback(clockcache *cc, * compare-and- swaps, so we retry as long as the status remains one of the * cleanable states rather than spuriously failing on a page that stayed * cleanable. + * + * INVARIANT: a claim cannot be retracted. Once CC_WRITEBACK is set, a + * write must be issued and must run to completion, because another thread + * may already hold a CC_WRITEBACK_INFLIGHT receipt naming this page's + * dirty interval (see cache_writeback_page()). Clearing CC_WRITEBACK + * without writing would make that thread's next status poll read the page + * as REDIRTIED, which is indistinguishable from a completed write followed + * by a legitimate re-dirty -- a silent durability violation. So every + * failure between here and io_async_run() is fatal. + * + * Handling such a failure without crashing needs a state a waiter can + * observe and react to -- an error flag that some thread later claims a + * retry on, plus a status the waiter can fail on rather than hang. That is + * a bigger change, and it belongs with handling write-completion errors + * (clockcache_write_callback() asserts on those today), not ahead of it. *---------------------------------------------------------------------- */ static inline bool32 @@ -953,44 +968,6 @@ clockcache_write_callback(void *wbs) platform_free(PROCESS_PRIVATE_HEAP_ID, state); } -/* - * Retracts an in-progress writeback claim on I/O-layer setup failure (see - * clockcache_batch_start_writeback()). This leaves the affected pages dirty and - * eligible for writeback again, which is what lets - * clockcache_writeback_dirty() (and hence a checkpoint) recover from a - * transient allocation failure rather than fail outright. - * - * KNOWN GAP: if another thread already holds a CC_WRITEBACK_INFLIGHT receipt - * naming one of these pages -- i.e. it called cache_writeback_page() or - * cache_writeback_extent() on a page this function has claimed, before this - * function ran -- that thread's next status poll will read the page as - * REDIRTIED, which it cannot tell apart from a completed write followed by a - * legitimate re-dirty. That is a silent durability violation for that thread. - * clockcache_writeback_page() and clockcache_writeback_extent() close this for - * their own claims by crashing here instead of retracting; this call site does - * not, because retraction here is relied upon (see above). Closing it properly - * needs a distinguishable failure state that a waiter can observe and react - * to, which the cache does not have: it is built around I/O never failing, and - * fixing that is a larger, separate project. - */ -static void -clockcache_abort_writeback_range(clockcache *cc, - uint64 first_addr, - uint64 end_addr) -{ - uint64 page_size = clockcache_page_size(cc); - - for (uint64 addr = first_addr; addr < end_addr; addr += page_size) { - uint32 entry_number = clockcache_lookup(cc, addr); - platform_assert(entry_number != CC_UNMAPPED_ENTRY); - - debug_only uint32 was_writeback = - clockcache_clear_flag(cc, entry_number, CC_WRITEBACK); - debug_assert(was_writeback); - } -} - - /* *---------------------------------------------------------------------- * clockcache_batch_start_writeback -- @@ -1015,7 +992,6 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool32 is_urgent) uint64 end_entry_no = start_entry_no + CC_ENTRIES_PER_BATCH; clockcache_entry *entry, *next_entry; - platform_status result = STATUS_OK; debug_assert((tid < MAX_THREADS), "Invalid tid=%lu\n", tid); debug_assert(cc != NULL); @@ -1076,10 +1052,10 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool32 is_urgent) platform_error_log( "clockcache_batch_start_writeback: async_io_state allocation " "failed\n"); - clockcache_abort_writeback_range(cc, first_addr, end_addr); - result = STATUS_NO_MEMORY; - goto close_log; } + // Fatal: the claim taken above cannot be retracted. See + // clockcache_try_set_writeback(). + platform_assert(state != NULL); state->cc = cc; platform_status rc = io_async_state_init(state->iostate, @@ -1092,11 +1068,8 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool32 is_urgent) platform_error_log("clockcache_batch_start_writeback: " "io_async_state_init failed: %s\n", platform_status_to_string(rc)); - clockcache_abort_writeback_range(cc, first_addr, end_addr); - platform_free(PROCESS_PRIVATE_HEAP_ID, state); - result = rc; - goto close_log; } + platform_assert_status_ok(rc); uint64 req_count = clockcache_divide_by_page_size(cc, end_addr - first_addr); @@ -1117,12 +1090,8 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool32 is_urgent) platform_error_log("clockcache_batch_start_writeback: " "io_async_state_append_page failed: %s\n", platform_status_to_string(rc)); - io_async_state_deinit(state->iostate); - clockcache_abort_writeback_range(cc, first_addr, end_addr); - platform_free(PROCESS_PRIVATE_HEAP_ID, state); - result = rc; - goto close_log; } + platform_assert_status_ok(rc); } if (cc->cfg->use_stats) { @@ -1145,9 +1114,8 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool32 is_urgent) } } -close_log: clockcache_close_log_stream(); - return result; + return STATUS_OK; } /* From 2e6fffa32fd8708679d72107208ee680a3803198 Mon Sep 17 00:00:00 2001 From: Rob Johnson Date: Fri, 31 Jul 2026 13:53:30 -0700 Subject: [PATCH 5/8] start to handle writeback errors Signed-off-by: Rob Johnson --- src/cache.h | 22 ++++++-- src/clockcache.c | 130 +++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 139 insertions(+), 13 deletions(-) diff --git a/src/cache.h b/src/cache.h index 8dfaf8b6..21d42225 100644 --- a/src/cache.h +++ b/src/cache.h @@ -167,6 +167,17 @@ typedef enum cache_writeback_status { * request is satisfied; callers that do not expect concurrent writers to * their pages should treat this as a bug in their own locking. */ CACHE_WRITEBACK_REDIRTIED, + /* + * The write FAILED: these contents did not reach the device, and the request + * will never be satisfied without a successful retry. Polling again does + * not help -- the cache retries failed writes on its own schedule, so a + * caller that wants to keep waiting must be prepared to wait indefinitely; + * one that needs durability now must propagate the failure. + * + * For an extent request this outranks REDIRTIED but not PENDING, so by the + * time it is reported no write on the extent is still in flight. + */ + CACHE_WRITEBACK_FAILED, } cache_writeback_status; typedef platform_status (*page_writeback_fn)(cache *cc, @@ -527,6 +538,10 @@ cache_unpin(cache *cc, page_handle *page) * Returns STATUS_BUSY if the page is dirty but not writeback-able (locked or * claimed). Callers must treat that as a failure to make the page durable: no * write was issued and *req cannot report one. + * + * Returns STATUS_IO_ERROR if an earlier write of this page failed and no retry + * has yet succeeded. Unlike STATUS_BUSY this is not transient: it persists + * until the cache retries the write successfully. *----------------------------------------------------------------------------- */ static inline platform_status @@ -552,9 +567,10 @@ cache_writeback_page(cache *cc, * may be NULL. * * Pages of the extent that are clean or not resident need no write and are - * skipped. Returns STATUS_BUSY if any page is dirty but not writeback-able; as - * with cache_writeback_page(), the caller must treat that as a failure, since - * that page's contents will not reach the device. + * skipped. Returns STATUS_BUSY if any page is dirty but not writeback-able, or + * STATUS_IO_ERROR if an earlier write of any page failed; as with + * cache_writeback_page(), the caller must treat either as a failure, since that + * page's contents will not reach the device. * * Concurrent callers on the same extent are safe: only one can win a page's * writeback, and the other's request names that same in-flight interval, so diff --git a/src/clockcache.c b/src/clockcache.c index 6d53cfd9..7600c057 100644 --- a/src/clockcache.c +++ b/src/clockcache.c @@ -141,6 +141,16 @@ clockcache_print(platform_log_handle *log_handle, clockcache *cc); #define CC_LOADING (1u << 4) // page is actively being read from disk #define CC_WRITELOCKED (1u << 5) // write lock is held #define CC_CLAIMED (1u << 6) // claim is held +/* + * A writeback of this page failed. The page stays dirty and stays in + * CC_WRITEBACK, which leaves it un-claimable (try_set_writeback needs a + * cleanable status), un-evictable (try_evict needs CC_CLEAN) and un-writable + * (get_write excludes pages in writeback) -- exactly the exclusions a retry + * needs. It also pins dirty_generation, which is what keeps an outstanding + * cache_writeback_request naming this page valid. Cleared only by a thread that + * claims the retry. + */ +#define CC_WRITEBACK_ERROR (1u << 7) /* Common status flag combinations */ // free entry @@ -268,6 +278,21 @@ clockcache_writeback_get_generation(clockcache *cc, uint32 entry_number) return __atomic_load_n(&entry->dirty_generation, __ATOMIC_RELAXED); } +/* + * Record a failed writeback. Deliberately leaves both dirty_generation and + * CC_WRITEBACK alone: the page must stay excluded from claiming, eviction and + * writing until a retry succeeds, and its generation must stay pinned so that + * an outstanding writeback receipt naming it stays valid. See + * CC_WRITEBACK_ERROR. + */ +static void +clockcache_dirty_fail_writeback(clockcache *cc, uint32 entry_number) +{ + debug_only uint32 was_error = + clockcache_set_flag(cc, entry_number, CC_WRITEBACK_ERROR); + debug_assert(!was_error, "writeback failed twice with no retry in between"); +} + static void clockcache_dirty_complete_writeback(clockcache *cc, uint32 entry_number) { @@ -735,8 +760,28 @@ clockcache_get_write(clockcache *cc, uint32 entry_number) * background threads. */ debug_assert(clockcache_get_ref(cc, entry_number, tid) >= 1); - // Wait for flushing to finish + /* + * Wait for flushing to finish. + * + * A page whose writeback failed stays in CC_WRITEBACK (see + * CC_WRITEBACK_ERROR), so this waits for a retry to succeed. Proceeding + * instead is not an option: granting the write lock would let + * dirty_generation advance, and an outstanding cache_writeback_request + * naming this page would then read as REDIRTIED -- i.e. satisfied -- for + * contents that never reached the device. Blocking here is what keeps that + * receipt honest, so the stall is deliberate rather than merely tolerated. + */ + bool32 logged_writeback_error = FALSE; while (clockcache_test_flag(cc, entry_number, CC_WRITEBACK)) { + if (!logged_writeback_error + && clockcache_test_flag(cc, entry_number, CC_WRITEBACK_ERROR)) + { + platform_error_log( + "clockcache_get_write: blocked on a failed writeback of addr %lu; " + "waiting for a retry to succeed\n", + clockcache_get_entry(cc, entry_number)->page.disk_addr); + logged_writeback_error = TRUE; + } clockcache_wait(cc); } @@ -787,7 +832,12 @@ clockcache_try_get_write(clockcache *cc, uint32 entry_number) debug_assert(!was_writing); debug_assert(!clockcache_test_flag(cc, entry_number, CC_LOADING)); - // if flushing, then bail + /* + * If flushing, then bail. This also covers a page whose writeback failed and + * is awaiting a retry, since such a page stays in CC_WRITEBACK; the caller + * will keep retrying until a writeback retry succeeds. See + * clockcache_get_write() for why we must not grant the lock instead. + */ if (clockcache_test_flag(cc, entry_number, CC_WRITEBACK)) { rc = GET_RC_FLUSHING; goto failed; @@ -934,7 +984,6 @@ clockcache_write_callback(void *wbs) platform_error_log("clockcache_write_callback: async write failed: %s\n", platform_status_to_string(rc)); } - platform_assert_status_ok(rc); const struct iovec *iovec; uint64 count; @@ -961,7 +1010,15 @@ clockcache_write_callback(void *wbs) entry_number, addr); - clockcache_dirty_complete_writeback(cc, entry_number); + if (SUCCESS(rc)) { + clockcache_dirty_complete_writeback(cc, entry_number); + } else { + /* + * One status covers the whole request, so a failure fails every + * page in it. Some may in fact have landed; retrying is harmless. + */ + clockcache_dirty_fail_writeback(cc, entry_number); + } } io_async_state_deinit(state->iostate); @@ -1172,6 +1229,7 @@ clockcache_writeback_dirty(clockcache *cc) } // Wait for each pre-cut interval's writeback to complete, in a single pass. + platform_status result = STATUS_OK; for (uint32 entry_no = 0; entry_no < cc->cfg->page_capacity; entry_no++) { clockcache_entry *entry = clockcache_get_entry(cc, entry_no); while (TRUE) { @@ -1182,11 +1240,24 @@ clockcache_writeback_dirty(clockcache *cc) { break; } + if (clockcache_test_flag(cc, entry_no, CC_WRITEBACK_ERROR)) { + /* + * This page's write failed and it stays in CC_WRITEBACK until a + * retry succeeds, so waiting on it here would never return. Record + * the failure and keep draining the remaining entries, so that when + * we do return, no pre-cut write is still outstanding. + */ + platform_error_log("clockcache_writeback_dirty: writeback of addr " + "%lu failed\n", + entry->page.disk_addr); + result = STATUS_IO_ERROR; + break; + } clockcache_wait(cc); } } - return STATUS_OK; + return result; } /* @@ -2412,6 +2483,7 @@ typedef enum clockcache_writeback_claim { CC_WRITEBACK_NOT_NEEDED, // already clean: nothing to write CC_WRITEBACK_INFLIGHT, // someone else's write already covers it CC_WRITEBACK_UNAVAILABLE, // dirty, but locked or claimed + CC_WRITEBACK_FAILED, // an earlier write failed; awaiting a retry } clockcache_writeback_claim; static clockcache_writeback_claim @@ -2448,6 +2520,14 @@ clockcache_writeback_claim_page(clockcache *cc, *gen = 0; return CC_WRITEBACK_NOT_NEEDED; } + if (status & CC_WRITEBACK_ERROR) { + /* + * An earlier write of this page failed and no retry has succeeded yet. + * We cannot claim it, because CC_WRITEBACK is still set, and must + * not report it as in flight, because no write is coming. + */ + return CC_WRITEBACK_FAILED; + } if (status & CC_WRITEBACK) { /* * A pressure cleaner or a checkpoint fence got here first. Report that @@ -2514,6 +2594,8 @@ clockcache_writeback_page(clockcache *cc, return STATUS_OK; case CC_WRITEBACK_UNAVAILABLE: return STATUS_BUSY; + case CC_WRITEBACK_FAILED: + return STATUS_IO_ERROR; case CC_WRITEBACK_CLAIMED: break; } @@ -2624,7 +2706,12 @@ clockcache_writeback_extent(clockcache *cc, claim = clockcache_writeback_claim_page(cc, entry_number, &gen); } max_gen = MAX(max_gen, gen); - if (claim == CC_WRITEBACK_UNAVAILABLE) { + if (claim == CC_WRITEBACK_FAILED) { + // An earlier write of this page failed, so the extent is not fully + // written and will not be until a retry succeeds. Outranks BUSY, which + // is merely transient. + result = STATUS_IO_ERROR; + } else if (claim == CC_WRITEBACK_UNAVAILABLE && SUCCESS(result)) { // No write will be issued for this page, so the extent will not be // fully written. The caller has to hear about it. result = STATUS_BUSY; @@ -2738,8 +2825,12 @@ clockcache_writeback_get_page_status(clockcache *cc, uint64 addr, uint64 gen) return CACHE_WRITEBACK_COMPLETE; } - bool32 in_writeback = clockcache_test_flag(cc, entry_number, CC_WRITEBACK); - uint64 cur = clockcache_writeback_get_generation(cc, entry_number); + /* + * One snapshot, so that CC_WRITEBACK and CC_WRITEBACK_ERROR are read + * consistently with each other as well as with the generation below. + */ + entry_status status = clockcache_get_status(cc, entry_number); + uint64 cur = clockcache_writeback_get_generation(cc, entry_number); if (clockcache_get_entry(cc, entry_number)->page.disk_addr != addr) { // The entry was rebound under us, so it was evicted -- and hence written @@ -2754,13 +2845,20 @@ clockcache_writeback_get_page_status(clockcache *cc, uint64 addr, uint64 gen) // Drained, then dirtied again in a later window. return CACHE_WRITEBACK_REDIRTIED; } + if (status & CC_WRITEBACK_ERROR) { + // The write covering this interval failed. Reported ahead of PENDING: the + // page stays in CC_WRITEBACK until a retry succeeds, so a caller told + // PENDING here would poll forever. + return CACHE_WRITEBACK_FAILED; + } /* * cur <= gen: the page may still be in the interval we wrote. Note that * cur < gen is legitimate for an extent request, whose gen is the newest * interval among its pages -- a writeback_dirty() landing while the extent * was being filled leaves its pages spread over two windows. */ - return in_writeback ? CACHE_WRITEBACK_PENDING : CACHE_WRITEBACK_REDIRTIED; + return (status & CC_WRITEBACK) ? CACHE_WRITEBACK_PENDING + : CACHE_WRITEBACK_REDIRTIED; } cache_writeback_status @@ -2769,13 +2867,22 @@ clockcache_writeback_get_status(clockcache *cc, { uint64 num_pages = req->is_extent ? cc->cfg->pages_per_extent : 1; bool32 redirtied = FALSE; + bool32 failed = FALSE; for (uint64 i = 0; i < num_pages; i++) { uint64 addr = req->addr + clockcache_multiply_by_page_size(cc, i); switch (clockcache_writeback_get_page_status(cc, addr, req->gen)) { case CACHE_WRITEBACK_PENDING: - // One outstanding page is enough; no need to look at the rest. + /* + * One outstanding page is enough; no need to look at the rest. Note + * this outranks a failure found earlier in the extent, which + * is what makes FAILED safe to act on: by the time the caller sees + * it, no write on the extent is still in flight. + */ return CACHE_WRITEBACK_PENDING; + case CACHE_WRITEBACK_FAILED: + failed = TRUE; + break; case CACHE_WRITEBACK_REDIRTIED: // Keep going: a later page may still be pending, which the caller // has to keep waiting for. @@ -2785,6 +2892,9 @@ clockcache_writeback_get_status(clockcache *cc, break; } } + if (failed) { + return CACHE_WRITEBACK_FAILED; + } return redirtied ? CACHE_WRITEBACK_REDIRTIED : CACHE_WRITEBACK_COMPLETE; } From 0c54d7cdd0bf240795995d4d026a2bfbe76491c9 Mon Sep 17 00:00:00 2001 From: Rob Johnson Date: Fri, 31 Jul 2026 13:53:35 -0700 Subject: [PATCH 6/8] formatting Signed-off-by: Rob Johnson --- src/clockcache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clockcache.c b/src/clockcache.c index 7600c057..ad1e96b7 100644 --- a/src/clockcache.c +++ b/src/clockcache.c @@ -2830,7 +2830,7 @@ clockcache_writeback_get_page_status(clockcache *cc, uint64 addr, uint64 gen) * consistently with each other as well as with the generation below. */ entry_status status = clockcache_get_status(cc, entry_number); - uint64 cur = clockcache_writeback_get_generation(cc, entry_number); + uint64 cur = clockcache_writeback_get_generation(cc, entry_number); if (clockcache_get_entry(cc, entry_number)->page.disk_addr != addr) { // The entry was rebound under us, so it was evicted -- and hence written From 1ddcac3dc9ad3d46a48ad3684c2a840b4ddc8a44 Mon Sep 17 00:00:00 2001 From: Rob Johnson Date: Fri, 31 Jul 2026 14:18:39 -0700 Subject: [PATCH 7/8] more writeback error handling Signed-off-by: Rob Johnson --- src/cache.h | 13 +++--- src/clockcache.c | 100 +++++++++++++++++++++++++++++++++++------------ 2 files changed, 80 insertions(+), 33 deletions(-) diff --git a/src/cache.h b/src/cache.h index 21d42225..cdd133f7 100644 --- a/src/cache.h +++ b/src/cache.h @@ -539,9 +539,9 @@ cache_unpin(cache *cc, page_handle *page) * claimed). Callers must treat that as a failure to make the page durable: no * write was issued and *req cannot report one. * - * Returns STATUS_IO_ERROR if an earlier write of this page failed and no retry - * has yet succeeded. Unlike STATUS_BUSY this is not transient: it persists - * until the cache retries the write successfully. + * If an earlier write of this page failed, this claims the retry of it, so the + * returned request covers a fresh attempt rather than reporting the old + * failure. *----------------------------------------------------------------------------- */ static inline platform_status @@ -567,10 +567,9 @@ cache_writeback_page(cache *cc, * may be NULL. * * Pages of the extent that are clean or not resident need no write and are - * skipped. Returns STATUS_BUSY if any page is dirty but not writeback-able, or - * STATUS_IO_ERROR if an earlier write of any page failed; as with - * cache_writeback_page(), the caller must treat either as a failure, since that - * page's contents will not reach the device. + * skipped. Returns STATUS_BUSY if any page is dirty but not writeback-able; as + * with cache_writeback_page(), the caller must treat that as a failure, since + * that page's contents will not reach the device. * * Concurrent callers on the same extent are safe: only one can win a page's * writeback, and the other's request names that same in-flight interval, so diff --git a/src/clockcache.c b/src/clockcache.c index ad1e96b7..5bfef020 100644 --- a/src/clockcache.c +++ b/src/clockcache.c @@ -964,6 +964,55 @@ clockcache_try_set_writeback(clockcache *cc, } } +/* + *---------------------------------------------------------------------- + * clockcache_try_retry_writeback + * + * Claims the retry of a failed writeback, returning TRUE if we took it. + * Clearing CC_WRITEBACK_ERROR *is* the claim: only one thread can observe + * the bit set, so no exact-match compare-and-swap is needed and the other + * status bits are irrelevant. + * + * CC_WRITEBACK stays set throughout, so the page remains excluded from + * claiming, eviction and writing across the whole failure-and-retry + * window, and its dirty_generation stays pinned -- which is what keeps an + * outstanding cache_writeback_request naming it valid. The winner must + * issue the write, just as if it had won clockcache_try_set_writeback(). + *---------------------------------------------------------------------- + */ +static inline bool32 +clockcache_try_retry_writeback(clockcache *cc, uint32 entry_number) +{ + return clockcache_clear_flag(cc, entry_number, CC_WRITEBACK_ERROR) != 0; +} + +/* + *---------------------------------------------------------------------- + * clockcache_try_claim_writeback + * + * Takes ownership of writing a page back, either freshly (it is dirty and + * cleanable) or as a retry (an earlier write of it failed). Both outcomes + * leave the page in the same state -- CC_WRITEBACK set and owned by us -- + * so a caller that goes on to issue the I/O need not tell them apart. + * That is what lets clockcache_batch_start_writeback()'s extent coalescing + * treat a retried page and a freshly dirtied one as interchangeable, and + * keeps its backward/forward walk unchanged. + * + * A retry is deliberately not gated on with_access: that bit is a hint + * about eviction cost, whereas a failed write must be re-issued regardless + * in order to release whatever is blocked behind it. + *---------------------------------------------------------------------- + */ +static inline bool32 +clockcache_try_claim_writeback(clockcache *cc, + uint32 entry_number, + bool32 with_access) +{ + return (clockcache_ok_to_writeback(cc, entry_number, with_access) + && clockcache_try_set_writeback(cc, entry_number, with_access)) + || clockcache_try_retry_writeback(cc, entry_number); +} + typedef struct async_io_state { clockcache *cc; io_async_state_buffer iostate; @@ -1070,9 +1119,7 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool32 is_urgent) entry = &cc->entry[entry_no]; addr = entry->page.disk_addr; // test and test and set in the if condition - if (clockcache_ok_to_writeback(cc, entry_no, is_urgent) - && clockcache_try_set_writeback(cc, entry_no, is_urgent)) - { + if (clockcache_try_claim_writeback(cc, entry_no, is_urgent)) { debug_assert(clockcache_lookup(cc, addr) == entry_no); first_addr = entry->page.disk_addr; // walk backwards through extent to find first cleanable entry @@ -1083,10 +1130,9 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool32 is_urgent) next_entry_no = clockcache_lookup(cc, first_addr); else next_entry_no = CC_UNMAPPED_ENTRY; - } while ( - next_entry_no != CC_UNMAPPED_ENTRY - && clockcache_ok_to_writeback(cc, next_entry_no, is_urgent) - && clockcache_try_set_writeback(cc, next_entry_no, is_urgent)); + } while (next_entry_no != CC_UNMAPPED_ENTRY + && clockcache_try_claim_writeback( + cc, next_entry_no, is_urgent)); first_addr += page_size; end_addr = entry->page.disk_addr; // walk forwards through extent to find last cleanable entry @@ -1097,11 +1143,9 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool32 is_urgent) next_entry_no = clockcache_lookup(cc, end_addr); else next_entry_no = CC_UNMAPPED_ENTRY; - } while ( - next_entry_no != CC_UNMAPPED_ENTRY - && clockcache_ok_to_writeback(cc, next_entry_no, is_urgent) - && clockcache_try_set_writeback(cc, next_entry_no, is_urgent)); - + } while (next_entry_no != CC_UNMAPPED_ENTRY + && clockcache_try_claim_writeback( + cc, next_entry_no, is_urgent)); async_io_state *state; state = TYPED_MALLOC(PROCESS_PRIVATE_HEAP_ID, state); @@ -2483,7 +2527,6 @@ typedef enum clockcache_writeback_claim { CC_WRITEBACK_NOT_NEEDED, // already clean: nothing to write CC_WRITEBACK_INFLIGHT, // someone else's write already covers it CC_WRITEBACK_UNAVAILABLE, // dirty, but locked or claimed - CC_WRITEBACK_FAILED, // an earlier write failed; awaiting a retry } clockcache_writeback_claim; static clockcache_writeback_claim @@ -2500,7 +2543,7 @@ clockcache_writeback_claim_page(clockcache *cc, * write covers. */ *gen = clockcache_writeback_get_generation(cc, entry_number); - debug_assert(*gen != 0, "a cleanable page must have a dirty interval"); + debug_assert(*gen != 0, "a page claimed for writeback must be dirty"); return CC_WRITEBACK_CLAIMED; } @@ -2522,11 +2565,23 @@ clockcache_writeback_claim_page(clockcache *cc, } if (status & CC_WRITEBACK_ERROR) { /* - * An earlier write of this page failed and no retry has succeeded yet. - * We cannot claim it, because CC_WRITEBACK is still set, and must - * not report it as in flight, because no write is coming. + * An earlier write of this page failed. Our caller wants a write, + * so claim the retry and hand it back as an ordinary claim rather + * than reporting an error the caller could do nothing about. The + * generation is unchanged since the failed write, so the receipt + * still names the same interval. + * + * If another thread beat us to the retry, loop: it now holds + * CC_WRITEBACK without the error bit, so the next pass reports the + * page as in flight. */ - return CC_WRITEBACK_FAILED; + if (clockcache_try_retry_writeback(cc, entry_number)) { + *gen = clockcache_writeback_get_generation(cc, entry_number); + debug_assert(*gen != 0, + "a page claimed for writeback must be dirty"); + return CC_WRITEBACK_CLAIMED; + } + continue; } if (status & CC_WRITEBACK) { /* @@ -2594,8 +2649,6 @@ clockcache_writeback_page(clockcache *cc, return STATUS_OK; case CC_WRITEBACK_UNAVAILABLE: return STATUS_BUSY; - case CC_WRITEBACK_FAILED: - return STATUS_IO_ERROR; case CC_WRITEBACK_CLAIMED: break; } @@ -2706,12 +2759,7 @@ clockcache_writeback_extent(clockcache *cc, claim = clockcache_writeback_claim_page(cc, entry_number, &gen); } max_gen = MAX(max_gen, gen); - if (claim == CC_WRITEBACK_FAILED) { - // An earlier write of this page failed, so the extent is not fully - // written and will not be until a retry succeeds. Outranks BUSY, which - // is merely transient. - result = STATUS_IO_ERROR; - } else if (claim == CC_WRITEBACK_UNAVAILABLE && SUCCESS(result)) { + if (claim == CC_WRITEBACK_UNAVAILABLE) { // No write will be issued for this page, so the extent will not be // fully written. The caller has to hear about it. result = STATUS_BUSY; From 19face6e177d54b075fc99a9e893004e3a1c0d5b Mon Sep 17 00:00:00 2001 From: Rob Johnson Date: Fri, 31 Jul 2026 14:18:46 -0700 Subject: [PATCH 8/8] formatting Signed-off-by: Rob Johnson --- src/clockcache.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/clockcache.c b/src/clockcache.c index 5bfef020..36645c85 100644 --- a/src/clockcache.c +++ b/src/clockcache.c @@ -1130,9 +1130,9 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool32 is_urgent) next_entry_no = clockcache_lookup(cc, first_addr); else next_entry_no = CC_UNMAPPED_ENTRY; - } while (next_entry_no != CC_UNMAPPED_ENTRY - && clockcache_try_claim_writeback( - cc, next_entry_no, is_urgent)); + } while ( + next_entry_no != CC_UNMAPPED_ENTRY + && clockcache_try_claim_writeback(cc, next_entry_no, is_urgent)); first_addr += page_size; end_addr = entry->page.disk_addr; // walk forwards through extent to find last cleanable entry @@ -1143,9 +1143,9 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool32 is_urgent) next_entry_no = clockcache_lookup(cc, end_addr); else next_entry_no = CC_UNMAPPED_ENTRY; - } while (next_entry_no != CC_UNMAPPED_ENTRY - && clockcache_try_claim_writeback( - cc, next_entry_no, is_urgent)); + } while ( + next_entry_no != CC_UNMAPPED_ENTRY + && clockcache_try_claim_writeback(cc, next_entry_no, is_urgent)); async_io_state *state; state = TYPED_MALLOC(PROCESS_PRIVATE_HEAP_ID, state);