Skip to content

Commit 691b052

Browse files
waychaxboe
authored andcommitted
block: fix race in blk_time_get_ns() returning 0
blk_time_get_ns() populates the per-plug cached timestamp and then returns it by re-reading the field: if (!plug->cur_ktime) { plug->cur_ktime = ktime_get_ns(); current->flags |= PF_BLOCK_TS; } return plug->cur_ktime; This is problematic when the compiler emits the final "return plug->cur_ktime" as a reload from memory, after PF_BLOCK_TS has already been set. Since the cached timestamp is now invalidated from finish_task_switch() (fad156c "block: invalidate cached plug timestamp after task switch"), a task preempted between setting PF_BLOCK_TS and that reload has plug->cur_ktime zeroed by blk_plug_invalidate_ts() when it is scheduled back in. The reload then returns 0. A 0 handed back here is stored as a start timestamp -- e.g. blk_account_io_start() writes it to rq->start_time_ns -- and later subtracted from "now". blk_account_io_done() then adds (now - 0), i.e. roughly the system uptime, to the per-group nsecs[] counters. On an otherwise idle, healthy device this appears as sudden ~uptime-sized jumps in the diskstats time fields (write_ticks/discard_ticks/time_in_queue). The solution is to be explicit in our reads and writes to this field that is preemption volatile. We also add a barrier() to ensure that any setting of PF_BLOCK_TS is ordered to happen after the cur_ktime update. This issue was discovered using AI-assisted kprobes looking for paths that were leaking zeroed timestamps in a live system, based on the observation that we were sometimes seeing uptime-sized jumps in kernel exported counters. This was flagged by NodeDiskIOSaturation prometheus alerts that started firing on all hosts post 7.1.3 kernel upgrade, due to node-exporter now exporting a nonsensical node_disk_io_time_weighted_seconds_total. Fixes: fad156c ("block: invalidate cached plug timestamp after task switch") Cc: stable@vger.kernel.org Signed-off-by: Mike Waychison <mike@waychison.com> Assisted-by: Claude:claude-opus-4.8 Link: https://patch.msgid.link/20260715192950.2488921-1-mike@waychison.com Signed-off-by: Jens Axboe <axboe@kernel.dk>
1 parent 181bb9c commit 691b052

1 file changed

Lines changed: 10 additions & 3 deletions

File tree

block/blk.h

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -717,6 +717,7 @@ static inline int req_ref_read(struct request *req)
717717
static inline u64 blk_time_get_ns(void)
718718
{
719719
struct blk_plug *plug = current->plug;
720+
u64 now;
720721

721722
if (!plug || !in_task())
722723
return ktime_get_ns();
@@ -725,12 +726,18 @@ static inline u64 blk_time_get_ns(void)
725726
* 0 could very well be a valid time, but rather than flag "this is
726727
* a valid timestamp" separately, just accept that we'll do an extra
727728
* ktime_get_ns() if we just happen to get 0 as the current time.
729+
*
730+
* cur_ktime can be zeroed by pre-emption the moment PF_BLOCK_TS is set.
728731
*/
729-
if (!plug->cur_ktime) {
730-
plug->cur_ktime = ktime_get_ns();
732+
now = READ_ONCE(plug->cur_ktime);
733+
if (!now) {
734+
now = ktime_get_ns();
735+
WRITE_ONCE(plug->cur_ktime, now);
736+
/* Ensure PF_BLOCK_TS is set after cur_ktime. */
737+
barrier();
731738
current->flags |= PF_BLOCK_TS;
732739
}
733-
return plug->cur_ktime;
740+
return now;
734741
}
735742

736743
static inline ktime_t blk_time_get(void)

0 commit comments

Comments
 (0)