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

[Paolo Bonzini]

On 18/07/2016 09:55, Peter Lieven wrote:
> 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.

That's okay.

Paolo

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

[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)

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.

In addition, there is some code duplication so put these computations in
two variables.

Thanks!

Paolo

> +}
> +
> +if (iscsilun->allocmap_valid == NULL) {
> +return;
> +}
> +if (valid) {
> +if (nb_clusters > 0) {
> +bitmap_set(iscsilun->allocmap_valid, cluster_num, nb_clusters);
> +}
> +} else {
> +bitmap_clear(iscsilun->allocmap_valid,
> + sector_num / iscsilun->cluster_sectors,
> + DIV_ROUND_UP(nb_sectors, iscsilun->cluster_sectors));
> +}