[Devel] [PATCH 1/3] blk-cbt: Refactor cbt_find_next_extent for readability

[Alexander Atanasov]

On 31.01.23 19:18, nb wrote:
> 
> 
> On 31.01.23 г. 18:27 ч., Alexander Atanasov wrote:
>> Hi,
>>
>>
>> On 31.01.23 14:23, Nikolay Borisov wrote:
>>> Replace all shift rights/shift lefts by divide and multiply operations.
>>
>> What's wrong with shifts?
>>
>>> I've checked that this doesn't generate worse assembly and that shifts
>>> are indeed continued to be used. PAGE_SHIFT + 3 is actually
>>> BITS_PER_PAGE and is a constant integer expression so the compiler is
>>> perfectly capable of figuring that out and generatin optimal assembly.
>>>
>>> Replace an if/else construct with just an if which handles the error
>>> condition. This makes the code somewhat easier to follow in a linear
>>> fashion.
>>>
>>> Remove a superfluous continue and make off2 variable local the sole
>>> context it's being used.
>>>
>>> Signed-off-by: Nikolay Borisov <nikolay.borisov at virtuozzo.com>
>>> ---
>>>   block/blk-cbt.c | 21 +++++++++++----------
>>>   1 file changed, 11 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/block/blk-cbt.c b/block/blk-cbt.c
>>> index 967533a2a7a3..8baeb2804a61 100644
>>> --- a/block/blk-cbt.c
>>> +++ b/block/blk-cbt.c
>>> @@ -758,14 +758,14 @@ static void cbt_flush_cache(struct cbt_info *cbt)
>>>   static void cbt_find_next_extent(struct cbt_info *cbt, blkcnt_t 
>>> block, struct cbt_extent *ex)
>>>   {
>>> -    unsigned long off, off2, idx;
>>> +    unsigned long off, idx;
>>>       struct page *page;
>>>       bool found = 0;
>>>       ex->start = cbt->block_max;
>>>       ex->len = 0;
>>> -    idx = block >> (PAGE_SHIFT + 3);
>>> +    idx = block / BITS_PER_PAGE;
>>
>> As we discussed about the previous series is that this is valid only 
>> when working with powers of two - what guarantees that BITS_PER_PAGE 
>> is power of two ? Same bellow.
> 
> BITS_PER_PAGE is PAGE_SIZE * 8, so long as PAGE_SIZE Is power of 2, 
> which at this point is pretty much guaranteed for every architecture, 
> let alone for every arch VHI is used on (x86_64).

Sure but why put such a constraint while it is fine without it?
VHI may be used only on x86_64 but where the kernel is used we do not know.

>>
>> - Assembly generation is compiler dependant
> 
> Sure, but the macro is a guaranteed integer constant expression which is 
> power of 2 so shifts are pretty much "guaranteed".


i preffer to be explicit.

> 
>> - It is well known that division must be avoided inside the kernel
>>
>>>       while (block < cbt->block_max) {
>>>           off = block & (BITS_PER_PAGE -1);
>>>           page = CBT_PAGE(cbt, idx);
>>> @@ -778,17 +778,19 @@ static void cbt_find_next_extent(struct 
>>> cbt_info *cbt, blkcnt_t block, struct cb
>>>           /* Find extent start */
>>>           if (!found) {
>>>               ex->start = find_next_bit(page_address(page), 
>>> BITS_PER_PAGE, off);
>>> -            if (ex->start != BITS_PER_PAGE) {
>>> -                off = ex->start;
>>> -                ex->start += idx << (PAGE_SHIFT + 3);
>>> -                found = 1;
>>> -            } else {
>>> +            /* No set bits in current page, skip to next one */
>>> +            if (ex->start == BITS_PER_PAGE) {
>>>                   unlock_page(page);
>>>                   goto next;
>>>               }
>>> +
>>> +            off = ex->start;
>>> +            ex->start += idx / BITS_PER_PAGE;
>>> +            found = 1;
>>>           }
>>>           if (found) {
>>> -            off2 = find_next_zero_bit(page_address(page), 
>>> BITS_PER_PAGE, off);
>>> +            unsigned long off2 = find_next_zero_bit(page_address(page),
>>> +                                BITS_PER_PAGE, off);
>>>               ex->len += off2 - off;
>>>               if (off2 != BITS_PER_PAGE) {
>>>                   unlock_page(page);
>>> @@ -798,8 +800,7 @@ static void cbt_find_next_extent(struct cbt_info 
>>> *cbt, blkcnt_t block, struct cb
>>>           unlock_page(page);
>>>       next:
>>>           idx++;
>>> -        block = idx << (PAGE_SHIFT + 3);
>>> -        continue;
>>> +        block = idx * BITS_PER_PAGE;
>>>       }
>>>   }
>>
>>

-- 
Regards,
Alexander Atanasov