From: "Denis V. Lunev" <[email protected]> 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]> Message-ID: <[email protected]> Reviewed-by: Kevin Wolf <[email protected]> Signed-off-by: Kevin Wolf <[email protected]> (cherry picked from commit 16f94ef4c6ce3e7ca6b02b796580e2cfc41e9789) Signed-off-by: Michael Tokarev <[email protected]> diff --git a/block/accounting.c b/block/accounting.c index c5584abb76e..79e2a37e8a7 100644 --- a/block/accounting.c +++ b/block/accounting.c @@ -306,10 +306,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 2c50a6bf3b8..33cb1b18b77 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -498,6 +498,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]; @@ -587,6 +589,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 a59e39f49d9..6982add4d57 100644 --- a/include/block/accounting.h +++ b/include/block/accounting.h @@ -115,6 +115,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.47.3
