Re: [PATCH v2 1/9] block/write-threshold: don't use write notifiers
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 --- 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. +bdrv_write_threshold_set(bs, 0); I’d keep the comment from before_write_notify() here (i.e. “autodisable to avoid flooding the monitor”). But other than that, I have no complaints: Reviewed-by: Max Reitz +} +}
Re: [PATCH v2 1/9] block/write-threshold: don't use write notifiers
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 --- 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 Thanks for reviewing my patches! -- Best regards, Vladimir
Re: [PATCH v2 1/9] block/write-threshold: don't use write notifiers
05.05.2021 15:37, Max Reitz 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.) You mean, we probably could only add new interface here, keeping other things as is, and drop them in a separate patch? If keep as is we can add the following here: So, create a new direct interface for bdrv_co_write_req_prepare() and drop all write-notifier related logic from write-threshold.c. -- Best regards, Vladimir
Re: [PATCH v2 1/9] block/write-threshold: don't use write notifiers
On 05.05.21 15:35, Vladimir Sementsov-Ogievskiy wrote: 05.05.2021 15:37, Max Reitz 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.) You mean, we probably could only add new interface here, keeping other things as is, and drop them in a separate patch? Something like that, yes. But not important. If keep as is we can add the following here: So, create a new direct interface for bdrv_co_write_req_prepare() and drop all write-notifier related logic from write-threshold.c. Sounds good! Max