Re: [PATCH 04/17] btrfs: make btrfs_fs_info::buffer_radix to take sector size devided values

[Qu Wenruo <wqu@suse.com>]

On 2020/9/11 下午6:11, Nikolay Borisov wrote:
> 
> 
> On 8.09.20 г. 10:52 ч., Qu Wenruo wrote:
>> For subpage size sector size support, one page can contain mutliple tree
>> blocks, thus we can no longer use (eb->start >> PAGE_SHIFT) any more, or
>> we can easily get extent buffer doesn't belongs to us.
>>
>> This patch will use (extent_buffer::start / sectorsize) as index for radix
>> tree so that we can get correct extent buffer for subpage size support.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> That's fine, however now that we are moving towards sectorsize I wonder
> if it would make more sense to fs_info->sector_bits which would be

Exactly what david is doing.

IIRC he mentioned such shift bits simplification in IRC, and I'm just
waiting for that bit to land and use that to further simplify the code.

Thanks,
Qu

> 
> log2(fs_info->sectorsize) and have expressions such as:
> 
> start >> fs_info->sector_bits. I * think* we can rely on the compiler
> doing the right thing given fs_info->sectorsize is guaranteed to be a
> power of 2 value.
> 
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
>> ---
>>  fs/btrfs/extent_io.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 6def411b2eba..5d969340275e 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -5142,7 +5142,7 @@ struct extent_buffer *find_extent_buffer(struct btrfs_fs_info *fs_info,
>>  
>>  	rcu_read_lock();
>>  	eb = radix_tree_lookup(&fs_info->buffer_radix,
>> -			       start >> PAGE_SHIFT);
>> +			       start / fs_info->sectorsize);
>>  	if (eb && atomic_inc_not_zero(&eb->refs)) {
>>  		rcu_read_unlock();
>>  		/*
>> @@ -5194,7 +5194,7 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
>>  	}
>>  	spin_lock(&fs_info->buffer_lock);
>>  	ret = radix_tree_insert(&fs_info->buffer_radix,
>> -				start >> PAGE_SHIFT, eb);
>> +				start / fs_info->sectorsize, eb);
>>  	spin_unlock(&fs_info->buffer_lock);
>>  	radix_tree_preload_end();
>>  	if (ret == -EEXIST) {
>> @@ -5302,7 +5302,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>>  
>>  	spin_lock(&fs_info->buffer_lock);
>>  	ret = radix_tree_insert(&fs_info->buffer_radix,
>> -				start >> PAGE_SHIFT, eb);
>> +				start / fs_info->sectorsize, eb);
>>  	spin_unlock(&fs_info->buffer_lock);
>>  	radix_tree_preload_end();
>>  	if (ret == -EEXIST) {
>> @@ -5358,7 +5358,7 @@ static int release_extent_buffer(struct extent_buffer *eb)
>>  
>>  			spin_lock(&fs_info->buffer_lock);
>>  			radix_tree_delete(&fs_info->buffer_radix,
>> -					  eb->start >> PAGE_SHIFT);
>> +					  eb->start / fs_info->sectorsize);
>>  			spin_unlock(&fs_info->buffer_lock);
>>  		} else {
>>  			spin_unlock(&eb->refs_lock);
>>
>