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