Re: [PATCH 1/3] btrfs: simplify counting number of eb pages

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2018年04月24日 18:29, David Sterba wrote:
> On Tue, Apr 24, 2018 at 02:22:15PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2018年04月24日 13:59, Nikolay Borisov wrote:
>>>
>>>
>>> On 24.04.2018 02:03, David Sterba wrote:
>>>> The eb length is nodesize, as initialized in __alloc_extent_buffer.
>>>> Regardless of start, we should always get the same number of pages, so
>>>> use that fact.
>>>>
>>>> Signed-off-by: David Sterba <dsterba@suse.com>
>>>> ---
>>>>  fs/btrfs/extent_io.h | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
>>>> index a53009694b16..ee92c1289edd 100644
>>>> --- a/fs/btrfs/extent_io.h
>>>> +++ b/fs/btrfs/extent_io.h
>>>> @@ -454,8 +454,7 @@ void wait_on_extent_buffer_writeback(struct extent_buffer *eb);
>>>>  
>>>>  static inline unsigned long num_extent_pages(u64 start, u64 len)
>>>>  {
>>>> -	return ((start + len + PAGE_SIZE - 1) >> PAGE_SHIFT) -
>>>> -		(start >> PAGE_SHIFT);
>>>> +	return len >> PAGE_SHIFT;
>>>
>>> Shouldn't this really be len + PAGE_SIZE -1 or in fact DIV_ROUND_DOWN
>>> (len, PAGE_SIZE). Because with a nodesize of 4k (and basically less than
>>> a page size) we can get into a situation where we do:
>>>
>>> 4096 >> 13 =
>>>
>>> On powerpc for example we have:
>>>
>>> arch/powerpc/include/asm/page.h:#define PAGE_SHIFT              18
>>> arch/powerpc/include/asm/page.h:#define PAGE_SHIFT              16
>>> arch/powerpc/include/asm/page.h:#define PAGE_SHIFT              14
>>
>> For such case, the fs won't be mounted as we don't have sub-pagesized
>> nodesize support yet.
>> So won't hit the problem.
>>
>> Although a WARN_ON(len < PAGE_SIZE || IS_ALIGNED(start, PAGE_SIZE))
>> would do no harm here.
> 
> Such check is fine, but would be better placed in __alloc_extent_buffer,
> not each time we access the eb.

Yep, makes more sense than my initial idea.

Thanks,
Qu

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>