bdrv_query_blk_stats() reads BlockAcctStats's counters, latency histogram, and per-interval TimedAverage stats without stats->lock, while block_account_one_io() updates the same fields under that lock from an iothread. timed_average_min()/max()/avg() make this worse than a stale read: they call check_expirations(), which can reset a window's sum/count/min/max -- a write, not just a read -- so this is a genuine race with a concurrent writer, not merely a slower reader like the scalar counters.
Take stats->lock for the whole call, both to close the race and to make the returned snapshot internally consistent (previously each field could reflect a different instant relative to concurrent updates). block_acct_queue_depth() used to take the lock itself on every call; since bdrv_query_blk_stats() is its only caller and now already holds the lock, that would self-deadlock. Make it require the caller to hold stats->lock instead (documented and asserted). Signed-off-by: Denis V. Lunev <[email protected]> CC: Kevin Wolf <[email protected]> CC: Hanna Reitz <[email protected]> CC: Vladimir Sementsov-Ogievskiy <[email protected]> CC: Andrey Drobyshev <[email protected]> --- block/accounting.c | 3 +-- block/qapi.c | 3 +++ include/block/accounting.h | 1 + 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/block/accounting.c b/block/accounting.c index 6e06c7609e..038af37017 100644 --- a/block/accounting.c +++ b/block/accounting.c @@ -318,10 +318,9 @@ double block_acct_queue_depth(BlockAcctTimedStats *stats, uint64_t sum, elapsed; assert(type < BLOCK_MAX_IOTYPE); + assert(qemu_mutex_trylock(&stats->stats->lock) == -EBUSY); - qemu_mutex_lock(&stats->stats->lock); sum = timed_average_sum(&stats->latency[type], &elapsed); - qemu_mutex_unlock(&stats->stats->lock); return (double) sum / elapsed; } diff --git a/block/qapi.c b/block/qapi.c index eabfbfc258..1dfac51091 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -535,6 +535,8 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk) BlockAcctTimedStats *ts = NULL; BlockLatencyHistogram *hgram; + qemu_mutex_lock(&stats->lock); + ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ]; ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE]; ds->zone_append_bytes = stats->nr_bytes[BLOCK_ACCT_ZONE_APPEND]; @@ -624,6 +626,7 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk) = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_ZONE_APPEND]); ds->flush_latency_histogram = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_FLUSH]); + qemu_mutex_unlock(&stats->lock); } static BlockStats * GRAPH_RDLOCK diff --git a/include/block/accounting.h b/include/block/accounting.h index b1cf417b57..12d3246092 100644 --- a/include/block/accounting.h +++ b/include/block/accounting.h @@ -116,6 +116,7 @@ void block_acct_invalid(BlockAcctStats *stats, enum BlockAcctType type); void block_acct_merge_done(BlockAcctStats *stats, enum BlockAcctType type, int num_requests); int64_t block_acct_idle_time_ns(BlockAcctStats *stats); +/* Caller must hold stats->stats->lock. */ double block_acct_queue_depth(BlockAcctTimedStats *stats, enum BlockAcctType type); int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type, -- 2.53.0
