Re: [Qemu-devel] [PATCH V4] block/iscsi: allow caching of the allocation map

[Peter Lieven <pl@kamp.de>]

Am 16.07.2016 um 15:38 schrieb Paolo Bonzini:
>
> On 30/06/2016 13:07, Peter Lieven wrote:
>> +static void
>> +iscsi_allocmap_update(IscsiLun *iscsilun, int64_t sector_num,
>> +                      int nb_sectors, bool allocated, bool valid)
>>   {
>>       int64_t cluster_num, nb_clusters;
>> -    if (iscsilun->allocationmap == NULL) {
>> +
>> +    if (iscsilun->allocmap == NULL) {
>>           return;
>>       }
>>       cluster_num = DIV_ROUND_UP(sector_num, iscsilun->cluster_sectors);
>>       nb_clusters = (sector_num + nb_sectors) / iscsilun->cluster_sectors
>>                     - cluster_num;
>> -    if (nb_clusters > 0) {
>> -        bitmap_clear(iscsilun->allocationmap, cluster_num, nb_clusters);
>> +    if (allocated) {
>> +        bitmap_set(iscsilun->allocmap,
>> +                   sector_num / iscsilun->cluster_sectors,
>> +                   DIV_ROUND_UP(nb_sectors, iscsilun->cluster_sectors));
>> +    } else if (nb_clusters > 0) {
>> +        bitmap_clear(iscsilun->allocmap, cluster_num, nb_clusters);
> I'm sorry for the delay in review, but this is still wrong.
>
> Suppose cluster_sectors is 2, sector_num = 1, nb_sectors = 6:
>
>     0--.--2--.--4--.--6--.--8
>        |_________________|
>
> Here in the "mark unallocated" case you want to set 2..6, i.e.
> cluster_num=2, nb_clusters=2.  This is right.
>
>     0--.--2--.--4--.--6--.--8
>        xxx|___________|xxx     (x = shrunk)

cluster_num = 1 and nb_clusters = 2

>
> In the "mark allocated" case, instead, you want to set 0..8, i.e.
> cluster_num=0, nb_clusters=4.
>
>     0--.--2--.--4--.--6--.--8
>     <--|_________________|-->  (<--> = expanded)
>
>    Instead you are setting nb_clusters=3, so that 6..8 is not marked
> allocated and reading 6..7 will return zeroes:
>
>     0--.--2--.--4--.--6--.--8
>     <--|______________|!!!     (! = wrong)
>
>   Instead you need to use
>
>     DIV_ROUND_UP(sector_num + nb_sectors, ...)
>         - sector_num / iscsilun->cluster_sectors
>
> which does the rounding correctly.

Right, but this has been wrong even in the currently active version.

So I would propose to send 2 patches.

1) fix the rounding in the old version cc'ing qemu-stable
2) rebase the new patch on top of that.

Thanks for catching this,
Peter