Re: [PATCH v2 1/8] block: prepare write threshold code for thread safety
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. -- Best regards, Vladimir
Re: [PATCH v2 1/8] block: prepare write threshold code for thread safety
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 signature.asc Description: PGP signature
Re: [PATCH v2 1/8] block: prepare write threshold code for thread safety
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
Co-developed-by: Paolo Bonzini
Signed-off-by: Emanuele Giuseppe Esposito
---
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
--
Best regards,
Vladimir
Re: [PATCH v2 1/8] block: prepare write threshold code for thread safety
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
> Co-developed-by: Paolo Bonzini
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> 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
>
signature.asc
Description: PGP signature
[PATCH v2 1/8] block: prepare write threshold code for thread safety
Reviewed-by: Stefan Hajnoczi
Co-developed-by: Paolo Bonzini
Signed-off-by: Emanuele Giuseppe Esposito
---
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);
}
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
