[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/3] parallels: Add checking and repairing duplicate offse
|
From: |
Vladimir Sementsov-Ogievskiy |
|
Subject: |
Re: [PATCH v2 2/3] parallels: Add checking and repairing duplicate offsets in BAT |
|
Date: |
Sat, 6 Aug 2022 23:45:30 +0300 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 |
On 8/5/22 18:47, alexander.ivanov@virtuozzo.com wrote:
From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
There could be corruptions in the image file:
two guest memory areas refer to the same host cluster.
Is that written in parallels spec (docs/interop/parallels.txt)?
Hmm yes: "- the value must be unique among all BAT entries".
If a duplicate offset is found fix it by copying the content
of the referred cluster to a new allocated cluster and
replace one of the two referring entries by the new cluster offset.
Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
block/parallels.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 135 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index a0eb7ec3c3..73264b4bd1 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -64,6 +64,11 @@ static QEnumLookup prealloc_mode_lookup = {
#define PARALLELS_OPT_PREALLOC_MODE "prealloc-mode"
#define PARALLELS_OPT_PREALLOC_SIZE "prealloc-size"
+#define REVERSED_BAT_UNTOUCHED 0xFFFFFFFF
+
+#define HOST_CLUSTER_INDEX(s, off) \
+ ((off - ((s->header->data_off) << BDRV_SECTOR_BITS)) / (s->cluster_size))
Let it be a static function, not a macro.
+
static QemuOptsList parallels_runtime_opts = {
.name = "parallels",
.head = QTAILQ_HEAD_INITIALIZER(parallels_runtime_opts.head),
@@ -559,6 +564,131 @@ static int check_leak(BlockDriverState *bs,
return 0;
}
+static int check_duplicate(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
+{
+ BDRVParallelsState *s = bs->opaque;
+ QEMUIOVector qiov;
+ int64_t off, high_off, size, sector_num;
+ uint32_t i, idx_host;
+ int ret = 0, n;
+ g_autofree uint32_t *reversed_bat = NULL;
+ g_autofree int64_t *cluster_buf = NULL;
+ bool sync_and_truncate = false;
+
+ /*
+ * Make a reversed BAT.
+ * Each cluster in the host file could represent only one cluster
+ * from the guest point of view. Reversed BAT provides mapping of that
type.
+ * Initially the table is filled with REVERSED_BAT_UNTOUCHED values.
+ */
+ reversed_bat = g_malloc(s->bat_size * sizeof(uint32_t));
Hmm. Why size of reversed_bat equal to bat_size? Couldn't host file size be
larger than that?
Seems that we want calculate the highest offset first, and then allocate
corresponding table.
Also, here we probably allocate too much memory. Better use g_try_malloc and
clean error-out instead of crash.
+ for (i = 0; i < s->bat_size; i++) {
+ reversed_bat[i] = REVERSED_BAT_UNTOUCHED;
+ }
+
+ cluster_buf = g_malloc(s->cluster_size);
+ qemu_iovec_init(&qiov, 0);
+ qemu_iovec_add(&qiov, cluster_buf, s->cluster_size);
+
+ high_off = 0;
+ for (i = 0; i < s->bat_size; i++) {
+ off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+ if (off == 0) {
+ continue;
+ }
+
+ if (off > high_off) {
+ high_off = off;
+ }
+
+ idx_host = HOST_CLUSTER_INDEX(s, off);
+ if (idx_host >= s->bat_size) {
+ res->check_errors++;
As I understand, check_errors is mostly for IO errors during the check.
If it's incorrect for parallels format to have such cluster, we want
res->corruptions++ here instead.
But is it really incorrect, what the spec say?
+ goto out;
+ }
+
+ if (reversed_bat[idx_host] != REVERSED_BAT_UNTOUCHED) {
+ /*
+ * We have alreade written a guest cluster index for this
already
+ * host cluster. It means there is a duplicated offset in BAT.
+ */
+ fprintf(stderr,
+ "%s BAT offset in entry %u duplicates offset in entry
%u\n",
+ fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
+ i, reversed_bat[idx_host]);
+ res->corruptions++;
+
+ if (fix & BDRV_FIX_ERRORS) {
+ /*
+ * Write zero to the current BAT entry, allocate a new cluster
+ * for the relevant guest offset and copy the host cluster
+ * to the new allocated cluster.
+ * In this way mwe will have two identical clusters and two
+ * BAT entries with the offsets of these clusters.
+ */
+ s->bat_bitmap[i] = 0;
I don't understand that. So you just remove that guest cluster. Shouldn't we
instead set it to new allocated off?
+
+ sector_num = bat2sect(s, reversed_bat[idx_host]);
+ ret = bdrv_pread(bs->file, sector_num << BDRV_SECTOR_BITS,
+ s->cluster_size, cluster_buf, 0);
+ if (ret < 0) {
+ res->check_errors++;
+ goto out;
+ }
+
+ sector_num = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
+ off = allocate_clusters(bs, sector_num, s->tracks, &n);
you probably want to update high_off here
+ if (off < 0) {
+ res->check_errors++;
+ ret = off;
+ goto out;
+ }
+ off <<= BDRV_SECTOR_BITS;
+
+ ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov,
0);
+ if (ret < 0) {
+ res->check_errors++;
+ goto out;
+ }
+
+ /* off is new and we should repair idx_host accordingly. */
+ idx_host = HOST_CLUSTER_INDEX(s, off);
+ res->corruptions_fixed++;
+ sync_and_truncate = true;
+ }
+ }
+ reversed_bat[idx_host] = i;
+ }
+
+ if (sync_and_truncate) {
+ ret = sync_header(bs, res);
+ if (ret < 0) {
+ goto out;
+ }
+
+ size = bdrv_getlength(bs->file->bs);
+ if (size < 0) {
+ res->check_errors++;
+ ret = size;
+ goto out;
+ }
+
+ res->image_end_offset = high_off + s->cluster_size;
+ if (size > res->image_end_offset) {
+ ret = truncate_file(bs, res->image_end_offset);
that's already done in check_leak, why we need it again?
+ if (ret < 0) {
+ goto out;
+ }
+ }
+ }
+
+out:
+ qemu_iovec_destroy(&qiov);
+ return ret;
+}
+
static void collect_statistics(BlockDriverState *bs, BdrvCheckResult *res)
{
BDRVParallelsState *s = bs->opaque;
@@ -598,6 +728,11 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
goto out;
}
+ ret = check_duplicate(bs, res, fix);
+ if (ret != 0) {
+ goto out;
+ }
+
collect_statistics(bs, res);
out:
--
Best regards,
Vladimir