diff mbox series

[v4,22/30] qcow2: Fix offset calculation in handle_dependencies()

Message ID 46d9ec6dca0b054a529ee776d1c04b002098c127.1584468723.git.berto@igalia.com (mailing list archive)
State New, archived
Headers show
Series Add subcluster allocation to qcow2 | expand

Commit Message

Alberto Garcia March 17, 2020, 6:16 p.m. UTC
l2meta_cow_start() and l2meta_cow_end() are not necessarily
cluster-aligned if the image has subclusters, so update the
calculation of old_start and old_end to guarantee that no two requests
try to write on the same cluster.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy April 22, 2020, 12:38 p.m. UTC | #1
17.03.2020 21:16, Alberto Garcia wrote:
> l2meta_cow_start() and l2meta_cow_end() are not necessarily
> cluster-aligned if the image has subclusters, so update the
> calculation of old_start and old_end to guarantee that no two requests
> try to write on the same cluster.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Somehow, this patch say me "hey, there may be a lot of other small places, which we forget to fix about subclusters, and you have no idea, how to find and check them all" :) Probably the only way is reviewing the whole qcow2 code, but it's too huge task.. [this is just thinking out loud]

Actually, you call it "Fix", and it seems to be a fix for your "[PATCH v4 17/30] qcow2: Add subcluster support to calculate_l2_meta()". Shouldn't it be squashed in?

> ---
>   block/qcow2-cluster.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 824c710760..ceacd91ea3 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1306,8 +1306,8 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
>   
>           uint64_t start = guest_offset;
>           uint64_t end = start + bytes;
> -        uint64_t old_start = l2meta_cow_start(old_alloc);
> -        uint64_t old_end = l2meta_cow_end(old_alloc);
> +        uint64_t old_start = start_of_cluster(s, l2meta_cow_start(old_alloc));
> +        uint64_t old_end = ROUND_UP(l2meta_cow_end(old_alloc), s->cluster_size);
>   
>           if (end <= old_start || start >= old_end) {
>               /* No intersection */
>
Alberto Garcia April 23, 2020, 3:50 p.m. UTC | #2
On Wed 22 Apr 2020 02:38:54 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 17.03.2020 21:16, Alberto Garcia wrote:
>> l2meta_cow_start() and l2meta_cow_end() are not necessarily
>> cluster-aligned if the image has subclusters, so update the
>> calculation of old_start and old_end to guarantee that no two requests
>> try to write on the same cluster.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> Somehow, this patch say me "hey, there may be a lot of other small
> places, which we forget to fix about subclusters, and you have no
> idea, how to find and check them all" :) Probably the only way is
> reviewing the whole qcow2 code, but it's too huge task.. [this is just
> thinking out loud]

:-)

> Actually, you call it "Fix", and it seems to be a fix for your "[PATCH
> v4 17/30] qcow2: Add subcluster support to
> calculate_l2_meta()". Shouldn't it be squashed in?

Maybe it't not a bad idea... I'll have a look.

Berto
diff mbox series

Patch

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 824c710760..ceacd91ea3 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1306,8 +1306,8 @@  static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
 
         uint64_t start = guest_offset;
         uint64_t end = start + bytes;
-        uint64_t old_start = l2meta_cow_start(old_alloc);
-        uint64_t old_end = l2meta_cow_end(old_alloc);
+        uint64_t old_start = start_of_cluster(s, l2meta_cow_start(old_alloc));
+        uint64_t old_end = ROUND_UP(l2meta_cow_end(old_alloc), s->cluster_size);
 
         if (end <= old_start || start >= old_end) {
             /* No intersection */