Re: [PATCH v3 2/8] parallels: create parallels_set_bat_entry_helper() to assign BAT value

[Alexander Ivanov]

On 17.08.2022 21:21, Vladimir Sementsov-Ogievskiy wrote:
> On 8/15/22 12:02, Alexander Ivanov wrote:
> > This helper will be reused in next patches during parallels_co_check
> > rework to simplify its code.
> >
> > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> > ---
> > v2: A new patch - a part of a splitted patch.
> > v3: Fix commit message.
> >
> >   block/parallels.c | 12 +++++++++---
> >   1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/parallels.c b/block/parallels.c
> > index a76cf9d993..7f68f3cbc9 100644
> > --- a/block/parallels.c
> > +++ b/block/parallels.c
> > @@ -165,6 +165,13 @@ static int64_t block_status(BDRVParallelsState 
> > *s, int64_t sector_num,
> >       return start_off;
> >   }
> >   +static void parallels_set_bat_entry(BDRVParallelsState *s,
> > +                                    uint32_t index, uint32_t offset)
>
> Rather unobvious that offset should be passed already converted to LE. 
> Worth a comment? Or may be do convertion inside function (depends on 
> further usages of the helper)
I agree, it would be better to convert the offset inside the helper.
> > +{
> > +    s->bat_bitmap[index] = offset;
> > +    bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / 
> > s->bat_dirty_block, 1);
> > +}
> > +
> >   static int64_t allocate_clusters(BlockDriverState *bs, int64_t 
> > sector_num,
> >                                    int nb_sectors, int *pnum)
> >   {
> > @@ -250,10 +257,9 @@ static int64_t 
> > allocate_clusters(BlockDriverState *bs, int64_t sector_num,
> >       }
> >         for (i = 0; i < to_allocate; i++) {
> > -        s->bat_bitmap[idx + i] = cpu_to_le32(s->data_end / 
> > s->off_multiplier);
> > +        parallels_set_bat_entry(s, idx + i,
> > +                                cpu_to_le32(s->data_end / 
> > s->off_multiplier));
> >           s->data_end += s->tracks;
> > -        bitmap_set(s->bat_dirty_bmap,
> > -                   bat_entry_off(idx + i) / s->bat_dirty_block, 1);
> >       }
> >         return bat2sect(s, idx) + sector_num % s->tracks;