On 7/15/21 7:48 AM, Christoph Hellwig wrote:
>> +static inline unsigned long pgmap_geometry(struct dev_pagemap *pgmap)
>> +{
>> + if (!pgmap || !pgmap->geometry)
>> + return PAGE_SIZE;
>> + return pgmap->geometry;
>
> Nit, but avoiding all the negations would make this a little easier to
> read:
>
> if (pgmap && pgmap->geometry)
> return pgmap->geometry;
> return PAGE_SIZE
>
Nicer indeed.
But this might be removed, should we follow Dan's suggestion on geometry representing nr
of pages.
>> + if (pgmap_geometry(pgmap) > PAGE_SIZE)
>> + percpu_ref_get_many(pgmap->ref, (pfn_end(pgmap, range_id)
>> + - pfn_first(pgmap, range_id)) / pgmap_pfn_geometry(pgmap));
>> + else
>> + percpu_ref_get_many(pgmap->ref, pfn_end(pgmap, range_id)
>> + - pfn_first(pgmap, range_id));
>
> This is a horrible undreadable mess, which is trivially fixed by a
> strategically used local variable:
>
> refs = pfn_end(pgmap, range_id) - pfn_first(pgmap, range_id);
> if (pgmap_geometry(pgmap) > PAGE_SIZE)
> refs /= pgmap_pfn_geometry(pgmap);
> percpu_ref_get_many(pgmap->ref, refs);
>
>
Yeap, much readable, thanks for the suggestion.
