diff mbox series

[v2,04/20] block/block-copy: More explicit call_state

Message ID 20200601181118.579-5-vsementsov@virtuozzo.com
State New
Headers show
Series backup performance: block_status + async | expand

Commit Message

Vladimir Sementsov-Ogievskiy June 1, 2020, 6:11 p.m. UTC 
Refactor common path to use BlockCopyCallState pointer as parameter, to
prepare it for use in asynchronous block-copy (at least, we'll need to
run block-copy in a coroutine, passing the whole parameters as one
pointer).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/block-copy.c | 51 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 13 deletions(-)

Comments

Max Reitz July 17, 2020, 1:45 p.m. UTC | #1 
On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> Refactor common path to use BlockCopyCallState pointer as parameter, to
> prepare it for use in asynchronous block-copy (at least, we'll need to
> run block-copy in a coroutine, passing the whole parameters as one
> pointer).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 51 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 43a018d190..75882a094c 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c

[...]

> @@ -646,16 +653,16 @@ out:
>   * it means that some I/O operation failed in context of _this_ block_copy call,
>   * not some parallel operation.
>   */
> -int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
> -                            bool *error_is_read)
> +static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
>  {
>      int ret;
>  
>      do {
> -        ret = block_copy_dirty_clusters(s, offset, bytes, error_is_read);
> +        ret = block_copy_dirty_clusters(call_state);

It’s possible that much of this code will change in a future patch of
this series, but as it is, it might be nice to make
block_copy_dirty_clusters’s argument a const pointer so it’s clear that
the call to block_copy_wait_one() below will use the original @offset
and @bytes values.

*shrug*

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

>  
>          if (ret == 0) {
> -            ret = block_copy_wait_one(s, offset, bytes);
> +            ret = block_copy_wait_one(call_state->s, call_state->offset,
> +                                      call_state->bytes);
>          }
>  
>          /*
Vladimir Sementsov-Ogievskiy Sept. 18, 2020, 8:11 p.m. UTC | #2 
17.07.2020 16:45, Max Reitz wrote:
> On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
>> Refactor common path to use BlockCopyCallState pointer as parameter, to
>> prepare it for use in asynchronous block-copy (at least, we'll need to
>> run block-copy in a coroutine, passing the whole parameters as one
>> pointer).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/block-copy.c | 51 ++++++++++++++++++++++++++++++++++------------
>>   1 file changed, 38 insertions(+), 13 deletions(-)
>>
>> diff --git a/block/block-copy.c b/block/block-copy.c
>> index 43a018d190..75882a094c 100644
>> --- a/block/block-copy.c
>> +++ b/block/block-copy.c
> 
> [...]
> 
>> @@ -646,16 +653,16 @@ out:
>>    * it means that some I/O operation failed in context of _this_ block_copy call,
>>    * not some parallel operation.
>>    */
>> -int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
>> -                            bool *error_is_read)
>> +static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
>>   {
>>       int ret;
>>   
>>       do {
>> -        ret = block_copy_dirty_clusters(s, offset, bytes, error_is_read);
>> +        ret = block_copy_dirty_clusters(call_state);
> 
> It’s possible that much of this code will change in a future patch of
> this series, but as it is, it might be nice to make
> block_copy_dirty_clusters’s argument a const pointer so it’s clear that
> the call to block_copy_wait_one() below will use the original @offset
> and @bytes values.

Hm. I'm trying this, and it doesn't work:

block_copy_task_entry() wants to change call_state:

    t->call_state->failed = true;

> 
> *shrug*
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
>>   
>>           if (ret == 0) {
>> -            ret = block_copy_wait_one(s, offset, bytes);
>> +            ret = block_copy_wait_one(call_state->s, call_state->offset,
>> +                                      call_state->bytes);
>>           }
>>   
>>           /*
>
Max Reitz Sept. 21, 2020, 8:54 a.m. UTC | #3 
On 18.09.20 22:11, Vladimir Sementsov-Ogievskiy wrote:
> 17.07.2020 16:45, Max Reitz wrote:
>> On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
>>> Refactor common path to use BlockCopyCallState pointer as parameter, to
>>> prepare it for use in asynchronous block-copy (at least, we'll need to
>>> run block-copy in a coroutine, passing the whole parameters as one
>>> pointer).
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/block-copy.c | 51 ++++++++++++++++++++++++++++++++++------------
>>>   1 file changed, 38 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>> index 43a018d190..75882a094c 100644
>>> --- a/block/block-copy.c
>>> +++ b/block/block-copy.c
>>
>> [...]
>>
>>> @@ -646,16 +653,16 @@ out:
>>>    * it means that some I/O operation failed in context of _this_
>>> block_copy call,
>>>    * not some parallel operation.
>>>    */
>>> -int coroutine_fn block_copy(BlockCopyState *s, int64_t offset,
>>> int64_t bytes,
>>> -                            bool *error_is_read)
>>> +static int coroutine_fn block_copy_common(BlockCopyCallState
>>> *call_state)
>>>   {
>>>       int ret;
>>>         do {
>>> -        ret = block_copy_dirty_clusters(s, offset, bytes,
>>> error_is_read);
>>> +        ret = block_copy_dirty_clusters(call_state);
>>
>> It’s possible that much of this code will change in a future patch of
>> this series, but as it is, it might be nice to make
>> block_copy_dirty_clusters’s argument a const pointer so it’s clear that
>> the call to block_copy_wait_one() below will use the original @offset
>> and @bytes values.
> 
> Hm. I'm trying this, and it doesn't work:
> 
> block_copy_task_entry() wants to change call_state:
> 
>    t->call_state->failed = true;

Too bad :)

>> *shrug*
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>>>             if (ret == 0) {
>>> -            ret = block_copy_wait_one(s, offset, bytes);
>>> +            ret = block_copy_wait_one(call_state->s,
>>> call_state->offset,
>>> +                                      call_state->bytes);
>>>           }
>>>             /*
>>
> 
>
diff mbox series

Patch

diff --git a/block/block-copy.c b/block/block-copy.c
index 43a018d190..75882a094c 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -30,7 +30,15 @@ 
 static coroutine_fn int block_copy_task_entry(AioTask *task);
 
 typedef struct BlockCopyCallState {
+    /* IN parameters */
+    BlockCopyState *s;
+    int64_t offset;
+    int64_t bytes;
+
+    /* State */
     bool failed;
+
+    /* OUT parameters */
     bool error_is_read;
 } BlockCopyCallState;
 
@@ -541,15 +549,17 @@  int64_t block_copy_reset_unallocated(BlockCopyState *s,
  * Returns 1 if dirty clusters found and successfully copied, 0 if no dirty
  * clusters found and -errno on failure.
  */
-static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
-                                                  int64_t offset, int64_t bytes,
-                                                  bool *error_is_read)
+static int coroutine_fn
+block_copy_dirty_clusters(BlockCopyCallState *call_state)
 {
+    BlockCopyState *s = call_state->s;
+    int64_t offset = call_state->offset;
+    int64_t bytes = call_state->bytes;
+
     int ret = 0;
     bool found_dirty = false;
     int64_t end = offset + bytes;
     AioTaskPool *aio = NULL;
-    BlockCopyCallState call_state = {false, false};
 
     /*
      * block_copy() user is responsible for keeping source and target in same
@@ -565,7 +575,7 @@  static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
         BlockCopyTask *task;
         int64_t status_bytes;
 
-        task = block_copy_task_create(s, &call_state, offset, bytes);
+        task = block_copy_task_create(s, call_state, offset, bytes);
         if (!task) {
             /* No more dirty bits in the bitmap */
             trace_block_copy_skip_range(s, offset, bytes);
@@ -630,15 +640,12 @@  out:
 
         aio_task_pool_free(aio);
     }
-    if (error_is_read && ret < 0) {
-        *error_is_read = call_state.error_is_read;
-    }
 
     return ret < 0 ? ret : found_dirty;
 }
 
 /*
- * block_copy
+ * block_copy_common
  *
  * Copy requested region, accordingly to dirty bitmap.
  * Collaborate with parallel block_copy requests: if they succeed it will help
@@ -646,16 +653,16 @@  out:
  * it means that some I/O operation failed in context of _this_ block_copy call,
  * not some parallel operation.
  */
-int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
-                            bool *error_is_read)
+static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
 {
     int ret;
 
     do {
-        ret = block_copy_dirty_clusters(s, offset, bytes, error_is_read);
+        ret = block_copy_dirty_clusters(call_state);
 
         if (ret == 0) {
-            ret = block_copy_wait_one(s, offset, bytes);
+            ret = block_copy_wait_one(call_state->s, call_state->offset,
+                                      call_state->bytes);
         }
 
         /*
@@ -672,6 +679,24 @@  int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
     return ret;
 }
 
+int coroutine_fn block_copy(BlockCopyState *s, int64_t start, int64_t bytes,
+                            bool *error_is_read)
+{
+    BlockCopyCallState call_state = {
+        .s = s,
+        .offset = start,
+        .bytes = bytes,
+    };
+
+    int ret = block_copy_common(&call_state);
+
+    if (error_is_read && ret < 0) {
+        *error_is_read = call_state.error_is_read;
+    }
+
+    return ret;
+}
+
 BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s)
 {
     return s->copy_bitmap;