Am 13.09.2011 10:10, schrieb Frediano Ziglio:
> 2011/9/12 Kevin Wolf <kw...@redhat.com>:
>> 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 <fredd...@gmail.com>
>>> ---
>>> 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).
Hm, I think you are right. How do you measure performance?
Kevin
