diff mbox series

[2/9] parallels: Move check of cluster outside image to a separate function

Message ID 20220808120734.1168314-3-alexander.ivanov@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series parallels: Refactor the code of images checks and fix a bug | expand

Commit Message

Alexander Ivanov Aug. 8, 2022, 12:07 p.m. UTC 
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 76 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 53 insertions(+), 23 deletions(-)

Comments

Denis V. Lunev Aug. 8, 2022, 12:19 p.m. UTC | #1 
On 08.08.2022 14:07, Alexander Ivanov wrote:
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 76 +++++++++++++++++++++++++++++++++--------------
>   1 file changed, 53 insertions(+), 23 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 108aa907b8..7b400ecdcc 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -413,6 +413,13 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
>       return ret;
>   }
>   
> +static void parallels_set_bat_entry(BDRVParallelsState *s,
> +                                    uint32_t index, uint32_t offset)
> +{
> +    s->bat_bitmap[index] = offset;
> +    bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 1);
> +}
> +
>   static void parallels_check_unclean(BlockDriverState *bs,
>                                       BdrvCheckResult *res,
>                                       BdrvCheckMode fix)

This helper seems unrelated to the patch subject and should be done 
separately.

> @@ -431,6 +438,47 @@ static void parallels_check_unclean(BlockDriverState *bs,
>       }
>   }
>   
> +static int parallels_check_outside_image(BlockDriverState *bs,
> +                                         BdrvCheckResult *res,
> +                                         BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    uint32_t i;
> +    int64_t off, size;
> +    int ret;
> +    bool flush_bat = false;
> +
> +    size = bdrv_getlength(bs->file->bs);
> +    if (size < 0) {
> +        res->check_errors++;
> +        return size;
> +    }
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (off > size) {
> +            fprintf(stderr, "%s cluster %u is outside image\n",
> +                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> +            res->corruptions++;
> +            if (fix & BDRV_FIX_ERRORS) {
> +                parallels_set_bat_entry(s, i, 0);
> +                res->corruptions_fixed++;
> +                flush_bat = true;
> +            }
> +        }
> +    }
> +
> +    if (flush_bat) {
> +        ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
> +        if (ret < 0) {
> +            res->check_errors++;
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>   static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>                                              BdrvCheckResult *res,
>                                              BdrvCheckMode fix)
> @@ -439,7 +487,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       int64_t size, prev_off, high_off;
>       int ret;
>       uint32_t i;
> -    bool flush_bat = false;
>   
>       size = bdrv_getlength(bs->file->bs);
>       if (size < 0) {
> @@ -451,6 +498,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>   
>       parallels_check_unclean(bs, res, fix);
>   
> +    ret = parallels_check_outside_image(bs, res, fix);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
>       res->bfi.total_clusters = s->bat_size;
>       res->bfi.compressed_clusters = 0; /* compression is not supported */
>   
> @@ -463,20 +515,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>               continue;
>           }
>   
> -        /* cluster outside the image */
> -        if (off > size) {
> -            fprintf(stderr, "%s cluster %u is outside image\n",
> -                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> -            res->corruptions++;
> -            if (fix & BDRV_FIX_ERRORS) {
> -                prev_off = 0;
> -                s->bat_bitmap[i] = 0;
> -                res->corruptions_fixed++;
> -                flush_bat = true;
> -                continue;
> -            }
> -        }
> -
>           res->bfi.allocated_clusters++;
>           if (off > high_off) {
>               high_off = off;
> @@ -489,14 +527,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       }
>   
>       ret = 0;
> -    if (flush_bat) {
> -        ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
> -        if (ret < 0) {
> -            res->check_errors++;
> -            goto out;
> -        }
> -    }
> -
>       res->image_end_offset = high_off + s->cluster_size;
>       if (size > res->image_end_offset) {
>           int64_t count;
Denis V. Lunev Aug. 8, 2022, 12:31 p.m. UTC | #2 
On 08.08.2022 14:19, Denis V. Lunev wrote:
> On 08.08.2022 14:07, Alexander Ivanov wrote:
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>>   block/parallels.c | 76 +++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 53 insertions(+), 23 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 108aa907b8..7b400ecdcc 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -413,6 +413,13 @@ static coroutine_fn int 
>> parallels_co_readv(BlockDriverState *bs,
>>       return ret;
>>   }
>>   +static void parallels_set_bat_entry(BDRVParallelsState *s,
>> +                                    uint32_t index, uint32_t offset)
>> +{
>> +    s->bat_bitmap[index] = offset;
>> +    bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / 
>> s->bat_dirty_block, 1);
>> +}
>> +
>>   static void parallels_check_unclean(BlockDriverState *bs,
>>                                       BdrvCheckResult *res,
>>                                       BdrvCheckMode fix)
>
> This helper seems unrelated to the patch subject and should be done 
> separately.

more specifically - you have created the helper BUT you have not used
it at ALL places where we update BAT. That is seriously wrong from the
code completness POW. If you have getter - you MUST use it everywhere,
even within allocate_clusters().

Normally you create helper, f.e. refactoring code of allocate cluster 
promising
that the code will be reused later (non-functional changes) and after that
in the separate patch you spread its usage refactoring other code, f.e.
to avoid flush_bat tracking.
diff mbox series

Patch

diff --git a/block/parallels.c b/block/parallels.c
index 108aa907b8..7b400ecdcc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -413,6 +413,13 @@  static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
     return ret;
 }
 
+static void parallels_set_bat_entry(BDRVParallelsState *s,
+                                    uint32_t index, uint32_t offset)
+{
+    s->bat_bitmap[index] = offset;
+    bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 1);
+}
+
 static void parallels_check_unclean(BlockDriverState *bs,
                                     BdrvCheckResult *res,
                                     BdrvCheckMode fix)
@@ -431,6 +438,47 @@  static void parallels_check_unclean(BlockDriverState *bs,
     }
 }
 
+static int parallels_check_outside_image(BlockDriverState *bs,
+                                         BdrvCheckResult *res,
+                                         BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint32_t i;
+    int64_t off, size;
+    int ret;
+    bool flush_bat = false;
+
+    size = bdrv_getlength(bs->file->bs);
+    if (size < 0) {
+        res->check_errors++;
+        return size;
+    }
+
+    for (i = 0; i < s->bat_size; i++) {
+        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off > size) {
+            fprintf(stderr, "%s cluster %u is outside image\n",
+                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+            res->corruptions++;
+            if (fix & BDRV_FIX_ERRORS) {
+                parallels_set_bat_entry(s, i, 0);
+                res->corruptions_fixed++;
+                flush_bat = true;
+            }
+        }
+    }
+
+    if (flush_bat) {
+        ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
+        if (ret < 0) {
+            res->check_errors++;
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
 static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                                            BdrvCheckResult *res,
                                            BdrvCheckMode fix)
@@ -439,7 +487,6 @@  static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     int64_t size, prev_off, high_off;
     int ret;
     uint32_t i;
-    bool flush_bat = false;
 
     size = bdrv_getlength(bs->file->bs);
     if (size < 0) {
@@ -451,6 +498,11 @@  static int coroutine_fn parallels_co_check(BlockDriverState *bs,
 
     parallels_check_unclean(bs, res, fix);
 
+    ret = parallels_check_outside_image(bs, res, fix);
+    if (ret < 0) {
+        goto out;
+    }
+
     res->bfi.total_clusters = s->bat_size;
     res->bfi.compressed_clusters = 0; /* compression is not supported */
 
@@ -463,20 +515,6 @@  static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             continue;
         }
 
-        /* cluster outside the image */
-        if (off > size) {
-            fprintf(stderr, "%s cluster %u is outside image\n",
-                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
-            res->corruptions++;
-            if (fix & BDRV_FIX_ERRORS) {
-                prev_off = 0;
-                s->bat_bitmap[i] = 0;
-                res->corruptions_fixed++;
-                flush_bat = true;
-                continue;
-            }
-        }
-
         res->bfi.allocated_clusters++;
         if (off > high_off) {
             high_off = off;
@@ -489,14 +527,6 @@  static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     }
 
     ret = 0;
-    if (flush_bat) {
-        ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
-        if (ret < 0) {
-            res->check_errors++;
-            goto out;
-        }
-    }
-
     res->image_end_offset = high_off + s->cluster_size;
     if (size > res->image_end_offset) {
         int64_t count;