diff mbox series

[v3,3/5] block: refactor error handling of commit_iteration

Message ID 20240901142405.3183874-4-libvirt-e6954efa@volkihar.be (mailing list archive)
State New, archived
Headers show
Series block: allow commit to unmap zero blocks | expand

Commit Message

Vincent Vanlaer Sept. 1, 2024, 2:24 p.m. UTC 
Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
---
 block/commit.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Sept. 30, 2024, 2:11 p.m. UTC | #1 
On 01.09.24 17:24, Vincent Vanlaer wrote:
> Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
> ---
>   block/commit.c | 37 ++++++++++++++++++++++---------------
>   1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 9eedd1fa47..288e413be3 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -130,7 +130,6 @@ static void commit_clean(Job *job)
>   
>   static int commit_iteration(CommitBlockJob *s, int64_t offset, int64_t *n, void *buf) {
>       int ret = 0;
> -    bool copy;
>       bool error_in_source = true;
>   
>       /* Copy if allocated above the base */
> @@ -140,19 +139,34 @@ static int commit_iteration(CommitBlockJob *s, int64_t offset, int64_t *n, void
>               n, NULL, NULL, NULL);
>       }
>   
> -    copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
> +    if (ret < 0) {
> +        goto handle_error;
> +    }
> +
>       trace_commit_one_iteration(s, offset, *n, ret);

this way we loose trace point for error case. Let's move trace_... above "if (ret.."

> -    if (copy) {
> +
> +    if (ret & BDRV_BLOCK_ALLOCATED) {
>           assert(*n < SIZE_MAX);
>   
>           ret = blk_co_pread(s->top, offset, *n, buf, 0);
> -        if (ret >= 0) {
> -            ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
> -            if (ret < 0) {
> -                error_in_source = false;
> -            }
> +        if (ret < 0) {
> +            goto handle_error;
>           }
> +
> +        ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
> +        if (ret < 0) {
> +            error_in_source = false;
> +            goto handle_error;
> +        }
> +
> +        block_job_ratelimit_processed_bytes(&s->common, *n);
>       }
> +
> +    /* Publish progress */
> +
> +    job_progress_update(&s->common.job, *n);
> +

it looks a bit strange to fallthrough to "handle_error" from success path


I suggest:

@@ -166,17 +166,17 @@ static int commit_iteration(CommitBlockJob *s, int64_t offset, int64_t *n, void
  
      job_progress_update(&s->common.job, *n);
  
-handle_error:
-    if (ret < 0) {
-        BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
-                                                         error_in_source, -ret);
-        if (action == BLOCK_ERROR_ACTION_REPORT) {
-            return ret;
-        } else {
-            *n = 0;
-        }
+    return 0;
+
+fail:
+    BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
+                                                     error_in_source, -ret);
+    if (action == BLOCK_ERROR_ACTION_REPORT) {
+        return ret;
      }
  
+    /* Retry iteration */
+    *n = 0;
      return 0;
  }



(also, "fail" name is more popular for such labels:
$ git grep 'goto fail' | wc -l
1334
)



> +handle_error:
>       if (ret < 0) {
>           BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
>                                                            error_in_source, -ret);
> @@ -160,15 +174,8 @@ static int commit_iteration(CommitBlockJob *s, int64_t offset, int64_t *n, void
>               return ret;
>           } else {
>               *n = 0;
> -            return 0;
>           }
>       }
> -    /* Publish progress */
> -    job_progress_update(&s->common.job, *n);
> -
> -    if (copy) {
> -        block_job_ratelimit_processed_bytes(&s->common, *n);
> -    }
>   
>       return 0;
>   }
diff mbox series

Patch

diff --git a/block/commit.c b/block/commit.c
index 9eedd1fa47..288e413be3 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -130,7 +130,6 @@  static void commit_clean(Job *job)
 
 static int commit_iteration(CommitBlockJob *s, int64_t offset, int64_t *n, void *buf) {
     int ret = 0;
-    bool copy;
     bool error_in_source = true;
 
     /* Copy if allocated above the base */
@@ -140,19 +139,34 @@  static int commit_iteration(CommitBlockJob *s, int64_t offset, int64_t *n, void
             n, NULL, NULL, NULL);
     }
 
-    copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
+    if (ret < 0) {
+        goto handle_error;
+    }
+
     trace_commit_one_iteration(s, offset, *n, ret);
-    if (copy) {
+
+    if (ret & BDRV_BLOCK_ALLOCATED) {
         assert(*n < SIZE_MAX);
 
         ret = blk_co_pread(s->top, offset, *n, buf, 0);
-        if (ret >= 0) {
-            ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
-            if (ret < 0) {
-                error_in_source = false;
-            }
+        if (ret < 0) {
+            goto handle_error;
         }
+
+        ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
+        if (ret < 0) {
+            error_in_source = false;
+            goto handle_error;
+        }
+
+        block_job_ratelimit_processed_bytes(&s->common, *n);
     }
+
+    /* Publish progress */
+
+    job_progress_update(&s->common.job, *n);
+
+handle_error:
     if (ret < 0) {
         BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
                                                          error_in_source, -ret);
@@ -160,15 +174,8 @@  static int commit_iteration(CommitBlockJob *s, int64_t offset, int64_t *n, void
             return ret;
         } else {
             *n = 0;
-            return 0;
         }
     }
-    /* Publish progress */
-    job_progress_update(&s->common.job, *n);
-
-    if (copy) {
-        block_job_ratelimit_processed_bytes(&s->common, *n);
-    }
 
     return 0;
 }