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

[Alexander Atanasov]

On 31.01.23 19:15, nb wrote:
> 
> 
> On 31.01.23 г. 19:08 ч., Alexander Atanasov 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.
>>
>> And only in the context of DIV_ROUND_UP usage ...
>> Here 32768/15 = 2184 vs 32768>>15 = 1
> 
> Where did you get this? From patch 2:
> 
> +#define BITS_PER_PAGE  (1UL << (PAGE_SHIFT + 3))
> +#define NR_PAGES(bits) DIV_ROUND_UP((bits), BITS_PER_PAGE)
> 
> so BITS_PER_PAGE == 1 << 15 == 32786 so the code is correct.


There are other definitions of BITS_PER_PAGE in the tree -
may be CBT_BITS_PER_PAGE would be a better name.
And yes if it is behind DIV_ROUND_UP it looks correct.


> 
>>
>>
>>
>>> - Assembly generation is compiler dependant
>>> - 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