[PULL 03/17] block/qapi: take stats->lock when reading BlockAcctStats for query-blockstats

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]>
---
 include/block/accounting.h | 1 +
 block/accounting.c         | 3 +--
 block/qapi.c               | 3 +++
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/block/accounting.h b/include/block/accounting.h
index b1cf417b572..12d32460927 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,
diff --git a/block/accounting.c b/block/accounting.c
index 6e06c7609eb..038af370170 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 eabfbfc2585..1dfac510918 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
-- 
2.55.0