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));
> +    }