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

[Alexander Atanasov]

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.

- 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