[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qcow2: fix range check
From: |
Frediano Ziglio |
Subject: |
Re: [Qemu-devel] [PATCH] qcow2: fix range check |
Date: |
Tue, 13 Sep 2011 10:10:31 +0200 |
2011/9/12 Kevin Wolf <address@hidden>:
> Am 10.09.2011 10:23, schrieb Frediano Ziglio:
>> QCowL2Meta::offset is not cluster aligned but only sector aligned
>> however nb_clusters count cluster from cluster start.
>> This fix range check. Note that old code have no corruption issues
>> related to this check cause it only cause intersection to occur
>> when shouldn't.
>
> Are you sure? See below. (I think it doesn't corrupt the image, but for
> a different reason)
>
>>
>> Signed-off-by: Frediano Ziglio <address@hidden>
>> ---
>> block/qcow2-cluster.c | 14 +++++++-------
>> 1 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 428b5ad..2f76311 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -776,17 +776,17 @@ again:
>> */
>> QLIST_FOREACH(old_alloc, &s->cluster_allocs, next_in_flight) {
>>
>> - uint64_t end_offset = offset + nb_clusters * s->cluster_size;
>> - uint64_t old_offset = old_alloc->offset;
>> - uint64_t old_end_offset = old_alloc->offset +
>> - old_alloc->nb_clusters * s->cluster_size;
>> + uint64_t start = offset >> s->cluster_bits;
>> + uint64_t end = start + nb_clusters;
>> + uint64_t old_start = old_alloc->offset >> s->cluster_bits;
>> + uint64_t old_end = old_start + old_alloc->nb_clusters;
>>
>> - if (end_offset < old_offset || offset > old_end_offset) {
>> + if (end < old_start || start > old_end) {
>> /* No intersection */
>
> Consider request A from 0x0 + 0x1000 bytes and request B from 0x2000 +
> 0x1000 bytes. Both touch the same cluster and therefore should be
> serialised, but 0x2000 > 0x1000, so we decided here that there is no
> intersection and we don't have to care.
>
> Note that this doesn't corrupt the image, qcow2 can handle parallel
> requests allocating the same cluster. In qcow2_alloc_cluster_link_l2()
> we get an additional COW operation, so performance will be hurt, but
> correctness is maintained.
>
I tested this adding some printf and also with strace and I can
confirm that current code serialize allocation.
Using ranges A (0-0x1000) and B (0x2000-0x3000) and assuming 0x10000
(64k) as cluster size you get
A:
offset 0
nb_clusters 1
B:
offset 0x2000
nb_clusters 1
So without the patch you get two ranges
A: 0-0x10000
B: 0x2000-0x12000
which intersects.
>> } else {
>> - if (offset < old_offset) {
>> + if (start < old_start) {
>> /* Stop at the start of a running allocation */
>> - nb_clusters = (old_offset - offset) >> s->cluster_bits;
>> + nb_clusters = old_start - start;
>> } else {
>> nb_clusters = 0;
>> }
>
> Anyway, the patch looks good. Applied to the block branch.
>
> Kevin
>
Oh... I realize that ranges are [start, end) (end not inclusive) so
intersection test should be
if (end <= old_start || start >= old_end) {
intead of
if (end < old_start || start > old_end) {
However I don't understand why I got some small speed penalty with
this change (only done some small tests).
Frediano