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(-)
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
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; >
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
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;
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
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>
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
© 2016 - 2024 Red Hat, Inc.