* [PATCH v3 1/8] block/write-threshold: don't use write notifiers
2021-05-06 9:06 [PATCH v3 0/8] block: refactor write threshold Vladimir Sementsov-Ogievskiy
@ 2021-05-06 9:06 ` Vladimir Sementsov-Ogievskiy
2021-05-07 14:15 ` Eric Blake
2021-05-06 9:06 ` [PATCH v3 2/8] block: drop " Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
9 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-06 9:06 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, fam, stefanha, mreitz, kwolf, vsementsov, eesposit, pbonzini
write-notifiers are used only for write-threshold. New code for such
purpose should create filters.
Let's better special-case write-threshold and drop write notifiers at
all. (Actually, write-threshold is special-cased anyway, as the only
user of write-notifiers)
So, create a new direct interface for bdrv_co_write_req_prepare() and
drop all write-notifier related logic from write-threshold.c.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/block/block_int.h | 1 -
include/block/write-threshold.h | 9 +++++
block/io.c | 5 ++-
block/write-threshold.c | 70 +++++++--------------------------
4 files changed, 27 insertions(+), 58 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c823f5b1b3..51f51286a5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -959,7 +959,6 @@ struct BlockDriverState {
/* threshold limit for writes, in bytes. "High water mark". */
uint64_t write_threshold_offset;
- NotifierWithReturn write_threshold_notifier;
/* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
* Reading from the list can be done with either the BQL or the
diff --git a/include/block/write-threshold.h b/include/block/write-threshold.h
index c646f267a4..555afd0de6 100644
--- a/include/block/write-threshold.h
+++ b/include/block/write-threshold.h
@@ -59,4 +59,13 @@ bool bdrv_write_threshold_is_set(const BlockDriverState *bs);
uint64_t bdrv_write_threshold_exceeded(const BlockDriverState *bs,
const BdrvTrackedRequest *req);
+/*
+ * bdrv_write_threshold_check_write
+ *
+ * Check whether the specified request exceeds the write threshold.
+ * If it is, send corresponding event and disable write threshold checking.
+ */
+void bdrv_write_threshold_check_write(BlockDriverState *bs, int64_t offset,
+ int64_t bytes);
+
#endif
diff --git a/block/io.c b/block/io.c
index 35b6c56efc..3520de51bb 100644
--- a/block/io.c
+++ b/block/io.c
@@ -30,6 +30,7 @@
#include "block/blockjob_int.h"
#include "block/block_int.h"
#include "block/coroutines.h"
+#include "block/write-threshold.h"
#include "qemu/cutils.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
@@ -2008,8 +2009,8 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,
} else {
assert(child->perm & BLK_PERM_WRITE);
}
- return notifier_with_return_list_notify(&bs->before_write_notifiers,
- req);
+ bdrv_write_threshold_check_write(bs, offset, bytes);
+ return 0;
case BDRV_TRACKED_TRUNCATE:
assert(child->perm & BLK_PERM_RESIZE);
return 0;
diff --git a/block/write-threshold.c b/block/write-threshold.c
index 85b78dc2a9..71df3c434f 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -29,14 +29,6 @@ bool bdrv_write_threshold_is_set(const BlockDriverState *bs)
return bs->write_threshold_offset > 0;
}
-static void write_threshold_disable(BlockDriverState *bs)
-{
- if (bdrv_write_threshold_is_set(bs)) {
- notifier_with_return_remove(&bs->write_threshold_notifier);
- bs->write_threshold_offset = 0;
- }
-}
-
uint64_t bdrv_write_threshold_exceeded(const BlockDriverState *bs,
const BdrvTrackedRequest *req)
{
@@ -51,55 +43,9 @@ uint64_t bdrv_write_threshold_exceeded(const BlockDriverState *bs,
return 0;
}
-static int coroutine_fn before_write_notify(NotifierWithReturn *notifier,
- void *opaque)
-{
- BdrvTrackedRequest *req = opaque;
- BlockDriverState *bs = req->bs;
- uint64_t amount = 0;
-
- amount = bdrv_write_threshold_exceeded(bs, req);
- if (amount > 0) {
- qapi_event_send_block_write_threshold(
- bs->node_name,
- amount,
- bs->write_threshold_offset);
-
- /* autodisable to avoid flooding the monitor */
- write_threshold_disable(bs);
- }
-
- return 0; /* should always let other notifiers run */
-}
-
-static void write_threshold_register_notifier(BlockDriverState *bs)
-{
- bs->write_threshold_notifier.notify = before_write_notify;
- bdrv_add_before_write_notifier(bs, &bs->write_threshold_notifier);
-}
-
-static void write_threshold_update(BlockDriverState *bs,
- int64_t threshold_bytes)
-{
- bs->write_threshold_offset = threshold_bytes;
-}
-
void bdrv_write_threshold_set(BlockDriverState *bs, uint64_t threshold_bytes)
{
- if (bdrv_write_threshold_is_set(bs)) {
- if (threshold_bytes > 0) {
- write_threshold_update(bs, threshold_bytes);
- } else {
- write_threshold_disable(bs);
- }
- } else {
- if (threshold_bytes > 0) {
- /* avoid multiple registration */
- write_threshold_register_notifier(bs);
- write_threshold_update(bs, threshold_bytes);
- }
- /* discard bogus disable request */
- }
+ bs->write_threshold_offset = threshold_bytes;
}
void qmp_block_set_write_threshold(const char *node_name,
@@ -122,3 +68,17 @@ void qmp_block_set_write_threshold(const char *node_name,
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;
+
+ if (wtr > 0 && end > wtr) {
+ qapi_event_send_block_write_threshold(bs->node_name, end - wtr, wtr);
+
+ /* autodisable to avoid flooding the monitor */
+ bdrv_write_threshold_set(bs, 0);
+ }
+}
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/8] block/write-threshold: don't use write notifiers
2021-05-06 9:06 ` [PATCH v3 1/8] block/write-threshold: don't use write notifiers Vladimir Sementsov-Ogievskiy
@ 2021-05-07 14:15 ` Eric Blake
0 siblings, 0 replies; 15+ messages in thread
From: Eric Blake @ 2021-05-07 14:15 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, eesposit, qemu-devel, mreitz, stefanha, pbonzini
On 5/6/21 4:06 AM, Vladimir Sementsov-Ogievskiy wrote:
> write-notifiers are used only for write-threshold. New code for such
> purpose should create filters.
>
> Let's better special-case write-threshold and drop write notifiers at
> all. (Actually, write-threshold is special-cased anyway, as the only
> user of write-notifiers)
As write-threshold is already partially special-cased, let's finish that
special-casing and drop write notifiers altogether.
>
> So, create a new direct interface for bdrv_co_write_req_prepare() and
> drop all write-notifier related logic from write-threshold.c.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>
> +/*
> + * bdrv_write_threshold_check_write
> + *
> + * Check whether the specified request exceeds the write threshold.
> + * If it is, send corresponding event and disable write threshold checking.
s/it is, send/so, send a/
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 2/8] block: drop write notifiers
2021-05-06 9:06 [PATCH v3 0/8] block: refactor write threshold Vladimir Sementsov-Ogievskiy
2021-05-06 9:06 ` [PATCH v3 1/8] block/write-threshold: don't use write notifiers Vladimir Sementsov-Ogievskiy
@ 2021-05-06 9:06 ` Vladimir Sementsov-Ogievskiy
2021-05-06 9:06 ` [PATCH v3 3/8] test-write-threshold: rewrite test_threshold_(not_)trigger tests Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-06 9:06 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, fam, stefanha, mreitz, kwolf, vsementsov, eesposit, pbonzini
They are unused now.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/block/block_int.h | 12 ------------
block.c | 1 -
block/io.c | 6 ------
3 files changed, 19 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 51f51286a5..eab352f363 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -954,9 +954,6 @@ struct BlockDriverState {
*/
int64_t total_sectors;
- /* Callback before write request is processed */
- NotifierWithReturnList before_write_notifiers;
-
/* threshold limit for writes, in bytes. "High water mark". */
uint64_t write_threshold_offset;
@@ -1083,15 +1080,6 @@ void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
bool bdrv_backing_overridden(BlockDriverState *bs);
-/**
- * bdrv_add_before_write_notifier:
- *
- * Register a callback that is invoked before write requests are processed but
- * after any throttling or waiting for overlapping requests.
- */
-void bdrv_add_before_write_notifier(BlockDriverState *bs,
- NotifierWithReturn *notifier);
-
/**
* bdrv_add_aio_context_notifier:
*
diff --git a/block.c b/block.c
index 9ad725d205..75a82af641 100644
--- a/block.c
+++ b/block.c
@@ -400,7 +400,6 @@ BlockDriverState *bdrv_new(void)
for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
QLIST_INIT(&bs->op_blockers[i]);
}
- notifier_with_return_list_init(&bs->before_write_notifiers);
qemu_co_mutex_init(&bs->reqs_lock);
qemu_mutex_init(&bs->dirty_bitmap_mutex);
bs->refcnt = 1;
diff --git a/block/io.c b/block/io.c
index 3520de51bb..1e826ba9e8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3165,12 +3165,6 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
return true;
}
-void bdrv_add_before_write_notifier(BlockDriverState *bs,
- NotifierWithReturn *notifier)
-{
- notifier_with_return_list_add(&bs->before_write_notifiers, notifier);
-}
-
void bdrv_io_plug(BlockDriverState *bs)
{
BdrvChild *child;
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 3/8] test-write-threshold: rewrite test_threshold_(not_)trigger tests
2021-05-06 9:06 [PATCH v3 0/8] block: refactor write threshold Vladimir Sementsov-Ogievskiy
2021-05-06 9:06 ` [PATCH v3 1/8] block/write-threshold: don't use write notifiers Vladimir Sementsov-Ogievskiy
2021-05-06 9:06 ` [PATCH v3 2/8] block: drop " Vladimir Sementsov-Ogievskiy
@ 2021-05-06 9:06 ` Vladimir Sementsov-Ogievskiy
2021-05-06 9:06 ` [PATCH v3 4/8] block/write-threshold: drop extra APIs Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-06 9:06 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, fam, stefanha, mreitz, kwolf, vsementsov, eesposit, pbonzini
These tests use bdrv_write_threshold_exceeded() API, which is used only
for test (since pre-previous commit). Better is testing real API, which
is used in block.c as well.
So, let's call bdrv_write_threshold_check_write(), and check is
bs->write_threshold_offset cleared or not (it's cleared iff threshold
triggered).
Also we get rid of BdrvTrackedRequest use here. Note, that paranoiac
bdrv_check_request() calls were added in 8b1170012b1 to protect
BdrvTrackedRequest. Drop them now.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/unit/test-write-threshold.c | 22 ++++------------------
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index fc1c45a2eb..fd40a815b8 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -55,41 +55,27 @@ static void test_threshold_multi_set_get(void)
static void test_threshold_not_trigger(void)
{
- uint64_t amount = 0;
uint64_t threshold = 4 * 1024 * 1024;
BlockDriverState bs;
- BdrvTrackedRequest req;
memset(&bs, 0, sizeof(bs));
- memset(&req, 0, sizeof(req));
- req.offset = 1024;
- req.bytes = 1024;
-
- bdrv_check_request(req.offset, req.bytes, &error_abort);
bdrv_write_threshold_set(&bs, threshold);
- amount = bdrv_write_threshold_exceeded(&bs, &req);
- g_assert_cmpuint(amount, ==, 0);
+ bdrv_write_threshold_check_write(&bs, 1024, 1024);
+ g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, threshold);
}
static void test_threshold_trigger(void)
{
- uint64_t amount = 0;
uint64_t threshold = 4 * 1024 * 1024;
BlockDriverState bs;
- BdrvTrackedRequest req;
memset(&bs, 0, sizeof(bs));
- memset(&req, 0, sizeof(req));
- req.offset = (4 * 1024 * 1024) - 1024;
- req.bytes = 2 * 1024;
-
- bdrv_check_request(req.offset, req.bytes, &error_abort);
bdrv_write_threshold_set(&bs, threshold);
- amount = bdrv_write_threshold_exceeded(&bs, &req);
- g_assert_cmpuint(amount, >=, 1024);
+ bdrv_write_threshold_check_write(&bs, threshold - 1024, 2 * 1024);
+ g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, 0);
}
typedef struct TestStruct {
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 4/8] block/write-threshold: drop extra APIs
2021-05-06 9:06 [PATCH v3 0/8] block: refactor write threshold Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2021-05-06 9:06 ` [PATCH v3 3/8] test-write-threshold: rewrite test_threshold_(not_)trigger tests Vladimir Sementsov-Ogievskiy
@ 2021-05-06 9:06 ` Vladimir Sementsov-Ogievskiy
2021-05-07 14:18 ` Eric Blake
2021-05-06 9:06 ` [PATCH v3 5/8] block/write-threshold: don't use aio context lock Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
9 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-06 9:06 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, fam, stefanha, mreitz, kwolf, vsementsov, eesposit, pbonzini
bdrv_write_threshold_exceeded() is unused at all.
bdrv_write_threshold_is_set() is used only to double check the value of
bs->write_threshold_offset in tests. No real sense in it (both tests do
check real value with help of bdrv_write_threshold_get())
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/block/write-threshold.h | 24 ------------------------
block/write-threshold.c | 19 -------------------
tests/unit/test-write-threshold.c | 4 ----
3 files changed, 47 deletions(-)
diff --git a/include/block/write-threshold.h b/include/block/write-threshold.h
index 555afd0de6..c60b9954cd 100644
--- a/include/block/write-threshold.h
+++ b/include/block/write-threshold.h
@@ -35,30 +35,6 @@ void bdrv_write_threshold_set(BlockDriverState *bs, uint64_t threshold_bytes);
*/
uint64_t bdrv_write_threshold_get(const BlockDriverState *bs);
-/*
- * bdrv_write_threshold_is_set
- *
- * Tell if a write threshold is set for a given BDS.
- */
-bool bdrv_write_threshold_is_set(const BlockDriverState *bs);
-
-/*
- * bdrv_write_threshold_exceeded
- *
- * Return the extent of a write request that exceeded the threshold,
- * or zero if the request is below the threshold.
- * Return zero also if the threshold was not set.
- *
- * NOTE: here we assume the following holds for each request this code
- * deals with:
- *
- * assert((req->offset + req->bytes) <= UINT64_MAX)
- *
- * Please not there is *not* an actual C assert().
- */
-uint64_t bdrv_write_threshold_exceeded(const BlockDriverState *bs,
- const BdrvTrackedRequest *req);
-
/*
* bdrv_write_threshold_check_write
*
diff --git a/block/write-threshold.c b/block/write-threshold.c
index 71df3c434f..65a6acd142 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -24,25 +24,6 @@ uint64_t bdrv_write_threshold_get(const BlockDriverState *bs)
return bs->write_threshold_offset;
}
-bool bdrv_write_threshold_is_set(const BlockDriverState *bs)
-{
- return bs->write_threshold_offset > 0;
-}
-
-uint64_t bdrv_write_threshold_exceeded(const BlockDriverState *bs,
- const BdrvTrackedRequest *req)
-{
- if (bdrv_write_threshold_is_set(bs)) {
- if (req->offset > bs->write_threshold_offset) {
- return (req->offset - bs->write_threshold_offset) + req->bytes;
- }
- if ((req->offset + req->bytes) > bs->write_threshold_offset) {
- return (req->offset + req->bytes) - bs->write_threshold_offset;
- }
- }
- return 0;
-}
-
void bdrv_write_threshold_set(BlockDriverState *bs, uint64_t threshold_bytes)
{
bs->write_threshold_offset = threshold_bytes;
diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index fd40a815b8..bb5c1a5217 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -18,8 +18,6 @@ static void test_threshold_not_set_on_init(void)
BlockDriverState bs;
memset(&bs, 0, sizeof(bs));
- g_assert(!bdrv_write_threshold_is_set(&bs));
-
res = bdrv_write_threshold_get(&bs);
g_assert_cmpint(res, ==, 0);
}
@@ -33,8 +31,6 @@ static void test_threshold_set_get(void)
bdrv_write_threshold_set(&bs, threshold);
- g_assert(bdrv_write_threshold_is_set(&bs));
-
res = bdrv_write_threshold_get(&bs);
g_assert_cmpint(res, ==, threshold);
}
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 4/8] block/write-threshold: drop extra APIs
2021-05-06 9:06 ` [PATCH v3 4/8] block/write-threshold: drop extra APIs Vladimir Sementsov-Ogievskiy
@ 2021-05-07 14:18 ` Eric Blake
0 siblings, 0 replies; 15+ messages in thread
From: Eric Blake @ 2021-05-07 14:18 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, eesposit, qemu-devel, mreitz, stefanha, pbonzini
On 5/6/21 4:06 AM, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_write_threshold_exceeded() is unused at all.
either:
s/unused/not used/
or:
s/ at all//
>
> bdrv_write_threshold_is_set() is used only to double check the value of
> bs->write_threshold_offset in tests. No real sense in it (both tests do
> check real value with help of bdrv_write_threshold_get())
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 5/8] block/write-threshold: don't use aio context lock
2021-05-06 9:06 [PATCH v3 0/8] block: refactor write threshold Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2021-05-06 9:06 ` [PATCH v3 4/8] block/write-threshold: drop extra APIs Vladimir Sementsov-Ogievskiy
@ 2021-05-06 9:06 ` Vladimir Sementsov-Ogievskiy
2021-05-07 13:45 ` Paolo Bonzini
2021-05-06 9:06 ` [PATCH v3 6/8] test-write-threshold: drop extra tests Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
9 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-06 9:06 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, fam, stefanha, mreitz, kwolf, vsementsov, eesposit, pbonzini
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 <vsementsov@virtuozzo.com>
---
include/block/block_int.h | 6 +++++-
include/block/write-threshold.h | 6 ++++++
block/write-threshold.c | 34 +++++++++++++++++----------------
3 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index eab352f363..e3f3d79f5b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -954,7 +954,11 @@ struct BlockDriverState {
*/
int64_t total_sectors;
- /* threshold limit for writes, in bytes. "High water mark". */
+ /*
+ * Threshold limit for writes, in bytes. "High water mark".
+ * Don't access directly, use bdrw_write_threshold* interface.
+ * Protected by atomic access, no lock is needed.
+ */
uint64_t write_threshold_offset;
/* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
diff --git a/include/block/write-threshold.h b/include/block/write-threshold.h
index c60b9954cd..28c35a1c05 100644
--- a/include/block/write-threshold.h
+++ b/include/block/write-threshold.h
@@ -24,6 +24,8 @@
* To be used with thin-provisioned block devices.
*
* Use threshold_bytes == 0 to disable.
+ *
+ * Function is thread-safe, no lock is needed.
*/
void bdrv_write_threshold_set(BlockDriverState *bs, uint64_t threshold_bytes);
@@ -32,6 +34,8 @@ void bdrv_write_threshold_set(BlockDriverState *bs, uint64_t threshold_bytes);
*
* Get the configured write threshold, in bytes.
* Zero means no threshold configured.
+ *
+ * Function is thread-safe, no lock is needed.
*/
uint64_t bdrv_write_threshold_get(const BlockDriverState *bs);
@@ -40,6 +44,8 @@ uint64_t bdrv_write_threshold_get(const BlockDriverState *bs);
*
* Check whether the specified request exceeds the write threshold.
* If it is, send corresponding event and disable write threshold checking.
+ *
+ * Function is thread-safe, no lock is needed.
*/
void bdrv_write_threshold_check_write(BlockDriverState *bs, int64_t offset,
int64_t bytes);
diff --git a/block/write-threshold.c b/block/write-threshold.c
index 65a6acd142..8b46bb9a75 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 <fromani@redhat.com>
@@ -14,6 +15,7 @@
#include "block/block_int.h"
#include "qemu/coroutine.h"
#include "block/write-threshold.h"
+#include "qemu/atomic.h"
#include "qemu/notify.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-block-core.h"
@@ -21,45 +23,45 @@
uint64_t bdrv_write_threshold_get(const BlockDriverState *bs)
{
- return bs->write_threshold_offset;
+ return qatomic_read(&bs->write_threshold_offset);
}
void bdrv_write_threshold_set(BlockDriverState *bs, uint64_t threshold_bytes)
{
- bs->write_threshold_offset = threshold_bytes;
+ qatomic_set(&bs->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);
+retry:
+ wtr = bdrv_write_threshold_get(bs);
+ if (wtr == 0 || wtr >= end) {
+ return;
+ }
- /* autodisable to avoid flooding the monitor */
- bdrv_write_threshold_set(bs, 0);
+ /* autodisable to avoid flooding the monitor */
+ if (qatomic_cmpxchg(&bs->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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 5/8] block/write-threshold: don't use aio context lock
2021-05-06 9:06 ` [PATCH v3 5/8] block/write-threshold: don't use aio context lock Vladimir Sementsov-Ogievskiy
@ 2021-05-07 13:45 ` Paolo Bonzini
2021-05-10 9:30 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2021-05-07 13:45 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, eesposit, qemu-devel, mreitz, stefanha
On 06/05/21 11:06, Vladimir Sementsov-Ogievskiy wrote:
> 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);
> +retry:
> + wtr = bdrv_write_threshold_get(bs);
> + if (wtr == 0 || wtr >= end) {
> + return;
> + }
>
> - /* autodisable to avoid flooding the monitor */
> - bdrv_write_threshold_set(bs, 0);
> + /* autodisable to avoid flooding the monitor */
> + if (qatomic_cmpxchg(&bs->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);
> }
>
This has the problem that 64-bit atomics are not always possible on
32-bit builds. We can use a spinlock (and probably just drop this patch
for now).
Paolo
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 5/8] block/write-threshold: don't use aio context lock
2021-05-07 13:45 ` Paolo Bonzini
@ 2021-05-10 9:30 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-10 9:30 UTC (permalink / raw)
To: Paolo Bonzini, qemu-block
Cc: qemu-devel, fam, stefanha, mreitz, kwolf, eesposit
07.05.2021 16:45, Paolo Bonzini wrote:
> On 06/05/21 11:06, Vladimir Sementsov-Ogievskiy wrote:
>> 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);
>> +retry:
>> + wtr = bdrv_write_threshold_get(bs);
>> + if (wtr == 0 || wtr >= end) {
>> + return;
>> + }
>> - /* autodisable to avoid flooding the monitor */
>> - bdrv_write_threshold_set(bs, 0);
>> + /* autodisable to avoid flooding the monitor */
>> + if (qatomic_cmpxchg(&bs->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);
>> }
>>
>
> This has the problem that 64-bit atomics are not always possible on 32-bit builds. We can use a spinlock (and probably just drop this patch for now).
>
> Paolo
>
OK, let's just drop it for now, the series originally not intended to make something thread-safe, but only to clear the way for.
(And honestly I doubt that write-threshold worth the complexity of this atomic cmpxchg retry loop, mutex would be simpler anyway)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 6/8] test-write-threshold: drop extra tests
2021-05-06 9:06 [PATCH v3 0/8] block: refactor write threshold Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2021-05-06 9:06 ` [PATCH v3 5/8] block/write-threshold: don't use aio context lock Vladimir Sementsov-Ogievskiy
@ 2021-05-06 9:06 ` Vladimir Sementsov-Ogievskiy
2021-05-06 9:06 ` [PATCH v3 7/8] test-write-threshold: drop extra TestStruct structure Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-06 9:06 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, fam, stefanha, mreitz, kwolf, vsementsov, eesposit, pbonzini
Testing set/get of one 64bit variable doesn't seem necessary. We have a
lot of such variables. Also remaining tests do test set/get anyway.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/unit/test-write-threshold.c | 43 -------------------------------
1 file changed, 43 deletions(-)
diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index bb5c1a5217..9e9986aefc 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -12,43 +12,6 @@
#include "block/write-threshold.h"
-static void test_threshold_not_set_on_init(void)
-{
- uint64_t res;
- BlockDriverState bs;
- memset(&bs, 0, sizeof(bs));
-
- res = bdrv_write_threshold_get(&bs);
- g_assert_cmpint(res, ==, 0);
-}
-
-static void test_threshold_set_get(void)
-{
- uint64_t threshold = 4 * 1024 * 1024;
- uint64_t res;
- BlockDriverState bs;
- memset(&bs, 0, sizeof(bs));
-
- bdrv_write_threshold_set(&bs, threshold);
-
- res = bdrv_write_threshold_get(&bs);
- g_assert_cmpint(res, ==, threshold);
-}
-
-static void test_threshold_multi_set_get(void)
-{
- uint64_t threshold1 = 4 * 1024 * 1024;
- uint64_t threshold2 = 15 * 1024 * 1024;
- uint64_t res;
- BlockDriverState bs;
- memset(&bs, 0, sizeof(bs));
-
- bdrv_write_threshold_set(&bs, threshold1);
- bdrv_write_threshold_set(&bs, threshold2);
- res = bdrv_write_threshold_get(&bs);
- g_assert_cmpint(res, ==, threshold2);
-}
-
static void test_threshold_not_trigger(void)
{
uint64_t threshold = 4 * 1024 * 1024;
@@ -84,12 +47,6 @@ int main(int argc, char **argv)
{
size_t i;
TestStruct tests[] = {
- { "/write-threshold/not-set-on-init",
- test_threshold_not_set_on_init },
- { "/write-threshold/set-get",
- test_threshold_set_get },
- { "/write-threshold/multi-set-get",
- test_threshold_multi_set_get },
{ "/write-threshold/not-trigger",
test_threshold_not_trigger },
{ "/write-threshold/trigger",
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 7/8] test-write-threshold: drop extra TestStruct structure
2021-05-06 9:06 [PATCH v3 0/8] block: refactor write threshold Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2021-05-06 9:06 ` [PATCH v3 6/8] test-write-threshold: drop extra tests Vladimir Sementsov-Ogievskiy
@ 2021-05-06 9:06 ` Vladimir Sementsov-Ogievskiy
2021-05-06 9:06 ` [PATCH v3 8/8] write-threshold: deal with includes Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-06 9:06 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, fam, stefanha, mreitz, kwolf, vsementsov, eesposit, pbonzini
We don't need this extra logic: it doesn't make code simpler.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/unit/test-write-threshold.c | 20 +++-----------------
1 file changed, 3 insertions(+), 17 deletions(-)
diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index 9e9986aefc..49b1ef7a20 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -37,26 +37,12 @@ static void test_threshold_trigger(void)
g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, 0);
}
-typedef struct TestStruct {
- const char *name;
- void (*func)(void);
-} TestStruct;
-
int main(int argc, char **argv)
{
- size_t i;
- TestStruct tests[] = {
- { "/write-threshold/not-trigger",
- test_threshold_not_trigger },
- { "/write-threshold/trigger",
- test_threshold_trigger },
- { NULL, NULL }
- };
-
g_test_init(&argc, &argv, NULL);
- for (i = 0; tests[i].name != NULL; i++) {
- g_test_add_func(tests[i].name, tests[i].func);
- }
+ g_test_add_func("/write-threshold/not-trigger", test_threshold_not_trigger);
+ g_test_add_func("/write-threshold/trigger", test_threshold_trigger);
+
return g_test_run();
}
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 8/8] write-threshold: deal with includes
2021-05-06 9:06 [PATCH v3 0/8] block: refactor write threshold Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2021-05-06 9:06 ` [PATCH v3 7/8] test-write-threshold: drop extra TestStruct structure Vladimir Sementsov-Ogievskiy
@ 2021-05-06 9:06 ` Vladimir Sementsov-Ogievskiy
2021-05-12 16:03 ` [PATCH v3 0/8] block: refactor write threshold Stefan Hajnoczi
2021-05-12 17:31 ` Max Reitz
9 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-06 9:06 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, fam, stefanha, mreitz, kwolf, vsementsov, eesposit, pbonzini
"qemu/typedefs.h" is enough for include/block/write-threshold.h header
with forward declaration of BlockDriverState. Also drop extra includes
from block/write-threshold.c and tests/unit/test-write-threshold.c
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/write-threshold.h | 2 +-
block/write-threshold.c | 2 --
tests/unit/test-write-threshold.c | 1 -
3 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/include/block/write-threshold.h b/include/block/write-threshold.h
index 28c35a1c05..e3acbc249d 100644
--- a/include/block/write-threshold.h
+++ b/include/block/write-threshold.h
@@ -13,7 +13,7 @@
#ifndef BLOCK_WRITE_THRESHOLD_H
#define BLOCK_WRITE_THRESHOLD_H
-#include "block/block_int.h"
+#include "qemu/typedefs.h"
/*
* bdrv_write_threshold_set:
diff --git a/block/write-threshold.c b/block/write-threshold.c
index 8b46bb9a75..7578c0599a 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -13,10 +13,8 @@
#include "qemu/osdep.h"
#include "block/block_int.h"
-#include "qemu/coroutine.h"
#include "block/write-threshold.h"
#include "qemu/atomic.h"
-#include "qemu/notify.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-block-core.h"
#include "qapi/qapi-events-block-core.h"
diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index 49b1ef7a20..0158e4637a 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -7,7 +7,6 @@
*/
#include "qemu/osdep.h"
-#include "qapi/error.h"
#include "block/block_int.h"
#include "block/write-threshold.h"
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 0/8] block: refactor write threshold
2021-05-06 9:06 [PATCH v3 0/8] block: refactor write threshold Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2021-05-06 9:06 ` [PATCH v3 8/8] write-threshold: deal with includes Vladimir Sementsov-Ogievskiy
@ 2021-05-12 16:03 ` Stefan Hajnoczi
2021-05-12 17:31 ` Max Reitz
9 siblings, 0 replies; 15+ messages in thread
From: Stefan Hajnoczi @ 2021-05-12 16:03 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: fam, kwolf, qemu-block, eesposit, qemu-devel, mreitz, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]
On Thu, May 06, 2021 at 12:06:13PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> v3:
> 01-04,06,07: add Max's r-b.
> 01: improve commit msg and comments
> 03: improve commit msg
> 05: add more comments and qemu/atomic.h include
> 08: new, replacement for v2:08,09
>
> Vladimir Sementsov-Ogievskiy (8):
> block/write-threshold: don't use write notifiers
> block: drop write notifiers
> test-write-threshold: rewrite test_threshold_(not_)trigger tests
> block/write-threshold: drop extra APIs
> block/write-threshold: don't use aio context lock
> test-write-threshold: drop extra tests
> test-write-threshold: drop extra TestStruct structure
> write-threshold: deal with includes
>
> include/block/block_int.h | 19 ++---
> include/block/write-threshold.h | 31 +++------
> block.c | 1 -
> block/io.c | 11 +--
> block/write-threshold.c | 111 +++++++-----------------------
> tests/unit/test-write-threshold.c | 90 ++----------------------
> 6 files changed, 52 insertions(+), 211 deletions(-)
>
> --
> 2.29.2
>
Aside from comments:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 0/8] block: refactor write threshold
2021-05-06 9:06 [PATCH v3 0/8] block: refactor write threshold Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2021-05-12 16:03 ` [PATCH v3 0/8] block: refactor write threshold Stefan Hajnoczi
@ 2021-05-12 17:31 ` Max Reitz
9 siblings, 0 replies; 15+ messages in thread
From: Max Reitz @ 2021-05-12 17:31 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, eesposit, qemu-devel, stefanha, pbonzini
On 06.05.21 11:06, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> v3:
> 01-04,06,07: add Max's r-b.
> 01: improve commit msg and comments
> 03: improve commit msg
> 05: add more comments and qemu/atomic.h include
> 08: new, replacement for v2:08,09
>
> Vladimir Sementsov-Ogievskiy (8):
> block/write-threshold: don't use write notifiers
> block: drop write notifiers
> test-write-threshold: rewrite test_threshold_(not_)trigger tests
> block/write-threshold: drop extra APIs
> block/write-threshold: don't use aio context lock
> test-write-threshold: drop extra tests
> test-write-threshold: drop extra TestStruct structure
> write-threshold: deal with includes
>
> include/block/block_int.h | 19 ++---
> include/block/write-threshold.h | 31 +++------
> block.c | 1 -
> block/io.c | 11 +--
> block/write-threshold.c | 111 +++++++-----------------------
> tests/unit/test-write-threshold.c | 90 ++----------------------
> 6 files changed, 52 insertions(+), 211 deletions(-)
Thanks, I’ve applied all patches but patch 5 to my block branch, with
the changes Eric has suggested:
https://github.com/XanClic/qemu/commits/block
Max
^ permalink raw reply [flat|nested] 15+ messages in thread