05.05.2021 15:37, Max Reitz wrote:
On 04.05.21 10:25, 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)
Not noted here: That write-threshold.c is also reorganized. (Doesn’t seem
entirely necessary to do right in this patch, but why not.)
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
include/block/block_int.h | 1 -
include/block/write-threshold.h | 9 +++++
block/io.c | 5 ++-
block/write-threshold.c | 68 +++++++--------------------------
4 files changed, 25 insertions(+), 58 deletions(-)
[...]
diff --git a/include/block/write-threshold.h b/include/block/write-threshold.h
index c646f267a4..7942dcc368 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, does specified request exceeds write threshold. If it is, send
I’d prefer either “Check: Does the specified request exceed the write
threshold?” or “Check whether the specified request exceeds the write
threshold.”
+ * corresponding event and unset write threshold.
I’d call it “disable write threshold checking” instead of “unset write
threshold”, because I don’t it immediately clear what unsetting the threshold
means.
+ */
+void bdrv_write_threshold_check_write(BlockDriverState *bs, int64_t offset,
+ int64_t bytes);
+
#endif
[...]
@@ -122,3 +68,15 @@ 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);
OK, don’t understand why bdrv_write_threshold_exceeded() had two cases (one for
offset > wtr, one for end > wtr). Perhaps overflow considerations? Shouldn’t
matter though.
I don't think it helps with overflow:) Still, offset + bytes should never
overflow in block layer, requests are checked at the entrance.
+ bdrv_write_threshold_set(bs, 0);
I’d keep the comment from before_write_notify() here (i.e. “autodisable to
avoid flooding the monitor”).
I'm OK with it and both your rewording suggestions. Will apply if v3 needed.
But other than that, I have no complaints:
Reviewed-by: Max Reitz <mre...@redhat.com>
Thanks for reviewing my patches!
--
Best regards,
Vladimir