| Message ID | 20210504082553.20377-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: refactor write threshold | expand |
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 <vsementsov@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. > + 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 <mreitz@redhat.com> > + } > +} >
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 <vsementsov@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 <mreitz@redhat.com> > Thanks for reviewing my patches!
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.
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
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..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 + * corresponding event and unset write threshold. + */ +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..11152c60df 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,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); + bdrv_write_threshold_set(bs, 0); + } +}
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) Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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(-)