[PATCH v6 09/25] parallels: Drop unused clusters at the end of the image

From: Denis V. Lunev <[email protected]>

On an image closing there can be unused clusters in the end of the
image. Since we have used bitmap, they can be found without a leak
check, so parallels_check_unused_clusters() answers both questions:
inactivation asks it to truncate them away, and the leak check asks it
how much there is.

A repairing leak check truncates the file, so the used bitmap is
recreated afterwards, as it would no longer comply to it.

The helper answers with the size of the leak, which is a byte count and
does not belong in an int. parallels_inactivate() keeps it in an int64_t
and reports only a failure, as a leak of 2 GiB or more would otherwise
look like one and fail the inactivation of an image which was truncated
just fine.

A BAT entry pointing at a cluster which does not fit the file makes the
used bitmap reach further than the file does, and that difference is
not a leak of negative size. parallels_check_outside_image() reports it
as corruption on its own, so answer with no leak at all rather than
-EINVAL, which would fail the whole check before the duplicate check
and the statistics ever run.

Repairing such an entry clears it from the BAT, and the used bitmap has
to follow the way parallels_check_data_off() already makes it follow a
repaired data_off. Otherwise it keeps a bit for a cluster the image no
longer has, and 'qemu-img check -r all' leaves the space behind it
untouched instead of truncating it away.

Rebuilding the used bitmap after the truncation fails with -EBUSY or
-E2BIG on a BAT which points a cluster twice or out of the image.
parallels_open() calls those correctable and repairs them, so they must
not abort the check that is meant to do the repairing.

Based on the original work from Alexander Ivanov.

Cc: Stefan Hajnoczi <[email protected]>
Signed-off-by: Denis V. Lunev <[email protected]>
---
 block/parallels.c                             | 131 +++++++++++++-----
 tests/qemu-iotests/tests/parallels-checks     |  11 ++
 tests/qemu-iotests/tests/parallels-checks.out |  22 ++-
 3 files changed, 126 insertions(+), 38 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 307e90ec71..2be7c20338 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -750,6 +750,7 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
     BDRVParallelsState *s = bs->opaque;
     uint32_t i;
     int64_t off, high_off, size, data_start_off;
+    bool fixed = false;
 
     size = bdrv_co_getlength(bs->file->bs);
     if (size < 0) {
@@ -771,6 +772,7 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
             if (fix & BDRV_FIX_ERRORS) {
                 parallels_set_bat_entry(s, i, 0);
                 res->corruptions_fixed++;
+                fixed = true;
             }
             continue;
         }
@@ -779,6 +781,17 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
         }
     }
 
+    if (fixed) {
+        int err;
+
+        parallels_free_used_bitmap(bs);
+        err = parallels_fill_used_bitmap(bs);
+        if (err == -ENOMEM) {
+            res->check_errors++;
+            return err;
+        }
+    }
+
     if (high_off == 0) {
         res->image_end_offset = s->data_end << BDRV_SECTOR_BITS;
     } else {
@@ -786,51 +799,96 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
         s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
     }
 
+
     return 0;
 }
 
+static int64_t GRAPH_RDLOCK
+parallels_check_unused_clusters(BlockDriverState *bs, bool truncate)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t leak, file_size, end_off = 0;
+    int ret;
+
+    file_size = bdrv_getlength(bs->file->bs);
+    if (file_size < 0) {
+        return file_size;
+    }
+
+    if (s->used_bmap_size > 0) {
+        end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
+        if (end_off == s->used_bmap_size) {
+            end_off = 0;
+        } else {
+            end_off = (end_off + 1) * s->cluster_size;
+        }
+    }
+
+    end_off += s->data_start * BDRV_SECTOR_SIZE;
+
+    /*
+     * A cluster in use behind the end of the file is corruption which
+     * parallels_check_outside_image() reports on its own. There is no
+     * leaked space to reclaim behind it, and nothing to truncate.
+     */
+    if (end_off >= file_size) {
+        return 0;
+    }
+
+    leak = file_size - end_off;
+    if (!truncate) {
+        return leak;
+    }
+
+    ret = bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
+    if (ret) {
+        return ret;
+    }
+
+    parallels_free_used_bitmap(bs);
+    ret = parallels_fill_used_bitmap(bs);
+    if (ret == -ENOMEM) {
+        return ret;
+    }
+
+    return leak;
+}
+
 static int coroutine_fn GRAPH_RDLOCK
 parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
                      BdrvCheckMode fix, bool explicit)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size;
-    int ret;
+    int64_t leak, count, size;
+
+    leak = parallels_check_unused_clusters(bs, fix & BDRV_FIX_LEAKS);
+    if (leak < 0) {
+        res->check_errors++;
+        return leak;
+    }
+    if (leak == 0) {
+        return 0;
+    }
 
     size = bdrv_co_getlength(bs->file->bs);
     if (size < 0) {
         res->check_errors++;
         return size;
     }
+    res->image_end_offset = size;
 
-    if (size > res->image_end_offset) {
-        int64_t count;
-        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
-        if (explicit) {
-            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;
+    if (!explicit) {
+        return 0;
+    }
 
-            /*
-             * 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;
-            }
-            if (explicit) {
-                res->leaks_fixed += count;
-            }
-        }
+    count = DIV_ROUND_UP(leak, s->cluster_size);
+    fprintf(stderr,
+            "%s space leaked at the end of the image %" PRId64 "\n",
+            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak);
+    res->leaks += count;
+
+    if (fix & BDRV_FIX_LEAKS) {
+        res->leaks_fixed += count;
     }
 
     return 0;
@@ -849,7 +907,10 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
     bool fixed = false;
 
     /*
-     * Create a bitmap of used clusters.
+     * Create a bitmap of used clusters. Please note that this bitmap is not
+     * related to used_bmap field in BDRVParallelsState and is created only for
+     * local usage.
+     *
      * If a bit is set, there is a BAT entry pointing to this cluster.
      * Loop through the BAT entries, check bits relevant to an entry offset.
      * If bit is set, this entry is duplicated. Otherwise set the bit.
@@ -1521,16 +1582,16 @@ fail:
 static int GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
-    int ret;
+    int64_t leak;
 
     if (!(bs->open_flags & BDRV_O_RDWR) || (bs->open_flags & BDRV_O_INACTIVE)) {
         return 0;
     }
 
-    ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
-                        PREALLOC_MODE_OFF, 0, NULL);
-    if (ret < 0) {
-        return ret;
+    leak = parallels_check_unused_clusters(bs, true);
+    if (leak < 0) {
+        error_report("Failed to truncate image: %s", strerror(-leak));
+        return leak;
     }
 
     s->header->inuse = 0;
diff --git a/tests/qemu-iotests/tests/parallels-checks b/tests/qemu-iotests/tests/parallels-checks
index cf90eaf152..c9dcd715ac 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -352,6 +352,17 @@ truncate -s $((file_size - CLUSTER_SIZE / 2)) "$TEST_IMG"
 echo "== the check completes and reports the cluster =="
 _check_test_img
 
+echo "== nothing can be reclaimed behind it =="
+_check_test_img -r leaks
+echo "file size: `stat --printf="%s" "$TEST_IMG"`"
+
+echo "== a full repair drops the entry and truncates the image =="
+_check_test_img -r all
+echo "file size: `stat --printf="%s" "$TEST_IMG"`"
+
+echo "== the first cluster survived =="
+{ $QEMU_IO -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 # Clear image
 _make_test_img $SIZE
 
diff --git a/tests/qemu-iotests/tests/parallels-checks.out b/tests/qemu-iotests/tests/parallels-checks.out
index 645c4b3679..6699848996 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -209,13 +209,29 @@ wrote 1048576/1048576 bytes at offset 1048576
 == cut the second one in half ==
 == the check completes and reports the cluster ==
 ERROR cluster 1 is outside image
-ERROR space leaked at the end of the image 524288
 
 1 errors were found on the image.
 Data may be corrupted, or further writes to the image may corrupt it.
+== nothing can be reclaimed behind it ==
+ERROR cluster 1 is outside image
 
-1 leaked clusters were found on the image.
-This means waste of disk space, but no harm to data.
+1 errors were found on the image.
+Data may be corrupted, or further writes to the image may corrupt it.
+file size: 2621440
+== a full repair drops the entry and truncates the image ==
+Repairing cluster 1 is outside image
+Repairing space leaked at the end of the image 524288
+The following inconsistencies were found and repaired:
+
+    1 leaked clusters
+    1 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+file size: 2097152
+== the first cluster survived ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304
 == TEST A DUPLICATE IN THE LAST ALLOCATED BAT ENTRY ==
 == write two clusters ==
-- 
2.53.0