Re: [PATCH v2 5/9] block/write-threshold: don't use aio context lock

[Max Reitz]

On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:

Instead of relying on aio context lock, let's make use of atomic
operations.

The tricky place is bdrv_write_threshold_check_write(): we want
atomically unset bs->write_threshold_offset iff
   offset + bytes > bs->write_threshold_offset
We don't have such atomic operation, so let's go in a loop:


This could also be solved by untangling the overloaded meaning of 
write_threshold_offset – if we had an additional boolean 
write_threshold_check, then this wouldn’t be a problem, and we could do 
this:


if (end > bdrv_write_threshold_get(bs)) {
if (qatomic_xchg(>write_threshold_check, false) == true) {
qapi_event_send(...);
}
}

However, the problem then becomes thinking about the memory access 
semantics, because if some other thread sets the threshold offset and 
enables the threshold check, we need to ensure that we see the updated 
offset here...


So I suppose your loop is simpler then.


1. fetch wtr atomically
2. if condition satisfied, try cmpxchg (if not satisfied, we are done,
don't send event)
3. if cmpxchg succeeded, we are done (send event), else go to [1]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/write-threshold.c | 32 +---
  1 file changed, 17 insertions(+), 15 deletions(-)


Reviewed-by: Max Reitz 

[PATCH v2 5/9] block/write-threshold: don't use aio context lock

[Vladimir Sementsov-Ogievskiy]

Instead of relying on aio context lock, let's make use of atomic
operations.

The tricky place is bdrv_write_threshold_check_write(): we want
atomically unset bs->write_threshold_offset iff
  offset + bytes > bs->write_threshold_offset
We don't have such atomic operation, so let's go in a loop:

1. fetch wtr atomically
2. if condition satisfied, try cmpxchg (if not satisfied, we are done,
   don't send event)
3. if cmpxchg succeeded, we are done (send event), else go to [1]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/write-threshold.c | 32 +---
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/block/write-threshold.c b/block/write-threshold.c
index ae54ee05dc..fbf4e6f5c4 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -2,6 +2,7 @@
  * QEMU System Emulator block write threshold notification
  *
  * Copyright Red Hat, Inc. 2014
+ * Copyright (c) 2021 Virtuozzo International GmbH.
  *
  * Authors:
  *  Francesco Romani 
@@ -21,43 +22,44 @@
 
 uint64_t bdrv_write_threshold_get(const BlockDriverState *bs)
 {
-return bs->write_threshold_offset;
+return qatomic_read(>write_threshold_offset);
 }
 
 void bdrv_write_threshold_set(BlockDriverState *bs, uint64_t threshold_bytes)
 {
-bs->write_threshold_offset = threshold_bytes;
+qatomic_set(>write_threshold_offset, threshold_bytes);
 }
 
 void qmp_block_set_write_threshold(const char *node_name,
uint64_t threshold_bytes,
Error **errp)
 {
-BlockDriverState *bs;
-AioContext *aio_context;
-
-bs = bdrv_find_node(node_name);
+BlockDriverState *bs = bdrv_find_node(node_name);
 if (!bs) {
 error_setg(errp, "Device '%s' not found", node_name);
 return;
 }
 
-aio_context = bdrv_get_aio_context(bs);
-aio_context_acquire(aio_context);
-
 bdrv_write_threshold_set(bs, threshold_bytes);
-
-aio_context_release(aio_context);
 }
 
 void bdrv_write_threshold_check_write(BlockDriverState *bs, int64_t offset,
   int64_t bytes)
 {
 int64_t end = offset + bytes;
-uint64_t wtr = bs->write_threshold_offset;
+uint64_t wtr;
 
-if (wtr > 0 && end > wtr) {
-qapi_event_send_block_write_threshold(bs->node_name, end - wtr, wtr);
-bdrv_write_threshold_set(bs, 0);
+retry:
+wtr = bdrv_write_threshold_get(bs);
+if (wtr == 0 || wtr >= end) {
+return;
 }
+
+if (qatomic_cmpxchg(>write_threshold_offset, wtr, 0) != wtr) {
+/* bs->write_threshold_offset changed in parallel */
+goto retry;
+}
+
+/* We have cleared bs->write_threshold_offset, so let's send event */
+qapi_event_send_block_write_threshold(bs->node_name, end - wtr, wtr);
 }
-- 
2.29.2