Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode
On 2/28/24 11:25, Alexander Ivanov wrote:
On 1/18/24 14:31, Denis V. Lunev wrote:
On 1/16/24 15:45, Denis V. Lunev wrote:
On 12/28/23 11:12, Alexander Ivanov wrote:
Now we support extensions saving and can let to work with them in
read-write mode.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 4
block/parallels.c | 17 -
2 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index c83d1ea393..195b01b109 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs,
uint8_t *data, size_t data_size,
return NULL;
}
- /* We support format extension only for RO parallels images. */
- assert(!(bs->open_flags & BDRV_O_RDWR));
- bdrv_dirty_bitmap_set_readonly(bitmap, true);
-
return bitmap;
}
diff --git a/block/parallels.c b/block/parallels.c
index a49922c6a7..d5d87984cf 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1374,19 +1374,10 @@ static int parallels_open(BlockDriverState
*bs, QDict *options, int flags,
}
if (ph.ext_off) {
- if (flags & BDRV_O_RDWR) {
- /*
- * It's unsafe to open image RW if there is an
extension (as we
- * don't support it). But parallels driver in QEMU
historically
- * ignores the extension, so print warning and don't
care.
- */
- warn_report("Format Extension ignored in RW mode");
- } else {
- ret = parallels_read_format_extension(
- bs, le64_to_cpu(ph.ext_off) <<
BDRV_SECTOR_BITS, errp);
- if (ret < 0) {
- goto fail;
- }
+ ret = parallels_read_format_extension(
+ bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS,
errp);
+ if (ret < 0) {
+ goto fail;
}
}
Reviewed-by: Denis V. Lunev
This patch also deserves a note, what will happen with
format extensions clusters. According to the current
policy, we have only transient extensions, i.e.
CBT. Cluster allocation mechanism will reuse these
clusters as they are not marked as used.
Thus we should either set format extension offset
in the header to 0 or perform any other correct
measures to properly handle this.
Yes, all the clusters used by extensions are marked as unused
on loading. In further work they can be reallocated for other
purposes.
Agree that we need to set ext_off to zero.
It should also be noted, that on any QEMU crash
appropriate format extensions are to be properly
treated. We could not make them RW until this would
not be addressed as we could easily mess up with
trashed metadata.
If QEMU crashes after extensions loading there will be
zero in the ext_off field and an inappropriate dirty bitmap
will be ignored.
That should be considered as a minimal kludge.
Normally we should mark format extension cluster
as used and nullify references from the bitmaps
there.
Anyway, even if the ext_off is not zero and
bitmaps are present, bitmaps loading over
unclean image should not be performed. They should
be considered outdated.
Den
Re: [PATCH] iotests/277: Use iotests.sock_dir for socket creation
On 1/24/24 18:43, Eric Blake wrote:
On Wed, Jan 24, 2024 at 06:22:57PM +0200, Andrey Drobyshev wrote:
If socket path is too long (longer than 108 bytes), socket can't be
opened. This might lead to failure when test dir path is long enough.
Make sure socket is created in iotests.sock_dir to avoid such a case.
This commit basically aligns iotests/277 with the rest of iotests.
Signed-off-by: Andrey Drobyshev
---
tests/qemu-iotests/277 | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Eric Blake
diff --git a/tests/qemu-iotests/277 b/tests/qemu-iotests/277
index 24833e7eb6..4224202ac2 100755
--- a/tests/qemu-iotests/277
+++ b/tests/qemu-iotests/277
@@ -27,7 +27,8 @@ from iotests import file_path, log
iotests.script_initialize()
-nbd_sock, conf_file = file_path('nbd-sock', 'nbd-fault-injector.conf')
+conf_file = file_path('nbd-fault-injector.conf')
+nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
def make_conf_file(event):
--
2.39.3
I would say that potentially the same code is present
in 264, it is :
disk_a, disk_b, nbd_sock = file_path('disk_a', 'disk_b', 'nbd-sock')
nbd_uri = 'nbd+unix:///?socket=' + nbd_sock
Re: [PATCH v4 00/21] parallels: Add full dirty bitmap support
On 12/28/23 11:12, Alexander Ivanov wrote: Parallels format driver: * make some preparation * add dirty bitmap saving * make dirty bitmap RW * fix broken checks * refactor leak check * add parallels format support to several tests You could find these patches in my repo: https://github.com/AlexanderIvanov-Virtuozzo/qemu/tree/parallels-v4 v4: 4: A new patch with limitation of search in parallels_mark_used. 5: Previously 4. Search is limited to (cluster_index + count). 6: Previously 5. Added GRAPH_RDLOCK annotation, added a note in the commit message. 11: Previously 10. Added GRAPH_RDLOCK annotation. 16-18: Added GRAPH_RDLOCK annotations. v3: 1: Fixed the order of g_free() and s->used_bmap = NULL. 3,4: Made mark_used() a global function before mark_unused() addition. In this way we can avoid compilation warnings. 5-9: Patches shifted. 11: Added GRAPH_RDLOCK annotation to parallels_inactivate(). Guard parallels_close() with GRAPH_RDLOCK_GUARD_MAINLOOP(). 12-21: Patches shifted. v2: 1: New patch to fix double free error. 4: Fixed clusters leaks. 15: Fixed (end_off != s->used_bmap_size) handling in parallels_truncate_unused_clusters(). 16,17: Changed the sequence of the patches - in this way we have correct leaks check. Alexander Ivanov (21): parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap() parallels: Move inactivation code to a separate function parallels: Make mark_used() a global function parallels: Limit search in parallels_mark_used to the last marked claster parallels: Add parallels_mark_unused() helper parallels: Move host clusters allocation to a separate function parallels: Set data_end value in parallels_check_leak() parallels: Recreate used bitmap in parallels_check_leak() parallels: Add a note about used bitmap in parallels_check_duplicate() parallels: Create used bitmap even if checks needed parallels: Add dirty bitmaps saving parallels: Let image extensions work in RW mode parallels: Handle L1 entries equal to one parallels: Make a loaded dirty bitmap persistent parallels: Reverse a conditional in parallels_check_leak() to reduce indents parallels: Truncate images on the last used cluster parallels: Check unused clusters in parallels_check_leak() parallels: Remove unnecessary data_end field tests: Add parallels images support to test 165 tests: Turned on 256, 299, 304 and block-status-cache for parallels format tests: Add parallels format support to image-fleecing block/parallels-ext.c | 183 +- block/parallels.c | 371 block/parallels.h | 14 +- tests/qemu-iotests/165 | 40 ++- tests/qemu-iotests/256 | 2 +- tests/qemu-iotests/299 | 2 +- tests/qemu-iotests/304 | 2 +- tests/qemu-iotests/tests/block-status-cache | 2 +- tests/qemu-iotests/tests/image-fleecing | 13 +- 9 files changed, 453 insertions(+), 176 deletions(-) I would also say that if we add parallels extension in read-write mode, we should also add appropriates clauses into parallels_check. Den
Re: [PATCH v4 18/21] parallels: Remove unnecessary data_end field
On 12/28/23 11:12, Alexander Ivanov wrote:
Since we have used bitmap, field data_end in BDRVParallelsState is
redundant and can be removed.
Add parallels_data_end() helper and remove data_end handling.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 33 +
block/parallels.h | 1 -
2 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 5ed58826bb..2803119699 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -268,6 +268,13 @@ static void parallels_free_used_bitmap(BlockDriverState
*bs)
s->used_bmap = NULL;
}
+static int64_t parallels_data_end(BDRVParallelsState *s)
+{
+int64_t data_end = s->data_start * BDRV_SECTOR_SIZE;
+data_end += s->used_bmap_size * s->cluster_size;
+return data_end;
+}
+
int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
int64_t *clusters)
{
@@ -279,7 +286,7 @@ int64_t GRAPH_RDLOCK
parallels_allocate_host_clusters(BlockDriverState *bs,
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
-host_off = s->data_end * BDRV_SECTOR_SIZE;
+host_off = parallels_data_end(s);
prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
bytes = *clusters * s->cluster_size;
prealloc_bytes = prealloc_clusters * s->cluster_size;
@@ -302,9 +309,6 @@ int64_t GRAPH_RDLOCK
parallels_allocate_host_clusters(BlockDriverState *bs,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
-if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
-s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
-}
} else {
next_used = find_next_bit(s->used_bmap, s->used_bmap_size,
first_free);
@@ -320,8 +324,7 @@ int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
* branch. In the other case we are likely re-using hole. Preallocate
* the space if required by the prealloc_mode.
*/
-if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
-host_off < s->data_end * BDRV_SECTOR_SIZE) {
This seems wrong. The check whether the offset is in a tail area
or not has been deleted. This looks incorrect.
+if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
if (ret < 0) {
return ret;
@@ -758,13 +761,7 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
}
}
-if (high_off == 0) {
-res->image_end_offset = s->data_end << BDRV_SECTOR_BITS;
-} else {
-res->image_end_offset = high_off + s->cluster_size;
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-}
-
+res->image_end_offset = parallels_data_end(s);
return 0;
}
@@ -803,8 +800,6 @@ parallels_check_unused_clusters(BlockDriverState *bs, bool truncate)
return ret;
}
-s->data_end = end_off / BDRV_SECTOR_SIZE;
-
parallels_free_used_bitmap(bs);
ret = parallels_fill_used_bitmap(bs);
if (ret < 0) {
@@ -1394,8 +1389,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
s->data_start = data_start;
-s->data_end = s->data_start;
-if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
+if (s->data_start < (s->header_size >> BDRV_SECTOR_BITS)) {
/*
* There is not enough unused space to fit to block align between BAT
* and actual data. We can't avoid read-modify-write...
@@ -1436,11 +1430,10 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
for (i = 0; i < s->bat_size; i++) {
sector = bat2sect(s, i);
-if (sector + s->tracks > s->data_end) {
-s->data_end = sector + s->tracks;
+if (sector + s->tracks > file_nb_sectors) {
+need_check = true;
break;
}
}
-need_check = need_check || s->data_end > file_nb_sectors;
ret = parallels_fill_used_bitmap(bs);
if (ret == -ENOMEM) {
diff --git a/block/parallels.h b/block/parallels.h
index 9db4f5c908..b494d93139 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -79,7 +79,6 @@ typedef struct BDRVParallelsState {
unsigned int bat_size;
int64_t data_start;
-int64_t data_end;
uint64_t prealloc_size;
ParallelsPreallocMode prealloc_mode;
Re: [PATCH v4 17/21] parallels: Check unused clusters in parallels_check_leak()
On 12/28/23 11:12, Alexander Ivanov wrote:
Since we have used bitmap, leak check is useless. Transform
parallels_truncate_unused_clusters() to parallels_check_unused_clusters()
helper and use it in leak check.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 121 +-
1 file changed, 67 insertions(+), 54 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 136865d53e..5ed58826bb 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -768,57 +768,87 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
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;
+leak = file_size - end_off;
+if (leak < 0) {
+return -EINVAL;
+}
+if (!truncate || leak == 0) {
+return leak;
+}
+
+ret = bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
+if (ret) {
+return ret;
+}
+
+s->data_end = end_off / BDRV_SECTOR_SIZE;
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret < 0) {
+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, count;
-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;
}
-if (size <= res->image_end_offset) {
+res->image_end_offset = size;
+
+if (!explicit) {
return 0;
}
-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;
-}
+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) {
-Error *local_err = NULL;
-
-/*
- * 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, _err);
-if (ret < 0) {
-error_report_err(local_err);
-res->check_errors++;
-return ret;
-}
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-
-parallels_free_used_bitmap(bs);
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-res->check_errors++;
-return ret;
-}
-
-if (explicit) {
-res->leaks_fixed += count;
-}
+res->leaks_fixed += count;
}
return 0;
@@ -1454,23 +1484,6 @@ fail:
return ret;
}
-static int GRAPH_RDLOCK parallels_truncate_unused_clusters(BlockDriverState *bs)
-{
-BDRVParallelsState *s = bs->opaque;
-uint64_t end_off = 0;
-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;
-s->data_end = end_off / BDRV_SECTOR_SIZE;
-return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
-}
-
static int GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
@@ -1488,7 +1501,7 @@ static int GRAPH_RDLOCK
parallels_inactivate(BlockDriverState *bs)
parallels_update_header(bs);
/* errors are ignored, so we might as well pass exact=true */
-ret = parallels_truncate_unused_clusters(bs);
+ret =
Re: [PATCH v4 16/21] parallels: Truncate images on the last used cluster
On 12/28/23 11:12, Alexander Ivanov wrote:
On an image closing there can be unused clusters in the end of the image.
Truncate these clusters and update data_end field.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index fb7bc5e555..136865d53e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1454,6 +1454,23 @@ fail:
return ret;
}
+static int GRAPH_RDLOCK parallels_truncate_unused_clusters(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+uint64_t end_off = 0;
+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;
+s->data_end = end_off / BDRV_SECTOR_SIZE;
+return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
+}
+
static int GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
@@ -1471,8 +1488,7 @@ static int GRAPH_RDLOCK
parallels_inactivate(BlockDriverState *bs)
parallels_update_header(bs);
/* errors are ignored, so we might as well pass exact=true */
-ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS,
-true, PREALLOC_MODE_OFF, 0, NULL);
+ret = parallels_truncate_unused_clusters(bs);
if (ret < 0) {
error_report("Failed to truncate image: %s", strerror(-ret));
}
Reviewed-by: Denis V. Lunev
Re: [PATCH v4 15/21] parallels: Reverse a conditional in parallels_check_leak() to reduce indents
On 12/28/23 11:12, Alexander Ivanov wrote:
Let the function return a success code if a file size is not bigger than
image_end_offset. Thus we can decrease indents in the next code block.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 72 +++
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index d5d87984cf..fb7bc5e555 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -773,7 +773,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
BdrvCheckMode fix, bool explicit)
{
BDRVParallelsState *s = bs->opaque;
-int64_t size;
+int64_t size, count;
int ret;
size = bdrv_co_getlength(bs->file->bs);
@@ -781,43 +781,43 @@ parallels_check_leak(BlockDriverState *bs,
BdrvCheckResult *res,
res->check_errors++;
return size;
}
+if (size <= res->image_end_offset) {
+return 0;
+}
+
+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;
+
+/*
+ * 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, _err);
+if (ret < 0) {
+error_report_err(local_err);
+res->check_errors++;
+return ret;
+}
+s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+res->check_errors++;
+return ret;
+}
-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;
-
-/*
- * 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, _err);
-if (ret < 0) {
-error_report_err(local_err);
-res->check_errors++;
-return ret;
-}
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-
-parallels_free_used_bitmap(bs);
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-res->check_errors++;
-return ret;
-}
-
-if (explicit) {
-res->leaks_fixed += count;
-}
+res->leaks_fixed += count;
}
}
Reviewed-by: Denis V. Lunev
Re: [PATCH v4 14/21] parallels: Make a loaded dirty bitmap persistent
On 12/28/23 11:12, Alexander Ivanov wrote:
After bitmap loading the bitmap is not persistent and is removed on image
saving. Set bitmap persistence to true.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 033ca3ec3a..2a7ff6e35b 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -255,6 +255,7 @@ parallels_parse_format_extension(BlockDriverState *bs,
uint8_t *ext_cluster,
if (!bitmap) {
goto fail;
}
+bdrv_dirty_bitmap_set_persistence(bitmap, true);
bitmaps = g_slist_append(bitmaps, bitmap);
break;
Reviewed-by: Denis V. Lunev
Re: [PATCH v4 13/21] parallels: Handle L1 entries equal to one
On 12/28/23 11:12, Alexander Ivanov wrote:
If all the bits in a dirty bitmap cluster are ones, the cluster shouldn't
be written. Instead the corresponding L1 entry should be set to 1.
Check if all bits in a memory region are ones and set 1 to L1 entries
corresponding clusters filled with ones.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 195b01b109..033ca3ec3a 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -354,7 +354,7 @@ static void GRAPH_RDLOCK
parallels_save_bitmap(BlockDriverState *bs,
offset = 0;
while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size))
>= 0) {
uint64_t idx = offset / limit;
-int64_t cluster_off, end, write_size;
+int64_t cluster_off, end, write_size, first_zero;
offset = QEMU_ALIGN_DOWN(offset, limit);
end = MIN(bm_size, offset + limit);
@@ -367,6 +367,16 @@ static void GRAPH_RDLOCK
parallels_save_bitmap(BlockDriverState *bs,
memset(bm_buf + write_size, 0, s->cluster_size - write_size);
}
+first_zero = bdrv_dirty_bitmap_next_zero(bitmap, offset, bm_size);
+if (first_zero < 0) {
+goto end;
+}
+if (first_zero - offset >= s->cluster_size) {
+l1_table[idx] = 1;
+offset = end;
+continue;
+}
+
cluster_off = parallels_allocate_host_clusters(bs, _size);
if (cluster_off <= 0) {
goto end;
That is not enough. We should handle all-one and all-zeroes according
to the spec and all-zeroes would be much more common.
Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode
On 12/28/23 11:12, Alexander Ivanov wrote:
Now we support extensions saving and can let to work with them in
read-write mode.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 4
block/parallels.c | 17 -
2 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index c83d1ea393..195b01b109 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, uint8_t *data,
size_t data_size,
return NULL;
}
-/* We support format extension only for RO parallels images. */
-assert(!(bs->open_flags & BDRV_O_RDWR));
-bdrv_dirty_bitmap_set_readonly(bitmap, true);
-
return bitmap;
}
diff --git a/block/parallels.c b/block/parallels.c
index a49922c6a7..d5d87984cf 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1374,19 +1374,10 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
if (ph.ext_off) {
-if (flags & BDRV_O_RDWR) {
-/*
- * It's unsafe to open image RW if there is an extension (as we
- * don't support it). But parallels driver in QEMU historically
- * ignores the extension, so print warning and don't care.
- */
-warn_report("Format Extension ignored in RW mode");
-} else {
-ret = parallels_read_format_extension(
-bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
-if (ret < 0) {
-goto fail;
-}
+ret = parallels_read_format_extension(
+bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
+if (ret < 0) {
+goto fail;
}
}
something like attached should be taken into the account.
Though the destiny of cluster with old
extension offset requires some thinking.
I would say that it could be marked as used on read.
Anyway, this requires at least detailed thinking.From 2f70166ef640304726d5dfcee3e906b0ba1676dd Mon Sep 17 00:00:00 2001
From: "Denis V. Lunev"
Date: Thu, 18 Jan 2024 13:29:56 +0100
Subject: [PATCH 1/1] parallels: drop dirty bitmap data if the image was not
properly closed
This data is obsolete.
The approach is exactly the same like we use with QCOW2.
Signed-off-by: Denis V. Lunev
---
block/parallels-ext.c | 8
1 file changed, 8 insertions(+)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index c83d1ea393..54e8bb66a6 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -255,6 +255,14 @@ parallels_parse_format_extension(BlockDriverState *bs, uint8_t *ext_cluster,
return 0;
case PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC:
+if (s->header_unclean) {
+/*
+ * The image was not closed correctly and thus dirty bitmap
+ * data inside the image is considered as incorrect and thus
+ * it should be dropper, exactly like we do for QCOW2.
+ */
+break;
+}
bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp);
if (!bitmap) {
goto fail;
--
2.34.1
Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode
On 1/16/24 15:45, Denis V. Lunev wrote:
On 12/28/23 11:12, Alexander Ivanov wrote:
Now we support extensions saving and can let to work with them in
read-write mode.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 4
block/parallels.c | 17 -
2 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index c83d1ea393..195b01b109 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs,
uint8_t *data, size_t data_size,
return NULL;
}
- /* We support format extension only for RO parallels images. */
- assert(!(bs->open_flags & BDRV_O_RDWR));
- bdrv_dirty_bitmap_set_readonly(bitmap, true);
-
return bitmap;
}
diff --git a/block/parallels.c b/block/parallels.c
index a49922c6a7..d5d87984cf 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1374,19 +1374,10 @@ static int parallels_open(BlockDriverState
*bs, QDict *options, int flags,
}
if (ph.ext_off) {
- if (flags & BDRV_O_RDWR) {
- /*
- * It's unsafe to open image RW if there is an extension
(as we
- * don't support it). But parallels driver in QEMU
historically
- * ignores the extension, so print warning and don't care.
- */
- warn_report("Format Extension ignored in RW mode");
- } else {
- ret = parallels_read_format_extension(
- bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS,
errp);
- if (ret < 0) {
- goto fail;
- }
+ ret = parallels_read_format_extension(
+ bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
+ if (ret < 0) {
+ goto fail;
}
}
Reviewed-by: Denis V. Lunev
This patch also deserves a note, what will happen with
format extensions clusters. According to the current
policy, we have only transient extensions, i.e.
CBT. Cluster allocation mechanism will reuse these
clusters as they are not marked as used.
Thus we should either set format extension offset
in the header to 0 or perform any other correct
measures to properly handle this.
It should also be noted, that on any QEMU crash
appropriate format extensions are to be properly
treated. We could not make them RW until this would
not be addressed as we could easily mess up with
trashed metadata.
Re: [PATCH v4 11/21] parallels: Add dirty bitmaps saving
On 12/28/23 11:12, Alexander Ivanov wrote:
Now dirty bitmaps can be loaded but there is no their saving. Add code for
dirty bitmap storage.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 168 ++
block/parallels.c | 16 +++-
block/parallels.h | 5 ++
3 files changed, 187 insertions(+), 2 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index b4e14c88f2..c83d1ea393 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -24,6 +24,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/error-report.h"
#include "qapi/error.h"
#include "block/block-io.h"
#include "block/block_int.h"
@@ -300,3 +301,170 @@ out:
return ret;
}
+
+static void GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ uint8_t **buf, int *buf_size)
Do we need a error?
+{
+BDRVParallelsState *s = bs->opaque;
+ParallelsFeatureHeader *fh;
+ParallelsDirtyBitmapFeature *bh;
+uint64_t *l1_table, l1_size, granularity, limit;
I would say that 'limit' here means 'bits_in_cluster'
We are writing the new code and I would prefer if we
would have bits, bytes, clusters, sectors etc are
clearly seen in variable names. It is quite complex
to track various measurables.
+int64_t bm_size, ser_size, offset, buf_used;
+int64_t alloc_size = 1;
+const char *name;
+uint8_t *bm_buf;
+QemuUUID uuid;
+int ret = 0;
+
+if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
+bdrv_dirty_bitmap_inconsistent(bitmap)) {
+return;
+}
+
+bm_size = bdrv_dirty_bitmap_size(bitmap);
+granularity = bdrv_dirty_bitmap_granularity(bitmap);
+limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
+ser_size = bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size);
+l1_size = DIV_ROUND_UP(ser_size, s->cluster_size);
+
+buf_used = l1_size * 8 + sizeof(*fh) + sizeof(*bh);
As far as I can see, bdrv_dirty_bitmap_serialization_size() returns bytes.
That is correct. Thus multiplying it by 8 seems fatal mistake.
I am also quite unsure that we should roundup to the cluster, that
will occupy more clusters than needed. Can you please take a look
here
https://src.openvz.org/users/ibazhitov/repos/ploop/browse/tools/ploop-cbt.c
+/* Check if there is enough space for the final section */
+if (*buf_size - buf_used < sizeof(*fh)) {
+return;
+}
+
+name = bdrv_dirty_bitmap_name(bitmap);
+ret = qemu_uuid_parse(name, );
+if (ret < 0) {
+error_report("Can't save dirty bitmap: ID parsing error: '%s'", name);
+return;
+}
+
+fh = (ParallelsFeatureHeader *)*buf;
+bh = (ParallelsDirtyBitmapFeature *)(*buf + sizeof(*fh));
bh = fh + 1 ?
+l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh));
l1_table = bh + 1 ?
+
+fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC);
+fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh));
I am quite concerned here. Please compare with
int save_dirty_bitmap(int devfd, struct delta *delta, off_t offset,
void *buf, __u32 *size, void *or_data, writer_fn wr,
void *data)
{
int ret = 0;
struct ploop_pvd_header *vh;
size_t block_size;
__u64 bits, bytes, *p;
__u32 byte_granularity;
void *block;
struct ploop_pvd_dirty_bitmap_raw *raw = (struct
ploop_pvd_dirty_bitmap_raw *)buf;
char x[50];
vh = (struct ploop_pvd_header *)delta->hdr0;
/* granularity and uuid */
if ((ret = cbt_get_dirty_bitmap_metadata(devfd, raw->m_Id,
>m_Granularity)))
return ret;
raw->m_Granularity /= SECTOR_SIZE;
block_size = vh->m_Sectors * SECTOR_SIZE;
if (p_memalign((void **), 4096, block_size))
return SYSEXIT_MALLOC;
raw->m_Size = vh->m_SizeInSectors_v2;
byte_granularity = raw->m_Granularity * SECTOR_SIZE;
bits = ((raw->m_Size + raw->m_Granularity - 1) / raw->m_Granularity);
bytes = (bits + 7) >> 3;
raw->m_L1Size = (bytes + block_size - 1) / block_size;
which means that the header is rotten. In that case can you pls
take a look why this has not been caught by tests?
+
+bh->l1_size = cpu_to_le32(l1_size);
+bh->size = cpu_to_le64(bm_size >> BDRV_SECTOR_BITS);
+bh->granularity = cpu_to_le32(granularity >> BDRV_SECTOR_BITS);
+memcpy(bh->id, , sizeof(uuid));
+
+bm_buf = qemu_blockalign(bs, s->cluster_size);
+
+offset = 0;
+while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >=
0) {
+uint64_t idx = offset / limit;
+int64_t cluster_off, end, write_size;
+
+offset = QEMU_ALIGN_DOWN(offset, limit);
+end = MIN(bm_size, offset + limit);
+write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset,
+ end - offset);
+
Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode
On 12/28/23 11:12, Alexander Ivanov wrote:
Now we support extensions saving and can let to work with them in
read-write mode.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 4
block/parallels.c | 17 -
2 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index c83d1ea393..195b01b109 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, uint8_t *data,
size_t data_size,
return NULL;
}
-/* We support format extension only for RO parallels images. */
-assert(!(bs->open_flags & BDRV_O_RDWR));
-bdrv_dirty_bitmap_set_readonly(bitmap, true);
-
return bitmap;
}
diff --git a/block/parallels.c b/block/parallels.c
index a49922c6a7..d5d87984cf 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1374,19 +1374,10 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
if (ph.ext_off) {
-if (flags & BDRV_O_RDWR) {
-/*
- * It's unsafe to open image RW if there is an extension (as we
- * don't support it). But parallels driver in QEMU historically
- * ignores the extension, so print warning and don't care.
- */
-warn_report("Format Extension ignored in RW mode");
-} else {
-ret = parallels_read_format_extension(
-bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
-if (ret < 0) {
-goto fail;
-}
+ret = parallels_read_format_extension(
+bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
+if (ret < 0) {
+goto fail;
}
}
Reviewed-by: Denis V. Lunev
Re: [PATCH v4 10/21] parallels: Create used bitmap even if checks needed
On 12/28/23 11:12, Alexander Ivanov wrote:
All the checks were fixed to work with used bitmap. Create used bitmap in
parallels_open() even if need_check is true.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 0ae06ec0b1..f38dd2309c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1421,13 +1421,11 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
need_check = need_check || s->data_end > file_nb_sectors;
-if (!need_check) {
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-goto fail;
-}
-need_check = need_check || ret < 0; /* These are correctable errors */
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+goto fail;
}
+need_check = need_check || ret < 0; /* These are correctable errors */
/*
* We don't repair the image here if it's opened for checks. Also we don't
Why do we need it? Most likely we will have to recreate it
on a error. If there is some sense - we need a real motivation
why do we need used bitmap.
Here, at this point, we have already detected that there is
something very bad happened and we can have too long file
or something like that.
Re: [PATCH v4 09/21] parallels: Add a note about used bitmap in parallels_check_duplicate()
On 12/28/23 11:12, Alexander Ivanov wrote: In parallels_check_duplicate() We use a bitmap for duplication detection. This bitmap is not related to used_bmap field in BDRVParallelsState. Add a comment about it to avoid confusion. Signed-off-by: Alexander Ivanov --- block/parallels.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/parallels.c b/block/parallels.c index 04c114f696..0ae06ec0b1 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -837,7 +837,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. Reviewed-by: Denis V. Lunev
Re: [PATCH v4 08/21] parallels: Recreate used bitmap in parallels_check_leak()
On 12/28/23 11:12, Alexander Ivanov wrote:
In parallels_check_leak() file can be truncated. In this case the used
bitmap would not comply to the file. Recreate the bitmap after file
truncation.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 8
1 file changed, 8 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 8a6e2ba7ee..04c114f696 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -807,6 +807,14 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
return ret;
}
s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+res->check_errors++;
+return ret;
+}
+
if (explicit) {
res->leaks_fixed += count;
}
Reviewed-by: Denis V. Lunev
Re: [PATCH v4 07/21] parallels: Set data_end value in parallels_check_leak()
On 12/28/23 11:12, Alexander Ivanov wrote:
In parallels_check_leak() we change file size but don't correct data_end
field of BDRVParallelsState structure. Fix it.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels.c b/block/parallels.c
index 658902ae51..8a6e2ba7ee 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -806,6 +806,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
res->check_errors++;
return ret;
}
+s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
if (explicit) {
res->leaks_fixed += count;
}
Reviewed-by: Denis V. Lunev
Re: [PATCH v4 06/21] parallels: Move host clusters allocation to a separate function
ize;
host_off = s->data_start * BDRV_SECTOR_SIZE;
host_off += first_free * s->cluster_size;
@@ -349,14 +322,59 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
*/
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
host_off < s->data_end * BDRV_SECTOR_SIZE) {
-ret = bdrv_co_pwrite_zeroes(bs->file, host_off,
-s->cluster_size * to_allocate, 0);
+ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
if (ret < 0) {
return ret;
}
}
}
+ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
+ host_off, *clusters);
+if (ret < 0) {
+/* Image consistency is broken. Alarm! */
+return ret;
+}
+
+return host_off;
+}
+
+static int64_t coroutine_fn GRAPH_RDLOCK
+allocate_clusters(BlockDriverState *bs, int64_t sector_num,
+ int nb_sectors, int *pnum)
+{
+int ret = 0;
+BDRVParallelsState *s = bs->opaque;
+int64_t i, pos, idx, to_allocate, host_off;
+
+pos = block_status(s, sector_num, nb_sectors, pnum);
+if (pos > 0) {
+return pos;
+}
+
+idx = sector_num / s->tracks;
+to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
+
+/*
+ * This function is called only by parallels_co_writev(), which will never
+ * pass a sector_num at or beyond the end of the image (because the block
+ * layer never passes such a sector_num to that function). Therefore, idx
+ * is always below s->bat_size.
+ * block_status() will limit *pnum so that sector_num + *pnum will not
+ * exceed the image end. Therefore, idx + to_allocate cannot exceed
+ * s->bat_size.
+ * Note that s->bat_size is an unsigned int, therefore idx + to_allocate
+ * will always fit into a uint32_t.
+ */
+assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
+
+host_off = parallels_allocate_host_clusters(bs, _allocate);
+if (host_off < 0) {
+return host_off;
+}
+
+*pnum = MIN(*pnum, (idx + to_allocate) * s->tracks - sector_num);
+
/*
* Try to read from backing to fill empty clusters
* FIXME: 1. previous write_zeroes may be redundant
@@ -373,33 +391,23 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
ret = bdrv_co_pread(bs->backing, idx * s->tracks * BDRV_SECTOR_SIZE,
nb_cow_bytes, buf, 0);
-if (ret < 0) {
-qemu_vfree(buf);
-return ret;
+if (ret == 0) {
+ret = bdrv_co_pwrite(bs->file, host_off, nb_cow_bytes, buf, 0);
}
-ret = bdrv_co_pwrite(bs->file, s->data_end * BDRV_SECTOR_SIZE,
- nb_cow_bytes, buf, 0);
qemu_vfree(buf);
if (ret < 0) {
+parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
+ host_off, to_allocate);
return ret;
}
}
-ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
- host_off, to_allocate);
-if (ret < 0) {
-/* Image consistency is broken. Alarm! */
-return ret;
-}
for (i = 0; i < to_allocate; i++) {
parallels_set_bat_entry(s, idx + i,
host_off / BDRV_SECTOR_SIZE / s->off_multiplier);
host_off += s->cluster_size;
}
-if (host_off > s->data_end * BDRV_SECTOR_SIZE) {
-s->data_end = host_off / BDRV_SECTOR_SIZE;
-}
return bat2sect(s, idx) + sector_num % s->tracks;
}
diff --git a/block/parallels.h b/block/parallels.h
index 02b857b4a4..493c89e976 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -95,6 +95,9 @@ int parallels_mark_used(BlockDriverState *bs, unsigned long
*bitmap,
int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
uint32_t bitmap_size, int64_t off, uint32_t count);
+int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
+ int64_t *clusters);
+
int GRAPH_RDLOCK
parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
Error **errp);
There is a difference in between what we have had before
this patch and after. On a error originally we have had
data_end unchanged, now it points to a wrong location.
May be this would be mitigated later, but I'd prefer
to have data_end updated in mark_unused. That would make
a lot of sense.
Anyway, with data_end dropped at the end of the series,
this would not worth efforts. Thus this is fine.
With a note about comment,
Reviewed-by: Denis V. Lunev
Re: [PATCH v4 05/21] parallels: Add parallels_mark_unused() helper
On 12/28/23 11:12, Alexander Ivanov wrote:
Add a helper to set unused areas in the used bitmap.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 18 ++
block/parallels.h | 2 ++
2 files changed, 20 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 4470519656..13726fb3d5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -196,6 +196,24 @@ int parallels_mark_used(BlockDriverState *bs, unsigned
long *bitmap,
return 0;
}
+int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count)
+{
+BDRVParallelsState *s = bs->opaque;
+uint32_t cluster_end, cluster_index = host_cluster_index(s, off);
+unsigned long next_unused;
+cluster_end = cluster_index + count;
+if (cluster_end > bitmap_size) {
+return -E2BIG;
+}
+next_unused = find_next_zero_bit(bitmap, cluster_end, cluster_index);
+if (next_unused < cluster_end) {
+return -EINVAL;
+}
+bitmap_clear(bitmap, cluster_index, count);
+return 0;
+}
+
/*
* Collect used bitmap. The image can contain errors, we should fill the
* bitmap anyway, as much as we can. This information will be used for
diff --git a/block/parallels.h b/block/parallels.h
index 68077416b1..02b857b4a4 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -92,6 +92,8 @@ typedef struct BDRVParallelsState {
int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
uint32_t bitmap_size, int64_t off, uint32_t count);
+int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count);
int GRAPH_RDLOCK
parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
Reviewed-by: Denis V. Lunev
Re: [PATCH v4 04/21] parallels: Limit search in parallels_mark_used to the last marked claster
On 12/28/23 11:12, Alexander Ivanov wrote:
There is no necessity to search to the end of the bitmap. Limit the search
area as cluster_index + count.
Add cluster_end variable to avoid its calculation in a few places.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index ae524f1820..4470519656 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -182,13 +182,14 @@ int parallels_mark_used(BlockDriverState *bs, unsigned
long *bitmap,
uint32_t bitmap_size, int64_t off, uint32_t count)
{
BDRVParallelsState *s = bs->opaque;
-uint32_t cluster_index = host_cluster_index(s, off);
+uint32_t cluster_end, cluster_index = host_cluster_index(s, off);
unsigned long next_used;
-if (cluster_index + count > bitmap_size) {
+cluster_end = cluster_index + count;
+if (cluster_end > bitmap_size) {
return -E2BIG;
}
-next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
-if (next_used < cluster_index + count) {
+next_used = find_next_bit(bitmap, cluster_end, cluster_index);
+if (next_used < cluster_end) {
return -EBUSY;
}
bitmap_set(bitmap, cluster_index, count);
Reviewed-by: Denis V. Lunev
Re: [PATCH] block: allocate aligned write buffer for 'truncate -m full'
On 12/11/23 11:55, Andrey Drobyshev wrote:
In case we're truncating an image opened with O_DIRECT, we might get
-EINVAL on write with unaligned buffer. In particular, when running
iotests/298 with '-nocache' we get:
qemu-io: Failed to resize underlying file: Could not write zeros for
preallocation: Invalid argument
Let's just allocate the buffer using qemu_blockalign0() instead.
Signed-off-by: Andrey Drobyshev
---
block/file-posix.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index b862406c71..cee8de510b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2354,7 +2354,7 @@ static int handle_aiocb_truncate(void *opaque)
goto out;
}
-buf = g_malloc0(65536);
+buf = qemu_blockalign0(aiocb->bs, 65536);
seek_result = lseek(fd, current_length, SEEK_SET);
if (seek_result < 0) {
@@ -2413,7 +2413,7 @@ out:
}
}
-g_free(buf);
+qemu_vfree(buf);
return result;
}
Reviewed-by: Denis V. Lunev
Re: [PATCH 2/3] i386: kvm: disable KVM_CAP_PMU_CAPABILITY if "pmu" is disabled
On 11/19/22 13:29, Dongli Zhang wrote:
The "perf stat" at the VM side still works even we set "-cpu host,-pmu" in
the QEMU command line. That is, neither "-cpu host,-pmu" nor "-cpu EPYC"
could disable the pmu virtualization in an AMD environment.
We still see below at VM kernel side ...
[0.510611] Performance Events: Fam17h+ core perfctr, AMD PMU driver.
... although we expect something like below.
[0.596381] Performance Events: PMU not available due to virtualization,
using software events only.
[0.600972] NMI watchdog: Perf NMI watchdog permanently disabled
This is because the AMD pmu (v1) does not rely on cpuid to decide if the
pmu virtualization is supported.
We disable KVM_CAP_PMU_CAPABILITY if the 'pmu' is disabled in the vcpu
properties.
Cc: Joe Jin
Signed-off-by: Dongli Zhang
---
target/i386/kvm/kvm.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 8fec0bc5b5..0b1226ff7f 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -137,6 +137,8 @@ static int has_triple_fault_event;
static bool has_msr_mcg_ext_ctl;
+static int has_pmu_cap;
+
static struct kvm_cpuid2 *cpuid_cache;
static struct kvm_cpuid2 *hv_cpuid_cache;
static struct kvm_msr_list *kvm_feature_msrs;
@@ -1725,6 +1727,19 @@ static void kvm_init_nested_state(CPUX86State *env)
void kvm_arch_pre_create_vcpu(CPUState *cs)
{
+X86CPU *cpu = X86_CPU(cs);
+int ret;
+
+if (has_pmu_cap && !cpu->enable_pmu) {
+ret = kvm_vm_enable_cap(kvm_state, KVM_CAP_PMU_CAPABILITY, 0,
+KVM_PMU_CAP_DISABLE);
+if (ret < 0) {
+error_report("kvm: Failed to disable pmu cap: %s",
+ strerror(-ret));
+}
+
+has_pmu_cap = 0;
+}
}
int kvm_arch_init_vcpu(CPUState *cs)
@@ -2517,6 +2532,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
}
}
+has_pmu_cap = kvm_check_extension(s, KVM_CAP_PMU_CAPABILITY);
+
ret = kvm_get_supported_msrs(s);
if (ret < 0) {
return ret;
This patch is very important in particular.
It boosts performance of any single VMexit
is 13% for AMD. Intel is being measured.
At my opinion v1 of the patch is better that
version 2. We should not introduce any
new capability but disable PMU if we can
while it is disabled according to the configuration.
The discussion about performance improvement
is here
https://lore.kernel.org/lkml/zu2d3f6kc0mdz...@google.com/T/
Den
Re: [PATCH v2 1/1] qemu-img: do not erase destination file in qemu-img dd command
On 11/1/23 17:51, Daniel P. Berrangé wrote: On Tue, Oct 31, 2023 at 03:33:52PM +0100, Hanna Czenczek wrote: On 01.10.23 22:46, Denis V. Lunev wrote: Can you please not top-post. This makes the discussion complex. This approach is followed in this mailing list and in other similar lists like LKML. On 10/1/23 19:08, Mike Maslenkin wrote: I thought about "conv=notrunc", but my main concern is changed virtual disk metadata. It depends on how qemu-img used. May be I followed to wrong pattern, but pros and cons of adding "conv" parameter was not in my mind in scope of the first patch version. I see 4 obvious ways of using `qemu-img dd`: 1. Copy virtual disk data between images of same format. I think disk geometry must be preserved in this case. 2. Copy virtual disk data between different formats. It is a valid pattern? May be `qemu-img convert` should to be used instead? 3. Merge snapshots to specified disk image, i.e read current state and write it to new disk image. 4. Copy virtual disk data to raw binary file. Actually this patch breaks 'dd' behavior for this case when source image is less (in terms of logical blocks) than existed raw binary file. May be for this case condition can be improved to smth like if (strcmp(fmt, "raw") || !g_file_test(out.filename, G_FILE_TEST_EXISTS)) . And parameter "conv=notrunc" may be implemented additionally for this case. My personal opinion is that qemu dd when you will need to extract the SOME data from the original image and process it further. Thus I use it to copy some data into raw binary file. My next goal here would add ability to put data into stdout that would be beneficial for. Though this is out of the equation at the moment. Though, speaking about the approach, I would say that the patch changes current behavior which is not totally buggy under a matter of this or that taste. It should be noted that we are here in Linux world, not in the Mac world where we were in position to avoid options and selections. Thus my opinion that original behavior is to be preserved as somebody is relying on it. The option you are proposing seems valuable to me also and thus the switch is to be added. The switch is well-defined in the original 'dd' world thus either conv= option would be good, either nocreat or notrunc. For me 'nocreat' seems more natural. Anyway, the last word here belongs to either Hanna or Kevin ;) Personally, and honestly, I see no actual use for qemu-img dd at all, because we’re trying to mimic a subset of an interface of a rather complex program that has been designed to do what it does. We can only fail at that. Personally, whenever I need dd functionality, I use qemu-storage-daemon’s fuse export, and then use the actual dd program on top. Alternatively, qemu-img convert is our native interface; unfortunately, its feature set is lacking when compared to qemu-img dd, but I think it would be better to improve that rather than working on qemu-img dd. Is there a clear view of what gaps exist in 'qemu-img convert', and more importantly, how much work is it to close the gaps, such that 'dd' could potentially be deprecated & eventually removed ? I am using 'qemu-img dd' as a way to get (some) content from the image. I have dreamed about getting it to stdout. Den
Re: [PATCH] kvm: emit GUEST_PANICKED event in case of abnormal KVM exit
On 11/1/23 16:23, Andrey Drobyshev wrote:
Currently we emit GUEST_PANICKED event in case kvm_vcpu_ioctl() returns
KVM_EXIT_SYSTEM_EVENT with the event type KVM_SYSTEM_EVENT_CRASH. Let's
extend this scenario and emit GUEST_PANICKED in case of an abnormal KVM
exit. That's a natural thing to do since in this case guest is no
longer operational anyway.
Signed-off-by: Andrey Drobyshev
Acked-by: Denis V. Lunev
---
accel/kvm/kvm-all.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index e39a810a4e..d74b3f0b0e 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2816,6 +2816,14 @@ static void kvm_eat_signals(CPUState *cpu)
} while (sigismember(, SIG_IPI));
}
+static void kvm_emit_guest_crash(CPUState *cpu)
+{
+kvm_cpu_synchronize_state(cpu);
+qemu_mutex_lock_iothread();
+qemu_system_guest_panicked(cpu_get_crash_info(cpu));
+qemu_mutex_unlock_iothread();
+}
+
int kvm_cpu_exec(CPUState *cpu)
{
struct kvm_run *run = cpu->kvm_run;
@@ -2969,21 +2977,24 @@ int kvm_cpu_exec(CPUState *cpu)
ret = EXCP_INTERRUPT;
break;
case KVM_SYSTEM_EVENT_CRASH:
-kvm_cpu_synchronize_state(cpu);
-qemu_mutex_lock_iothread();
-qemu_system_guest_panicked(cpu_get_crash_info(cpu));
-qemu_mutex_unlock_iothread();
+kvm_emit_guest_crash(cpu);
ret = 0;
break;
default:
DPRINTF("kvm_arch_handle_exit\n");
ret = kvm_arch_handle_exit(cpu, run);
+if (ret < 0) {
+kvm_emit_guest_crash(cpu);
+}
break;
}
break;
default:
DPRINTF("kvm_arch_handle_exit\n");
ret = kvm_arch_handle_exit(cpu, run);
+if (ret < 0) {
+kvm_emit_guest_crash(cpu);
+}
break;
}
} while (ret == 0);
This allows to gracefully handle this problem in production
and reset the guest using on_crash action in libvirt.
Re: [PATCH v3 04/21] parallels: Add parallels_mark_unused() helper
On 10/30/23 10:06, Denis V. Lunev wrote:
On 10/27/23 09:46, Alexander Ivanov wrote:
Add a helper to set unused areas in the used bitmap.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 17 +
block/parallels.h | 2 ++
2 files changed, 19 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index e9a8cbe430..a30bb5fe0d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -195,6 +195,23 @@ int parallels_mark_used(BlockDriverState *bs,
unsigned long *bitmap,
return 0;
}
+int parallels_mark_unused(BlockDriverState *bs, unsigned long
*bitmap,
+ uint32_t bitmap_size, int64_t off,
uint32_t count)
+{
+ BDRVParallelsState *s = bs->opaque;
+ uint32_t cluster_index = host_cluster_index(s, off);
+ unsigned long next_unused;
+ if (cluster_index + count > bitmap_size) {
+ return -E2BIG;
+ }
+ next_unused = find_next_zero_bit(bitmap, bitmap_size,
cluster_index);
+ if (next_unused < cluster_index + count) {
+ return -EINVAL;
+ }
I would limit the search with 'off + count'.
There is no necessity to traverse the bitmap further.
Den
I mean 'cluster_index + off' to avoid the confusion.
Den
Re: [PATCH v3 04/21] parallels: Add parallels_mark_unused() helper
On 10/27/23 09:46, Alexander Ivanov wrote:
Add a helper to set unused areas in the used bitmap.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 17 +
block/parallels.h | 2 ++
2 files changed, 19 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index e9a8cbe430..a30bb5fe0d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -195,6 +195,23 @@ int parallels_mark_used(BlockDriverState *bs, unsigned
long *bitmap,
return 0;
}
+int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count)
+{
+BDRVParallelsState *s = bs->opaque;
+uint32_t cluster_index = host_cluster_index(s, off);
+unsigned long next_unused;
+if (cluster_index + count > bitmap_size) {
+return -E2BIG;
+}
+next_unused = find_next_zero_bit(bitmap, bitmap_size, cluster_index);
+if (next_unused < cluster_index + count) {
+return -EINVAL;
+}
I would limit the search with 'off + count'.
There is no necessity to traverse the bitmap further.
Den
Re: [PATCH v3 03/21] parallels: Make mark_used() a global function
On 10/27/23 09:46, Alexander Ivanov wrote:
We will need this function and a function for marking unused clusters (will
be added in the next patch) in parallels-ext.c too. Let it be a global
function parallels_mark_used().
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 14 --
block/parallels.h | 3 +++
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 8962bc9fe5..e9a8cbe430 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,8 +178,8 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block,
1);
}
-static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
- uint32_t bitmap_size, int64_t off, uint32_t count)
+int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
+uint32_t bitmap_size, int64_t off, uint32_t count)
{
BDRVParallelsState *s = bs->opaque;
uint32_t cluster_index = host_cluster_index(s, off);
@@ -232,7 +232,8 @@ static int parallels_fill_used_bitmap(BlockDriverState *bs)
continue;
}
-err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 1);
+err2 = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
+ host_off, 1);
if (err2 < 0 && err == 0) {
err = err2;
}
@@ -366,7 +367,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
}
}
-ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, to_allocate);
+ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
+ host_off, to_allocate);
if (ret < 0) {
/* Image consistency is broken. Alarm! */
return ret;
@@ -831,7 +833,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
continue;
}
-ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
+ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
assert(ret != -E2BIG);
if (ret == 0) {
continue;
@@ -891,7 +893,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
* considered, and the bitmap size doesn't change. This specifically
* means that -E2BIG is OK.
*/
-ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
+ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
if (ret == -EBUSY) {
res->check_errors++;
goto out_repair_bat;
diff --git a/block/parallels.h b/block/parallels.h
index 6b199443cf..bb18ee0527 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -90,6 +90,9 @@ typedef struct BDRVParallelsState {
Error *migration_blocker;
} BDRVParallelsState;
+int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
+uint32_t bitmap_size, int64_t off, uint32_t count);
+
int parallels_read_format_extension(BlockDriverState *bs,
int64_t ext_off, Error **errp);
Reviewed-by: Denis V. Lunev
Re: [PATCH v3 02/21] parallels: Move inactivation code to a separate function
On 10/27/23 09:46, Alexander Ivanov wrote:
We are going to add parallels image extensions storage and need a separate
function for inactivation code.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 01a61a4ebd..8962bc9fe5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1428,18 +1428,27 @@ fail:
return ret;
}
+static int parallels_inactivate(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+int ret;
+
+s->header->inuse = 0;
+parallels_update_header(bs);
+
+/* errors are ignored, so we might as well pass exact=true */
+ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
+PREALLOC_MODE_OFF, 0, NULL);
+
+return ret;
+}
static void parallels_close(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE)) {
-s->header->inuse = 0;
-parallels_update_header(bs);
-
-/* errors are ignored, so we might as well pass exact=true */
-bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
- PREALLOC_MODE_OFF, 0, NULL);
+parallels_inactivate(bs);
}
parallels_free_used_bitmap(bs);
@@ -1478,6 +1487,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_check = parallels_co_check,
.bdrv_co_pdiscard = parallels_co_pdiscard,
.bdrv_co_pwrite_zeroes = parallels_co_pwrite_zeroes,
+.bdrv_inactivate= parallels_inactivate,
};
static void bdrv_parallels_init(void)
Reviewed-by: Denis V. Lunev
Re: [PATCH v3 01/21] parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap()
On 10/27/23 09:46, Alexander Ivanov wrote: After used bitmap freeng s->used_bmap points to the freed memory. If we try to free used bitmap one more time it leads to double free error. Set s->used_bmap to NULL to exclude double free error. Signed-off-by: Alexander Ivanov --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index 1d695ce7fb..01a61a4ebd 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -245,6 +245,7 @@ static void parallels_free_used_bitmap(BlockDriverState *bs) BDRVParallelsState *s = bs->opaque; s->used_bmap_size = 0; g_free(s->used_bmap); +s->used_bmap = NULL; } static int64_t coroutine_fn GRAPH_RDLOCK Reviewed-by: Denis V. Lunev
Re: [PATCH v2 0/3] vfio/pci: Fix buffer overrun when writing the VF token
On 10/26/23 09:06, Cédric Le Goater wrote: Hello, This series fixes a buffer overrun in VFIO. The buffer used in vfio_realize() by qemu_uuid_unparse() is too small, UUID_FMT_LEN lacks one byte for the trailing NUL. Instead of adding + 1, as done elsewhere, the changes introduce a UUID_STR_LEN define for the correct size and use it where required. Thanks, C. Changes in v2: - removal of UUID_FMT_LEN Cédric Le Goater (3): util/uuid: Add UUID_STR_LEN definition vfio/pci: Fix buffer overrun when writing the VF token util/uuid: Remove UUID_FMT_LEN include/qemu/uuid.h | 2 +- block/parallels-ext.c| 2 +- block/vdi.c | 2 +- hw/core/qdev-properties-system.c | 2 +- hw/hyperv/vmbus.c| 4 ++-- hw/vfio/pci.c| 2 +- migration/savevm.c | 4 ++-- tests/unit/test-uuid.c | 2 +- util/uuid.c | 2 +- 9 files changed, 11 insertions(+), 11 deletions(-) Reviwed-by: Denis V. Lunev
Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
On 9/7/23 23:53, Denis V. Lunev wrote:
Unfortunately 271 IO test is broken if started in non-cached mode.
Commits
commit a6b257a08e3d72219f03e461a52152672fec0612
Author: Nir Soffer
Date: Tue Aug 13 21:21:03 2019 +0300
file-posix: Handle undetectable alignment
and
commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
Author: Kevin Wolf
Date: Thu Jul 16 16:26:00 2020 +0200
block: Require aligned image size to avoid assertion failure
have interesting side effect if used togather.
If the image size is not multiple of 4k and that image falls under
original constraints of Nil's patch, the image can not be opened
due to the check in the bdrv_check_perm().
The patch tries to satisfy the requirements of bdrv_check_perm()
inside raw_probe_alignment(). This is at my opinion better that just
disallowing to run that test in non-cached mode. The operation is legal
by itself.
Signed-off-by: Denis V. Lunev
CC: Nir Soffer
CC: Kevin Wolf
CC: Hanna Reitz
CC: Alberto Garcia
---
block/file-posix.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index b16e9c21a1..988cfdc76c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -447,8 +447,21 @@ static void raw_probe_alignment(BlockDriverState *bs, int
fd, Error **errp)
for (i = 0; i < ARRAY_SIZE(alignments); i++) {
align = alignments[i];
if (raw_is_io_aligned(fd, buf, align)) {
-/* Fallback to safe value. */
-bs->bl.request_alignment = (align != 1) ? align : max_align;
+if (align != 1) {
+bs->bl.request_alignment = align;
+break;
+}
+/*
+ * Fallback to safe value. max_align is perfect, but the size
of the device must be multiple of
+ * the virtual length of the device. In the other case we will
get a error in
+ * bdrv_node_refresh_perm().
+ */
+for (align = max_align; align > 1; align /= 2) {
+if ((bs->total_sectors * BDRV_SECTOR_SIZE) % align == 0) {
+break;
+}
+}
+bs->bl.request_alignment = align;
break;
}
}
ping
Re: [PATCH v2 03/21] preallocate: Don't poll during permission updates
On 10/6/23 20:10, Vladimir Sementsov-Ogievskiy wrote: On 06.10.23 11:56, Kevin Wolf wrote: Am 05.10.2023 um 21:55 hat Vladimir Sementsov-Ogievskiy geschrieben: On 11.09.23 12:46, Kevin Wolf wrote: When the permission related BlockDriver callbacks are called, we are in the middle of an operation traversing the block graph. Polling in such a place is a very bad idea because the graph could change in unexpected ways. In the future, callers will also hold the graph lock, which is likely to turn polling into a deadlock. So we need to get rid of calls to functions like bdrv_getlength() or bdrv_truncate() there as these functions poll internally. They are currently used so that when no parent has write/resize permissions on the image any more, the preallocate filter drops the extra preallocated area in the image file and gives up write/resize permissions itself. In order to achieve this without polling in .bdrv_check_perm, don't immediately truncate the image, but only schedule a BH to do so. The filter keeps the write/resize permissions a bit longer now until the BH has executed. There is one case in which delaying doesn't work: Reopening the image read-only. In this case, bs->file will likely be reopened read-only, too, so keeping write permissions a bit longer on it doesn't work. But we can already cover this case in preallocate_reopen_prepare() and not rely on the permission updates for it. Hmm, now I found one more "future" case. I now try to rebase my "[PATCH v7 0/7] blockdev-replace" https://patchew.org/QEMU/20230421114102.884457-1-vsement...@yandex-team.ru/ And it breaks after this commit. By accident, blockdev-replace series uses exactly "preallocate" filter to test insertion/removing of filters. And removing is broken now. Removing is done as follows: 1. We have filter inserted: disk0 -file-> filter -file-> file0 2. blockdev-replace, replaces file child of disk0, so we should get the picture*: disk0 -file-> file0 <-file- filter 3. blockdev-del filter But step [2] fails, as now preallocate filter doesn't drop permissions during the operation (postponing this for a while) and the picture* is impossible. Permission check fails. Hmmm... Any idea how blockdev-replace and preallocate filter should work :) ? Maybe, doing truncation in .drain_begin() will help? Will try Hm... What preallocate tries to do is really tricky... Of course, the error is correct, this is an invalid configuration if preallocate can still resize the image. So it would have to truncate the file earlier, but the first time that preallocate knows of the change is already too late to run requests. Truncating on drain_begin feels more like a hack, but as long as it does the job... Of course, this will have the preallocation truncated away on events that have nothing to do with removing the filter. It's not necessarily a disaster because preallocation is only an optimisation, but it doesn't feel great. Hmm, yes, that's not good. Maybe let's take a step back: Which scenario is the preallocate driver meant for and why do we even need to truncate the image file after removing the filter? I suppose the filter doesn't make sense with raw images because these are fixed size anyway, and pretty much any other image format should be able to tolerate a permanently rounded up file size. As long as you don't write to the preallocated area, it shouldn't take space either on any sane filesystem. Hmm, actually both VHD and VMDK can have footers, better avoid it with those... But if truncating the image file on close is critical, what do you do on crashes? Maybe preallocate should just not be considered compatible with these formats? Originally preallocate filter was made to be used with qcow2, on some proprietary storage, where: 1. Allocating of big chunk works a lot fater than allocating several smaller chunks 2. Holes are not free and/or file length is not free, so we really want to truncate the file back on close Den, correct me if I'm wrong. 1. Absolutely correct. This is true when the file attributes are stored in a centralized place aka metadata storage and requests to it does not scale well. 2. This is at my opinion has different meaning. We have tried to make local storage behavior and distributed storage behavior to be the same when VM is off, i.e. the file should be in the same state (no free blocks at the end of the file). Good thing is that in this scenario we don't need to remove the filter in runtime, so there is no problem. Yes, this filter is not dynamic in that respect. It is either here or not here. Now I think that the generic solution is just add a new handler .bdrv_pre_replace, so blockdev-replace may work as follows: drain_begin call .bdrv_pre_replace for all affected nodes do the replace drain_end And prellocate filter would do truncation in this .bdrv_pre_replace handler and set some flag, that we have nothing to trunctate (the flag is
Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
On 9/7/23 23:53, Denis V. Lunev wrote:
Unfortunately 271 IO test is broken if started in non-cached mode.
Commits
commit a6b257a08e3d72219f03e461a52152672fec0612
Author: Nir Soffer
Date: Tue Aug 13 21:21:03 2019 +0300
file-posix: Handle undetectable alignment
and
commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
Author: Kevin Wolf
Date: Thu Jul 16 16:26:00 2020 +0200
block: Require aligned image size to avoid assertion failure
have interesting side effect if used togather.
If the image size is not multiple of 4k and that image falls under
original constraints of Nil's patch, the image can not be opened
due to the check in the bdrv_check_perm().
The patch tries to satisfy the requirements of bdrv_check_perm()
inside raw_probe_alignment(). This is at my opinion better that just
disallowing to run that test in non-cached mode. The operation is legal
by itself.
Signed-off-by: Denis V. Lunev
CC: Nir Soffer
CC: Kevin Wolf
CC: Hanna Reitz
CC: Alberto Garcia
---
block/file-posix.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index b16e9c21a1..988cfdc76c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -447,8 +447,21 @@ static void raw_probe_alignment(BlockDriverState *bs, int
fd, Error **errp)
for (i = 0; i < ARRAY_SIZE(alignments); i++) {
align = alignments[i];
if (raw_is_io_aligned(fd, buf, align)) {
-/* Fallback to safe value. */
-bs->bl.request_alignment = (align != 1) ? align : max_align;
+if (align != 1) {
+bs->bl.request_alignment = align;
+break;
+}
+/*
+ * Fallback to safe value. max_align is perfect, but the size
of the device must be multiple of
+ * the virtual length of the device. In the other case we will
get a error in
+ * bdrv_node_refresh_perm().
+ */
+for (align = max_align; align > 1; align /= 2) {
+if ((bs->total_sectors * BDRV_SECTOR_SIZE) % align == 0) {
+break;
+}
+}
+bs->bl.request_alignment = align;
break;
}
}
ping v3
Re: [PATCH v2 1/1] qemu-img: do not erase destination file in qemu-img dd command
Can you please not top-post. This makes the discussion complex. This
approach is followed in this mailing list and in other similar lists
like LKML.
On 10/1/23 19:08, Mike Maslenkin wrote:
I thought about "conv=notrunc", but my main concern is changed virtual
disk metadata.
It depends on how qemu-img used.
May be I followed to wrong pattern, but pros and cons of adding "conv"
parameter was not in my mind in scope of the first patch version.
I see 4 obvious ways of using `qemu-img dd`:
1. Copy virtual disk data between images of same format. I think disk
geometry must be preserved in this case.
2. Copy virtual disk data between different formats. It is a valid
pattern? May be `qemu-img convert` should to be used instead?
3. Merge snapshots to specified disk image, i.e read current state and
write it to new disk image.
4. Copy virtual disk data to raw binary file. Actually this patch
breaks 'dd' behavior for this case when source image is less (in terms
of logical blocks) than existed raw binary file.
May be for this case condition can be improved to smth like
if (strcmp(fmt, "raw") || !g_file_test(out.filename,
G_FILE_TEST_EXISTS)) . And parameter "conv=notrunc" may be implemented
additionally for this case.
My personal opinion is that qemu dd when you will need to
extract the SOME data from the original image and process
it further. Thus I use it to copy some data into raw binary
file. My next goal here would add ability to put data into
stdout that would be beneficial for. Though this is out of the
equation at the moment.
Though, speaking about the approach, I would say that the
patch changes current behavior which is not totally buggy
under a matter of this or that taste. It should be noted that
we are here in Linux world, not in the Mac world where we
were in position to avoid options and selections.
Thus my opinion that original behavior is to be preserved
as somebody is relying on it. The option you are proposing
seems valuable to me also and thus the switch is to be added.
The switch is well-defined in the original 'dd' world thus
either conv= option would be good, either nocreat or notrunc.
For me 'nocreat' seems more natural.
Anyway, the last word here belongs to either Hanna or Kevin ;)
Den
Three of above do not require "conv=" parameter from my point of view.
I would be glad to hear other opinions.
Regards,
Mike.
On Sun, Oct 1, 2023 at 3:25 PM Denis V. Lunev wrote:
On 9/30/23 22:31, Mike Maslenkin wrote:
Add a check that destination file exists and do not call bdrv_create for
this case.
Currently `qemu-img dd` command destroys content of destination file.
Effectively this means that parameters (geometry) of destination image
file are changing. This can be undesirable behavior for user especially
if format of destination image does not support resizing.
Steps to reproduce:
1. Create empty disk image with some non default size.
`qemu-img create -f qcow2 $DEST_IMG 3T`
Remember that `qemu-img info $DEST_IMG` returns:
virtual size: 3 TiB (3298534883328 bytes)
disk size: 240 KiB
cluster_size: 65536
2. Run `qemu-img dd -O qcow2 of=$DEST_IMG if=$SRC_IMG bs=1M count=100`
3. Check `qemu-img info $DEST_IMG` output:
virtual size: 100 MiB (104857600 bytes)
disk size: 112 MiB
cluster_size: 65536
Parameters of $DEST_IMG were changed. Actually `qemu-img dd` has created
a new disk based on current default geometry for particular format.
For example for "parallels" format default BAT for 256GB disk is written
to empty file prior writing disk image data.
With this patch virtual disk metadata and geometry of a destination image
are preserved. As another visible change of `qemu-img dd` behavior is that
if destination image is less than source it can finish with error (similar
to "dd" utility):
qemu-img: error while writing to output image file: Input/output error
Signed-off-by: Mike Maslenkin
---
diff from v1: removed additional fprintf call leaved in patch by accident
---
qemu-img.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index a48edb71015c..1a83c14212fb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5150,13 +5150,15 @@ static int img_dd(int argc, char **argv)
size - in.bsz * in.offset, _abort);
}
-ret = bdrv_create(drv, out.filename, opts, _err);
-if (ret < 0) {
-error_reportf_err(local_err,
- "%s: error while creating output image: ",
- out.filename);
-ret = -1;
-goto out;
+if (!g_file_test(out.filename, G_FILE_TEST_EXISTS)) {
+ret = bdrv_create(drv, out.filename, opts, _err);
+if (ret < 0) {
+error_reportf_err(local_err,
+ "%s: error while cre
Re: [PATCH v2 1/1] qemu-img: do not erase destination file in qemu-img dd command
On 9/30/23 22:31, Mike Maslenkin wrote:
Add a check that destination file exists and do not call bdrv_create for
this case.
Currently `qemu-img dd` command destroys content of destination file.
Effectively this means that parameters (geometry) of destination image
file are changing. This can be undesirable behavior for user especially
if format of destination image does not support resizing.
Steps to reproduce:
1. Create empty disk image with some non default size.
`qemu-img create -f qcow2 $DEST_IMG 3T`
Remember that `qemu-img info $DEST_IMG` returns:
virtual size: 3 TiB (3298534883328 bytes)
disk size: 240 KiB
cluster_size: 65536
2. Run `qemu-img dd -O qcow2 of=$DEST_IMG if=$SRC_IMG bs=1M count=100`
3. Check `qemu-img info $DEST_IMG` output:
virtual size: 100 MiB (104857600 bytes)
disk size: 112 MiB
cluster_size: 65536
Parameters of $DEST_IMG were changed. Actually `qemu-img dd` has created
a new disk based on current default geometry for particular format.
For example for "parallels" format default BAT for 256GB disk is written
to empty file prior writing disk image data.
With this patch virtual disk metadata and geometry of a destination image
are preserved. As another visible change of `qemu-img dd` behavior is that
if destination image is less than source it can finish with error (similar
to "dd" utility):
qemu-img: error while writing to output image file: Input/output error
Signed-off-by: Mike Maslenkin
---
diff from v1: removed additional fprintf call leaved in patch by accident
---
qemu-img.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index a48edb71015c..1a83c14212fb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5150,13 +5150,15 @@ static int img_dd(int argc, char **argv)
size - in.bsz * in.offset, _abort);
}
-ret = bdrv_create(drv, out.filename, opts, _err);
-if (ret < 0) {
-error_reportf_err(local_err,
- "%s: error while creating output image: ",
- out.filename);
-ret = -1;
-goto out;
+if (!g_file_test(out.filename, G_FILE_TEST_EXISTS)) {
+ret = bdrv_create(drv, out.filename, opts, _err);
+if (ret < 0) {
+error_reportf_err(local_err,
+ "%s: error while creating output image: ",
+ out.filename);
+ret = -1;
+goto out;
+}
}
/* TODO, we can't honour --image-opts for the target,
may be it would be worth to follow conventional
'dd' approach, i.e. add conv=nocreat option which
will do the trick?
Den
Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
On 9/7/23 23:53, Denis V. Lunev wrote:
Unfortunately 271 IO test is broken if started in non-cached mode.
Commits
commit a6b257a08e3d72219f03e461a52152672fec0612
Author: Nir Soffer
Date: Tue Aug 13 21:21:03 2019 +0300
file-posix: Handle undetectable alignment
and
commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
Author: Kevin Wolf
Date: Thu Jul 16 16:26:00 2020 +0200
block: Require aligned image size to avoid assertion failure
have interesting side effect if used togather.
If the image size is not multiple of 4k and that image falls under
original constraints of Nil's patch, the image can not be opened
due to the check in the bdrv_check_perm().
The patch tries to satisfy the requirements of bdrv_check_perm()
inside raw_probe_alignment(). This is at my opinion better that just
disallowing to run that test in non-cached mode. The operation is legal
by itself.
Signed-off-by: Denis V. Lunev
CC: Nir Soffer
CC: Kevin Wolf
CC: Hanna Reitz
CC: Alberto Garcia
---
block/file-posix.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index b16e9c21a1..988cfdc76c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -447,8 +447,21 @@ static void raw_probe_alignment(BlockDriverState *bs, int
fd, Error **errp)
for (i = 0; i < ARRAY_SIZE(alignments); i++) {
align = alignments[i];
if (raw_is_io_aligned(fd, buf, align)) {
-/* Fallback to safe value. */
-bs->bl.request_alignment = (align != 1) ? align : max_align;
+if (align != 1) {
+bs->bl.request_alignment = align;
+break;
+}
+/*
+ * Fallback to safe value. max_align is perfect, but the size
of the device must be multiple of
+ * the virtual length of the device. In the other case we will
get a error in
+ * bdrv_node_refresh_perm().
+ */
+for (align = max_align; align > 1; align /= 2) {
+if ((bs->total_sectors * BDRV_SECTOR_SIZE) % align == 0) {
+break;
+}
+}
+bs->bl.request_alignment = align;
break;
}
}
ping v2
[PULL v2 16/22] parallels: update used bitmap in allocate_cluster
We should extend the bitmap if the file is extended and set the bit in
the image used bitmap once the cluster is allocated. Sanity check at
that moment also looks like a good idea.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 3df73aa8a0..ec35237119 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -282,6 +282,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
return len;
}
if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
+uint32_t new_usedsize;
+
space += s->prealloc_size;
/*
* We require the expanded size to read back as zero. If the
@@ -305,6 +307,12 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
if (ret < 0) {
return ret;
}
+
+new_usedsize = s->used_bmap_size +
+ (space << BDRV_SECTOR_BITS) / s->cluster_size;
+s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
+ new_usedsize);
+s->used_bmap_size = new_usedsize;
}
/*
@@ -336,6 +344,12 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
}
}
+ret = mark_used(bs, s->used_bmap, s->used_bmap_size,
+s->data_end << BDRV_SECTOR_BITS, to_allocate);
+if (ret < 0) {
+/* Image consistency is broken. Alarm! */
+return ret;
+}
for (i = 0; i < to_allocate; i++) {
parallels_set_bat_entry(s, idx + i, s->data_end / s->off_multiplier);
s->data_end += s->tracks;
--
2.34.1
[PULL v2 17/22] parallels: naive implementation of allocate_clusters with used bitmap
The access to the bitmap is not optimized completely.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 51 ---
1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index ec35237119..ebcdff8736 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -253,7 +253,7 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
{
int ret = 0;
BDRVParallelsState *s = bs->opaque;
-int64_t pos, space, idx, to_allocate, i, len;
+int64_t i, pos, idx, to_allocate, first_free, host_off;
pos = block_status(s, sector_num, nb_sectors, pnum);
if (pos > 0) {
@@ -276,15 +276,13 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
*/
assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
-space = to_allocate * s->tracks;
-len = bdrv_co_getlength(bs->file->bs);
-if (len < 0) {
-return len;
-}
-if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
+first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
+if (first_free == s->used_bmap_size) {
uint32_t new_usedsize;
+int64_t space = to_allocate * s->tracks + s->prealloc_size;
+
+host_off = s->data_end * BDRV_SECTOR_SIZE;
-space += s->prealloc_size;
/*
* We require the expanded size to read back as zero. If the
* user permitted truncation, we try that; but if it fails, we
@@ -313,6 +311,32 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
+} else {
+int64_t next_used;
+next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free);
+
+/* Not enough continuous clusters in the middle, adjust the size */
+if (next_used - first_free < to_allocate) {
+to_allocate = next_used - first_free;
+*pnum = (idx + to_allocate) * s->tracks - sector_num;
+}
+
+host_off = s->data_start * BDRV_SECTOR_SIZE;
+host_off += first_free * s->cluster_size;
+
+/*
+ * No need to preallocate if we are using tail area from the above
+ * branch. In the other case we are likely re-using hole. Preallocate
+ * the space if required by the prealloc_mode.
+ */
+if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
+host_off < s->data_end * BDRV_SECTOR_SIZE) {
+ret = bdrv_co_pwrite_zeroes(bs->file, host_off,
+s->cluster_size * to_allocate, 0);
+if (ret < 0) {
+return ret;
+}
+}
}
/*
@@ -344,15 +368,18 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
}
}
-ret = mark_used(bs, s->used_bmap, s->used_bmap_size,
-s->data_end << BDRV_SECTOR_BITS, to_allocate);
+ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off,
to_allocate);
if (ret < 0) {
/* Image consistency is broken. Alarm! */
return ret;
}
for (i = 0; i < to_allocate; i++) {
-parallels_set_bat_entry(s, idx + i, s->data_end / s->off_multiplier);
-s->data_end += s->tracks;
+parallels_set_bat_entry(s, idx + i,
+host_off / BDRV_SECTOR_SIZE / s->off_multiplier);
+host_off += s->cluster_size;
+}
+if (host_off > s->data_end * BDRV_SECTOR_SIZE) {
+s->data_end = host_off / BDRV_SECTOR_SIZE;
}
return bat2sect(s, idx) + sector_num % s->tracks;
--
2.34.1
[PULL v2 11/22] parallels: add test which will validate data_off fixes through repair
We have only check through self-repair and that proven to be not enough.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/tests/parallels-checks | 17 +
tests/qemu-iotests/tests/parallels-checks.out | 18 ++
2 files changed, 35 insertions(+)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index 5917ee079d..f4ca50295e 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -140,6 +140,23 @@ poke_file "$TEST_IMG" "$DATA_OFF_OFFSET" "\xff\xff\xff\xff"
echo "== check first cluster =="
{ $QEMU_IO -c "read -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+# Clear image
+_make_test_img $SIZE
+
+echo "== TEST DATA_OFF THROUGH REPAIR =="
+
+echo "== write pattern to first cluster =="
+{ $QEMU_IO -c "write -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== spoil data_off field =="
+poke_file "$TEST_IMG" "$DATA_OFF_OFFSET" "\xff\xff\xff\xff"
+
+echo "== repair image =="
+_check_test_img -r all
+
+echo "== check first cluster =="
+{ $QEMU_IO -r -c "read -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/tests/parallels-checks.out
b/tests/qemu-iotests/tests/parallels-checks.out
index 98a3a7f55e..74a5e29260 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -72,4 +72,22 @@ wrote 1048576/1048576 bytes at offset 0
Repairing data_off field has incorrect value
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 DATA_OFF THROUGH REPAIR ==
+== write pattern to first cluster ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== spoil data_off field ==
+== repair image ==
+Repairing data_off field has incorrect value
+The following inconsistencies were found and repaired:
+
+0 leaked clusters
+1 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+== check first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
*** done
--
2.34.1
[PULL v2 13/22] tests: fix broken deduplication check in parallels format test
Original check is broken as supposed reading from 2 different clusters
results in read from the same file offset twice. This is definitely
wrong.
We should be sure that
* the content of both clusters is correct after repair
* clusters are at the different offsets after repair
In order to check the latter we write some content into the first one
and validate that fact.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/tests/parallels-checks | 14 ++
tests/qemu-iotests/tests/parallels-checks.out | 16
2 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index f4ca50295e..df99558486 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -117,14 +117,20 @@ echo "== check second cluster =="
echo "== repair image =="
_check_test_img -r all
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
echo "== check second cluster =="
{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
-echo "== check first cluster on host =="
-printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
+echo "== write another pattern to the first clusters =="
+{ $QEMU_IO -c "write -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
-echo "== check second cluster on host =="
-printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
+echo "== check the second cluster (deduplicated) =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $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 74a5e29260..1325d2b611 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -55,13 +55,21 @@ The following inconsistencies were found and repaired:
Double checking the fixed image now...
No errors were found on the image.
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== check second cluster ==
read 1048576/1048576 bytes at offset 1048576
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-== check first cluster on host ==
-content: 0x11
-== check second cluster on host ==
-content: 0x11
+== write another pattern to the first clusters ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the second cluster (deduplicated) ==
+read 1048576/1048576 bytes at offset 1048576
+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 DATA_OFF CHECK ==
== write pattern to first cluster ==
--
2.34.1
[PULL v2 06/22] parallels: return earlier from parallels_open() function on error
At the beginning of the function we can return immediately until we
really allocate s->header.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 12f38cf70b..bd26c8db63 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1090,7 +1090,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
ret = bdrv_pread(bs->file, 0, sizeof(ph), , 0);
if (ret < 0) {
-goto fail;
+return ret;
}
bs->total_sectors = le64_to_cpu(ph.nb_sectors);
@@ -1110,13 +1110,11 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->tracks = le32_to_cpu(ph.tracks);
if (s->tracks == 0) {
error_setg(errp, "Invalid image: Zero sectors per track");
-ret = -EINVAL;
-goto fail;
+return -EINVAL;
}
if (s->tracks > INT32_MAX/513) {
error_setg(errp, "Invalid image: Too big cluster");
-ret = -EFBIG;
-goto fail;
+return -EFBIG;
}
s->prealloc_size = MAX(s->tracks, s->prealloc_size);
s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
@@ -1124,16 +1122,14 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->bat_size = le32_to_cpu(ph.bat_entries);
if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
error_setg(errp, "Catalog too large");
-ret = -EFBIG;
-goto fail;
+return -EFBIG;
}
size = bat_entry_off(s->bat_size);
s->header_size = ROUND_UP(size, bdrv_opt_mem_align(bs->file->bs));
s->header = qemu_try_blockalign(bs->file->bs, s->header_size);
if (s->header == NULL) {
-ret = -ENOMEM;
-goto fail;
+return -ENOMEM;
}
ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
--
2.34.1
[PULL v2 22/22] tests: extend test 131 to cover availability of the write-zeroes
This patch contains test which minimally tests write-zeroes on top of
working discard.
The following checks are added:
* write 2 clusters, write-zero to the first allocated cluster
* write 2 cluster, write-zero to the half the first allocated cluster
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/131 | 21 +
tests/qemu-iotests/131.out | 22 ++
2 files changed, 43 insertions(+)
diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
index 324008b3f6..3119100e78 100755
--- a/tests/qemu-iotests/131
+++ b/tests/qemu-iotests/131
@@ -105,6 +105,27 @@ _make_test_img $size
{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
{ $QEMU_IO -c "read -P 0 $((CLUSTER_SIZE + CLUSTER_HALF_SIZE))
$CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+echo "== check write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_DBL_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io
| _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check cluster-partial write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_HALF_SIZE $CLUSTER_HALF_SIZE"
"$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+
echo "== allocate with backing =="
# Verify that allocating clusters works fine even when there is a backing
image.
# Regression test for a bug where we would pass a buffer read from the backing
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
index 27df91ca97..86a2d2a49b 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.out
@@ -64,6 +64,28 @@ read 524288/524288 bytes at offset 0
512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 524288/524288 bytes at offset 1572864
512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0x100x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check cluster-partial write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== allocate with backing ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
--
2.34.1
[PULL v2 09/22] tests: ensure that image validation will not cure the corruption
Since
commit cfce1091d55322789582480798a891cbaf66924e
Author: Alexander Ivanov
Date: Tue Jul 18 12:44:29 2023 +0200
parallels: Image repairing in parallels_open()
there is a potential pit fall with calling
qemu-io -c "read"
The image is opened in read-write mode and thus could be potentially
repaired. This could ruin testing process.
The patch forces read-only opening for reads. In that case repairing
is impossible.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/tests/parallels-checks | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index a7a1b357b5..5917ee079d 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -91,7 +91,7 @@ file_size=`stat --printf="%s" "$TEST_IMG"`
echo "file size: $file_size"
echo "== check last cluster =="
-{ $QEMU_IO -c "read -P 0x11 $LAST_CLUSTER_OFF $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $LAST_CLUSTER_OFF $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
# Clear image
_make_test_img $SIZE
@@ -105,19 +105,20 @@ echo "== write another pattern to second cluster =="
{ $QEMU_IO -c "write -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1
| _filter_qemu_io | _filter_testdir
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
echo "== corrupt image =="
poke_file "$TEST_IMG" "$(($BAT_OFFSET + 4))" "\x01\x00\x00\x00"
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
echo "== repair image =="
_check_test_img -r all
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
echo "== check first cluster on host =="
printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
--
2.34.1
[PULL v2 04/22] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts
This patch creates above mentioned helper and moves its usage to the
beginning of parallels_open(). This simplifies parallels_open() a bit.
The patch also ensures that we store prealloc_size on block driver state
always in sectors. This makes code cleaner and avoids wrong opinion at
the assignment that the value is in bytes.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 72 +--
1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index af7be427c9..ae006e7fc7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1025,6 +1025,44 @@ static int parallels_update_header(BlockDriverState *bs)
return bdrv_pwrite_sync(bs->file, 0, size, s->header, 0);
}
+
+static int parallels_opts_prealloc(BlockDriverState *bs, QDict *options,
+ Error **errp)
+{
+int err;
+char *buf;
+int64_t bytes;
+BDRVParallelsState *s = bs->opaque;
+Error *local_err = NULL;
+QemuOpts *opts = qemu_opts_create(_runtime_opts, NULL, 0, errp);
+if (!opts) {
+return -ENOMEM;
+}
+
+err = -EINVAL;
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
+goto done;
+}
+
+bytes = qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
+s->prealloc_size = bytes >> BDRV_SECTOR_BITS;
+buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
+/* prealloc_mode can be downgraded later during allocate_clusters */
+s->prealloc_mode = qapi_enum_parse(_mode_lookup, buf,
+ PRL_PREALLOC_MODE_FALLOCATE,
+ _err);
+g_free(buf);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+goto done;
+}
+err = 0;
+
+done:
+qemu_opts_del(opts);
+return err;
+}
+
static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -1033,11 +1071,13 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
int ret, size, i;
int64_t file_nb_sectors, sector;
uint32_t data_start;
-QemuOpts *opts = NULL;
-Error *local_err = NULL;
-char *buf;
bool data_off_is_correct;
+ret = parallels_opts_prealloc(bs, options, errp);
+if (ret < 0) {
+return ret;
+}
+
ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
if (ret < 0) {
return ret;
@@ -1078,6 +1118,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
ret = -EFBIG;
goto fail;
}
+s->prealloc_size = MAX(s->tracks, s->prealloc_size);
s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
s->bat_size = le32_to_cpu(ph.bat_entries);
@@ -1117,29 +1158,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->header_size = size;
}
-opts = qemu_opts_create(_runtime_opts, NULL, 0, errp);
-if (!opts) {
-goto fail_options;
-}
-
-if (!qemu_opts_absorb_qdict(opts, options, errp)) {
-goto fail_options;
-}
-
-s->prealloc_size =
-qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
-s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
-buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
-/* prealloc_mode can be downgraded later during allocate_clusters */
-s->prealloc_mode = qapi_enum_parse(_mode_lookup, buf,
- PRL_PREALLOC_MODE_FALLOCATE,
- _err);
-g_free(buf);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-goto fail_options;
-}
-
if (ph.ext_off) {
if (flags & BDRV_O_RDWR) {
/*
@@ -1214,10 +1232,8 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
fail_format:
error_setg(errp, "Image not in Parallels format");
-fail_options:
ret = -EINVAL;
fail:
-qemu_opts_del(opts);
/*
* "s" object was allocated by g_malloc0 so we can safely
* try to free its fields even they were not allocated.
--
2.34.1
[PULL v2 10/22] parallels: fix broken parallels_check_data_off()
Once we have repaired data_off field in the header we should update
s->data_start which is calculated on the base of it.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels.c b/block/parallels.c
index 66c86d87e3..2b5f2b54a0 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -531,6 +531,7 @@ parallels_check_data_off(BlockDriverState *bs,
BdrvCheckResult *res,
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
s->header->data_off = cpu_to_le32(data_off);
+s->data_start = data_off;
res->corruptions_fixed++;
}
--
2.34.1
[PULL v2 15/22] parallels: accept multiple clusters in mark_used()
This would be useful in the next patch in allocate_clusters(). This
change would not imply serious performance drawbacks as usually image
is full of data or are at the end of the bitmap.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index a997880c34..3df73aa8a0 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,18 +178,20 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block,
1);
}
-static int mark_used(BlockDriverState *bs,
- unsigned long *bitmap, uint32_t bitmap_size, int64_t off)
+static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count)
{
BDRVParallelsState *s = bs->opaque;
uint32_t cluster_index = host_cluster_index(s, off);
-if (cluster_index >= bitmap_size) {
+unsigned long next_used;
+if (cluster_index + count > bitmap_size) {
return -E2BIG;
}
-if (test_bit(cluster_index, bitmap)) {
+next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
+if (next_used < cluster_index + count) {
return -EBUSY;
}
-bitmap_set(bitmap, cluster_index, 1);
+bitmap_set(bitmap, cluster_index, count);
return 0;
}
@@ -230,7 +232,7 @@ static int parallels_fill_used_bitmap(BlockDriverState *bs)
continue;
}
-err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off);
+err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 1);
if (err2 < 0 && err == 0) {
err = err2;
}
@@ -732,7 +734,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
continue;
}
-ret = mark_used(bs, bitmap, bitmap_size, host_off);
+ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
assert(ret != -E2BIG);
if (ret == 0) {
continue;
@@ -792,7 +794,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
* considered, and the bitmap size doesn't change. This specifically
* means that -E2BIG is OK.
*/
-ret = mark_used(bs, bitmap, bitmap_size, host_off);
+ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
if (ret == -EBUSY) {
res->check_errors++;
goto out_repair_bat;
--
2.34.1
[PULL v2 20/22] tests: extend test 131 to cover availability of the discard operation
This patch contains test which minimally tests discard and new cluster
allocation logic.
The following checks are added:
* write 2 clusters, discard the first allocated
* write another cluster, check that the hole is filled
* write 2 clusters, discard the first allocated, write 1 cluster at
non-aligned to cluster offset (2 new clusters should be allocated)
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/131 | 31 +++
tests/qemu-iotests/131.out | 38 ++
2 files changed, 69 insertions(+)
diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
index 304bbb3f61..324008b3f6 100755
--- a/tests/qemu-iotests/131
+++ b/tests/qemu-iotests/131
@@ -74,6 +74,37 @@ poke_file "$TEST_IMG" "$inuse_offset" "\x59\x6e\x6f\x74"
echo "== read corrupted image with repairing =="
{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+echo "== check discard =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_DBL_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "discard 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io
| _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check simple allocation over the discarded hole =="
+
+{ $QEMU_IO -c "write -P 0x11 $CLUSTER_DBL_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_DBL_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== check more complex allocation over the discard hole =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 $CLUSTER_DBL_SIZE $CLUSTER_DBL_SIZE" "$TEST_IMG";
} 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "discard $CLUSTER_DBL_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+# There is 1 cluster hole. Fill it fully and allocate 1 cluster at the end
+{ $QEMU_IO -c "write -P 0x12 $CLUSTER_HALF_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0x12 $CLUSTER_HALF_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 $((CLUSTER_SIZE + CLUSTER_HALF_SIZE))
$CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+
echo "== allocate with backing =="
# Verify that allocating clusters works fine even when there is a backing
image.
# Regression test for a bug where we would pass a buffer read from the backing
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
index d2904578df..27df91ca97 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.out
@@ -26,6 +26,44 @@ read 524288/524288 bytes at offset 0
Repairing image was not closed correctly
read 1048576/1048576 bytes at offset 1048576
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check discard ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0 0x20TEST_DIR/t.IMGFMT
+discard 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0x100x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check simple allocation over the discarded hole ==
+wrote 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0x100x10TEST_DIR/t.IMGFMT
+0x200x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check more complex allocation over the discard hole ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 2097152
+2 MiB, X
[PULL v2 08/22] parallels: create mark_used() helper which sets bit in used bitmap
This functionality is used twice already and next patch will add more
code with it.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index af3b4894d7..66c86d87e3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,6 +178,21 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block,
1);
}
+static int mark_used(BlockDriverState *bs,
+ unsigned long *bitmap, uint32_t bitmap_size, int64_t off)
+{
+BDRVParallelsState *s = bs->opaque;
+uint32_t cluster_index = host_cluster_index(s, off);
+if (cluster_index >= bitmap_size) {
+return -E2BIG;
+}
+if (test_bit(cluster_index, bitmap)) {
+return -EBUSY;
+}
+bitmap_set(bitmap, cluster_index, 1);
+return 0;
+}
+
static int64_t coroutine_fn GRAPH_RDLOCK
allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
@@ -621,7 +636,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
BDRVParallelsState *s = bs->opaque;
int64_t host_off, host_sector, guest_sector;
unsigned long *bitmap;
-uint32_t i, bitmap_size, cluster_index, bat_entry;
+uint32_t i, bitmap_size, bat_entry;
int n, ret = 0;
uint64_t *buf = NULL;
bool fixed = false;
@@ -655,10 +670,9 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
continue;
}
-cluster_index = host_cluster_index(s, host_off);
-assert(cluster_index < bitmap_size);
-if (!test_bit(cluster_index, bitmap)) {
-bitmap_set(bitmap, cluster_index, 1);
+ret = mark_used(bs, bitmap, bitmap_size, host_off);
+assert(ret != -E2BIG);
+if (ret == 0) {
continue;
}
@@ -713,11 +727,13 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
* consistent for the new allocated clusters too.
*
* Note, clusters allocated outside the current image are not
- * considered, and the bitmap size doesn't change.
+ * considered, and the bitmap size doesn't change. This specifically
+ * means that -E2BIG is OK.
*/
-cluster_index = host_cluster_index(s, host_off);
-if (cluster_index < bitmap_size) {
-bitmap_set(bitmap, cluster_index, 1);
+ret = mark_used(bs, bitmap, bitmap_size, host_off);
+if (ret == -EBUSY) {
+res->check_errors++;
+goto out_repair_bat;
}
fixed = true;
--
2.34.1
[PULL v2 18/22] parallels: improve readability of allocate_clusters
Replace 'space' representing the amount of data to preallocate with
'bytes'.
Rationale:
* 'space' at each place is converted to bytes
* the unit is more close to the variable name
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index ebcdff8736..a97fb8b506 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -279,7 +279,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
uint32_t new_usedsize;
-int64_t space = to_allocate * s->tracks + s->prealloc_size;
+int64_t bytes = to_allocate * s->cluster_size;
+bytes += s->prealloc_size * BDRV_SECTOR_SIZE;
host_off = s->data_end * BDRV_SECTOR_SIZE;
@@ -289,8 +290,7 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
* force the safer-but-slower fallocate.
*/
if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
-ret = bdrv_co_truncate(bs->file,
- (s->data_end + space) << BDRV_SECTOR_BITS,
+ret = bdrv_co_truncate(bs->file, host_off + bytes,
false, PREALLOC_MODE_OFF,
BDRV_REQ_ZERO_WRITE, NULL);
if (ret == -ENOTSUP) {
@@ -298,16 +298,13 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
}
}
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-ret = bdrv_co_pwrite_zeroes(bs->file,
-s->data_end << BDRV_SECTOR_BITS,
-space << BDRV_SECTOR_BITS, 0);
+ret = bdrv_co_pwrite_zeroes(bs->file, host_off, bytes, 0);
}
if (ret < 0) {
return ret;
}
-new_usedsize = s->used_bmap_size +
- (space << BDRV_SECTOR_BITS) / s->cluster_size;
+new_usedsize = s->used_bmap_size + bytes / s->cluster_size;
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
--
2.34.1
[PULL v2 12/22] parallels: collect bitmap of used clusters at open
If the operation is failed, we need to check image consistency if the
problem is not about memory allocation.
Bitmap adjustments in allocate_cluster are not performed yet.
They worth to be separate. This was proven useful during debug of this
series. Kept as is for future bissecting.
It should be specifically noted that used bitmap must be recalculated
if data_off has been fixed during image consistency check.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 73 +++
block/parallels.h | 3 ++
2 files changed, 76 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 2b5f2b54a0..a997880c34 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -193,6 +193,58 @@ static int mark_used(BlockDriverState *bs,
return 0;
}
+/*
+ * Collect used bitmap. The image can contain errors, we should fill the
+ * bitmap anyway, as much as we can. This information will be used for
+ * error resolution.
+ */
+static int parallels_fill_used_bitmap(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+int64_t payload_bytes;
+uint32_t i;
+int err = 0;
+
+payload_bytes = bdrv_getlength(bs->file->bs);
+if (payload_bytes < 0) {
+return payload_bytes;
+}
+payload_bytes -= s->data_start * BDRV_SECTOR_SIZE;
+if (payload_bytes < 0) {
+return -EINVAL;
+}
+
+s->used_bmap_size = DIV_ROUND_UP(payload_bytes, s->cluster_size);
+if (s->used_bmap_size == 0) {
+return 0;
+}
+s->used_bmap = bitmap_try_new(s->used_bmap_size);
+if (s->used_bmap == NULL) {
+return -ENOMEM;
+}
+
+for (i = 0; i < s->bat_size; i++) {
+int err2;
+int64_t host_off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+if (host_off == 0) {
+continue;
+}
+
+err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off);
+if (err2 < 0 && err == 0) {
+err = err2;
+}
+}
+return err;
+}
+
+static void parallels_free_used_bitmap(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+s->used_bmap_size = 0;
+g_free(s->used_bmap);
+}
+
static int64_t coroutine_fn GRAPH_RDLOCK
allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
@@ -530,8 +582,17 @@ parallels_check_data_off(BlockDriverState *bs,
BdrvCheckResult *res,
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
+int err;
s->header->data_off = cpu_to_le32(data_off);
s->data_start = data_off;
+
+parallels_free_used_bitmap(bs);
+err = parallels_fill_used_bitmap(bs);
+if (err == -ENOMEM) {
+res->check_errors++;
+return err;
+}
+
res->corruptions_fixed++;
}
@@ -1222,6 +1283,14 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
need_check = need_check || s->data_end > file_nb_sectors;
+if (!need_check) {
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+goto fail;
+}
+need_check = need_check || ret < 0; /* These are correctable errors */
+}
+
/*
* We don't repair the image here if it's opened for checks. Also we don't
* want to change inactive images and can't change readonly images.
@@ -1251,6 +1320,8 @@ fail:
* "s" object was allocated by g_malloc0 so we can safely
* try to free its fields even they were not allocated.
*/
+parallels_free_used_bitmap(bs);
+
error_free(s->migration_blocker);
g_free(s->bat_dirty_bmap);
qemu_vfree(s->header);
@@ -1271,6 +1342,8 @@ static void parallels_close(BlockDriverState *bs)
PREALLOC_MODE_OFF, 0, NULL);
}
+parallels_free_used_bitmap(bs);
+
g_free(s->bat_dirty_bmap);
qemu_vfree(s->header);
diff --git a/block/parallels.h b/block/parallels.h
index 4e53e9572d..6b199443cf 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -72,6 +72,9 @@ typedef struct BDRVParallelsState {
unsigned long *bat_dirty_bmap;
unsigned int bat_dirty_block;
+unsigned long *used_bmap;
+unsigned long used_bmap_size;
+
uint32_t *bat_bitmap;
unsigned int bat_size;
--
2.34.1
[PULL v2 21/22] parallels: naive implementation of parallels_co_pwrite_zeroes
The zero flag is missed in the Parallels format specification. We can
resort to discard if we have no backing file.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 1808029f14..d026ce9e2f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -582,6 +582,19 @@ done:
return ret;
}
+static int coroutine_fn GRAPH_RDLOCK
+parallels_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ BdrvRequestFlags flags)
+{
+/*
+ * The zero flag is missed in the Parallels format specification. We can
+ * resort to discard if we have no backing file (this condition is checked
+ * inside parallels_co_pdiscard().
+ */
+return parallels_co_pdiscard(bs, offset, bytes);
+}
+
+
static void parallels_check_unclean(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -1463,6 +1476,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_create_opts= parallels_co_create_opts,
.bdrv_co_check = parallels_co_check,
.bdrv_co_pdiscard = parallels_co_pdiscard,
+.bdrv_co_pwrite_zeroes = parallels_co_pwrite_zeroes,
};
static void bdrv_parallels_init(void)
--
2.34.1
[PULL v2 14/22] tests: test self-cure of parallels image with duplicated clusters
The test is quite similar with the original one for duplicated clusters.
There is the only difference in the operation which should fix the
image.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/tests/parallels-checks | 36 +++
tests/qemu-iotests/tests/parallels-checks.out | 31
2 files changed, 67 insertions(+)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index df99558486..b281246a42 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -135,6 +135,42 @@ echo "== check the second cluster (deduplicated) =="
# Clear image
_make_test_img $SIZE
+echo "== TEST DUPLICATION SELF-CURE =="
+
+echo "== write pattern to whole image =="
+{ $QEMU_IO -c "write -P 0x11 0 $SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io |
_filter_testdir
+
+echo "== write another pattern to second cluster =="
+{ $QEMU_IO -c "write -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1
| _filter_qemu_io | _filter_testdir
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+
+echo "== corrupt image =="
+poke_file "$TEST_IMG" "$(($BAT_OFFSET + 4))" "\x01\x00\x00\x00"
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster with self-repair =="
+{ $QEMU_IO -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== write another pattern to the first clusters =="
+{ $QEMU_IO -c "write -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the second cluster (deduplicated) =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+# Clear image
+_make_test_img $SIZE
+
echo "== TEST DATA_OFF CHECK =="
echo "== write pattern to first cluster =="
diff --git a/tests/qemu-iotests/tests/parallels-checks.out
b/tests/qemu-iotests/tests/parallels-checks.out
index 1325d2b611..9793423111 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -71,6 +71,37 @@ read 1048576/1048576 bytes at offset 0
read 1048576/1048576 bytes at offset 1048576
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 DUPLICATION SELF-CURE ==
+== write pattern to whole image ==
+wrote 4194304/4194304 bytes at offset 0
+4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== write another pattern to second cluster ==
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== corrupt image ==
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster with self-repair ==
+Repairing duplicate offset in BAT entry 1
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== write another pattern to the first clusters ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the second cluster (deduplicated) ==
+read 1048576/1048576 bytes at offset 1048576
+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 DATA_OFF CHECK ==
== write pattern to first cluster ==
wrote 1048576/1048576 bytes at offset 0
--
2.34.1
[PULL v2 19/22] parallels: naive implementation of parallels_co_pdiscard
* Discarding with backing stores is not supported by the format.
* There is no buffering/queueing of the discard operation.
* Only operations aligned to the cluster are supported.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 46 ++
1 file changed, 46 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index a97fb8b506..1808029f14 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -537,6 +537,51 @@ parallels_co_readv(BlockDriverState *bs, int64_t
sector_num, int nb_sectors,
return ret;
}
+
+static int coroutine_fn GRAPH_RDLOCK
+parallels_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
+{
+int ret = 0;
+uint32_t cluster, count;
+BDRVParallelsState *s = bs->opaque;
+
+/*
+ * The image does not support ZERO mark inside the BAT, which means that
+ * stale data could be exposed from the backing file.
+ */
+if (bs->backing) {
+return -ENOTSUP;
+}
+
+if (!QEMU_IS_ALIGNED(offset, s->cluster_size)) {
+return -ENOTSUP;
+} else if (!QEMU_IS_ALIGNED(bytes, s->cluster_size)) {
+return -ENOTSUP;
+}
+
+cluster = offset / s->cluster_size;
+count = bytes / s->cluster_size;
+
+qemu_co_mutex_lock(>lock);
+for (; count > 0; cluster++, count--) {
+int64_t host_off = bat2sect(s, cluster) << BDRV_SECTOR_BITS;
+if (host_off == 0) {
+continue;
+}
+
+ret = bdrv_co_pdiscard(bs->file, host_off, s->cluster_size);
+if (ret < 0) {
+goto done;
+}
+
+parallels_set_bat_entry(s, cluster, 0);
+bitmap_clear(s->used_bmap, host_cluster_index(s, host_off), 1);
+}
+done:
+qemu_co_mutex_unlock(>lock);
+return ret;
+}
+
static void parallels_check_unclean(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -1417,6 +1462,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_create = parallels_co_create,
.bdrv_co_create_opts= parallels_co_create_opts,
.bdrv_co_check = parallels_co_check,
+.bdrv_co_pdiscard = parallels_co_pdiscard,
};
static void bdrv_parallels_init(void)
--
2.34.1
[PULL v2 05/22] parallels: return earler in fail_format branch in parallels_open()
We do not need to perform any deallocation/cleanup if wrong format is detected. Signed-off-by: Denis V. Lunev Reviewed-by: Alexander Ivanov --- block/parallels.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/parallels.c b/block/parallels.c index ae006e7fc7..12f38cf70b 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -1232,7 +1232,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, fail_format: error_setg(errp, "Image not in Parallels format"); -ret = -EINVAL; +return -EINVAL; + fail: /* * "s" object was allocated by g_malloc0 so we can safely -- 2.34.1
[PULL v2 03/22] parallels: fix memory leak in parallels_open()
We should free opts allocated through qemu_opts_create() at the end. Signed-off-by: Denis V. Lunev Reviewed-by: Alexander Ivanov --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index 428f72de1c..af7be427c9 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -1217,6 +1217,7 @@ fail_format: fail_options: ret = -EINVAL; fail: +qemu_opts_del(opts); /* * "s" object was allocated by g_malloc0 so we can safely * try to free its fields even they were not allocated. -- 2.34.1
[PULL v2 02/22] parallels: mark driver as supporting CBT
Parallels driver indeed support Parallels Dirty Bitmap Feature in
read-only mode. The patch adds bdrv_supports_persistent_dirty_bitmap()
callback which always return 1 to indicate that.
This will allow to copy CBT from Parallels image with qemu-img.
Note: read-write support is signalled through
bdrv_co_can_store_new_dirty_bitmap() and is different.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 2ebd8e1301..428f72de1c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1248,6 +1248,11 @@ static void parallels_close(BlockDriverState *bs)
error_free(s->migration_blocker);
}
+static bool parallels_is_support_dirty_bitmaps(BlockDriverState *bs)
+{
+return 1;
+}
+
static BlockDriver bdrv_parallels = {
.format_name= "parallels",
.instance_size = sizeof(BDRVParallelsState),
@@ -1256,6 +1261,7 @@ static BlockDriver bdrv_parallels = {
.supports_backing = true,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_supports_persistent_dirty_bitmap =
parallels_is_support_dirty_bitmaps,
.bdrv_probe = parallels_probe,
.bdrv_open = parallels_open,
--
2.34.1
[PULL v2 01/22] parallels: fix formatting in bdrv_parallels initialization
Old code is ugly and contains tabulations. There are no functional
changes in this patch.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 48c32d6821..2ebd8e1301 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1249,23 +1249,25 @@ static void parallels_close(BlockDriverState *bs)
}
static BlockDriver bdrv_parallels = {
-.format_name = "parallels",
-.instance_size = sizeof(BDRVParallelsState),
-.bdrv_probe= parallels_probe,
-.bdrv_open = parallels_open,
-.bdrv_close= parallels_close,
-.bdrv_child_perm = bdrv_default_perms,
-.bdrv_co_block_status = parallels_co_block_status,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
-.bdrv_co_flush_to_os = parallels_co_flush_to_os,
-.bdrv_co_readv = parallels_co_readv,
-.bdrv_co_writev = parallels_co_writev,
-.is_format = true,
-.supports_backing = true,
-.bdrv_co_create = parallels_co_create,
-.bdrv_co_create_opts = parallels_co_create_opts,
-.bdrv_co_check = parallels_co_check,
-.create_opts= _create_opts,
+.format_name= "parallels",
+.instance_size = sizeof(BDRVParallelsState),
+.create_opts= _create_opts,
+.is_format = true,
+.supports_backing = true,
+
+.bdrv_has_zero_init = bdrv_has_zero_init_1,
+
+.bdrv_probe = parallels_probe,
+.bdrv_open = parallels_open,
+.bdrv_close = parallels_close,
+.bdrv_child_perm= bdrv_default_perms,
+.bdrv_co_block_status = parallels_co_block_status,
+.bdrv_co_flush_to_os= parallels_co_flush_to_os,
+.bdrv_co_readv = parallels_co_readv,
+.bdrv_co_writev = parallels_co_writev,
+.bdrv_co_create = parallels_co_create,
+.bdrv_co_create_opts= parallels_co_create_opts,
+.bdrv_co_check = parallels_co_check,
};
static void bdrv_parallels_init(void)
--
2.34.1
[PULL v2 07/22] parallels: refactor path when we need to re-check image in parallels_open
More conditions follows thus the check should be more scalable.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index bd26c8db63..af3b4894d7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1071,7 +1071,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
int ret, size, i;
int64_t file_nb_sectors, sector;
uint32_t data_start;
-bool data_off_is_correct;
+bool need_check = false;
ret = parallels_opts_prealloc(bs, options, errp);
if (ret < 0) {
@@ -1139,11 +1139,14 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->bat_bitmap = (uint32_t *)(s->header + 1);
if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-s->header_unclean = true;
+need_check = s->header_unclean = true;
+}
+
+{
+bool ok = parallels_test_data_off(s, file_nb_sectors, _start);
+need_check = need_check || !ok;
}
-data_off_is_correct = parallels_test_data_off(s, file_nb_sectors,
- _start);
s->data_start = data_start;
s->data_end = s->data_start;
if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
@@ -1200,6 +1203,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->data_end = sector + s->tracks;
}
}
+need_check = need_check || s->data_end > file_nb_sectors;
/*
* We don't repair the image here if it's opened for checks. Also we don't
@@ -1209,12 +1213,8 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
return 0;
}
-/*
- * Repair the image if it's dirty or
- * out-of-image corruption was detected.
- */
-if (s->data_end > file_nb_sectors || s->header_unclean
-|| !data_off_is_correct) {
+/* Repair the image if corruption was detected. */
+if (need_check) {
BdrvCheckResult res;
ret = bdrv_check(bs, , BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
if (ret < 0) {
@@ -1223,7 +1223,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
goto fail;
}
}
-
return 0;
fail_format:
--
2.34.1
[PULL v2 00/22] implement discard operation for Parallels images
The following changes since commit 4907644841e3200aea6475c0f72d3d987e9f3d93: Merge tag 'mem-2023-09-19' of https://github.com/davidhildenbrand/qemu into staging (2023-09-19 13:22:19 -0400) are available in the Git repository at: https://src.openvz.org/scm/~den/qemu.git tags/pull-parallels-2023-09-20-v2 for you to fetch changes up to 1dba99e34d1bcb3c0de69c4270f9587b01e225fe: tests: extend test 131 to cover availability of the write-zeroes (2023-09-21 08:49:28 +0200) Parallels format driver: * regular calculation of cluster used bitmap of the image file * cluster allocation on the base of that bitmap (effectively allocation of new clusters could be done inside the image if that offset space is unused) * support of DISCARD and WRITE_ZEROES operations * image check bugfixes * unit tests fixes * unit tests covering new functionality Changes from v1: * patch 12: bdrv_co_getlength -> bdrv_getlength in parallels_fill_used_bitmap as called from open() * patch 19: added GRAPH_RDLOCK specifier for parallels_co_pdiscard * patch 21: added GRAPH_RDLOCK specifier for parallels_co_pwrite_zeroes ---- Denis V. Lunev (22): parallels: fix formatting in bdrv_parallels initialization parallels: mark driver as supporting CBT parallels: fix memory leak in parallels_open() parallels: invent parallels_opts_prealloc() helper to parse prealloc opts parallels: return earler in fail_format branch in parallels_open() parallels: return earlier from parallels_open() function on error parallels: refactor path when we need to re-check image in parallels_open parallels: create mark_used() helper which sets bit in used bitmap tests: ensure that image validation will not cure the corruption parallels: fix broken parallels_check_data_off() parallels: add test which will validate data_off fixes through repair parallels: collect bitmap of used clusters at open tests: fix broken deduplication check in parallels format test tests: test self-cure of parallels image with duplicated clusters parallels: accept multiple clusters in mark_used() parallels: update used bitmap in allocate_cluster parallels: naive implementation of allocate_clusters with used bitmap parallels: improve readability of allocate_clusters parallels: naive implementation of parallels_co_pdiscard tests: extend test 131 to cover availability of the discard operation parallels: naive implementation of parallels_co_pwrite_zeroes tests: extend test 131 to cover availability of the write-zeroes block/parallels.c | 389 -- block/parallels.h | 3 + tests/qemu-iotests/131| 52 tests/qemu-iotests/131.out| 60 tests/qemu-iotests/tests/parallels-checks | 76 - tests/qemu-iotests/tests/parallels-checks.out | 65 - 6 files changed, 544 insertions(+), 101 deletions(-) -- 2.34.1
Re: [PULL 00/22] implement discard operation for Parallels images
On 9/20/23 19:55, Stefan Hajnoczi wrote: On Wed, 20 Sept 2023 at 05:22, Denis V. Lunev wrote: The following changes since commit 4907644841e3200aea6475c0f72d3d987e9f3d93: Merge tag 'mem-2023-09-19' of https://github.com/davidhildenbrand/qemu into staging (2023-09-19 13:22:19 -0400) are available in the Git repository at: https://src.openvz.org/scm/~den/qemu.git tags/pull-parallels-2023-09-20 Hi Denis, Please take a look at the following CI failure. I have dropped this series for now. clang -m64 -mcx16 -Ilibblock.fa.p -I. -I.. -Iqapi -Itrace -Iui -Iui/shader -Iblock -I/usr/include/p11-kit-1 -I/usr/include/uuid -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -flto -fcolor-diagnostics -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -fsanitize=safe-stack -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wmissing-format-attribute -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -Wno-psabi -Wno-gnu-variable-sized-type-not-at-end -Wthread-safety -isystem /builds/qemu-project/qemu/linux-headers -isystem linux-headers -iquote . -iquote /builds/qemu-project/qemu -iquote /builds/qemu-project/qemu/include -iquote /builds/qemu-project/qemu/host/include/x86_64 -iquote /builds/qemu-project/qemu/host/include/generic -iquote /builds/qemu-project/qemu/tcg/i386 -pthread -D_GNU_SOURCE -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -fsanitize=cfi-icall -fsanitize-cfi-icall-generalize-pointers -fno-sanitize-trap=cfi-icall -fPIE -D_FILE_OFFSET_BITS=64 -D__USE_FILE_OFFSET64 -D__USE_LARGEFILE64 -DUSE_POSIX_ACLS=1 -MD -MQ libblock.fa.p/block_parallels.c.o -MF libblock.fa.p/block_parallels.c.o.d -o libblock.fa.p/block_parallels.c.o -c ../block/parallels.c ../block/parallels.c:210:21: error: calling function 'bdrv_co_getlength' requires holding mutex 'graph_lock' [-Werror,-Wthread-safety-analysis] payload_bytes = bdrv_co_getlength(bs->file->bs); ^ ../block/parallels.c:572:15: error: calling function 'bdrv_co_pdiscard' requires holding mutex 'graph_lock' [-Werror,-Wthread-safety-analysis] ret = bdrv_co_pdiscard(bs->file, host_off, s->cluster_size); ^ 2 errors generated. https://gitlab.com/qemu-project/qemu/-/jobs/5131277794 Stefan It seems that GCC and CLANG environments are different nowadays. I have had a smell of that but have not have a proof. Will try to understand. Den
[PULL 17/22] parallels: naive implementation of allocate_clusters with used bitmap
The access to the bitmap is not optimized completely.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 51 ---
1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 3beb18e44f..6a5bff4fcb 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -253,7 +253,7 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
{
int ret = 0;
BDRVParallelsState *s = bs->opaque;
-int64_t pos, space, idx, to_allocate, i, len;
+int64_t i, pos, idx, to_allocate, first_free, host_off;
pos = block_status(s, sector_num, nb_sectors, pnum);
if (pos > 0) {
@@ -276,15 +276,13 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
*/
assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
-space = to_allocate * s->tracks;
-len = bdrv_co_getlength(bs->file->bs);
-if (len < 0) {
-return len;
-}
-if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
+first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
+if (first_free == s->used_bmap_size) {
uint32_t new_usedsize;
+int64_t space = to_allocate * s->tracks + s->prealloc_size;
+
+host_off = s->data_end * BDRV_SECTOR_SIZE;
-space += s->prealloc_size;
/*
* We require the expanded size to read back as zero. If the
* user permitted truncation, we try that; but if it fails, we
@@ -313,6 +311,32 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
+} else {
+int64_t next_used;
+next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free);
+
+/* Not enough continuous clusters in the middle, adjust the size */
+if (next_used - first_free < to_allocate) {
+to_allocate = next_used - first_free;
+*pnum = (idx + to_allocate) * s->tracks - sector_num;
+}
+
+host_off = s->data_start * BDRV_SECTOR_SIZE;
+host_off += first_free * s->cluster_size;
+
+/*
+ * No need to preallocate if we are using tail area from the above
+ * branch. In the other case we are likely re-using hole. Preallocate
+ * the space if required by the prealloc_mode.
+ */
+if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
+host_off < s->data_end * BDRV_SECTOR_SIZE) {
+ret = bdrv_co_pwrite_zeroes(bs->file, host_off,
+s->cluster_size * to_allocate, 0);
+if (ret < 0) {
+return ret;
+}
+}
}
/*
@@ -344,15 +368,18 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
}
}
-ret = mark_used(bs, s->used_bmap, s->used_bmap_size,
-s->data_end << BDRV_SECTOR_BITS, to_allocate);
+ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off,
to_allocate);
if (ret < 0) {
/* Image consistency is broken. Alarm! */
return ret;
}
for (i = 0; i < to_allocate; i++) {
-parallels_set_bat_entry(s, idx + i, s->data_end / s->off_multiplier);
-s->data_end += s->tracks;
+parallels_set_bat_entry(s, idx + i,
+host_off / BDRV_SECTOR_SIZE / s->off_multiplier);
+host_off += s->cluster_size;
+}
+if (host_off > s->data_end * BDRV_SECTOR_SIZE) {
+s->data_end = host_off / BDRV_SECTOR_SIZE;
}
return bat2sect(s, idx) + sector_num % s->tracks;
--
2.34.1
[PULL 19/22] parallels: naive implementation of parallels_co_pdiscard
* Discarding with backing stores is not supported by the format.
* There is no buffering/queueing of the discard operation.
* Only operations aligned to the cluster are supported.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 46 ++
1 file changed, 46 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index d9d36c514b..1ef23f6669 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -537,6 +537,51 @@ parallels_co_readv(BlockDriverState *bs, int64_t
sector_num, int nb_sectors,
return ret;
}
+
+static int coroutine_fn
+parallels_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
+{
+int ret = 0;
+uint32_t cluster, count;
+BDRVParallelsState *s = bs->opaque;
+
+/*
+ * The image does not support ZERO mark inside the BAT, which means that
+ * stale data could be exposed from the backing file.
+ */
+if (bs->backing) {
+return -ENOTSUP;
+}
+
+if (!QEMU_IS_ALIGNED(offset, s->cluster_size)) {
+return -ENOTSUP;
+} else if (!QEMU_IS_ALIGNED(bytes, s->cluster_size)) {
+return -ENOTSUP;
+}
+
+cluster = offset / s->cluster_size;
+count = bytes / s->cluster_size;
+
+qemu_co_mutex_lock(>lock);
+for (; count > 0; cluster++, count--) {
+int64_t host_off = bat2sect(s, cluster) << BDRV_SECTOR_BITS;
+if (host_off == 0) {
+continue;
+}
+
+ret = bdrv_co_pdiscard(bs->file, host_off, s->cluster_size);
+if (ret < 0) {
+goto done;
+}
+
+parallels_set_bat_entry(s, cluster, 0);
+bitmap_clear(s->used_bmap, host_cluster_index(s, host_off), 1);
+}
+done:
+qemu_co_mutex_unlock(>lock);
+return ret;
+}
+
static void parallels_check_unclean(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -1417,6 +1462,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_create = parallels_co_create,
.bdrv_co_create_opts= parallels_co_create_opts,
.bdrv_co_check = parallels_co_check,
+.bdrv_co_pdiscard = parallels_co_pdiscard,
};
static void bdrv_parallels_init(void)
--
2.34.1
[PULL 13/22] tests: fix broken deduplication check in parallels format test
Original check is broken as supposed reading from 2 different clusters
results in read from the same file offset twice. This is definitely
wrong.
We should be sure that
* the content of both clusters is correct after repair
* clusters are at the different offsets after repair
In order to check the latter we write some content into the first one
and validate that fact.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/tests/parallels-checks | 14 ++
tests/qemu-iotests/tests/parallels-checks.out | 16
2 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index f4ca50295e..df99558486 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -117,14 +117,20 @@ echo "== check second cluster =="
echo "== repair image =="
_check_test_img -r all
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
echo "== check second cluster =="
{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
-echo "== check first cluster on host =="
-printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
+echo "== write another pattern to the first clusters =="
+{ $QEMU_IO -c "write -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
-echo "== check second cluster on host =="
-printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
+echo "== check the second cluster (deduplicated) =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $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 74a5e29260..1325d2b611 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -55,13 +55,21 @@ The following inconsistencies were found and repaired:
Double checking the fixed image now...
No errors were found on the image.
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== check second cluster ==
read 1048576/1048576 bytes at offset 1048576
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-== check first cluster on host ==
-content: 0x11
-== check second cluster on host ==
-content: 0x11
+== write another pattern to the first clusters ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the second cluster (deduplicated) ==
+read 1048576/1048576 bytes at offset 1048576
+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 DATA_OFF CHECK ==
== write pattern to first cluster ==
--
2.34.1
[PULL 15/22] parallels: accept multiple clusters in mark_used()
This would be useful in the next patch in allocate_clusters(). This
change would not imply serious performance drawbacks as usually image
is full of data or are at the end of the bitmap.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index c41511398f..b6505fcc5b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,18 +178,20 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block,
1);
}
-static int mark_used(BlockDriverState *bs,
- unsigned long *bitmap, uint32_t bitmap_size, int64_t off)
+static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count)
{
BDRVParallelsState *s = bs->opaque;
uint32_t cluster_index = host_cluster_index(s, off);
-if (cluster_index >= bitmap_size) {
+unsigned long next_used;
+if (cluster_index + count > bitmap_size) {
return -E2BIG;
}
-if (test_bit(cluster_index, bitmap)) {
+next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
+if (next_used < cluster_index + count) {
return -EBUSY;
}
-bitmap_set(bitmap, cluster_index, 1);
+bitmap_set(bitmap, cluster_index, count);
return 0;
}
@@ -230,7 +232,7 @@ static int parallels_fill_used_bitmap(BlockDriverState *bs)
continue;
}
-err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off);
+err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 1);
if (err2 < 0 && err == 0) {
err = err2;
}
@@ -732,7 +734,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
continue;
}
-ret = mark_used(bs, bitmap, bitmap_size, host_off);
+ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
assert(ret != -E2BIG);
if (ret == 0) {
continue;
@@ -792,7 +794,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
* considered, and the bitmap size doesn't change. This specifically
* means that -E2BIG is OK.
*/
-ret = mark_used(bs, bitmap, bitmap_size, host_off);
+ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
if (ret == -EBUSY) {
res->check_errors++;
goto out_repair_bat;
--
2.34.1
[PULL 04/22] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts
This patch creates above mentioned helper and moves its usage to the
beginning of parallels_open(). This simplifies parallels_open() a bit.
The patch also ensures that we store prealloc_size on block driver state
always in sectors. This makes code cleaner and avoids wrong opinion at
the assignment that the value is in bytes.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 72 +--
1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index af7be427c9..ae006e7fc7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1025,6 +1025,44 @@ static int parallels_update_header(BlockDriverState *bs)
return bdrv_pwrite_sync(bs->file, 0, size, s->header, 0);
}
+
+static int parallels_opts_prealloc(BlockDriverState *bs, QDict *options,
+ Error **errp)
+{
+int err;
+char *buf;
+int64_t bytes;
+BDRVParallelsState *s = bs->opaque;
+Error *local_err = NULL;
+QemuOpts *opts = qemu_opts_create(_runtime_opts, NULL, 0, errp);
+if (!opts) {
+return -ENOMEM;
+}
+
+err = -EINVAL;
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
+goto done;
+}
+
+bytes = qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
+s->prealloc_size = bytes >> BDRV_SECTOR_BITS;
+buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
+/* prealloc_mode can be downgraded later during allocate_clusters */
+s->prealloc_mode = qapi_enum_parse(_mode_lookup, buf,
+ PRL_PREALLOC_MODE_FALLOCATE,
+ _err);
+g_free(buf);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+goto done;
+}
+err = 0;
+
+done:
+qemu_opts_del(opts);
+return err;
+}
+
static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -1033,11 +1071,13 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
int ret, size, i;
int64_t file_nb_sectors, sector;
uint32_t data_start;
-QemuOpts *opts = NULL;
-Error *local_err = NULL;
-char *buf;
bool data_off_is_correct;
+ret = parallels_opts_prealloc(bs, options, errp);
+if (ret < 0) {
+return ret;
+}
+
ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
if (ret < 0) {
return ret;
@@ -1078,6 +1118,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
ret = -EFBIG;
goto fail;
}
+s->prealloc_size = MAX(s->tracks, s->prealloc_size);
s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
s->bat_size = le32_to_cpu(ph.bat_entries);
@@ -1117,29 +1158,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->header_size = size;
}
-opts = qemu_opts_create(_runtime_opts, NULL, 0, errp);
-if (!opts) {
-goto fail_options;
-}
-
-if (!qemu_opts_absorb_qdict(opts, options, errp)) {
-goto fail_options;
-}
-
-s->prealloc_size =
-qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
-s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
-buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
-/* prealloc_mode can be downgraded later during allocate_clusters */
-s->prealloc_mode = qapi_enum_parse(_mode_lookup, buf,
- PRL_PREALLOC_MODE_FALLOCATE,
- _err);
-g_free(buf);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-goto fail_options;
-}
-
if (ph.ext_off) {
if (flags & BDRV_O_RDWR) {
/*
@@ -1214,10 +1232,8 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
fail_format:
error_setg(errp, "Image not in Parallels format");
-fail_options:
ret = -EINVAL;
fail:
-qemu_opts_del(opts);
/*
* "s" object was allocated by g_malloc0 so we can safely
* try to free its fields even they were not allocated.
--
2.34.1
[PULL 06/22] parallels: return earlier from parallels_open() function on error
At the beginning of the function we can return immediately until we
really allocate s->header.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 12f38cf70b..bd26c8db63 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1090,7 +1090,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
ret = bdrv_pread(bs->file, 0, sizeof(ph), , 0);
if (ret < 0) {
-goto fail;
+return ret;
}
bs->total_sectors = le64_to_cpu(ph.nb_sectors);
@@ -1110,13 +1110,11 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->tracks = le32_to_cpu(ph.tracks);
if (s->tracks == 0) {
error_setg(errp, "Invalid image: Zero sectors per track");
-ret = -EINVAL;
-goto fail;
+return -EINVAL;
}
if (s->tracks > INT32_MAX/513) {
error_setg(errp, "Invalid image: Too big cluster");
-ret = -EFBIG;
-goto fail;
+return -EFBIG;
}
s->prealloc_size = MAX(s->tracks, s->prealloc_size);
s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
@@ -1124,16 +1122,14 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->bat_size = le32_to_cpu(ph.bat_entries);
if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
error_setg(errp, "Catalog too large");
-ret = -EFBIG;
-goto fail;
+return -EFBIG;
}
size = bat_entry_off(s->bat_size);
s->header_size = ROUND_UP(size, bdrv_opt_mem_align(bs->file->bs));
s->header = qemu_try_blockalign(bs->file->bs, s->header_size);
if (s->header == NULL) {
-ret = -ENOMEM;
-goto fail;
+return -ENOMEM;
}
ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
--
2.34.1
[PULL 20/22] tests: extend test 131 to cover availability of the discard operation
This patch contains test which minimally tests discard and new cluster
allocation logic.
The following checks are added:
* write 2 clusters, discard the first allocated
* write another cluster, check that the hole is filled
* write 2 clusters, discard the first allocated, write 1 cluster at
non-aligned to cluster offset (2 new clusters should be allocated)
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/131 | 31 +++
tests/qemu-iotests/131.out | 38 ++
2 files changed, 69 insertions(+)
diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
index 304bbb3f61..324008b3f6 100755
--- a/tests/qemu-iotests/131
+++ b/tests/qemu-iotests/131
@@ -74,6 +74,37 @@ poke_file "$TEST_IMG" "$inuse_offset" "\x59\x6e\x6f\x74"
echo "== read corrupted image with repairing =="
{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+echo "== check discard =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_DBL_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "discard 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io
| _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check simple allocation over the discarded hole =="
+
+{ $QEMU_IO -c "write -P 0x11 $CLUSTER_DBL_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_DBL_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== check more complex allocation over the discard hole =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 $CLUSTER_DBL_SIZE $CLUSTER_DBL_SIZE" "$TEST_IMG";
} 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "discard $CLUSTER_DBL_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+# There is 1 cluster hole. Fill it fully and allocate 1 cluster at the end
+{ $QEMU_IO -c "write -P 0x12 $CLUSTER_HALF_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0x12 $CLUSTER_HALF_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 $((CLUSTER_SIZE + CLUSTER_HALF_SIZE))
$CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+
echo "== allocate with backing =="
# Verify that allocating clusters works fine even when there is a backing
image.
# Regression test for a bug where we would pass a buffer read from the backing
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
index d2904578df..27df91ca97 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.out
@@ -26,6 +26,44 @@ read 524288/524288 bytes at offset 0
Repairing image was not closed correctly
read 1048576/1048576 bytes at offset 1048576
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check discard ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0 0x20TEST_DIR/t.IMGFMT
+discard 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0x100x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check simple allocation over the discarded hole ==
+wrote 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0x100x10TEST_DIR/t.IMGFMT
+0x200x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check more complex allocation over the discard hole ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 2097152
+2 MiB, X
[PULL 10/22] parallels: fix broken parallels_check_data_off()
Once we have repaired data_off field in the header we should update
s->data_start which is calculated on the base of it.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels.c b/block/parallels.c
index 66c86d87e3..2b5f2b54a0 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -531,6 +531,7 @@ parallels_check_data_off(BlockDriverState *bs,
BdrvCheckResult *res,
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
s->header->data_off = cpu_to_le32(data_off);
+s->data_start = data_off;
res->corruptions_fixed++;
}
--
2.34.1
[PULL 07/22] parallels: refactor path when we need to re-check image in parallels_open
More conditions follows thus the check should be more scalable.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index bd26c8db63..af3b4894d7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1071,7 +1071,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
int ret, size, i;
int64_t file_nb_sectors, sector;
uint32_t data_start;
-bool data_off_is_correct;
+bool need_check = false;
ret = parallels_opts_prealloc(bs, options, errp);
if (ret < 0) {
@@ -1139,11 +1139,14 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->bat_bitmap = (uint32_t *)(s->header + 1);
if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-s->header_unclean = true;
+need_check = s->header_unclean = true;
+}
+
+{
+bool ok = parallels_test_data_off(s, file_nb_sectors, _start);
+need_check = need_check || !ok;
}
-data_off_is_correct = parallels_test_data_off(s, file_nb_sectors,
- _start);
s->data_start = data_start;
s->data_end = s->data_start;
if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
@@ -1200,6 +1203,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->data_end = sector + s->tracks;
}
}
+need_check = need_check || s->data_end > file_nb_sectors;
/*
* We don't repair the image here if it's opened for checks. Also we don't
@@ -1209,12 +1213,8 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
return 0;
}
-/*
- * Repair the image if it's dirty or
- * out-of-image corruption was detected.
- */
-if (s->data_end > file_nb_sectors || s->header_unclean
-|| !data_off_is_correct) {
+/* Repair the image if corruption was detected. */
+if (need_check) {
BdrvCheckResult res;
ret = bdrv_check(bs, , BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
if (ret < 0) {
@@ -1223,7 +1223,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
goto fail;
}
}
-
return 0;
fail_format:
--
2.34.1
[PULL 01/22] parallels: fix formatting in bdrv_parallels initialization
Old code is ugly and contains tabulations. There are no functional
changes in this patch.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 48c32d6821..2ebd8e1301 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1249,23 +1249,25 @@ static void parallels_close(BlockDriverState *bs)
}
static BlockDriver bdrv_parallels = {
-.format_name = "parallels",
-.instance_size = sizeof(BDRVParallelsState),
-.bdrv_probe= parallels_probe,
-.bdrv_open = parallels_open,
-.bdrv_close= parallels_close,
-.bdrv_child_perm = bdrv_default_perms,
-.bdrv_co_block_status = parallels_co_block_status,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
-.bdrv_co_flush_to_os = parallels_co_flush_to_os,
-.bdrv_co_readv = parallels_co_readv,
-.bdrv_co_writev = parallels_co_writev,
-.is_format = true,
-.supports_backing = true,
-.bdrv_co_create = parallels_co_create,
-.bdrv_co_create_opts = parallels_co_create_opts,
-.bdrv_co_check = parallels_co_check,
-.create_opts= _create_opts,
+.format_name= "parallels",
+.instance_size = sizeof(BDRVParallelsState),
+.create_opts= _create_opts,
+.is_format = true,
+.supports_backing = true,
+
+.bdrv_has_zero_init = bdrv_has_zero_init_1,
+
+.bdrv_probe = parallels_probe,
+.bdrv_open = parallels_open,
+.bdrv_close = parallels_close,
+.bdrv_child_perm= bdrv_default_perms,
+.bdrv_co_block_status = parallels_co_block_status,
+.bdrv_co_flush_to_os= parallels_co_flush_to_os,
+.bdrv_co_readv = parallels_co_readv,
+.bdrv_co_writev = parallels_co_writev,
+.bdrv_co_create = parallels_co_create,
+.bdrv_co_create_opts= parallels_co_create_opts,
+.bdrv_co_check = parallels_co_check,
};
static void bdrv_parallels_init(void)
--
2.34.1
[PULL 14/22] tests: test self-cure of parallels image with duplicated clusters
The test is quite similar with the original one for duplicated clusters.
There is the only difference in the operation which should fix the
image.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/tests/parallels-checks | 36 +++
tests/qemu-iotests/tests/parallels-checks.out | 31
2 files changed, 67 insertions(+)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index df99558486..b281246a42 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -135,6 +135,42 @@ echo "== check the second cluster (deduplicated) =="
# Clear image
_make_test_img $SIZE
+echo "== TEST DUPLICATION SELF-CURE =="
+
+echo "== write pattern to whole image =="
+{ $QEMU_IO -c "write -P 0x11 0 $SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io |
_filter_testdir
+
+echo "== write another pattern to second cluster =="
+{ $QEMU_IO -c "write -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1
| _filter_qemu_io | _filter_testdir
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+
+echo "== corrupt image =="
+poke_file "$TEST_IMG" "$(($BAT_OFFSET + 4))" "\x01\x00\x00\x00"
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster with self-repair =="
+{ $QEMU_IO -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== write another pattern to the first clusters =="
+{ $QEMU_IO -c "write -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the second cluster (deduplicated) =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+# Clear image
+_make_test_img $SIZE
+
echo "== TEST DATA_OFF CHECK =="
echo "== write pattern to first cluster =="
diff --git a/tests/qemu-iotests/tests/parallels-checks.out
b/tests/qemu-iotests/tests/parallels-checks.out
index 1325d2b611..9793423111 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -71,6 +71,37 @@ read 1048576/1048576 bytes at offset 0
read 1048576/1048576 bytes at offset 1048576
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 DUPLICATION SELF-CURE ==
+== write pattern to whole image ==
+wrote 4194304/4194304 bytes at offset 0
+4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== write another pattern to second cluster ==
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== corrupt image ==
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster with self-repair ==
+Repairing duplicate offset in BAT entry 1
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== write another pattern to the first clusters ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the second cluster (deduplicated) ==
+read 1048576/1048576 bytes at offset 1048576
+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 DATA_OFF CHECK ==
== write pattern to first cluster ==
wrote 1048576/1048576 bytes at offset 0
--
2.34.1
[PULL 08/22] parallels: create mark_used() helper which sets bit in used bitmap
This functionality is used twice already and next patch will add more
code with it.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index af3b4894d7..66c86d87e3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,6 +178,21 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block,
1);
}
+static int mark_used(BlockDriverState *bs,
+ unsigned long *bitmap, uint32_t bitmap_size, int64_t off)
+{
+BDRVParallelsState *s = bs->opaque;
+uint32_t cluster_index = host_cluster_index(s, off);
+if (cluster_index >= bitmap_size) {
+return -E2BIG;
+}
+if (test_bit(cluster_index, bitmap)) {
+return -EBUSY;
+}
+bitmap_set(bitmap, cluster_index, 1);
+return 0;
+}
+
static int64_t coroutine_fn GRAPH_RDLOCK
allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
@@ -621,7 +636,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
BDRVParallelsState *s = bs->opaque;
int64_t host_off, host_sector, guest_sector;
unsigned long *bitmap;
-uint32_t i, bitmap_size, cluster_index, bat_entry;
+uint32_t i, bitmap_size, bat_entry;
int n, ret = 0;
uint64_t *buf = NULL;
bool fixed = false;
@@ -655,10 +670,9 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
continue;
}
-cluster_index = host_cluster_index(s, host_off);
-assert(cluster_index < bitmap_size);
-if (!test_bit(cluster_index, bitmap)) {
-bitmap_set(bitmap, cluster_index, 1);
+ret = mark_used(bs, bitmap, bitmap_size, host_off);
+assert(ret != -E2BIG);
+if (ret == 0) {
continue;
}
@@ -713,11 +727,13 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
* consistent for the new allocated clusters too.
*
* Note, clusters allocated outside the current image are not
- * considered, and the bitmap size doesn't change.
+ * considered, and the bitmap size doesn't change. This specifically
+ * means that -E2BIG is OK.
*/
-cluster_index = host_cluster_index(s, host_off);
-if (cluster_index < bitmap_size) {
-bitmap_set(bitmap, cluster_index, 1);
+ret = mark_used(bs, bitmap, bitmap_size, host_off);
+if (ret == -EBUSY) {
+res->check_errors++;
+goto out_repair_bat;
}
fixed = true;
--
2.34.1
[PULL 02/22] parallels: mark driver as supporting CBT
Parallels driver indeed support Parallels Dirty Bitmap Feature in
read-only mode. The patch adds bdrv_supports_persistent_dirty_bitmap()
callback which always return 1 to indicate that.
This will allow to copy CBT from Parallels image with qemu-img.
Note: read-write support is signalled through
bdrv_co_can_store_new_dirty_bitmap() and is different.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 2ebd8e1301..428f72de1c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1248,6 +1248,11 @@ static void parallels_close(BlockDriverState *bs)
error_free(s->migration_blocker);
}
+static bool parallels_is_support_dirty_bitmaps(BlockDriverState *bs)
+{
+return 1;
+}
+
static BlockDriver bdrv_parallels = {
.format_name= "parallels",
.instance_size = sizeof(BDRVParallelsState),
@@ -1256,6 +1261,7 @@ static BlockDriver bdrv_parallels = {
.supports_backing = true,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_supports_persistent_dirty_bitmap =
parallels_is_support_dirty_bitmaps,
.bdrv_probe = parallels_probe,
.bdrv_open = parallels_open,
--
2.34.1
[PULL 16/22] parallels: update used bitmap in allocate_cluster
We should extend the bitmap if the file is extended and set the bit in
the image used bitmap once the cluster is allocated. Sanity check at
that moment also looks like a good idea.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index b6505fcc5b..3beb18e44f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -282,6 +282,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
return len;
}
if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
+uint32_t new_usedsize;
+
space += s->prealloc_size;
/*
* We require the expanded size to read back as zero. If the
@@ -305,6 +307,12 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
if (ret < 0) {
return ret;
}
+
+new_usedsize = s->used_bmap_size +
+ (space << BDRV_SECTOR_BITS) / s->cluster_size;
+s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
+ new_usedsize);
+s->used_bmap_size = new_usedsize;
}
/*
@@ -336,6 +344,12 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
}
}
+ret = mark_used(bs, s->used_bmap, s->used_bmap_size,
+s->data_end << BDRV_SECTOR_BITS, to_allocate);
+if (ret < 0) {
+/* Image consistency is broken. Alarm! */
+return ret;
+}
for (i = 0; i < to_allocate; i++) {
parallels_set_bat_entry(s, idx + i, s->data_end / s->off_multiplier);
s->data_end += s->tracks;
--
2.34.1
[PULL 21/22] parallels: naive implementation of parallels_co_pwrite_zeroes
The zero flag is missed in the Parallels format specification. We can
resort to discard if we have no backing file.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 1ef23f6669..a6d64d0d47 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -582,6 +582,19 @@ done:
return ret;
}
+static int coroutine_fn
+parallels_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ BdrvRequestFlags flags)
+{
+/*
+ * The zero flag is missed in the Parallels format specification. We can
+ * resort to discard if we have no backing file (this condition is checked
+ * inside parallels_co_pdiscard().
+ */
+return parallels_co_pdiscard(bs, offset, bytes);
+}
+
+
static void parallels_check_unclean(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -1463,6 +1476,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_create_opts= parallels_co_create_opts,
.bdrv_co_check = parallels_co_check,
.bdrv_co_pdiscard = parallels_co_pdiscard,
+.bdrv_co_pwrite_zeroes = parallels_co_pwrite_zeroes,
};
static void bdrv_parallels_init(void)
--
2.34.1
[PULL 00/22] implement discard operation for Parallels images
The following changes since commit 4907644841e3200aea6475c0f72d3d987e9f3d93: Merge tag 'mem-2023-09-19' of https://github.com/davidhildenbrand/qemu into staging (2023-09-19 13:22:19 -0400) are available in the Git repository at: https://src.openvz.org/scm/~den/qemu.git tags/pull-parallels-2023-09-20 for you to fetch changes up to ead1064587ba6534aa2c3da6383713a009dafcb1: tests: extend test 131 to cover availability of the write-zeroes (2023-09-20 10:14:15 +0200) Parallels format driver: * regular calculation of cluster used bitmap of the image file * cluster allocation on the base of that bitmap (effectively allocation of new clusters could be done inside the image if that offset space is unused) * support of DISCARD and WRITE_ZEROES operations * image check bugfixes * unit tests fixes * unit tests covering new functionality Denis V. Lunev (22): parallels: fix formatting in bdrv_parallels initialization parallels: mark driver as supporting CBT parallels: fix memory leak in parallels_open() parallels: invent parallels_opts_prealloc() helper to parse prealloc opts parallels: return earler in fail_format branch in parallels_open() parallels: return earlier from parallels_open() function on error parallels: refactor path when we need to re-check image in parallels_open parallels: create mark_used() helper which sets bit in used bitmap tests: ensure that image validation will not cure the corruption parallels: fix broken parallels_check_data_off() parallels: add test which will validate data_off fixes through repair parallels: collect bitmap of used clusters at open tests: fix broken deduplication check in parallels format test tests: test self-cure of parallels image with duplicated clusters parallels: accept multiple clusters in mark_used() parallels: update used bitmap in allocate_cluster parallels: naive implementation of allocate_clusters with used bitmap parallels: improve readability of allocate_clusters parallels: naive implementation of parallels_co_pdiscard tests: extend test 131 to cover availability of the discard operation parallels: naive implementation of parallels_co_pwrite_zeroes tests: extend test 131 to cover availability of the write-zeroes block/parallels.c | 389 -- block/parallels.h | 3 + tests/qemu-iotests/131| 52 tests/qemu-iotests/131.out| 60 tests/qemu-iotests/tests/parallels-checks | 76 - tests/qemu-iotests/tests/parallels-checks.out | 65 - 6 files changed, 544 insertions(+), 101 deletions(-) -- 2.34.1
[PULL 11/22] parallels: add test which will validate data_off fixes through repair
We have only check through self-repair and that proven to be not enough.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/tests/parallels-checks | 17 +
tests/qemu-iotests/tests/parallels-checks.out | 18 ++
2 files changed, 35 insertions(+)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index 5917ee079d..f4ca50295e 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -140,6 +140,23 @@ poke_file "$TEST_IMG" "$DATA_OFF_OFFSET" "\xff\xff\xff\xff"
echo "== check first cluster =="
{ $QEMU_IO -c "read -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+# Clear image
+_make_test_img $SIZE
+
+echo "== TEST DATA_OFF THROUGH REPAIR =="
+
+echo "== write pattern to first cluster =="
+{ $QEMU_IO -c "write -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== spoil data_off field =="
+poke_file "$TEST_IMG" "$DATA_OFF_OFFSET" "\xff\xff\xff\xff"
+
+echo "== repair image =="
+_check_test_img -r all
+
+echo "== check first cluster =="
+{ $QEMU_IO -r -c "read -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/tests/parallels-checks.out
b/tests/qemu-iotests/tests/parallels-checks.out
index 98a3a7f55e..74a5e29260 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -72,4 +72,22 @@ wrote 1048576/1048576 bytes at offset 0
Repairing data_off field has incorrect value
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 DATA_OFF THROUGH REPAIR ==
+== write pattern to first cluster ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== spoil data_off field ==
+== repair image ==
+Repairing data_off field has incorrect value
+The following inconsistencies were found and repaired:
+
+0 leaked clusters
+1 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+== check first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
*** done
--
2.34.1
[PULL 12/22] parallels: collect bitmap of used clusters at open
If the operation is failed, we need to check image consistency if the
problem is not about memory allocation.
Bitmap adjustments in allocate_cluster are not performed yet.
They worth to be separate. This was proven useful during debug of this
series. Kept as is for future bissecting.
It should be specifically noted that used bitmap must be recalculated
if data_off has been fixed during image consistency check.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 73 +++
block/parallels.h | 3 ++
2 files changed, 76 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 2b5f2b54a0..c41511398f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -193,6 +193,58 @@ static int mark_used(BlockDriverState *bs,
return 0;
}
+/*
+ * Collect used bitmap. The image can contain errors, we should fill the
+ * bitmap anyway, as much as we can. This information will be used for
+ * error resolution.
+ */
+static int parallels_fill_used_bitmap(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+int64_t payload_bytes;
+uint32_t i;
+int err = 0;
+
+payload_bytes = bdrv_co_getlength(bs->file->bs);
+if (payload_bytes < 0) {
+return payload_bytes;
+}
+payload_bytes -= s->data_start * BDRV_SECTOR_SIZE;
+if (payload_bytes < 0) {
+return -EINVAL;
+}
+
+s->used_bmap_size = DIV_ROUND_UP(payload_bytes, s->cluster_size);
+if (s->used_bmap_size == 0) {
+return 0;
+}
+s->used_bmap = bitmap_try_new(s->used_bmap_size);
+if (s->used_bmap == NULL) {
+return -ENOMEM;
+}
+
+for (i = 0; i < s->bat_size; i++) {
+int err2;
+int64_t host_off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+if (host_off == 0) {
+continue;
+}
+
+err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off);
+if (err2 < 0 && err == 0) {
+err = err2;
+}
+}
+return err;
+}
+
+static void parallels_free_used_bitmap(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+s->used_bmap_size = 0;
+g_free(s->used_bmap);
+}
+
static int64_t coroutine_fn GRAPH_RDLOCK
allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
@@ -530,8 +582,17 @@ parallels_check_data_off(BlockDriverState *bs,
BdrvCheckResult *res,
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
+int err;
s->header->data_off = cpu_to_le32(data_off);
s->data_start = data_off;
+
+parallels_free_used_bitmap(bs);
+err = parallels_fill_used_bitmap(bs);
+if (err == -ENOMEM) {
+res->check_errors++;
+return err;
+}
+
res->corruptions_fixed++;
}
@@ -1222,6 +1283,14 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
need_check = need_check || s->data_end > file_nb_sectors;
+if (!need_check) {
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+goto fail;
+}
+need_check = need_check || ret < 0; /* These are correctable errors */
+}
+
/*
* We don't repair the image here if it's opened for checks. Also we don't
* want to change inactive images and can't change readonly images.
@@ -1251,6 +1320,8 @@ fail:
* "s" object was allocated by g_malloc0 so we can safely
* try to free its fields even they were not allocated.
*/
+parallels_free_used_bitmap(bs);
+
error_free(s->migration_blocker);
g_free(s->bat_dirty_bmap);
qemu_vfree(s->header);
@@ -1271,6 +1342,8 @@ static void parallels_close(BlockDriverState *bs)
PREALLOC_MODE_OFF, 0, NULL);
}
+parallels_free_used_bitmap(bs);
+
g_free(s->bat_dirty_bmap);
qemu_vfree(s->header);
diff --git a/block/parallels.h b/block/parallels.h
index 4e53e9572d..6b199443cf 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -72,6 +72,9 @@ typedef struct BDRVParallelsState {
unsigned long *bat_dirty_bmap;
unsigned int bat_dirty_block;
+unsigned long *used_bmap;
+unsigned long used_bmap_size;
+
uint32_t *bat_bitmap;
unsigned int bat_size;
--
2.34.1
[PULL 22/22] tests: extend test 131 to cover availability of the write-zeroes
This patch contains test which minimally tests write-zeroes on top of
working discard.
The following checks are added:
* write 2 clusters, write-zero to the first allocated cluster
* write 2 cluster, write-zero to the half the first allocated cluster
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/131 | 21 +
tests/qemu-iotests/131.out | 22 ++
2 files changed, 43 insertions(+)
diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
index 324008b3f6..3119100e78 100755
--- a/tests/qemu-iotests/131
+++ b/tests/qemu-iotests/131
@@ -105,6 +105,27 @@ _make_test_img $size
{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
{ $QEMU_IO -c "read -P 0 $((CLUSTER_SIZE + CLUSTER_HALF_SIZE))
$CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+echo "== check write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_DBL_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io
| _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check cluster-partial write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_HALF_SIZE $CLUSTER_HALF_SIZE"
"$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+
echo "== allocate with backing =="
# Verify that allocating clusters works fine even when there is a backing
image.
# Regression test for a bug where we would pass a buffer read from the backing
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
index 27df91ca97..86a2d2a49b 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.out
@@ -64,6 +64,28 @@ read 524288/524288 bytes at offset 0
512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 524288/524288 bytes at offset 1572864
512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0x100x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check cluster-partial write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== allocate with backing ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
--
2.34.1
[PULL 18/22] parallels: improve readability of allocate_clusters
Replace 'space' representing the amount of data to preallocate with
'bytes'.
Rationale:
* 'space' at each place is converted to bytes
* the unit is more close to the variable name
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 6a5bff4fcb..d9d36c514b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -279,7 +279,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
uint32_t new_usedsize;
-int64_t space = to_allocate * s->tracks + s->prealloc_size;
+int64_t bytes = to_allocate * s->cluster_size;
+bytes += s->prealloc_size * BDRV_SECTOR_SIZE;
host_off = s->data_end * BDRV_SECTOR_SIZE;
@@ -289,8 +290,7 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
* force the safer-but-slower fallocate.
*/
if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
-ret = bdrv_co_truncate(bs->file,
- (s->data_end + space) << BDRV_SECTOR_BITS,
+ret = bdrv_co_truncate(bs->file, host_off + bytes,
false, PREALLOC_MODE_OFF,
BDRV_REQ_ZERO_WRITE, NULL);
if (ret == -ENOTSUP) {
@@ -298,16 +298,13 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
}
}
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-ret = bdrv_co_pwrite_zeroes(bs->file,
-s->data_end << BDRV_SECTOR_BITS,
-space << BDRV_SECTOR_BITS, 0);
+ret = bdrv_co_pwrite_zeroes(bs->file, host_off, bytes, 0);
}
if (ret < 0) {
return ret;
}
-new_usedsize = s->used_bmap_size +
- (space << BDRV_SECTOR_BITS) / s->cluster_size;
+new_usedsize = s->used_bmap_size + bytes / s->cluster_size;
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
--
2.34.1
[PULL 05/22] parallels: return earler in fail_format branch in parallels_open()
We do not need to perform any deallocation/cleanup if wrong format is detected. Signed-off-by: Denis V. Lunev Reviewed-by: Alexander Ivanov --- block/parallels.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/parallels.c b/block/parallels.c index ae006e7fc7..12f38cf70b 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -1232,7 +1232,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, fail_format: error_setg(errp, "Image not in Parallels format"); -ret = -EINVAL; +return -EINVAL; + fail: /* * "s" object was allocated by g_malloc0 so we can safely -- 2.34.1
[PULL 09/22] tests: ensure that image validation will not cure the corruption
Since
commit cfce1091d55322789582480798a891cbaf66924e
Author: Alexander Ivanov
Date: Tue Jul 18 12:44:29 2023 +0200
parallels: Image repairing in parallels_open()
there is a potential pit fall with calling
qemu-io -c "read"
The image is opened in read-write mode and thus could be potentially
repaired. This could ruin testing process.
The patch forces read-only opening for reads. In that case repairing
is impossible.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/tests/parallels-checks | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index a7a1b357b5..5917ee079d 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -91,7 +91,7 @@ file_size=`stat --printf="%s" "$TEST_IMG"`
echo "file size: $file_size"
echo "== check last cluster =="
-{ $QEMU_IO -c "read -P 0x11 $LAST_CLUSTER_OFF $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $LAST_CLUSTER_OFF $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
# Clear image
_make_test_img $SIZE
@@ -105,19 +105,20 @@ echo "== write another pattern to second cluster =="
{ $QEMU_IO -c "write -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1
| _filter_qemu_io | _filter_testdir
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
echo "== corrupt image =="
poke_file "$TEST_IMG" "$(($BAT_OFFSET + 4))" "\x01\x00\x00\x00"
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
echo "== repair image =="
_check_test_img -r all
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
echo "== check first cluster on host =="
printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
--
2.34.1
[PULL 03/22] parallels: fix memory leak in parallels_open()
We should free opts allocated through qemu_opts_create() at the end. Signed-off-by: Denis V. Lunev Reviewed-by: Alexander Ivanov --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index 428f72de1c..af7be427c9 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -1217,6 +1217,7 @@ fail_format: fail_options: ret = -EINVAL; fail: +qemu_opts_del(opts); /* * "s" object was allocated by g_malloc0 so we can safely * try to free its fields even they were not allocated. -- 2.34.1
Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
On 9/7/23 23:53, Denis V. Lunev wrote:
Unfortunately 271 IO test is broken if started in non-cached mode.
Commits
commit a6b257a08e3d72219f03e461a52152672fec0612
Author: Nir Soffer
Date: Tue Aug 13 21:21:03 2019 +0300
file-posix: Handle undetectable alignment
and
commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
Author: Kevin Wolf
Date: Thu Jul 16 16:26:00 2020 +0200
block: Require aligned image size to avoid assertion failure
have interesting side effect if used togather.
If the image size is not multiple of 4k and that image falls under
original constraints of Nil's patch, the image can not be opened
due to the check in the bdrv_check_perm().
The patch tries to satisfy the requirements of bdrv_check_perm()
inside raw_probe_alignment(). This is at my opinion better that just
disallowing to run that test in non-cached mode. The operation is legal
by itself.
Signed-off-by: Denis V. Lunev
CC: Nir Soffer
CC: Kevin Wolf
CC: Hanna Reitz
CC: Alberto Garcia
---
block/file-posix.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index b16e9c21a1..988cfdc76c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -447,8 +447,21 @@ static void raw_probe_alignment(BlockDriverState *bs, int
fd, Error **errp)
for (i = 0; i < ARRAY_SIZE(alignments); i++) {
align = alignments[i];
if (raw_is_io_aligned(fd, buf, align)) {
-/* Fallback to safe value. */
-bs->bl.request_alignment = (align != 1) ? align : max_align;
+if (align != 1) {
+bs->bl.request_alignment = align;
+break;
+}
+/*
+ * Fallback to safe value. max_align is perfect, but the size
of the device must be multiple of
+ * the virtual length of the device. In the other case we will
get a error in
+ * bdrv_node_refresh_perm().
+ */
+for (align = max_align; align > 1; align /= 2) {
+if ((bs->total_sectors * BDRV_SECTOR_SIZE) % align == 0) {
+break;
+}
+}
+bs->bl.request_alignment = align;
break;
}
}
ping
[PATCH 12/22] parallels: collect bitmap of used clusters at open
If the operation is failed, we need to check image consistency if the
problem is not about memory allocation.
Bitmap adjustments in allocate_cluster are not performed yet.
They worth to be separate. This was proven useful during debug of this
series. Kept as is for future bissecting.
It should be specifically noted that used bitmap must be recalculated
if data_off has been fixed during image consistency check.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 73 +++
block/parallels.h | 3 ++
2 files changed, 76 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 2b5f2b54a0..c41511398f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -193,6 +193,58 @@ static int mark_used(BlockDriverState *bs,
return 0;
}
+/*
+ * Collect used bitmap. The image can contain errors, we should fill the
+ * bitmap anyway, as much as we can. This information will be used for
+ * error resolution.
+ */
+static int parallels_fill_used_bitmap(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+int64_t payload_bytes;
+uint32_t i;
+int err = 0;
+
+payload_bytes = bdrv_co_getlength(bs->file->bs);
+if (payload_bytes < 0) {
+return payload_bytes;
+}
+payload_bytes -= s->data_start * BDRV_SECTOR_SIZE;
+if (payload_bytes < 0) {
+return -EINVAL;
+}
+
+s->used_bmap_size = DIV_ROUND_UP(payload_bytes, s->cluster_size);
+if (s->used_bmap_size == 0) {
+return 0;
+}
+s->used_bmap = bitmap_try_new(s->used_bmap_size);
+if (s->used_bmap == NULL) {
+return -ENOMEM;
+}
+
+for (i = 0; i < s->bat_size; i++) {
+int err2;
+int64_t host_off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+if (host_off == 0) {
+continue;
+}
+
+err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off);
+if (err2 < 0 && err == 0) {
+err = err2;
+}
+}
+return err;
+}
+
+static void parallels_free_used_bitmap(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+s->used_bmap_size = 0;
+g_free(s->used_bmap);
+}
+
static int64_t coroutine_fn GRAPH_RDLOCK
allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
@@ -530,8 +582,17 @@ parallels_check_data_off(BlockDriverState *bs,
BdrvCheckResult *res,
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
+int err;
s->header->data_off = cpu_to_le32(data_off);
s->data_start = data_off;
+
+parallels_free_used_bitmap(bs);
+err = parallels_fill_used_bitmap(bs);
+if (err == -ENOMEM) {
+res->check_errors++;
+return err;
+}
+
res->corruptions_fixed++;
}
@@ -1222,6 +1283,14 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
need_check = need_check || s->data_end > file_nb_sectors;
+if (!need_check) {
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+goto fail;
+}
+need_check = need_check || ret < 0; /* These are correctable errors */
+}
+
/*
* We don't repair the image here if it's opened for checks. Also we don't
* want to change inactive images and can't change readonly images.
@@ -1251,6 +1320,8 @@ fail:
* "s" object was allocated by g_malloc0 so we can safely
* try to free its fields even they were not allocated.
*/
+parallels_free_used_bitmap(bs);
+
error_free(s->migration_blocker);
g_free(s->bat_dirty_bmap);
qemu_vfree(s->header);
@@ -1271,6 +1342,8 @@ static void parallels_close(BlockDriverState *bs)
PREALLOC_MODE_OFF, 0, NULL);
}
+parallels_free_used_bitmap(bs);
+
g_free(s->bat_dirty_bmap);
qemu_vfree(s->header);
diff --git a/block/parallels.h b/block/parallels.h
index 4e53e9572d..6b199443cf 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -72,6 +72,9 @@ typedef struct BDRVParallelsState {
unsigned long *bat_dirty_bmap;
unsigned int bat_dirty_block;
+unsigned long *used_bmap;
+unsigned long used_bmap_size;
+
uint32_t *bat_bitmap;
unsigned int bat_size;
--
2.34.1
Re: [PATCH 3/3] tests: extend test 131 to cover availability of the write-zeroes
On 9/18/23 20:00, Denis V. Lunev wrote:
This patch contains test which minimally tests write-zeroes on top of
working discard.
The following checks are added:
* write 2 clusters, write-zero to the first allocated cluster
* write 2 cluster, write-zero to the half the first allocated cluster
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/131 | 20
tests/qemu-iotests/131.out | 20
2 files changed, 40 insertions(+)
diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
index e50a658f22..308732d84b 100755
--- a/tests/qemu-iotests/131
+++ b/tests/qemu-iotests/131
@@ -105,6 +105,26 @@ _make_test_img $size
{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
{ $QEMU_IO -c "read -P 0 $((CLUSTER_SIZE + CLUSTER_HALF_SIZE)) $CLUSTER_DBL_SIZE"
"$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+echo "== check write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_DBL_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io
| _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check cluster-partial write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_HALF_SIZE $CLUSTER_HALF_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
echo "== allocate with backing =="
# Verify that allocating clusters works fine even when there is a backing
image.
# Regression test for a bug where we would pass a buffer read from the backing
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
index 9882f9df6c..8493561bab 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.out
@@ -64,6 +64,26 @@ read 524288/524288 bytes at offset 0
512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 2097152/2097152 bytes at offset 1572864
2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0x100x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check cluster-partial write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== allocate with backing ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
This patch is actually patch 22, please disregard it.
Den
[PATCH 16/22] parallels: update used bitmap in allocate_cluster
We should extend the bitmap if the file is extended and set the bit in
the image used bitmap once the cluster is allocated. Sanity check at
that moment also looks like a good idea.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index b6505fcc5b..3beb18e44f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -282,6 +282,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
return len;
}
if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
+uint32_t new_usedsize;
+
space += s->prealloc_size;
/*
* We require the expanded size to read back as zero. If the
@@ -305,6 +307,12 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
if (ret < 0) {
return ret;
}
+
+new_usedsize = s->used_bmap_size +
+ (space << BDRV_SECTOR_BITS) / s->cluster_size;
+s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
+ new_usedsize);
+s->used_bmap_size = new_usedsize;
}
/*
@@ -336,6 +344,12 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
}
}
+ret = mark_used(bs, s->used_bmap, s->used_bmap_size,
+s->data_end << BDRV_SECTOR_BITS, to_allocate);
+if (ret < 0) {
+/* Image consistency is broken. Alarm! */
+return ret;
+}
for (i = 0; i < to_allocate; i++) {
parallels_set_bat_entry(s, idx + i, s->data_end / s->off_multiplier);
s->data_end += s->tracks;
--
2.34.1
[PATCH 09/22] tests: ensure that image validation will not cure the corruption
Since
commit cfce1091d55322789582480798a891cbaf66924e
Author: Alexander Ivanov
Date: Tue Jul 18 12:44:29 2023 +0200
parallels: Image repairing in parallels_open()
there is a potential pit fall with calling
qemu-io -c "read"
The image is opened in read-write mode and thus could be potentially
repaired. This could ruin testing process.
The patch forces read-only opening for reads. In that case repairing
is impossible.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/tests/parallels-checks | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index a7a1b357b5..5917ee079d 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -91,7 +91,7 @@ file_size=`stat --printf="%s" "$TEST_IMG"`
echo "file size: $file_size"
echo "== check last cluster =="
-{ $QEMU_IO -c "read -P 0x11 $LAST_CLUSTER_OFF $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $LAST_CLUSTER_OFF $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
# Clear image
_make_test_img $SIZE
@@ -105,19 +105,20 @@ echo "== write another pattern to second cluster =="
{ $QEMU_IO -c "write -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1
| _filter_qemu_io | _filter_testdir
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
echo "== corrupt image =="
poke_file "$TEST_IMG" "$(($BAT_OFFSET + 4))" "\x01\x00\x00\x00"
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
echo "== repair image =="
_check_test_img -r all
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
echo "== check first cluster on host =="
printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
--
2.34.1
[PATCH 22/22] tests: extend test 131 to cover availability of the write-zeroes
This patch contains test which minimally tests write-zeroes on top of
working discard.
The following checks are added:
* write 2 clusters, write-zero to the first allocated cluster
* write 2 cluster, write-zero to the half the first allocated cluster
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/131 | 21 +
tests/qemu-iotests/131.out | 22 ++
2 files changed, 43 insertions(+)
diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
index 324008b3f6..3119100e78 100755
--- a/tests/qemu-iotests/131
+++ b/tests/qemu-iotests/131
@@ -105,6 +105,27 @@ _make_test_img $size
{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
{ $QEMU_IO -c "read -P 0 $((CLUSTER_SIZE + CLUSTER_HALF_SIZE))
$CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+echo "== check write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_DBL_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io
| _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check cluster-partial write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_HALF_SIZE $CLUSTER_HALF_SIZE"
"$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+
echo "== allocate with backing =="
# Verify that allocating clusters works fine even when there is a backing
image.
# Regression test for a bug where we would pass a buffer read from the backing
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
index 27df91ca97..02aa55abf1 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.out
@@ -64,6 +64,28 @@ read 524288/524288 bytes at offset 0
512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 524288/524288 bytes at offset 1572864
512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0 0x20TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check cluster-partial write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== allocate with backing ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
--
2.34.1
[PATCH 02/22] parallels: mark driver as supporting CBT
Parallels driver indeed support Parallels Dirty Bitmap Feature in
read-only mode. The patch adds bdrv_supports_persistent_dirty_bitmap()
callback which always return 1 to indicate that.
This will allow to copy CBT from Parallels image with qemu-img.
Note: read-write support is signalled through
bdrv_co_can_store_new_dirty_bitmap() and is different.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 2ebd8e1301..428f72de1c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1248,6 +1248,11 @@ static void parallels_close(BlockDriverState *bs)
error_free(s->migration_blocker);
}
+static bool parallels_is_support_dirty_bitmaps(BlockDriverState *bs)
+{
+return 1;
+}
+
static BlockDriver bdrv_parallels = {
.format_name= "parallels",
.instance_size = sizeof(BDRVParallelsState),
@@ -1256,6 +1261,7 @@ static BlockDriver bdrv_parallels = {
.supports_backing = true,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_supports_persistent_dirty_bitmap =
parallels_is_support_dirty_bitmaps,
.bdrv_probe = parallels_probe,
.bdrv_open = parallels_open,
--
2.34.1
[PATCH 13/22] tests: fix broken deduplication check in parallels format test
Original check is broken as supposed reading from 2 different clusters
results in read from the same file offset twice. This is definitely
wrong.
We should be sure that
* the content of both clusters is correct after repair
* clusters are at the different offsets after repair
In order to check the latter we write some content into the first one
and validate that fact.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/tests/parallels-checks | 14 ++
tests/qemu-iotests/tests/parallels-checks.out | 16
2 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index f4ca50295e..df99558486 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -117,14 +117,20 @@ echo "== check second cluster =="
echo "== repair image =="
_check_test_img -r all
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
echo "== check second cluster =="
{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
-echo "== check first cluster on host =="
-printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
+echo "== write another pattern to the first clusters =="
+{ $QEMU_IO -c "write -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
-echo "== check second cluster on host =="
-printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
+echo "== check the second cluster (deduplicated) =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $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 74a5e29260..1325d2b611 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -55,13 +55,21 @@ The following inconsistencies were found and repaired:
Double checking the fixed image now...
No errors were found on the image.
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== check second cluster ==
read 1048576/1048576 bytes at offset 1048576
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-== check first cluster on host ==
-content: 0x11
-== check second cluster on host ==
-content: 0x11
+== write another pattern to the first clusters ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the second cluster (deduplicated) ==
+read 1048576/1048576 bytes at offset 1048576
+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 DATA_OFF CHECK ==
== write pattern to first cluster ==
--
2.34.1
[PATCH 04/22] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts
This patch creates above mentioned helper and moves its usage to the
beginning of parallels_open(). This simplifies parallels_open() a bit.
The patch also ensures that we store prealloc_size on block driver state
always in sectors. This makes code cleaner and avoids wrong opinion at
the assignment that the value is in bytes.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 72 +--
1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index af7be427c9..ae006e7fc7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1025,6 +1025,44 @@ static int parallels_update_header(BlockDriverState *bs)
return bdrv_pwrite_sync(bs->file, 0, size, s->header, 0);
}
+
+static int parallels_opts_prealloc(BlockDriverState *bs, QDict *options,
+ Error **errp)
+{
+int err;
+char *buf;
+int64_t bytes;
+BDRVParallelsState *s = bs->opaque;
+Error *local_err = NULL;
+QemuOpts *opts = qemu_opts_create(_runtime_opts, NULL, 0, errp);
+if (!opts) {
+return -ENOMEM;
+}
+
+err = -EINVAL;
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
+goto done;
+}
+
+bytes = qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
+s->prealloc_size = bytes >> BDRV_SECTOR_BITS;
+buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
+/* prealloc_mode can be downgraded later during allocate_clusters */
+s->prealloc_mode = qapi_enum_parse(_mode_lookup, buf,
+ PRL_PREALLOC_MODE_FALLOCATE,
+ _err);
+g_free(buf);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+goto done;
+}
+err = 0;
+
+done:
+qemu_opts_del(opts);
+return err;
+}
+
static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -1033,11 +1071,13 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
int ret, size, i;
int64_t file_nb_sectors, sector;
uint32_t data_start;
-QemuOpts *opts = NULL;
-Error *local_err = NULL;
-char *buf;
bool data_off_is_correct;
+ret = parallels_opts_prealloc(bs, options, errp);
+if (ret < 0) {
+return ret;
+}
+
ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
if (ret < 0) {
return ret;
@@ -1078,6 +1118,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
ret = -EFBIG;
goto fail;
}
+s->prealloc_size = MAX(s->tracks, s->prealloc_size);
s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
s->bat_size = le32_to_cpu(ph.bat_entries);
@@ -1117,29 +1158,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->header_size = size;
}
-opts = qemu_opts_create(_runtime_opts, NULL, 0, errp);
-if (!opts) {
-goto fail_options;
-}
-
-if (!qemu_opts_absorb_qdict(opts, options, errp)) {
-goto fail_options;
-}
-
-s->prealloc_size =
-qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
-s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
-buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
-/* prealloc_mode can be downgraded later during allocate_clusters */
-s->prealloc_mode = qapi_enum_parse(_mode_lookup, buf,
- PRL_PREALLOC_MODE_FALLOCATE,
- _err);
-g_free(buf);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-goto fail_options;
-}
-
if (ph.ext_off) {
if (flags & BDRV_O_RDWR) {
/*
@@ -1214,10 +1232,8 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
fail_format:
error_setg(errp, "Image not in Parallels format");
-fail_options:
ret = -EINVAL;
fail:
-qemu_opts_del(opts);
/*
* "s" object was allocated by g_malloc0 so we can safely
* try to free its fields even they were not allocated.
--
2.34.1
[PATCH 01/22] parallels: fix formatting in bdrv_parallels initialization
Old code is ugly and contains tabulations. There are no functional
changes in this patch.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 48c32d6821..2ebd8e1301 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1249,23 +1249,25 @@ static void parallels_close(BlockDriverState *bs)
}
static BlockDriver bdrv_parallels = {
-.format_name = "parallels",
-.instance_size = sizeof(BDRVParallelsState),
-.bdrv_probe= parallels_probe,
-.bdrv_open = parallels_open,
-.bdrv_close= parallels_close,
-.bdrv_child_perm = bdrv_default_perms,
-.bdrv_co_block_status = parallels_co_block_status,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
-.bdrv_co_flush_to_os = parallels_co_flush_to_os,
-.bdrv_co_readv = parallels_co_readv,
-.bdrv_co_writev = parallels_co_writev,
-.is_format = true,
-.supports_backing = true,
-.bdrv_co_create = parallels_co_create,
-.bdrv_co_create_opts = parallels_co_create_opts,
-.bdrv_co_check = parallels_co_check,
-.create_opts= _create_opts,
+.format_name= "parallels",
+.instance_size = sizeof(BDRVParallelsState),
+.create_opts= _create_opts,
+.is_format = true,
+.supports_backing = true,
+
+.bdrv_has_zero_init = bdrv_has_zero_init_1,
+
+.bdrv_probe = parallels_probe,
+.bdrv_open = parallels_open,
+.bdrv_close = parallels_close,
+.bdrv_child_perm= bdrv_default_perms,
+.bdrv_co_block_status = parallels_co_block_status,
+.bdrv_co_flush_to_os= parallels_co_flush_to_os,
+.bdrv_co_readv = parallels_co_readv,
+.bdrv_co_writev = parallels_co_writev,
+.bdrv_co_create = parallels_co_create,
+.bdrv_co_create_opts= parallels_co_create_opts,
+.bdrv_co_check = parallels_co_check,
};
static void bdrv_parallels_init(void)
--
2.34.1
[PATCH 10/22] parallels: fix broken parallels_check_data_off()
Once we have repaired data_off field in the header we should update
s->data_start which is calculated on the base of it.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels.c b/block/parallels.c
index 66c86d87e3..2b5f2b54a0 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -531,6 +531,7 @@ parallels_check_data_off(BlockDriverState *bs,
BdrvCheckResult *res,
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
s->header->data_off = cpu_to_le32(data_off);
+s->data_start = data_off;
res->corruptions_fixed++;
}
--
2.34.1
[PATCH 06/22] parallels: return earlier from parallels_open() function on error
At the beginning of the function we can return immediately until we
really allocate s->header.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 12f38cf70b..bd26c8db63 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1090,7 +1090,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
ret = bdrv_pread(bs->file, 0, sizeof(ph), , 0);
if (ret < 0) {
-goto fail;
+return ret;
}
bs->total_sectors = le64_to_cpu(ph.nb_sectors);
@@ -1110,13 +1110,11 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->tracks = le32_to_cpu(ph.tracks);
if (s->tracks == 0) {
error_setg(errp, "Invalid image: Zero sectors per track");
-ret = -EINVAL;
-goto fail;
+return -EINVAL;
}
if (s->tracks > INT32_MAX/513) {
error_setg(errp, "Invalid image: Too big cluster");
-ret = -EFBIG;
-goto fail;
+return -EFBIG;
}
s->prealloc_size = MAX(s->tracks, s->prealloc_size);
s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
@@ -1124,16 +1122,14 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->bat_size = le32_to_cpu(ph.bat_entries);
if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
error_setg(errp, "Catalog too large");
-ret = -EFBIG;
-goto fail;
+return -EFBIG;
}
size = bat_entry_off(s->bat_size);
s->header_size = ROUND_UP(size, bdrv_opt_mem_align(bs->file->bs));
s->header = qemu_try_blockalign(bs->file->bs, s->header_size);
if (s->header == NULL) {
-ret = -ENOMEM;
-goto fail;
+return -ENOMEM;
}
ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
--
2.34.1
[PATCH 03/22] parallels: fix memory leak in parallels_open()
We should free opts allocated through qemu_opts_create() at the end. Signed-off-by: Denis V. Lunev --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index 428f72de1c..af7be427c9 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -1217,6 +1217,7 @@ fail_format: fail_options: ret = -EINVAL; fail: +qemu_opts_del(opts); /* * "s" object was allocated by g_malloc0 so we can safely * try to free its fields even they were not allocated. -- 2.34.1
[PATCH 07/22] parallels: refactor path when we need to re-check image in parallels_open
More conditions follows thus the check should be more scalable.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index bd26c8db63..af3b4894d7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1071,7 +1071,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
int ret, size, i;
int64_t file_nb_sectors, sector;
uint32_t data_start;
-bool data_off_is_correct;
+bool need_check = false;
ret = parallels_opts_prealloc(bs, options, errp);
if (ret < 0) {
@@ -1139,11 +1139,14 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->bat_bitmap = (uint32_t *)(s->header + 1);
if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-s->header_unclean = true;
+need_check = s->header_unclean = true;
+}
+
+{
+bool ok = parallels_test_data_off(s, file_nb_sectors, _start);
+need_check = need_check || !ok;
}
-data_off_is_correct = parallels_test_data_off(s, file_nb_sectors,
- _start);
s->data_start = data_start;
s->data_end = s->data_start;
if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
@@ -1200,6 +1203,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->data_end = sector + s->tracks;
}
}
+need_check = need_check || s->data_end > file_nb_sectors;
/*
* We don't repair the image here if it's opened for checks. Also we don't
@@ -1209,12 +1213,8 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
return 0;
}
-/*
- * Repair the image if it's dirty or
- * out-of-image corruption was detected.
- */
-if (s->data_end > file_nb_sectors || s->header_unclean
-|| !data_off_is_correct) {
+/* Repair the image if corruption was detected. */
+if (need_check) {
BdrvCheckResult res;
ret = bdrv_check(bs, , BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
if (ret < 0) {
@@ -1223,7 +1223,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
goto fail;
}
}
-
return 0;
fail_format:
--
2.34.1
[PATCH 14/22] tests: test self-cure of parallels image with duplicated clusters
The test is quite similar with the original one for duplicated clusters.
There is the only difference in the operation which should fix the
image.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
tests/qemu-iotests/tests/parallels-checks | 36 +++
tests/qemu-iotests/tests/parallels-checks.out | 31
2 files changed, 67 insertions(+)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index df99558486..b281246a42 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -135,6 +135,42 @@ echo "== check the second cluster (deduplicated) =="
# Clear image
_make_test_img $SIZE
+echo "== TEST DUPLICATION SELF-CURE =="
+
+echo "== write pattern to whole image =="
+{ $QEMU_IO -c "write -P 0x11 0 $SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io |
_filter_testdir
+
+echo "== write another pattern to second cluster =="
+{ $QEMU_IO -c "write -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1
| _filter_qemu_io | _filter_testdir
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+
+echo "== corrupt image =="
+poke_file "$TEST_IMG" "$(($BAT_OFFSET + 4))" "\x01\x00\x00\x00"
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster with self-repair =="
+{ $QEMU_IO -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== write another pattern to the first clusters =="
+{ $QEMU_IO -c "write -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the second cluster (deduplicated) =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+# Clear image
+_make_test_img $SIZE
+
echo "== TEST DATA_OFF CHECK =="
echo "== write pattern to first cluster =="
diff --git a/tests/qemu-iotests/tests/parallels-checks.out
b/tests/qemu-iotests/tests/parallels-checks.out
index 1325d2b611..9793423111 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -71,6 +71,37 @@ read 1048576/1048576 bytes at offset 0
read 1048576/1048576 bytes at offset 1048576
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 DUPLICATION SELF-CURE ==
+== write pattern to whole image ==
+wrote 4194304/4194304 bytes at offset 0
+4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== write another pattern to second cluster ==
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== corrupt image ==
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster with self-repair ==
+Repairing duplicate offset in BAT entry 1
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== write another pattern to the first clusters ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the second cluster (deduplicated) ==
+read 1048576/1048576 bytes at offset 1048576
+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 DATA_OFF CHECK ==
== write pattern to first cluster ==
wrote 1048576/1048576 bytes at offset 0
--
2.34.1
[PATCH 21/22] parallels: naive implementation of parallels_co_pwrite_zeroes
The zero flag is missed in the Parallels format specification. We can
resort to discard if we have no backing file.
Signed-off-by: Denis V. Lunev
Reviewed-by: Alexander Ivanov
---
block/parallels.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 1ef23f6669..a6d64d0d47 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -582,6 +582,19 @@ done:
return ret;
}
+static int coroutine_fn
+parallels_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ BdrvRequestFlags flags)
+{
+/*
+ * The zero flag is missed in the Parallels format specification. We can
+ * resort to discard if we have no backing file (this condition is checked
+ * inside parallels_co_pdiscard().
+ */
+return parallels_co_pdiscard(bs, offset, bytes);
+}
+
+
static void parallels_check_unclean(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -1463,6 +1476,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_create_opts= parallels_co_create_opts,
.bdrv_co_check = parallels_co_check,
.bdrv_co_pdiscard = parallels_co_pdiscard,
+.bdrv_co_pwrite_zeroes = parallels_co_pwrite_zeroes,
};
static void bdrv_parallels_init(void)
--
2.34.1
[PATCH 19/22] parallels: naive implementation of parallels_co_pdiscard
* Discarding with backing stores is not supported by the format.
* There is no buffering/queueing of the discard operation.
* Only operations aligned to the cluster are supported.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 46 ++
1 file changed, 46 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index d9d36c514b..1ef23f6669 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -537,6 +537,51 @@ parallels_co_readv(BlockDriverState *bs, int64_t
sector_num, int nb_sectors,
return ret;
}
+
+static int coroutine_fn
+parallels_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
+{
+int ret = 0;
+uint32_t cluster, count;
+BDRVParallelsState *s = bs->opaque;
+
+/*
+ * The image does not support ZERO mark inside the BAT, which means that
+ * stale data could be exposed from the backing file.
+ */
+if (bs->backing) {
+return -ENOTSUP;
+}
+
+if (!QEMU_IS_ALIGNED(offset, s->cluster_size)) {
+return -ENOTSUP;
+} else if (!QEMU_IS_ALIGNED(bytes, s->cluster_size)) {
+return -ENOTSUP;
+}
+
+cluster = offset / s->cluster_size;
+count = bytes / s->cluster_size;
+
+qemu_co_mutex_lock(>lock);
+for (; count > 0; cluster++, count--) {
+int64_t host_off = bat2sect(s, cluster) << BDRV_SECTOR_BITS;
+if (host_off == 0) {
+continue;
+}
+
+ret = bdrv_co_pdiscard(bs->file, host_off, s->cluster_size);
+if (ret < 0) {
+goto done;
+}
+
+parallels_set_bat_entry(s, cluster, 0);
+bitmap_clear(s->used_bmap, host_cluster_index(s, host_off), 1);
+}
+done:
+qemu_co_mutex_unlock(>lock);
+return ret;
+}
+
static void parallels_check_unclean(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -1417,6 +1462,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_create = parallels_co_create,
.bdrv_co_create_opts= parallels_co_create_opts,
.bdrv_co_check = parallels_co_check,
+.bdrv_co_pdiscard = parallels_co_pdiscard,
};
static void bdrv_parallels_init(void)
--
2.34.1
