* [PATCH] block: simplify write-threshold and drop write notifiers
@ 2021-04-21 22:09 Vladimir Sementsov-Ogievskiy
2021-04-22 9:57 ` Emanuele Giuseppe Esposito
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-21 22:09 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, fam, stefanha, mreitz, kwolf, pbonzini, eesposit, vsementsov
write-notifiers are used only for write-threshold. New code for such
purpose should create filters.
Let's handle write-threshold simply in generic code and drop write
notifiers at all.
Also move part of write-threshold API that is used only for testing to
the test.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
I agree that this could be split into 2-3 parts and not combining
everything into one. But I'm tired now. I can send v2 if needed, so
consider it as RFC. Or take as is if you think it's not too much-in-one.
I also suggest this as a prepartion (and partly substitution) for
"[PATCH v2 0/8] Block layer thread-safety, continued"
include/block/block_int.h | 12 -----
include/block/write-threshold.h | 24 ---------
block.c | 1 -
block/io.c | 21 +++++---
block/write-threshold.c | 87 ++-----------------------------
tests/unit/test-write-threshold.c | 38 ++++++++++++++
6 files changed, 54 insertions(+), 129 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 88e4111939..50af58af75 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -957,12 +957,8 @@ 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;
- 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
@@ -1087,14 +1083,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/include/block/write-threshold.h b/include/block/write-threshold.h
index c646f267a4..23e1bfc123 100644
--- a/include/block/write-threshold.h
+++ b/include/block/write-threshold.h
@@ -35,28 +35,4 @@ 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);
-
#endif
diff --git a/block.c b/block.c
index c5b887cec1..001453105e 100644
--- a/block.c
+++ b/block.c
@@ -381,7 +381,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 ca2dca3007..e0aa775952 100644
--- a/block/io.c
+++ b/block/io.c
@@ -36,6 +36,8 @@
#include "qemu/main-loop.h"
#include "sysemu/replay.h"
+#include "qapi/qapi-events-block-core.h"
+
/* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
#define MAX_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)
@@ -1974,6 +1976,8 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,
child->perm & BLK_PERM_RESIZE);
switch (req->type) {
+ uint64_t write_threshold;
+
case BDRV_TRACKED_WRITE:
case BDRV_TRACKED_DISCARD:
if (flags & BDRV_REQ_WRITE_UNCHANGED) {
@@ -1981,8 +1985,15 @@ 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);
+ write_threshold = qatomic_read(&bs->write_threshold_offset);
+ if (write_threshold > 0 && offset + bytes > write_threshold) {
+ qapi_event_send_block_write_threshold(
+ bs->node_name,
+ offset + bytes - write_threshold,
+ write_threshold);
+ qatomic_set(&bs->write_threshold_offset, 0);
+ }
+ return 0;
case BDRV_TRACKED_TRUNCATE:
assert(child->perm & BLK_PERM_RESIZE);
return 0;
@@ -3137,12 +3148,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;
diff --git a/block/write-threshold.c b/block/write-threshold.c
index 85b78dc2a9..9bf4287c6e 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -21,104 +21,23 @@
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;
-}
-
-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)
-{
- 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;
-}
-
-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;
+ return qatomic_read(&bs->write_threshold_offset);
}
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 */
- }
+ 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);
}
diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index fc1c45a2eb..c2f4cd20d7 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -12,6 +12,44 @@
#include "block/write-threshold.h"
+/*
+ * bdrv_write_threshold_is_set
+ *
+ * Tell if a write threshold is set for a given BDS.
+ */
+static bool bdrv_write_threshold_is_set(const BlockDriverState *bs)
+{
+ return bs->write_threshold_offset > 0;
+}
+
+/*
+ * 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().
+ */
+static 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;
+}
+
static void test_threshold_not_set_on_init(void)
{
uint64_t res;
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] block: simplify write-threshold and drop write notifiers
2021-04-21 22:09 [PATCH] block: simplify write-threshold and drop write notifiers Vladimir Sementsov-Ogievskiy
@ 2021-04-22 9:57 ` Emanuele Giuseppe Esposito
2021-04-22 10:12 ` Vladimir Sementsov-Ogievskiy
2021-04-30 10:04 ` Max Reitz
2021-05-05 10:10 ` Stefan Hajnoczi
2 siblings, 1 reply; 7+ messages in thread
From: Emanuele Giuseppe Esposito @ 2021-04-22 9:57 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, qemu-devel, mreitz, stefanha, pbonzini
On 22/04/2021 00:09, Vladimir Sementsov-Ogievskiy wrote:
> write-notifiers are used only for write-threshold. New code for such
> purpose should create filters.
>
> Let's handle write-threshold simply in generic code and drop write
> notifiers at all.
>
> Also move part of write-threshold API that is used only for testing to
> the test.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> I agree that this could be split into 2-3 parts and not combining
> everything into one. But I'm tired now. I can send v2 if needed, so
> consider it as RFC. Or take as is if you think it's not too much-in-one.
Thank you for this patch. Since I am reworking on v2, if you want I can
also integrate this patch with mines and send everything together once I
am done.
Emanuele
>
> I also suggest this as a prepartion (and partly substitution) for
> "[PATCH v2 0/8] Block layer thread-safety, continued"
>
> include/block/block_int.h | 12 -----
> include/block/write-threshold.h | 24 ---------
> block.c | 1 -
> block/io.c | 21 +++++---
> block/write-threshold.c | 87 ++-----------------------------
> tests/unit/test-write-threshold.c | 38 ++++++++++++++
> 6 files changed, 54 insertions(+), 129 deletions(-)
>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 88e4111939..50af58af75 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -957,12 +957,8 @@ 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;
> - 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
> @@ -1087,14 +1083,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/include/block/write-threshold.h b/include/block/write-threshold.h
> index c646f267a4..23e1bfc123 100644
> --- a/include/block/write-threshold.h
> +++ b/include/block/write-threshold.h
> @@ -35,28 +35,4 @@ 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);
> -
> #endif
> diff --git a/block.c b/block.c
> index c5b887cec1..001453105e 100644
> --- a/block.c
> +++ b/block.c
> @@ -381,7 +381,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 ca2dca3007..e0aa775952 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -36,6 +36,8 @@
> #include "qemu/main-loop.h"
> #include "sysemu/replay.h"
>
> +#include "qapi/qapi-events-block-core.h"
> +
> /* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
> #define MAX_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)
>
> @@ -1974,6 +1976,8 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,
> child->perm & BLK_PERM_RESIZE);
>
> switch (req->type) {
> + uint64_t write_threshold;
> +
> case BDRV_TRACKED_WRITE:
> case BDRV_TRACKED_DISCARD:
> if (flags & BDRV_REQ_WRITE_UNCHANGED) {
> @@ -1981,8 +1985,15 @@ 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);
> + write_threshold = qatomic_read(&bs->write_threshold_offset);
> + if (write_threshold > 0 && offset + bytes > write_threshold) {
> + qapi_event_send_block_write_threshold(
> + bs->node_name,
> + offset + bytes - write_threshold,
> + write_threshold);
> + qatomic_set(&bs->write_threshold_offset, 0);
> + }
> + return 0;
> case BDRV_TRACKED_TRUNCATE:
> assert(child->perm & BLK_PERM_RESIZE);
> return 0;
> @@ -3137,12 +3148,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;
> diff --git a/block/write-threshold.c b/block/write-threshold.c
> index 85b78dc2a9..9bf4287c6e 100644
> --- a/block/write-threshold.c
> +++ b/block/write-threshold.c
> @@ -21,104 +21,23 @@
>
> 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;
> -}
> -
> -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)
> -{
> - 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;
> -}
> -
> -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;
> + return qatomic_read(&bs->write_threshold_offset);
> }
>
> 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 */
> - }
> + 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);
> }
> diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
> index fc1c45a2eb..c2f4cd20d7 100644
> --- a/tests/unit/test-write-threshold.c
> +++ b/tests/unit/test-write-threshold.c
> @@ -12,6 +12,44 @@
> #include "block/write-threshold.h"
>
>
> +/*
> + * bdrv_write_threshold_is_set
> + *
> + * Tell if a write threshold is set for a given BDS.
> + */
> +static bool bdrv_write_threshold_is_set(const BlockDriverState *bs)
> +{
> + return bs->write_threshold_offset > 0;
> +}
> +
> +/*
> + * 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().
> + */
> +static 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;
> +}
> +
> static void test_threshold_not_set_on_init(void)
> {
> uint64_t res;
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: simplify write-threshold and drop write notifiers
2021-04-22 9:57 ` Emanuele Giuseppe Esposito
@ 2021-04-22 10:12 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 10:12 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: qemu-devel, fam, stefanha, mreitz, kwolf, pbonzini
22.04.2021 12:57, Emanuele Giuseppe Esposito wrote:
>
>
> On 22/04/2021 00:09, Vladimir Sementsov-Ogievskiy wrote:
>> write-notifiers are used only for write-threshold. New code for such
>> purpose should create filters.
>>
>> Let's handle write-threshold simply in generic code and drop write
>> notifiers at all.
>>
>> Also move part of write-threshold API that is used only for testing to
>> the test.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> I agree that this could be split into 2-3 parts and not combining
>> everything into one. But I'm tired now. I can send v2 if needed, so
>> consider it as RFC. Or take as is if you think it's not too much-in-one.
>
> Thank you for this patch. Since I am reworking on v2, if you want I can also integrate this patch with mines and send everything together once I am done.
>
> Emanuele
I'd wait several days for comments. Maybe I'll have to resend v2 of this.
Also, after this patch, is something of your patches 1-4 needed? Probably you may just resend your series not touching write-threshold, and just note in cover-letter that write-threshold is covered by "[PATCH] block: simplify write-threshold and drop write notifiers"
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: simplify write-threshold and drop write notifiers
2021-04-21 22:09 [PATCH] block: simplify write-threshold and drop write notifiers Vladimir Sementsov-Ogievskiy
2021-04-22 9:57 ` Emanuele Giuseppe Esposito
@ 2021-04-30 10:04 ` Max Reitz
2021-05-03 8:21 ` Vladimir Sementsov-Ogievskiy
2021-05-05 10:10 ` Stefan Hajnoczi
2 siblings, 1 reply; 7+ messages in thread
From: Max Reitz @ 2021-04-30 10:04 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, eesposit, qemu-devel, stefanha, pbonzini
On 22.04.21 00:09, Vladimir Sementsov-Ogievskiy wrote:
> write-notifiers are used only for write-threshold. New code for such
> purpose should create filters.
>
> Let's handle write-threshold simply in generic code and drop write
> notifiers at all.
>
> Also move part of write-threshold API that is used only for testing to
> the test.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> I agree that this could be split into 2-3 parts and not combining
> everything into one. But I'm tired now.
Er... You should have put it off until the next day then? O:)
It should be multiple patches. At least one to move the write threshold
update to block/io.c, and then another to drop write notifiers.
> I can send v2 if needed, so
> consider it as RFC. Or take as is if you think it's not too much-in-one.
>
> I also suggest this as a prepartion (and partly substitution) for
> "[PATCH v2 0/8] Block layer thread-safety, continued"
>
> include/block/block_int.h | 12 -----
> include/block/write-threshold.h | 24 ---------
> block.c | 1 -
> block/io.c | 21 +++++---
> block/write-threshold.c | 87 ++-----------------------------
> tests/unit/test-write-threshold.c | 38 ++++++++++++++
> 6 files changed, 54 insertions(+), 129 deletions(-)
[...]
> diff --git a/block/io.c b/block/io.c
> index ca2dca3007..e0aa775952 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -36,6 +36,8 @@
> #include "qemu/main-loop.h"
> #include "sysemu/replay.h"
>
> +#include "qapi/qapi-events-block-core.h"
> +
> /* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
> #define MAX_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)
>
> @@ -1974,6 +1976,8 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,
> child->perm & BLK_PERM_RESIZE);
>
> switch (req->type) {
> + uint64_t write_threshold;
> +
Works, but I think this is the first time I see a variable declared in a
switch block. What I usually do for such cases is to put a block after
the label. (i.e. case X: { uint64_t write_threshold; ... })
But it wouldn’t hurt to just declare this at the beginning of
bdrv_co_write_req_prepare(), I think.
> case BDRV_TRACKED_WRITE:
> case BDRV_TRACKED_DISCARD:
> if (flags & BDRV_REQ_WRITE_UNCHANGED) {
> @@ -1981,8 +1985,15 @@ 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);
> + write_threshold = qatomic_read(&bs->write_threshold_offset);
> + if (write_threshold > 0 && offset + bytes > write_threshold) {
> + qapi_event_send_block_write_threshold(
> + bs->node_name,
> + offset + bytes - write_threshold,
> + write_threshold);
> + qatomic_set(&bs->write_threshold_offset, 0);
> + }
I’d put all of this into a function in block/write-threshold.c that’s
called from here.
Max
> + return 0;
> case BDRV_TRACKED_TRUNCATE:
> assert(child->perm & BLK_PERM_RESIZE);
> return 0;
> @@ -3137,12 +3148,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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: simplify write-threshold and drop write notifiers
2021-04-30 10:04 ` Max Reitz
@ 2021-05-03 8:21 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-03 8:21 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: qemu-devel, fam, stefanha, kwolf, pbonzini, eesposit
30.04.2021 13:04, Max Reitz wrote:
> On 22.04.21 00:09, Vladimir Sementsov-Ogievskiy wrote:
>> write-notifiers are used only for write-threshold. New code for such
>> purpose should create filters.
>>
>> Let's handle write-threshold simply in generic code and drop write
>> notifiers at all.
>>
>> Also move part of write-threshold API that is used only for testing to
>> the test.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> I agree that this could be split into 2-3 parts and not combining
>> everything into one. But I'm tired now.
>
> Er... You should have put it off until the next day then? O:)
Yes, sorry. I wanted to send that day... Don't remember why :) Checked now, that was not Friday.. I wanted to drop write notifiers long ago, and when I finally do it I couldn't wait, so cool it seemed to me :)
Thanks for comments, I'll send v2 soon.
>
> It should be multiple patches. At least one to move the write threshold update to block/io.c, and then another to drop write notifiers.
>
>> I can send v2 if needed, so
>> consider it as RFC. Or take as is if you think it's not too much-in-one.
>>
>> I also suggest this as a prepartion (and partly substitution) for
>> "[PATCH v2 0/8] Block layer thread-safety, continued"
>>
>> include/block/block_int.h | 12 -----
>> include/block/write-threshold.h | 24 ---------
>> block.c | 1 -
>> block/io.c | 21 +++++---
>> block/write-threshold.c | 87 ++-----------------------------
>> tests/unit/test-write-threshold.c | 38 ++++++++++++++
>> 6 files changed, 54 insertions(+), 129 deletions(-)
>
> [...]
>
>> diff --git a/block/io.c b/block/io.c
>> index ca2dca3007..e0aa775952 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -36,6 +36,8 @@
>> #include "qemu/main-loop.h"
>> #include "sysemu/replay.h"
>> +#include "qapi/qapi-events-block-core.h"
>> +
>> /* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
>> #define MAX_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)
>> @@ -1974,6 +1976,8 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,
>> child->perm & BLK_PERM_RESIZE);
>> switch (req->type) {
>> + uint64_t write_threshold;
>> +
>
> Works, but I think this is the first time I see a variable declared in a switch block. What I usually do for such cases is to put a block after the label. (i.e. case X: { uint64_t write_threshold; ... })
>
> But it wouldn’t hurt to just declare this at the beginning of bdrv_co_write_req_prepare(), I think.
>
>> case BDRV_TRACKED_WRITE:
>> case BDRV_TRACKED_DISCARD:
>> if (flags & BDRV_REQ_WRITE_UNCHANGED) {
>> @@ -1981,8 +1985,15 @@ 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);
>> + write_threshold = qatomic_read(&bs->write_threshold_offset);
>> + if (write_threshold > 0 && offset + bytes > write_threshold) {
>> + qapi_event_send_block_write_threshold(
>> + bs->node_name,
>> + offset + bytes - write_threshold,
>> + write_threshold);
>> + qatomic_set(&bs->write_threshold_offset, 0);
>> + }
>
> I’d put all of this into a function in block/write-threshold.c that’s called from here.
>
> Max
>
>> + return 0;
>> case BDRV_TRACKED_TRUNCATE:
>> assert(child->perm & BLK_PERM_RESIZE);
>> return 0;
>> @@ -3137,12 +3148,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;
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: simplify write-threshold and drop write notifiers
2021-04-21 22:09 [PATCH] block: simplify write-threshold and drop write notifiers Vladimir Sementsov-Ogievskiy
2021-04-22 9:57 ` Emanuele Giuseppe Esposito
2021-04-30 10:04 ` Max Reitz
@ 2021-05-05 10:10 ` Stefan Hajnoczi
2021-05-05 10:17 ` Vladimir Sementsov-Ogievskiy
2 siblings, 1 reply; 7+ messages in thread
From: Stefan Hajnoczi @ 2021-05-05 10:10 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: fam, kwolf, qemu-block, eesposit, qemu-devel, mreitz, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]
On Thu, Apr 22, 2021 at 01:09:50AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> @@ -1981,8 +1985,15 @@ 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);
> + write_threshold = qatomic_read(&bs->write_threshold_offset);
> + if (write_threshold > 0 && offset + bytes > write_threshold) {
> + qapi_event_send_block_write_threshold(
> + bs->node_name,
> + offset + bytes - write_threshold,
> + write_threshold);
> + qatomic_set(&bs->write_threshold_offset, 0);
It's safer to reset the threshold before emitting the event. That way
there is no race with the QMP client setting a new threshold via
bdrv_write_threshold_is_set(). I guess the race is possible since
qatomic is used and there is no lock.
I like the idea of dropping write notifiers:
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: simplify write-threshold and drop write notifiers
2021-05-05 10:10 ` Stefan Hajnoczi
@ 2021-05-05 10:17 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-05 10:17 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-block, qemu-devel, fam, mreitz, kwolf, pbonzini, eesposit
05.05.2021 13:10, Stefan Hajnoczi wrote:
> On Thu, Apr 22, 2021 at 01:09:50AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> @@ -1981,8 +1985,15 @@ 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);
>> + write_threshold = qatomic_read(&bs->write_threshold_offset);
>> + if (write_threshold > 0 && offset + bytes > write_threshold) {
>> + qapi_event_send_block_write_threshold(
>> + bs->node_name,
>> + offset + bytes - write_threshold,
>> + write_threshold);
>> + qatomic_set(&bs->write_threshold_offset, 0);
>
> It's safer to reset the threshold before emitting the event. That way
> there is no race with the QMP client setting a new threshold via
> bdrv_write_threshold_is_set(). I guess the race is possible since
> qatomic is used and there is no lock.
>
> I like the idea of dropping write notifiers:
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>
Sorry, this is an outdated patch, I've sent a v2:
[PATCH v2 0/9] block: refactor write threshold
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-05 10:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-21 22:09 [PATCH] block: simplify write-threshold and drop write notifiers Vladimir Sementsov-Ogievskiy
2021-04-22 9:57 ` Emanuele Giuseppe Esposito
2021-04-22 10:12 ` Vladimir Sementsov-Ogievskiy
2021-04-30 10:04 ` Max Reitz
2021-05-03 8:21 ` Vladimir Sementsov-Ogievskiy
2021-05-05 10:10 ` Stefan Hajnoczi
2021-05-05 10:17 ` Vladimir Sementsov-Ogievskiy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).