[v5] parallels: Add duplication check, repair at open, fix bugs

[PATCH v5 2/5] parallels: Split image leak handling to separate check and fix helpers

Posted by Alexander Ivanov 1 year, 5 months ago

We need to fix leak after deduplication in the next patch. Move leak
fixing to a separate helper parallels_fix_leak() and add
parallels_get_leak_size() helper wich used in parallels_fix_leak() and
parallels_check_leak().

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 86 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 61 insertions(+), 25 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 1ec98c722b..64850b9655 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -482,43 +482,79 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
     return 0;
 }
 
+static int64_t parallels_get_leak_size(BlockDriverState *bs,
+                                       BdrvCheckResult *res)
+{
+    int64_t size;
+
+    size = bdrv_getlength(bs->file->bs);
+    if (size < 0) {
+        return size;
+    }
+
+    /*
+     * Before any usage of this function, image_end_offset has to be set to the
+     * the highest offset in the BAT, excluding out-of-image offsets.
+     */
+    assert(size >= res->image_end_offset);
+
+    return size - res->image_end_offset;
+}
+
+static int parallels_fix_leak(BlockDriverState *bs,
+                              BdrvCheckResult *res)
+{
+    Error *local_err = NULL;
+    int64_t size;
+    int ret;
+
+    size = parallels_get_leak_size(bs, res);
+    if (size <= 0) {
+        return size;
+    }
+
+    /*
+     * In order to really repair the image, we must shrink it.
+     * That means we have to pass exact=true.
+     */
+    ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
+                           PREALLOC_MODE_OFF, 0, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+        return ret;
+    }
+
+    return 0;
+}
+
 static int coroutine_fn GRAPH_RDLOCK
 parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
                      BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size;
+    int64_t count, leak_size;
     int ret;
 
-    size = bdrv_getlength(bs->file->bs);
-    if (size < 0) {
+    leak_size = parallels_get_leak_size(bs, res);
+    if (leak_size < 0) {
         res->check_errors++;
-        return size;
+        return leak_size;
+    }
+    if (leak_size == 0) {
+        return 0;
     }
 
-    if (size > res->image_end_offset) {
-        int64_t count;
-        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
-        fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
-                fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
-                size - res->image_end_offset);
-        res->leaks += count;
-        if (fix & BDRV_FIX_LEAKS) {
-            Error *local_err = NULL;
+    count = DIV_ROUND_UP(leak_size, s->cluster_size);
+    res->leaks += count;
+    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
+            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
 
-            /*
-             * In order to really repair the image, we must shrink it.
-             * That means we have to pass exact=true.
-             */
-            ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
-                                   PREALLOC_MODE_OFF, 0, &local_err);
-            if (ret < 0) {
-                error_report_err(local_err);
-                res->check_errors++;
-                return ret;
-            }
-            res->leaks_fixed += count;
+    if (fix & BDRV_FIX_LEAKS) {
+        ret = parallels_fix_leak(bs, res);
+        if (ret < 0) {
+            return ret;
         }
+        res->leaks_fixed += count;
     }
 
     return 0;
-- 
2.34.1

Re: [PATCH v5 2/5] parallels: Split image leak handling to separate check and fix helpers

Posted by Hanna Czenczek 1 year, 4 months ago

On 29.05.23 17:15, Alexander Ivanov wrote:
> We need to fix leak after deduplication in the next patch. Move leak
> fixing to a separate helper parallels_fix_leak() and add
> parallels_get_leak_size() helper wich used in parallels_fix_leak() and
> parallels_check_leak().
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 86 +++++++++++++++++++++++++++++++++--------------
>   1 file changed, 61 insertions(+), 25 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 1ec98c722b..64850b9655 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -482,43 +482,79 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
>       return 0;
>   }
>   
> +static int64_t parallels_get_leak_size(BlockDriverState *bs,
> +                                       BdrvCheckResult *res)
> +{
> +    int64_t size;
> +
> +    size = bdrv_getlength(bs->file->bs);
> +    if (size < 0) {
> +        return size;
> +    }
> +
> +    /*
> +     * Before any usage of this function, image_end_offset has to be set to the
> +     * the highest offset in the BAT, excluding out-of-image offsets.
> +     */
> +    assert(size >= res->image_end_offset);

If `high_off == 0` in parallels_check_outside_image(), it will use 
s->data_end to determine image_end_offset, which is originally read from 
the image header.  I don’t see any place where we ensure that 
`s->data_end <= bdrv_getlength(bs->file->bs)`, so can we be certain the 
assertion holds even in that case?

> +
> +    return size - res->image_end_offset;
> +}
> +
> +static int parallels_fix_leak(BlockDriverState *bs,
> +                              BdrvCheckResult *res)
> +{
> +    Error *local_err = NULL;
> +    int64_t size;
> +    int ret;
> +
> +    size = parallels_get_leak_size(bs, res);
> +    if (size <= 0) {
> +        return size;
> +    }
> +
> +    /*
> +     * In order to really repair the image, we must shrink it.
> +     * That means we have to pass exact=true.
> +     */
> +    ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
> +                           PREALLOC_MODE_OFF, 0, &local_err);
> +    if (ret < 0) {
> +        error_report_err(local_err);
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
>   static int coroutine_fn GRAPH_RDLOCK
>   parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>                        BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    int64_t size;
> +    int64_t count, leak_size;
>       int ret;
>   
> -    size = bdrv_getlength(bs->file->bs);
> -    if (size < 0) {
> +    leak_size = parallels_get_leak_size(bs, res);
> +    if (leak_size < 0) {
>           res->check_errors++;
> -        return size;
> +        return leak_size;
> +    }
> +    if (leak_size == 0) {
> +        return 0;
>       }
>   
> -    if (size > res->image_end_offset) {
> -        int64_t count;
> -        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
> -        fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
> -                fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
> -                size - res->image_end_offset);
> -        res->leaks += count;
> -        if (fix & BDRV_FIX_LEAKS) {
> -            Error *local_err = NULL;
> +    count = DIV_ROUND_UP(leak_size, s->cluster_size);
> +    res->leaks += count;
> +    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
> +            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
>   
> -            /*
> -             * In order to really repair the image, we must shrink it.
> -             * That means we have to pass exact=true.
> -             */
> -            ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
> -                                   PREALLOC_MODE_OFF, 0, &local_err);
> -            if (ret < 0) {
> -                error_report_err(local_err);
> -                res->check_errors++;
> -                return ret;
> -            }
> -            res->leaks_fixed += count;
> +    if (fix & BDRV_FIX_LEAKS) {
> +        ret = parallels_fix_leak(bs, res);
> +        if (ret < 0) {
> +            return ret;

We used to increment res->check_errors here – should we still do that?

Hanna

>           }
> +        res->leaks_fixed += count;
>       }
>   
>       return 0;

Re: [PATCH v5 2/5] parallels: Split image leak handling to separate check and fix helpers

Posted by Alexander Ivanov 1 year, 4 months ago


On 6/2/23 16:08, Hanna Czenczek wrote:
> On 29.05.23 17:15, Alexander Ivanov wrote:
>> We need to fix leak after deduplication in the next patch. Move leak
>> fixing to a separate helper parallels_fix_leak() and add
>> parallels_get_leak_size() helper wich used in parallels_fix_leak() and
>> parallels_check_leak().
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>>   block/parallels.c | 86 +++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 61 insertions(+), 25 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 1ec98c722b..64850b9655 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -482,43 +482,79 @@ parallels_check_outside_image(BlockDriverState 
>> *bs, BdrvCheckResult *res,
>>       return 0;
>>   }
>>   +static int64_t parallels_get_leak_size(BlockDriverState *bs,
>> +                                       BdrvCheckResult *res)
>> +{
>> +    int64_t size;
>> +
>> +    size = bdrv_getlength(bs->file->bs);
>> +    if (size < 0) {
>> +        return size;
>> +    }
>> +
>> +    /*
>> +     * Before any usage of this function, image_end_offset has to be 
>> set to the
>> +     * the highest offset in the BAT, excluding out-of-image offsets.
>> +     */
>> +    assert(size >= res->image_end_offset);
>
> If `high_off == 0` in parallels_check_outside_image(), it will use 
> s->data_end to determine image_end_offset, which is originally read 
> from the image header.  I don’t see any place where we ensure that 
> `s->data_end <= bdrv_getlength(bs->file->bs)`, so can we be certain 
> the assertion holds even in that case?
Will add s->data_end > file_nb_sectors check to parallels_open(). Thank you.
>
>> +
>> +    return size - res->image_end_offset;
>> +}
>> +
>> +static int parallels_fix_leak(BlockDriverState *bs,
>> +                              BdrvCheckResult *res)
>> +{
>> +    Error *local_err = NULL;
>> +    int64_t size;
>> +    int ret;
>> +
>> +    size = parallels_get_leak_size(bs, res);
>> +    if (size <= 0) {
>> +        return size;
>> +    }
>> +
>> +    /*
>> +     * In order to really repair the image, we must shrink it.
>> +     * That means we have to pass exact=true.
>> +     */
>> +    ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
>> +                           PREALLOC_MODE_OFF, 0, &local_err);
>> +    if (ret < 0) {
>> +        error_report_err(local_err);
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int coroutine_fn GRAPH_RDLOCK
>>   parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>>                        BdrvCheckMode fix)
>>   {
>>       BDRVParallelsState *s = bs->opaque;
>> -    int64_t size;
>> +    int64_t count, leak_size;
>>       int ret;
>>   -    size = bdrv_getlength(bs->file->bs);
>> -    if (size < 0) {
>> +    leak_size = parallels_get_leak_size(bs, res);
>> +    if (leak_size < 0) {
>>           res->check_errors++;
>> -        return size;
>> +        return leak_size;
>> +    }
>> +    if (leak_size == 0) {
>> +        return 0;
>>       }
>>   -    if (size > res->image_end_offset) {
>> -        int64_t count;
>> -        count = DIV_ROUND_UP(size - res->image_end_offset, 
>> s->cluster_size);
>> -        fprintf(stderr, "%s space leaked at the end of the image %" 
>> PRId64 "\n",
>> -                fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
>> -                size - res->image_end_offset);
>> -        res->leaks += count;
>> -        if (fix & BDRV_FIX_LEAKS) {
>> -            Error *local_err = NULL;
>> +    count = DIV_ROUND_UP(leak_size, s->cluster_size);
>> +    res->leaks += count;
>> +    fprintf(stderr, "%s space leaked at the end of the image %" 
>> PRId64 "\n",
>> +            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
>>   -            /*
>> -             * In order to really repair the image, we must shrink it.
>> -             * That means we have to pass exact=true.
>> -             */
>> -            ret = bdrv_co_truncate(bs->file, res->image_end_offset, 
>> true,
>> -                                   PREALLOC_MODE_OFF, 0, &local_err);
>> -            if (ret < 0) {
>> -                error_report_err(local_err);
>> -                res->check_errors++;
>> -                return ret;
>> -            }
>> -            res->leaks_fixed += count;
>> +    if (fix & BDRV_FIX_LEAKS) {
>> +        ret = parallels_fix_leak(bs, res);
>> +        if (ret < 0) {
>> +            return ret;
>
> We used to increment res->check_errors here – should we still do that?
>
> Hanna
>
>>           }
>> +        res->leaks_fixed += count;
>>       }
>>         return 0;
>

-- 
Best regards,
Alexander Ivanov

[PATCH v5 1/5] parallels: Incorrect data end calculation in parallels_open()
[PATCH v5 2/5] parallels: Split image leak handling to separate check and fix helpers
[PATCH v5 3/5] parallels: Add checking and repairing duplicate offsets in BAT
[PATCH v5 4/5] parallels: Replace fprintf by qemu_log in check
[PATCH v5 5/5] parallels: Image repairing in parallels_open()