[PATCH v2 0/9] block: refactor write threshold

[PATCH v2 0/9] block: refactor write threshold

[Vladimir Sementsov-Ogievskiy]

Hi all!

This is a v2 for
"[PATCH] block: simplify write-threshold and drop write notifiers"
Supersedes: <20210421220950.105017-1-vsementsov@virtuozzo.com>

v2: split into several patches, improve thread-safety, more refactoring

Vladimir Sementsov-Ogievskiy (9):
  block/write-threshold: don't use write notifiers
  block: drop write notifiers
  test-write-threshold: rewrite test_threshold_(not_)trigger tests
  block/write-threshold: drop extra APIs
  block/write-threshold: don't use aio context lock
  test-write-threshold: drop extra tests
  test-write-threshold: drop extra TestStruct structure
  test-write-threshold: drop extra includes
  block/write-threshold: drop extra includes

 include/block/block_int.h         |  13 ----
 include/block/write-threshold.h   |  25 ++-----
 block.c                           |   1 -
 block/io.c                        |  11 +--
 block/write-threshold.c           | 110 +++++++-----------------------
 tests/unit/test-write-threshold.c |  91 ++----------------------
 6 files changed, 39 insertions(+), 212 deletions(-)

-- 
2.29.2

[PATCH v2 1/9] block/write-threshold: don't use write notifiers

[Vladimir Sementsov-Ogievskiy]

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(-)

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);
+    }
+}
-- 
2.29.2

Re: [PATCH v2 1/9] block/write-threshold: don't use write notifiers

[Max Reitz]

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>

> +    }
> +}
> 

Re: [PATCH v2 1/9] block/write-threshold: don't use write notifiers

[Vladimir Sementsov-Ogievskiy]

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!


-- 
Best regards,
Vladimir

Re: [PATCH v2 1/9] block/write-threshold: don't use write notifiers

[Vladimir Sementsov-Ogievskiy]

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

[Max Reitz]

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

[PATCH v2 2/9] block: drop write notifiers

[Vladimir Sementsov-Ogievskiy]

They are unused now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h | 12 ------------
 block.c                   |  1 -
 block/io.c                |  6 ------
 3 files changed, 19 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 51f51286a5..eab352f363 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -954,9 +954,6 @@ struct BlockDriverState {
      */
     int64_t total_sectors;
 
-    /* Callback before write request is processed */
-    NotifierWithReturnList before_write_notifiers;
-
     /* threshold limit for writes, in bytes. "High water mark". */
     uint64_t write_threshold_offset;
 
@@ -1083,15 +1080,6 @@ void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
 bool bdrv_backing_overridden(BlockDriverState *bs);
 
 
-/**
- * bdrv_add_before_write_notifier:
- *
- * Register a callback that is invoked before write requests are processed but
- * after any throttling or waiting for overlapping requests.
- */
-void bdrv_add_before_write_notifier(BlockDriverState *bs,
-                                    NotifierWithReturn *notifier);
-
 /**
  * bdrv_add_aio_context_notifier:
  *
diff --git a/block.c b/block.c
index 874c22c43e..e3c6c6ed93 100644
--- a/block.c
+++ b/block.c
@@ -401,7 +401,6 @@ BlockDriverState *bdrv_new(void)
     for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
         QLIST_INIT(&bs->op_blockers[i]);
     }
-    notifier_with_return_list_init(&bs->before_write_notifiers);
     qemu_co_mutex_init(&bs->reqs_lock);
     qemu_mutex_init(&bs->dirty_bitmap_mutex);
     bs->refcnt = 1;
diff --git a/block/io.c b/block/io.c
index 3520de51bb..1e826ba9e8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3165,12 +3165,6 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
     return true;
 }
 
-void bdrv_add_before_write_notifier(BlockDriverState *bs,
-                                    NotifierWithReturn *notifier)
-{
-    notifier_with_return_list_add(&bs->before_write_notifiers, notifier);
-}
-
 void bdrv_io_plug(BlockDriverState *bs)
 {
     BdrvChild *child;
-- 
2.29.2

Re: [PATCH v2 2/9] block: drop write notifiers

[Max Reitz]

On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
> They are unused now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/block_int.h | 12 ------------
>   block.c                   |  1 -
>   block/io.c                |  6 ------
>   3 files changed, 19 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH v2 3/9] test-write-threshold: rewrite test_threshold_(not_)trigger tests

[Vladimir Sementsov-Ogievskiy]

These tests use bdrv_write_threshold_exceeded() API, which is used only
for test. Better is testing real API, which is used in block.c as well.

So, let's call bdrv_write_threshold_check_write(), and check is
bs->write_threshold_offset cleared or not (it's cleared iff threshold
triggered).

Also we get rid of BdrvTrackedRequest use here. Tracked requests are
unrelated to write-threshold since we get rid of write notifiers.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/unit/test-write-threshold.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index fc1c45a2eb..fd40a815b8 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -55,41 +55,27 @@ static void test_threshold_multi_set_get(void)
 
 static void test_threshold_not_trigger(void)
 {
-    uint64_t amount = 0;
     uint64_t threshold = 4 * 1024 * 1024;
     BlockDriverState bs;
-    BdrvTrackedRequest req;
 
     memset(&bs, 0, sizeof(bs));
-    memset(&req, 0, sizeof(req));
-    req.offset = 1024;
-    req.bytes = 1024;
-
-    bdrv_check_request(req.offset, req.bytes, &error_abort);
 
     bdrv_write_threshold_set(&bs, threshold);
-    amount = bdrv_write_threshold_exceeded(&bs, &req);
-    g_assert_cmpuint(amount, ==, 0);
+    bdrv_write_threshold_check_write(&bs, 1024, 1024);
+    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, threshold);
 }
 
 
 static void test_threshold_trigger(void)
 {
-    uint64_t amount = 0;
     uint64_t threshold = 4 * 1024 * 1024;
     BlockDriverState bs;
-    BdrvTrackedRequest req;
 
     memset(&bs, 0, sizeof(bs));
-    memset(&req, 0, sizeof(req));
-    req.offset = (4 * 1024 * 1024) - 1024;
-    req.bytes = 2 * 1024;
-
-    bdrv_check_request(req.offset, req.bytes, &error_abort);
 
     bdrv_write_threshold_set(&bs, threshold);
-    amount = bdrv_write_threshold_exceeded(&bs, &req);
-    g_assert_cmpuint(amount, >=, 1024);
+    bdrv_write_threshold_check_write(&bs, threshold - 1024, 2 * 1024);
+    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, 0);
 }
 
 typedef struct TestStruct {
-- 
2.29.2

Re: [PATCH v2 3/9] test-write-threshold: rewrite test_threshold_(not_)trigger tests

[Max Reitz]

On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
> These tests use bdrv_write_threshold_exceeded() API, which is used only
> for test.

Well, now.  That used to be different before patch 1.

> Better is testing real API, which is used in block.c as well.
> 
> So, let's call bdrv_write_threshold_check_write(), and check is
> bs->write_threshold_offset cleared or not (it's cleared iff threshold
> triggered).
> 
> Also we get rid of BdrvTrackedRequest use here. Tracked requests are
> unrelated to write-threshold since we get rid of write notifiers.

The purpose behind the BdrvTrackedRequest was clearly because this is 
the object bdrv_write_threshold_exceeded() expected.  This reads like 
there was some other purpose (i.e. actually tracked requests), but there 
wasn’t.

The question that comes to my mind is why we had the 
bdrv_check_request() calls there, and why this patch removes them.  They 
seem unrelated to the write threshold, but someone must have thought 
something when adding them.

Looking into git blame, that someone is you :) (8b1170012b1)
Looks like you added those calls because BdrvTrackedRequest is a block 
layer structure, so getting rid of it means the reason for 
bdrv_check_request() disappears.  OK.

Reviewed-by: Max Reitz <mreitz@redhat.com>

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/unit/test-write-threshold.c | 22 ++++------------------
>   1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
> index fc1c45a2eb..fd40a815b8 100644
> --- a/tests/unit/test-write-threshold.c
> +++ b/tests/unit/test-write-threshold.c
> @@ -55,41 +55,27 @@ static void test_threshold_multi_set_get(void)
>   
>   static void test_threshold_not_trigger(void)
>   {
> -    uint64_t amount = 0;
>       uint64_t threshold = 4 * 1024 * 1024;
>       BlockDriverState bs;
> -    BdrvTrackedRequest req;
>   
>       memset(&bs, 0, sizeof(bs));
> -    memset(&req, 0, sizeof(req));
> -    req.offset = 1024;
> -    req.bytes = 1024;
> -
> -    bdrv_check_request(req.offset, req.bytes, &error_abort);
>   
>       bdrv_write_threshold_set(&bs, threshold);
> -    amount = bdrv_write_threshold_exceeded(&bs, &req);
> -    g_assert_cmpuint(amount, ==, 0);
> +    bdrv_write_threshold_check_write(&bs, 1024, 1024);
> +    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, threshold);
>   }
>   
>   
>   static void test_threshold_trigger(void)
>   {
> -    uint64_t amount = 0;
>       uint64_t threshold = 4 * 1024 * 1024;
>       BlockDriverState bs;
> -    BdrvTrackedRequest req;
>   
>       memset(&bs, 0, sizeof(bs));
> -    memset(&req, 0, sizeof(req));
> -    req.offset = (4 * 1024 * 1024) - 1024;
> -    req.bytes = 2 * 1024;
> -
> -    bdrv_check_request(req.offset, req.bytes, &error_abort);
>   
>       bdrv_write_threshold_set(&bs, threshold);
> -    amount = bdrv_write_threshold_exceeded(&bs, &req);
> -    g_assert_cmpuint(amount, >=, 1024);
> +    bdrv_write_threshold_check_write(&bs, threshold - 1024, 2 * 1024);
> +    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, 0);
>   }
>   
>   typedef struct TestStruct {
> 

Re: [PATCH v2 3/9] test-write-threshold: rewrite test_threshold_(not_)trigger tests

[Vladimir Sementsov-Ogievskiy]

05.05.2021 17:28, Max Reitz wrote:
> On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
>> These tests use bdrv_write_threshold_exceeded() API, which is used only
>> for test.
> 
> Well, now.  That used to be different before patch 1.
> 
>> Better is testing real API, which is used in block.c as well.
>>
>> So, let's call bdrv_write_threshold_check_write(), and check is
>> bs->write_threshold_offset cleared or not (it's cleared iff threshold
>> triggered).
>>
>> Also we get rid of BdrvTrackedRequest use here. Tracked requests are
>> unrelated to write-threshold since we get rid of write notifiers.
> 
> The purpose behind the BdrvTrackedRequest was clearly because this is the object bdrv_write_threshold_exceeded() expected.  This reads like there was some other purpose (i.e. actually tracked requests), but there wasn’t.
> 
> The question that comes to my mind is why we had the bdrv_check_request() calls there, and why this patch removes them.  They seem unrelated to the write threshold, but someone must have thought something when adding them.

I wanted to add a note for it but forget. Something like:

   "Such small requests are obviously good, no reason to check them" :)

> 
> Looking into git blame, that someone is you :) (8b1170012b1)

Oops:) When I read your "but someone must have thought something", I really doubt in it :) But that was me, and I really thought something. Respect for your punctuality!

> Looks like you added those calls because BdrvTrackedRequest is a block layer structure, so getting rid of it means the reason for bdrv_check_request() disappears.  OK.

Yes, I was worried about the fact that BdrvTrackedRequest is a public structure and is a potential backdoor for not-checked offset/bytes things. At some moment we'd better close it (hide structure and add some interfaces).

> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/unit/test-write-threshold.c | 22 ++++------------------
>>   1 file changed, 4 insertions(+), 18 deletions(-)
>>
>> diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
>> index fc1c45a2eb..fd40a815b8 100644
>> --- a/tests/unit/test-write-threshold.c
>> +++ b/tests/unit/test-write-threshold.c
>> @@ -55,41 +55,27 @@ static void test_threshold_multi_set_get(void)
>>   static void test_threshold_not_trigger(void)
>>   {
>> -    uint64_t amount = 0;
>>       uint64_t threshold = 4 * 1024 * 1024;
>>       BlockDriverState bs;
>> -    BdrvTrackedRequest req;
>>       memset(&bs, 0, sizeof(bs));
>> -    memset(&req, 0, sizeof(req));
>> -    req.offset = 1024;
>> -    req.bytes = 1024;
>> -
>> -    bdrv_check_request(req.offset, req.bytes, &error_abort);
>>       bdrv_write_threshold_set(&bs, threshold);
>> -    amount = bdrv_write_threshold_exceeded(&bs, &req);
>> -    g_assert_cmpuint(amount, ==, 0);
>> +    bdrv_write_threshold_check_write(&bs, 1024, 1024);
>> +    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, threshold);
>>   }
>>   static void test_threshold_trigger(void)
>>   {
>> -    uint64_t amount = 0;
>>       uint64_t threshold = 4 * 1024 * 1024;
>>       BlockDriverState bs;
>> -    BdrvTrackedRequest req;
>>       memset(&bs, 0, sizeof(bs));
>> -    memset(&req, 0, sizeof(req));
>> -    req.offset = (4 * 1024 * 1024) - 1024;
>> -    req.bytes = 2 * 1024;
>> -
>> -    bdrv_check_request(req.offset, req.bytes, &error_abort);
>>       bdrv_write_threshold_set(&bs, threshold);
>> -    amount = bdrv_write_threshold_exceeded(&bs, &req);
>> -    g_assert_cmpuint(amount, >=, 1024);
>> +    bdrv_write_threshold_check_write(&bs, threshold - 1024, 2 * 1024);
>> +    g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, 0);
>>   }
>>   typedef struct TestStruct {
>>
> 


-- 
Best regards,
Vladimir

[PATCH v2 4/9] block/write-threshold: drop extra APIs

[Vladimir Sementsov-Ogievskiy]

bdrv_write_threshold_exceeded() is unused at all.

bdrv_write_threshold_is_set() is used only to double check the value of
bs->write_threshold_offset in tests. No real sense in it (both tests do
check real value with help of bdrv_write_threshold_get())

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/write-threshold.h   | 24 ------------------------
 block/write-threshold.c           | 19 -------------------
 tests/unit/test-write-threshold.c |  4 ----
 3 files changed, 47 deletions(-)

diff --git a/include/block/write-threshold.h b/include/block/write-threshold.h
index 7942dcc368..072bc8f286 100644
--- a/include/block/write-threshold.h
+++ b/include/block/write-threshold.h
@@ -35,30 +35,6 @@ void bdrv_write_threshold_set(BlockDriverState *bs, uint64_t threshold_bytes);
  */
 uint64_t bdrv_write_threshold_get(const BlockDriverState *bs);
 
-/*
- * bdrv_write_threshold_is_set
- *
- * Tell if a write threshold is set for a given BDS.
- */
-bool bdrv_write_threshold_is_set(const BlockDriverState *bs);
-
-/*
- * bdrv_write_threshold_exceeded
- *
- * Return the extent of a write request that exceeded the threshold,
- * or zero if the request is below the threshold.
- * Return zero also if the threshold was not set.
- *
- * NOTE: here we assume the following holds for each request this code
- * deals with:
- *
- * assert((req->offset + req->bytes) <= UINT64_MAX)
- *
- * Please not there is *not* an actual C assert().
- */
-uint64_t bdrv_write_threshold_exceeded(const BlockDriverState *bs,
-                                       const BdrvTrackedRequest *req);
-
 /*
  * bdrv_write_threshold_check_write
  *
diff --git a/block/write-threshold.c b/block/write-threshold.c
index 11152c60df..ae54ee05dc 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -24,25 +24,6 @@ uint64_t bdrv_write_threshold_get(const BlockDriverState *bs)
     return bs->write_threshold_offset;
 }
 
-bool bdrv_write_threshold_is_set(const BlockDriverState *bs)
-{
-    return bs->write_threshold_offset > 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;
-}
-
 void bdrv_write_threshold_set(BlockDriverState *bs, uint64_t threshold_bytes)
 {
     bs->write_threshold_offset = threshold_bytes;
diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index fd40a815b8..bb5c1a5217 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -18,8 +18,6 @@ static void test_threshold_not_set_on_init(void)
     BlockDriverState bs;
     memset(&bs, 0, sizeof(bs));
 
-    g_assert(!bdrv_write_threshold_is_set(&bs));
-
     res = bdrv_write_threshold_get(&bs);
     g_assert_cmpint(res, ==, 0);
 }
@@ -33,8 +31,6 @@ static void test_threshold_set_get(void)
 
     bdrv_write_threshold_set(&bs, threshold);
 
-    g_assert(bdrv_write_threshold_is_set(&bs));
-
     res = bdrv_write_threshold_get(&bs);
     g_assert_cmpint(res, ==, threshold);
 }
-- 
2.29.2

Re: [PATCH v2 4/9] block/write-threshold: drop extra APIs

[Max Reitz]

On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_write_threshold_exceeded() is unused at all.
> 
> bdrv_write_threshold_is_set() is used only to double check the value of
> bs->write_threshold_offset in tests. No real sense in it (both tests do
> check real value with help of bdrv_write_threshold_get())

Well, depends on how one sees it.  One could also say that it neatly 
hides the fact that a threshold of 0 means disabled (i.e. the overloaded 
meaning of the write_threshold_offset attribute).

*shrug*

Reviewed-by: Max Reitz <mreitz@redhat.com>

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/write-threshold.h   | 24 ------------------------
>   block/write-threshold.c           | 19 -------------------
>   tests/unit/test-write-threshold.c |  4 ----
>   3 files changed, 47 deletions(-)

[PATCH v2 5/9] block/write-threshold: don't use aio context lock

[Vladimir Sementsov-Ogievskiy]

Instead of relying on aio context lock, let's make use of atomic
operations.

The tricky place is bdrv_write_threshold_check_write(): we want
atomically unset bs->write_threshold_offset iff
  offset + bytes > bs->write_threshold_offset
We don't have such atomic operation, so let's go in a loop:

1. fetch wtr atomically
2. if condition satisfied, try cmpxchg (if not satisfied, we are done,
   don't send event)
3. if cmpxchg succeeded, we are done (send event), else go to [1]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/write-threshold.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/block/write-threshold.c b/block/write-threshold.c
index ae54ee05dc..fbf4e6f5c4 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -2,6 +2,7 @@
  * QEMU System Emulator block write threshold notification
  *
  * Copyright Red Hat, Inc. 2014
+ * Copyright (c) 2021 Virtuozzo International GmbH.
  *
  * Authors:
  *  Francesco Romani <fromani@redhat.com>
@@ -21,43 +22,44 @@
 
 uint64_t bdrv_write_threshold_get(const BlockDriverState *bs)
 {
-    return bs->write_threshold_offset;
+    return qatomic_read(&bs->write_threshold_offset);
 }
 
 void bdrv_write_threshold_set(BlockDriverState *bs, uint64_t threshold_bytes)
 {
-    bs->write_threshold_offset = threshold_bytes;
+    qatomic_set(&bs->write_threshold_offset, threshold_bytes);
 }
 
 void qmp_block_set_write_threshold(const char *node_name,
                                    uint64_t threshold_bytes,
                                    Error **errp)
 {
-    BlockDriverState *bs;
-    AioContext *aio_context;
-
-    bs = bdrv_find_node(node_name);
+    BlockDriverState *bs = bdrv_find_node(node_name);
     if (!bs) {
         error_setg(errp, "Device '%s' not found", node_name);
         return;
     }
 
-    aio_context = bdrv_get_aio_context(bs);
-    aio_context_acquire(aio_context);
-
     bdrv_write_threshold_set(bs, threshold_bytes);
-
-    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;
+    uint64_t wtr;
 
-    if (wtr > 0 && end > wtr) {
-        qapi_event_send_block_write_threshold(bs->node_name, end - wtr, wtr);
-        bdrv_write_threshold_set(bs, 0);
+retry:
+    wtr = bdrv_write_threshold_get(bs);
+    if (wtr == 0 || wtr >= end) {
+        return;
     }
+
+    if (qatomic_cmpxchg(&bs->write_threshold_offset, wtr, 0) != wtr) {
+        /* bs->write_threshold_offset changed in parallel */
+        goto retry;
+    }
+
+    /* We have cleared bs->write_threshold_offset, so let's send event */
+    qapi_event_send_block_write_threshold(bs->node_name, end - wtr, wtr);
 }
-- 
2.29.2

Re: [PATCH v2 5/9] block/write-threshold: don't use aio context lock

[Max Reitz]

On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
> Instead of relying on aio context lock, let's make use of atomic
> operations.
> 
> The tricky place is bdrv_write_threshold_check_write(): we want
> atomically unset bs->write_threshold_offset iff
>    offset + bytes > bs->write_threshold_offset
> We don't have such atomic operation, so let's go in a loop:

This could also be solved by untangling the overloaded meaning of 
write_threshold_offset – if we had an additional boolean 
write_threshold_check, then this wouldn’t be a problem, and we could do 
this:

if (end > bdrv_write_threshold_get(bs)) {
     if (qatomic_xchg(&bs->write_threshold_check, false) == true) {
         qapi_event_send(...);
     }
}

However, the problem then becomes thinking about the memory access 
semantics, because if some other thread sets the threshold offset and 
enables the threshold check, we need to ensure that we see the updated 
offset here...

So I suppose your loop is simpler then.

> 1. fetch wtr atomically
> 2. if condition satisfied, try cmpxchg (if not satisfied, we are done,
>     don't send event)
> 3. if cmpxchg succeeded, we are done (send event), else go to [1]
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/write-threshold.c | 32 +++++++++++++++++---------------
>   1 file changed, 17 insertions(+), 15 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH v2 6/9] test-write-threshold: drop extra tests

[Vladimir Sementsov-Ogievskiy]

Testing set/get of one 64bit variable doesn't seem necessary. We have a
lot of such variables. Also remaining tests do test set/get anyway.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/unit/test-write-threshold.c | 43 -------------------------------
 1 file changed, 43 deletions(-)

diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index bb5c1a5217..9e9986aefc 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -12,43 +12,6 @@
 #include "block/write-threshold.h"
 
 
-static void test_threshold_not_set_on_init(void)
-{
-    uint64_t res;
-    BlockDriverState bs;
-    memset(&bs, 0, sizeof(bs));
-
-    res = bdrv_write_threshold_get(&bs);
-    g_assert_cmpint(res, ==, 0);
-}
-
-static void test_threshold_set_get(void)
-{
-    uint64_t threshold = 4 * 1024 * 1024;
-    uint64_t res;
-    BlockDriverState bs;
-    memset(&bs, 0, sizeof(bs));
-
-    bdrv_write_threshold_set(&bs, threshold);
-
-    res = bdrv_write_threshold_get(&bs);
-    g_assert_cmpint(res, ==, threshold);
-}
-
-static void test_threshold_multi_set_get(void)
-{
-    uint64_t threshold1 = 4 * 1024 * 1024;
-    uint64_t threshold2 = 15 * 1024 * 1024;
-    uint64_t res;
-    BlockDriverState bs;
-    memset(&bs, 0, sizeof(bs));
-
-    bdrv_write_threshold_set(&bs, threshold1);
-    bdrv_write_threshold_set(&bs, threshold2);
-    res = bdrv_write_threshold_get(&bs);
-    g_assert_cmpint(res, ==, threshold2);
-}
-
 static void test_threshold_not_trigger(void)
 {
     uint64_t threshold = 4 * 1024 * 1024;
@@ -84,12 +47,6 @@ int main(int argc, char **argv)
 {
     size_t i;
     TestStruct tests[] = {
-        { "/write-threshold/not-set-on-init",
-          test_threshold_not_set_on_init },
-        { "/write-threshold/set-get",
-          test_threshold_set_get },
-        { "/write-threshold/multi-set-get",
-          test_threshold_multi_set_get },
         { "/write-threshold/not-trigger",
           test_threshold_not_trigger },
         { "/write-threshold/trigger",
-- 
2.29.2

Re: [PATCH v2 6/9] test-write-threshold: drop extra tests

[Max Reitz]

On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
> Testing set/get of one 64bit variable doesn't seem necessary. We have a
> lot of such variables. Also remaining tests do test set/get anyway.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/unit/test-write-threshold.c | 43 -------------------------------
>   1 file changed, 43 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH v2 7/9] test-write-threshold: drop extra TestStruct structure

[Vladimir Sementsov-Ogievskiy]

We don't need this extra logic: it doesn't make code simpler.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/unit/test-write-threshold.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index 9e9986aefc..49b1ef7a20 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -37,26 +37,12 @@ static void test_threshold_trigger(void)
     g_assert_cmpuint(bdrv_write_threshold_get(&bs), ==, 0);
 }
 
-typedef struct TestStruct {
-    const char *name;
-    void (*func)(void);
-} TestStruct;
-
 
 int main(int argc, char **argv)
 {
-    size_t i;
-    TestStruct tests[] = {
-        { "/write-threshold/not-trigger",
-          test_threshold_not_trigger },
-        { "/write-threshold/trigger",
-          test_threshold_trigger },
-        { NULL, NULL }
-    };
-
     g_test_init(&argc, &argv, NULL);
-    for (i = 0; tests[i].name != NULL; i++) {
-        g_test_add_func(tests[i].name, tests[i].func);
-    }
+    g_test_add_func("/write-threshold/not-trigger", test_threshold_not_trigger);
+    g_test_add_func("/write-threshold/trigger", test_threshold_trigger);
+
     return g_test_run();
 }
-- 
2.29.2

Re: [PATCH v2 7/9] test-write-threshold: drop extra TestStruct structure

[Max Reitz]

On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
> We don't need this extra logic: it doesn't make code simpler.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/unit/test-write-threshold.c | 20 +++-----------------
>   1 file changed, 3 insertions(+), 17 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH v2 8/9] test-write-threshold: drop extra includes

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/unit/test-write-threshold.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tests/unit/test-write-threshold.c b/tests/unit/test-write-threshold.c
index 49b1ef7a20..761054eab2 100644
--- a/tests/unit/test-write-threshold.c
+++ b/tests/unit/test-write-threshold.c
@@ -7,8 +7,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qapi/error.h"
-#include "block/block_int.h"
 #include "block/write-threshold.h"
 
 
-- 
2.29.2

Re: [PATCH v2 8/9] test-write-threshold: drop extra includes

[Max Reitz]

On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/unit/test-write-threshold.c | 2 --
>   1 file changed, 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH v2 9/9] block/write-threshold: drop extra includes

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/write-threshold.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block/write-threshold.c b/block/write-threshold.c
index fbf4e6f5c4..db271c5537 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -12,10 +12,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "block/block_int.h"
-#include "qemu/coroutine.h"
 #include "block/write-threshold.h"
-#include "qemu/notify.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-block-core.h"
 #include "qapi/qapi-events-block-core.h"
-- 
2.29.2

Re: [PATCH v2 9/9] block/write-threshold: drop extra includes

[Max Reitz]

On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/write-threshold.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/block/write-threshold.c b/block/write-threshold.c
> index fbf4e6f5c4..db271c5537 100644
> --- a/block/write-threshold.c
> +++ b/block/write-threshold.c
> @@ -12,10 +12,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> -#include "block/block_int.h"

We need this (for bs->write_threshold_offset), but it’s included through 
block/block_int.h.  I’m not sure whether we should drop it from the 
includes.

Perhaps we should instead drop block_int.h from write-threshold.h? 
Shouldn’t including qemu/osdep.h (which includes qemu/typedefs.h, which 
forward-declares BDS) be sufficient there?

> -#include "qemu/coroutine.h"
>   #include "block/write-threshold.h"
> -#include "qemu/notify.h"
>   #include "qapi/error.h"
>   #include "qapi/qapi-commands-block-core.h"
>   #include "qapi/qapi-events-block-core.h"

Btw, where does qemu/atomic.h come in?  Looks like it comes through 
block_int.h.  I think we should include it explicitly.

Max

Re: [PATCH v2 9/9] block/write-threshold: drop extra includes

[Vladimir Sementsov-Ogievskiy]

05.05.2021 19:23, Max Reitz wrote:
> On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/write-threshold.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/block/write-threshold.c b/block/write-threshold.c
>> index fbf4e6f5c4..db271c5537 100644
>> --- a/block/write-threshold.c
>> +++ b/block/write-threshold.c
>> @@ -12,10 +12,7 @@
>>    */
>>   #include "qemu/osdep.h"
>> -#include "block/block_int.h"
> 
> We need this (for bs->write_threshold_offset), but it’s included through block/block_int.h.  I’m not sure whether we should drop it from the includes.
> 
> Perhaps we should instead drop block_int.h from write-threshold.h? Shouldn’t including qemu/osdep.h (which includes qemu/typedefs.h, which forward-declares BDS) be sufficient there?
> 
>> -#include "qemu/coroutine.h"
>>   #include "block/write-threshold.h"
>> -#include "qemu/notify.h"
>>   #include "qapi/error.h"
>>   #include "qapi/qapi-commands-block-core.h"
>>   #include "qapi/qapi-events-block-core.h"
> 
> Btw, where does qemu/atomic.h come in?  Looks like it comes through block_int.h.  I think we should include it explicitly.
> 

I'm not sure. If something is included through another include, why to include it explicitly?

For me, if statement can be removed with no effect, it's an extra statement.


-- 
Best regards,
Vladimir

Re: [PATCH v2 9/9] block/write-threshold: drop extra includes

[Max Reitz]

On 05.05.21 22:34, Vladimir Sementsov-Ogievskiy wrote:
> 05.05.2021 19:23, Max Reitz wrote:
>> On 04.05.21 10:25, Vladimir Sementsov-Ogievskiy wrote:
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/write-threshold.c | 3 ---
>>>   1 file changed, 3 deletions(-)
>>>
>>> diff --git a/block/write-threshold.c b/block/write-threshold.c
>>> index fbf4e6f5c4..db271c5537 100644
>>> --- a/block/write-threshold.c
>>> +++ b/block/write-threshold.c
>>> @@ -12,10 +12,7 @@
>>>    */
>>>   #include "qemu/osdep.h"
>>> -#include "block/block_int.h"
>>
>> We need this (for bs->write_threshold_offset), but it’s included 
>> through block/block_int.h.  I’m not sure whether we should drop it 
>> from the includes.
>>
>> Perhaps we should instead drop block_int.h from write-threshold.h? 
>> Shouldn’t including qemu/osdep.h (which includes qemu/typedefs.h, 
>> which forward-declares BDS) be sufficient there?
>>
>>> -#include "qemu/coroutine.h"
>>>   #include "block/write-threshold.h"
>>> -#include "qemu/notify.h"
>>>   #include "qapi/error.h"
>>>   #include "qapi/qapi-commands-block-core.h"
>>>   #include "qapi/qapi-events-block-core.h"
>>
>> Btw, where does qemu/atomic.h come in?  Looks like it comes through 
>> block_int.h.  I think we should include it explicitly.
>>
> 
> I'm not sure. If something is included through another include, why to 
> include it explicitly?

Because the other include may change.  I’d include something if you need 
the feature, and we need atomics here.

And block_int.h doesn’t even provide atomic.h, it comes through various 
of its includes (which are probably not included to provide atomics). 
So this is already indirect and basically just incidental; block_int.h 
doesn’t guarantee atomic.h.

Another thing: I see that other fields in BDS that are to be accessed 
with atomic ops have a comment saying so.  I think 
write_threshold_offset should have, too.

Max

> For me, if statement can be removed with no effect, it's an extra 
> statement.
> 
>