Re: [PATCH RFC v4] bcache: flush backing device before cleaning the writeback dirty keys

["Zhou Jifeng" <zhoujifeng@kylinos.com.cn>]

>> 2026年7月9日 14:23，Zhou Jifeng <zhoujifeng@kylinos.com.cn> 写道：
>>
>> From: Zhou Jifeng <zhoujifeng@kylinsec.com.cn>
>>
>> Currently, when writeback dirty data completes, write_dirty_finish()
>> immediately inserts the clean key into the btree.  If a power failure
>> occurs after the clean-key insert but before the backing device has
>> flushed its volatile write cache, the btree contains a clean key that
>> points at backing data that never reached stable media.  On the next
>> read, bcache serves stale data from the backing device.
>>
>> Fix this by deferring the clean-key btree insert until after an
>> explicit backing-device flush (REQ_PREFLUSH).  Written keys are first
>> parked in a new rbtree (writeback_flush_pending) keyed by START_KEY.
>> After enough read_dirty() passes, a full btree scan, or a wrap-around
>> in refill_dirty(), writeback_finish_batch() issues a synchronous
>> backing-device flush, and only on success inserts the clean keys into
>> the btree and removes them from the pending tree.  On flush failure
>> the entries are re-inserted into the current tree for a later retry.
>>
>> Key changes:
>>
>> - Introduce struct writeback_pending (separate kmalloc'd allocation,
>>  decoupled from the dirty_io closure so the closure can be freed
>>  immediately and the pending entry lives until the flush).
>>
>> - Add __writeback_pending_insert() — inserts into the START_KEY-
>>  ordered rbtree with overlap detection.  The rbtree is also used as
>>  an interval tree by bch_writeback_drop_pending() for front-end
>>  write overlap queries; that search is correct only because dirty
>>  btree keys are pairwise disjoint, an invariant enforced at insert
>>  time.
>>
>> - writeback_add_to_pending() parks a key after successful writeback
>>  IO.  Uses GFP_NOIO because the caller runs on the WQ_MEM_RECLAIM
>>  writeback workqueue.  kmalloc failure silently drops the key (it
>>  stays dirty and is re-selected by the next scan).
>>
>> - writeback_finish_batch() detaches the entire pending tree into a
>>  local list under spinlock, issues writeback_flush(), then either
>>  cleans the keys (on success) or re-inserts them (on failure).
>>
>> - writeback_flush() open-codes blkdev_issue_flush() to call
>>  bch_count_backing_io_errors() on failure.
>>
>> - bch_writeback_drop_pending() drops overlapping entries from the
>>  pending tree.  Called from cached_dev_write() alongside the
>>  existing bch_keybuf_check_overlapping(); overlapping front-end
>>  writes now force writeback instead of bypassing the cache.
>>
>> - refill_dirty() now returns enum writeback_scan_result (PARTIAL,
>>  PARTIAL_WRAP, FULL_SEARCHED) instead of a boolean.  Before
>>  wrapping around for a second scan pass, it flushes the pending
>>  tree so the wrap scan won't re-select keys that are still
>>  KEY_DIRTY in the btree but already written to backing and
>>  awaiting flush.  writeback_lock may be transiently released
>>  mid-function during this pre-wrap flush.
>>
>> - refill_full_stripes() no longer wraps internally; it returns
>>  at the end of the stripe range and delegates the wrap to
>>  refill_dirty().  buf->last_scanned is no longer rewound past
>>  the current position to avoid re-selecting keys already in
>>  writeback_flush_pending.
>>
>> - read_dirty() gates deferred flush-and-clean on PASS_PER_FLUSH
>>  (default 5) passes, a full btree scan, or a PARTIAL_WRAP.  It
>>  accepts the scan_result from refill_dirty() as a parameter.
>>
>> - write_dirty_finish() is simplified: it no longer inserts clean
>>  keys inline; it only calls writeback_add_to_pending().
>>
>> - BDEV_STATE_CLEAN eligibility in the main loop now also requires
>>  writeback_pending_empty(), so a device is not marked clean while
>>  entries are still in the pending tree.
>>
>> - Thread exit: the workqueue is destroyed first, then a final
>>  writeback_finish_batch() is attempted (skipped when the cache
>>  set is disabled or the device is detaching), then any remaining
>>  pending entries are drained.  The exit path intentionally does
>>  NOT set BDEV_STATE_CLEAN because it lacks the WB_SCAN_FULL_SEARCHED
>>  gate that the in-loop check requires; a kthread_stop mid-scan
>>  (e.g. from bch_cached_dev_attach's error path) must not mark a
>>  still-dirty device clean.
>>
>> Signed-off-by: Coly Li <colyli@fygo.io>
>> Signed-off-by: Zhou Jifeng <zhoujifeng@kylinsec.com.cn>

>
> You cannot add my SOB for me. I know you mean the base idea of this version was
> from me, but it is fine to only do your own SOB. I am good with this, since you already
> do a lot of analysis and testing.
>
> The v4 version format is much more easier to review.
> I will try my best to response you soon.
>
> Thanks for the fix up and testing.
>
> Coly Li


Hi Coly,​
I really appreciate your time and guidance on this patch.

Best regards,
Zhou Jifeng