diff mbox series

[16/21] parallels: naive implementation of allocate_clusters with used bitmap

Message ID 20230915184130.403366-19-den@openvz.org (mailing list archive)
State New, archived
Headers show
Series implement discard operation for Parallels images | expand

Commit Message

Denis V. Lunev Sept. 15, 2023, 6:41 p.m. UTC
The access to the bitmap is not optimized completely.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 51 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 12 deletions(-)

Comments

Alexander Ivanov Sept. 18, 2023, 1:09 p.m. UTC | #1
On 9/15/23 20:41, Denis V. Lunev wrote:
> The access to the bitmap is not optimized completely.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>   block/parallels.c | 51 ++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index a6d2f05863..2efa578e21 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;
It looks, we can write this simplier:
     *pnum = to_allocate * s->tracks;
because idx and sector_num aren't changed from idx calculation:
     idx = sector_num / s->tracks;
> +        }
> +
> +        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;
Denis V. Lunev Sept. 18, 2023, 1:14 p.m. UTC | #2
On 9/18/23 15:09, Alexander Ivanov wrote:
>
>
> On 9/15/23 20:41, Denis V. Lunev wrote:
>> The access to the bitmap is not optimized completely.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> ---
>>   block/parallels.c | 51 ++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 39 insertions(+), 12 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index a6d2f05863..2efa578e21 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;
> It looks, we can write this simplier:
>     *pnum = to_allocate * s->tracks;
> because idx and sector_num aren't changed from idx calculation:
>     idx = sector_num / s->tracks;

absolutely NO! sector_num can be unaligned. Here we get to the
situation when the end of the operation is aligned to cluster
and is calculated above.

Den
Alexander Ivanov Sept. 18, 2023, 1:18 p.m. UTC | #3
On 9/18/23 15:14, Denis V. Lunev wrote:
> On 9/18/23 15:09, Alexander Ivanov wrote:
>>
>>
>> On 9/15/23 20:41, Denis V. Lunev wrote:
>>> The access to the bitmap is not optimized completely.
>>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> ---
>>>   block/parallels.c | 51 
>>> ++++++++++++++++++++++++++++++++++++-----------
>>>   1 file changed, 39 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index a6d2f05863..2efa578e21 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;
>> It looks, we can write this simplier:
>>     *pnum = to_allocate * s->tracks;
>> because idx and sector_num aren't changed from idx calculation:
>>     idx = sector_num / s->tracks;
>
> absolutely NO! sector_num can be unaligned. Here we get to the
> situation when the end of the operation is aligned to cluster
> and is calculated above.
>
> Den

OK, now I got the idea.

Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
diff mbox series

Patch

diff --git a/block/parallels.c b/block/parallels.c
index a6d2f05863..2efa578e21 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;