| Message ID | 20210419085541.22310-2-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Block layer thread-safety, continued | expand |
On Mon, Apr 19, 2021 at 10:55:34AM +0200, Emanuele Giuseppe Esposito wrote: No commit description. What about write thresholds makes them thread unsafe? Without a commit description reviewers have to reverse-engineer the patch to figure out the author's intention, which can lead to misunderstandings and bugs slipping through. My guess is the point of this patch was to stop accessing fields in bs directly? > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Co-developed-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block/write-threshold.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/block/write-threshold.c b/block/write-threshold.c > index 85b78dc2a9..63040fa4f2 100644 > --- a/block/write-threshold.c > +++ b/block/write-threshold.c > @@ -37,18 +37,22 @@ static void write_threshold_disable(BlockDriverState *bs) > } > } > > +static uint64_t treshold_overage(const BdrvTrackedRequest *req, > + uint64_t thres) > +{ > + if (thres > 0 && req->offset + req->bytes > thres) { > + return req->offset + req->bytes - thres; > + } else { > + return 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; > + uint64_t thres = bdrv_write_threshold_get(bs); > + > + return treshold_overage(req, thres); > } Hmm...this function is only used by tests now. Should the tests be updated so that they are exercising the actual code instead of this test-only interface? > > static int coroutine_fn before_write_notify(NotifierWithReturn *notifier, > @@ -56,14 +60,14 @@ static int coroutine_fn before_write_notify(NotifierWithReturn *notifier, > { > BdrvTrackedRequest *req = opaque; > BlockDriverState *bs = req->bs; > - uint64_t amount = 0; > + uint64_t thres = bdrv_write_threshold_get(bs); > + uint64_t amount = treshold_overage(req, thres); > > - amount = bdrv_write_threshold_exceeded(bs, req); > if (amount > 0) { > qapi_event_send_block_write_threshold( > bs->node_name, > amount, > - bs->write_threshold_offset); > + thres); > > /* autodisable to avoid flooding the monitor */ > write_threshold_disable(bs); > -- > 2.30.2 >
Hi Stefan! Note my "[PATCH v2 0/9] block: refactor write threshold", it's a kind of counter-proposal for first half of this series. 05.05.2021 11:50, Stefan Hajnoczi wrote: > On Mon, Apr 19, 2021 at 10:55:34AM +0200, Emanuele Giuseppe Esposito wrote: > > No commit description. What about write thresholds makes them thread > unsafe? Without a commit description reviewers have to reverse-engineer > the patch to figure out the author's intention, which can lead to > misunderstandings and bugs slipping through. > > My guess is the point of this patch was to stop accessing fields in bs > directly? > >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> block/write-threshold.c | 28 ++++++++++++++++------------ >> 1 file changed, 16 insertions(+), 12 deletions(-) >> >> diff --git a/block/write-threshold.c b/block/write-threshold.c >> index 85b78dc2a9..63040fa4f2 100644 >> --- a/block/write-threshold.c >> +++ b/block/write-threshold.c >> @@ -37,18 +37,22 @@ static void write_threshold_disable(BlockDriverState *bs) >> } >> } >> >> +static uint64_t treshold_overage(const BdrvTrackedRequest *req, >> + uint64_t thres) >> +{ >> + if (thres > 0 && req->offset + req->bytes > thres) { >> + return req->offset + req->bytes - thres; >> + } else { >> + return 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; >> + uint64_t thres = bdrv_write_threshold_get(bs); >> + >> + return treshold_overage(req, thres); >> } > > Hmm...this function is only used by tests now. Should the tests be > updated so that they are exercising the actual code instead of this > test-only interface? > >> >> static int coroutine_fn before_write_notify(NotifierWithReturn *notifier, >> @@ -56,14 +60,14 @@ static int coroutine_fn before_write_notify(NotifierWithReturn *notifier, >> { >> BdrvTrackedRequest *req = opaque; >> BlockDriverState *bs = req->bs; >> - uint64_t amount = 0; >> + uint64_t thres = bdrv_write_threshold_get(bs); >> + uint64_t amount = treshold_overage(req, thres); >> >> - amount = bdrv_write_threshold_exceeded(bs, req); >> if (amount > 0) { >> qapi_event_send_block_write_threshold( >> bs->node_name, >> amount, >> - bs->write_threshold_offset); >> + thres); >> >> /* autodisable to avoid flooding the monitor */ >> write_threshold_disable(bs); >> -- >> 2.30.2 >>
On Wed, May 05, 2021 at 12:54:09PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Hi Stefan! > > Note my "[PATCH v2 0/9] block: refactor write threshold", it's a kind of counter-proposal for first half of this series. Thanks! Emanuele mentioned he will drop his patches. I took a look at your series and that approach looks good to me. Stefan
06.05.2021 12:04, Stefan Hajnoczi wrote: > On Wed, May 05, 2021 at 12:54:09PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> Hi Stefan! >> >> Note my "[PATCH v2 0/9] block: refactor write threshold", it's a kind of counter-proposal for first half of this series. > > Thanks! Emanuele mentioned he will drop his patches. > > I took a look at your series and that approach looks good to me. > Note, I've sent "[PATCH v3 0/8] block: refactor write threshold" a moment ago.
diff --git a/block/write-threshold.c b/block/write-threshold.c index 85b78dc2a9..63040fa4f2 100644 --- a/block/write-threshold.c +++ b/block/write-threshold.c @@ -37,18 +37,22 @@ static void write_threshold_disable(BlockDriverState *bs) } } +static uint64_t treshold_overage(const BdrvTrackedRequest *req, + uint64_t thres) +{ + if (thres > 0 && req->offset + req->bytes > thres) { + return req->offset + req->bytes - thres; + } else { + return 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; + uint64_t thres = bdrv_write_threshold_get(bs); + + return treshold_overage(req, thres); } static int coroutine_fn before_write_notify(NotifierWithReturn *notifier, @@ -56,14 +60,14 @@ static int coroutine_fn before_write_notify(NotifierWithReturn *notifier, { BdrvTrackedRequest *req = opaque; BlockDriverState *bs = req->bs; - uint64_t amount = 0; + uint64_t thres = bdrv_write_threshold_get(bs); + uint64_t amount = treshold_overage(req, thres); - amount = bdrv_write_threshold_exceeded(bs, req); if (amount > 0) { qapi_event_send_block_write_threshold( bs->node_name, amount, - bs->write_threshold_offset); + thres); /* autodisable to avoid flooding the monitor */ write_threshold_disable(bs);