* [PATCH 0/3] Simplify counting of extent buffer pages
@ 2018-04-23 23:03 David Sterba
2018-04-23 23:03 ` [PATCH 1/3] btrfs: simplify counting number of eb pages David Sterba
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: David Sterba @ 2018-04-23 23:03 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
Some low-hanging cleanup fruit. The argument bloat-o-meter shows some
improvements:
extent_io.c:cache_state_if_flags.part.27 -8 (8 -> 0)
extent_io.c:cache_state.part.28 -8 (8 -> 0)
extent_io.c:check_buffer_tree_ref.part.31 -24 (24 -> 0)
extent_io.c:mark_extent_buffer_accessed -8 (40 -> 32)
extent_io.c:alloc_extent_state_atomic.part.35 -8 (8 -> 0)
extent_io.c:flush_write_bio.isra.40 -16 (16 -> 0)
extent_io.c:set_page_extent_mapped.part.49 -8 (8 -> 0)
extent_io.c:extent_buffer_under_io.part.50 -8 (8 -> 0)
extent_io.c:__unlock_for_delalloc.isra.38 -8 (8 -> 0)
extent_io.c:merge_state.part.45 -48 (48 -> 0)
extent_io.c:repair_eb_io_failure -8 (72 -> 64)
extent_io.c:set_extent_buffer_dirty -8 (40 -> 32)
extent_io.c:__alloc_dummy_extent_buffer +8 (32 -> 40)
extent_io.c:write_one_eb +16 (152 -> 168)
David Sterba (3):
btrfs: simplify counting number of eb pages
btrfs: pass only eb to num_extent_pages
btrfs: switch types to int when counting eb pages
fs/btrfs/extent_io.c | 68 ++++++++++++++++++++++++++--------------------------
fs/btrfs/extent_io.h | 5 ++--
2 files changed, 36 insertions(+), 37 deletions(-)
--
2.16.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] btrfs: simplify counting number of eb pages
2018-04-23 23:03 [PATCH 0/3] Simplify counting of extent buffer pages David Sterba
@ 2018-04-23 23:03 ` David Sterba
2018-04-24 5:59 ` Nikolay Borisov
2018-04-23 23:03 ` [PATCH 2/3] btrfs: pass only eb to num_extent_pages David Sterba
2018-04-23 23:03 ` [PATCH 3/3] btrfs: switch types to int when counting eb pages David Sterba
2 siblings, 1 reply; 10+ messages in thread
From: David Sterba @ 2018-04-23 23:03 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
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;
}
static inline void extent_buffer_get(struct extent_buffer *eb)
--
2.16.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] btrfs: pass only eb to num_extent_pages
2018-04-23 23:03 [PATCH 0/3] Simplify counting of extent buffer pages David Sterba
2018-04-23 23:03 ` [PATCH 1/3] btrfs: simplify counting number of eb pages David Sterba
@ 2018-04-23 23:03 ` David Sterba
2018-04-24 13:26 ` Nikolay Borisov
2018-04-23 23:03 ` [PATCH 3/3] btrfs: switch types to int when counting eb pages David Sterba
2 siblings, 1 reply; 10+ messages in thread
From: David Sterba @ 2018-04-23 23:03 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
Almost all callers pass the start and len as 2 arguments but this is not
necessary, all the information is provided by the eb. By reordering the
calls to num_extent_pages, we don't need the local variables with
start/len.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/extent_io.c | 30 +++++++++++++++---------------
fs/btrfs/extent_io.h | 4 ++--
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index fb32394fd830..0cc5d6ae1876 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2062,7 +2062,7 @@ int repair_eb_io_failure(struct btrfs_fs_info *fs_info,
struct extent_buffer *eb, int mirror_num)
{
u64 start = eb->start;
- unsigned long i, num_pages = num_extent_pages(eb->start, eb->len);
+ unsigned long i, num_pages = num_extent_pages(eb);
int ret = 0;
if (sb_rdonly(fs_info->sb))
@@ -3591,7 +3591,7 @@ lock_extent_buffer_for_io(struct extent_buffer *eb,
if (!ret)
return ret;
- num_pages = num_extent_pages(eb->start, eb->len);
+ num_pages = num_extent_pages(eb);
for (i = 0; i < num_pages; i++) {
struct page *p = eb->pages[i];
@@ -3721,7 +3721,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
int ret = 0;
clear_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags);
- num_pages = num_extent_pages(eb->start, eb->len);
+ num_pages = num_extent_pages(eb);
atomic_set(&eb->io_pages, num_pages);
/* set btree blocks beyond nritems with 0 to avoid stale content. */
@@ -4654,7 +4654,7 @@ static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
BUG_ON(extent_buffer_under_io(eb));
- index = num_extent_pages(eb->start, eb->len);
+ index = num_extent_pages(eb);
if (index == 0)
return;
@@ -4747,7 +4747,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
unsigned long i;
struct page *p;
struct extent_buffer *new;
- unsigned long num_pages = num_extent_pages(src->start, src->len);
+ unsigned long num_pages = num_extent_pages(src);
new = __alloc_extent_buffer(src->fs_info, src->start, src->len);
if (new == NULL)
@@ -4779,12 +4779,11 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
unsigned long num_pages;
unsigned long i;
- num_pages = num_extent_pages(start, len);
-
eb = __alloc_extent_buffer(fs_info, start, len);
if (!eb)
return NULL;
+ num_pages = num_extent_pages(eb);
for (i = 0; i < num_pages; i++) {
eb->pages[i] = alloc_page(GFP_NOFS);
if (!eb->pages[i])
@@ -4848,7 +4847,7 @@ static void mark_extent_buffer_accessed(struct extent_buffer *eb,
check_buffer_tree_ref(eb);
- num_pages = num_extent_pages(eb->start, eb->len);
+ num_pages = num_extent_pages(eb);
for (i = 0; i < num_pages; i++) {
struct page *p = eb->pages[i];
@@ -4945,7 +4944,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
u64 start)
{
unsigned long len = fs_info->nodesize;
- unsigned long num_pages = num_extent_pages(start, len);
+ unsigned long num_pages;
unsigned long i;
unsigned long index = start >> PAGE_SHIFT;
struct extent_buffer *eb;
@@ -4968,6 +4967,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
if (!eb)
return ERR_PTR(-ENOMEM);
+ num_pages = num_extent_pages(eb);
for (i = 0; i < num_pages; i++, index++) {
p = find_or_create_page(mapping, index, GFP_NOFS|__GFP_NOFAIL);
if (!p) {
@@ -5164,7 +5164,7 @@ void clear_extent_buffer_dirty(struct extent_buffer *eb)
unsigned long num_pages;
struct page *page;
- num_pages = num_extent_pages(eb->start, eb->len);
+ num_pages = num_extent_pages(eb);
for (i = 0; i < num_pages; i++) {
page = eb->pages[i];
@@ -5198,7 +5198,7 @@ int set_extent_buffer_dirty(struct extent_buffer *eb)
was_dirty = test_and_set_bit(EXTENT_BUFFER_DIRTY, &eb->bflags);
- num_pages = num_extent_pages(eb->start, eb->len);
+ num_pages = num_extent_pages(eb);
WARN_ON(atomic_read(&eb->refs) == 0);
WARN_ON(!test_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags));
@@ -5214,7 +5214,7 @@ void clear_extent_buffer_uptodate(struct extent_buffer *eb)
unsigned long num_pages;
clear_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
- num_pages = num_extent_pages(eb->start, eb->len);
+ num_pages = num_extent_pages(eb);
for (i = 0; i < num_pages; i++) {
page = eb->pages[i];
if (page)
@@ -5229,7 +5229,7 @@ void set_extent_buffer_uptodate(struct extent_buffer *eb)
unsigned long num_pages;
set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
- num_pages = num_extent_pages(eb->start, eb->len);
+ num_pages = num_extent_pages(eb);
for (i = 0; i < num_pages; i++) {
page = eb->pages[i];
SetPageUptodate(page);
@@ -5253,7 +5253,7 @@ int read_extent_buffer_pages(struct extent_io_tree *tree,
if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))
return 0;
- num_pages = num_extent_pages(eb->start, eb->len);
+ num_pages = num_extent_pages(eb);
for (i = 0; i < num_pages; i++) {
page = eb->pages[i];
if (wait == WAIT_NONE) {
@@ -5581,7 +5581,7 @@ void copy_extent_buffer_full(struct extent_buffer *dst,
ASSERT(dst->len == src->len);
- num_pages = num_extent_pages(dst->start, dst->len);
+ num_pages = num_extent_pages(dst);
for (i = 0; i < num_pages; i++)
copy_page(page_address(dst->pages[i]),
page_address(src->pages[i]));
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index ee92c1289edd..d08abc9d385e 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -452,9 +452,9 @@ int read_extent_buffer_pages(struct extent_io_tree *tree,
int mirror_num);
void wait_on_extent_buffer_writeback(struct extent_buffer *eb);
-static inline unsigned long num_extent_pages(u64 start, u64 len)
+static inline unsigned long num_extent_pages(const struct extent_buffer *eb)
{
- return len >> PAGE_SHIFT;
+ return eb->len >> PAGE_SHIFT;
}
static inline void extent_buffer_get(struct extent_buffer *eb)
--
2.16.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] btrfs: switch types to int when counting eb pages
2018-04-23 23:03 [PATCH 0/3] Simplify counting of extent buffer pages David Sterba
2018-04-23 23:03 ` [PATCH 1/3] btrfs: simplify counting number of eb pages David Sterba
2018-04-23 23:03 ` [PATCH 2/3] btrfs: pass only eb to num_extent_pages David Sterba
@ 2018-04-23 23:03 ` David Sterba
2018-04-24 13:26 ` Nikolay Borisov
2 siblings, 1 reply; 10+ messages in thread
From: David Sterba @ 2018-04-23 23:03 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
The loops iterating eb pages use unsigned long, that's an overkill as
we know that there are at most 16 pages (64k / 4k), and 4 by default
(with nodesize 16k).
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/extent_io.c | 44 ++++++++++++++++++++++----------------------
fs/btrfs/extent_io.h | 2 +-
2 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 0cc5d6ae1876..5bdfdb9c8777 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2062,7 +2062,7 @@ int repair_eb_io_failure(struct btrfs_fs_info *fs_info,
struct extent_buffer *eb, int mirror_num)
{
u64 start = eb->start;
- unsigned long i, num_pages = num_extent_pages(eb);
+ int i, num_pages = num_extent_pages(eb);
int ret = 0;
if (sb_rdonly(fs_info->sb))
@@ -3541,7 +3541,7 @@ lock_extent_buffer_for_io(struct extent_buffer *eb,
struct btrfs_fs_info *fs_info,
struct extent_page_data *epd)
{
- unsigned long i, num_pages;
+ int i, num_pages;
int flush = 0;
int ret = 0;
@@ -3715,7 +3715,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
struct extent_io_tree *tree = &BTRFS_I(fs_info->btree_inode)->io_tree;
u64 offset = eb->start;
u32 nritems;
- unsigned long i, num_pages;
+ int i, num_pages;
unsigned long start, end;
unsigned int write_flags = wbc_to_write_flags(wbc) | REQ_META;
int ret = 0;
@@ -4648,7 +4648,7 @@ int extent_buffer_under_io(struct extent_buffer *eb)
*/
static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
{
- unsigned long index;
+ int index;
struct page *page;
int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
@@ -4744,10 +4744,10 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
{
- unsigned long i;
+ int i;
struct page *p;
struct extent_buffer *new;
- unsigned long num_pages = num_extent_pages(src);
+ int num_pages = num_extent_pages(src);
new = __alloc_extent_buffer(src->fs_info, src->start, src->len);
if (new == NULL)
@@ -4776,8 +4776,8 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
u64 start, unsigned long len)
{
struct extent_buffer *eb;
- unsigned long num_pages;
- unsigned long i;
+ int num_pages;
+ int i;
eb = __alloc_extent_buffer(fs_info, start, len);
if (!eb)
@@ -4843,7 +4843,7 @@ static void check_buffer_tree_ref(struct extent_buffer *eb)
static void mark_extent_buffer_accessed(struct extent_buffer *eb,
struct page *accessed)
{
- unsigned long num_pages, i;
+ int num_pages, i;
check_buffer_tree_ref(eb);
@@ -4944,8 +4944,8 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
u64 start)
{
unsigned long len = fs_info->nodesize;
- unsigned long num_pages;
- unsigned long i;
+ int num_pages;
+ int i;
unsigned long index = start >> PAGE_SHIFT;
struct extent_buffer *eb;
struct extent_buffer *exists = NULL;
@@ -5160,8 +5160,8 @@ void free_extent_buffer_stale(struct extent_buffer *eb)
void clear_extent_buffer_dirty(struct extent_buffer *eb)
{
- unsigned long i;
- unsigned long num_pages;
+ int i;
+ int num_pages;
struct page *page;
num_pages = num_extent_pages(eb);
@@ -5190,8 +5190,8 @@ void clear_extent_buffer_dirty(struct extent_buffer *eb)
int set_extent_buffer_dirty(struct extent_buffer *eb)
{
- unsigned long i;
- unsigned long num_pages;
+ int i;
+ int num_pages;
int was_dirty = 0;
check_buffer_tree_ref(eb);
@@ -5209,9 +5209,9 @@ int set_extent_buffer_dirty(struct extent_buffer *eb)
void clear_extent_buffer_uptodate(struct extent_buffer *eb)
{
- unsigned long i;
+ int i;
struct page *page;
- unsigned long num_pages;
+ int num_pages;
clear_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
num_pages = num_extent_pages(eb);
@@ -5224,9 +5224,9 @@ void clear_extent_buffer_uptodate(struct extent_buffer *eb)
void set_extent_buffer_uptodate(struct extent_buffer *eb)
{
- unsigned long i;
+ int i;
struct page *page;
- unsigned long num_pages;
+ int num_pages;
set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
num_pages = num_extent_pages(eb);
@@ -5239,13 +5239,13 @@ void set_extent_buffer_uptodate(struct extent_buffer *eb)
int read_extent_buffer_pages(struct extent_io_tree *tree,
struct extent_buffer *eb, int wait, int mirror_num)
{
- unsigned long i;
+ int i;
struct page *page;
int err;
int ret = 0;
int locked_pages = 0;
int all_uptodate = 1;
- unsigned long num_pages;
+ int num_pages;
unsigned long num_reads = 0;
struct bio *bio = NULL;
unsigned long bio_flags = 0;
@@ -5577,7 +5577,7 @@ void copy_extent_buffer_full(struct extent_buffer *dst,
struct extent_buffer *src)
{
int i;
- unsigned num_pages;
+ int num_pages;
ASSERT(dst->len == src->len);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index d08abc9d385e..3248553a3aa2 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -452,7 +452,7 @@ int read_extent_buffer_pages(struct extent_io_tree *tree,
int mirror_num);
void wait_on_extent_buffer_writeback(struct extent_buffer *eb);
-static inline unsigned long num_extent_pages(const struct extent_buffer *eb)
+static inline int num_extent_pages(const struct extent_buffer *eb)
{
return eb->len >> PAGE_SHIFT;
}
--
2.16.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] btrfs: simplify counting number of eb pages
2018-04-23 23:03 ` [PATCH 1/3] btrfs: simplify counting number of eb pages David Sterba
@ 2018-04-24 5:59 ` Nikolay Borisov
2018-04-24 6:22 ` Qu Wenruo
0 siblings, 1 reply; 10+ messages in thread
From: Nikolay Borisov @ 2018-04-24 5:59 UTC (permalink / raw)
To: David Sterba, linux-btrfs
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 = 0
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
arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 12
> }
>
> static inline void extent_buffer_get(struct extent_buffer *eb)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] btrfs: simplify counting number of eb pages
2018-04-24 5:59 ` Nikolay Borisov
@ 2018-04-24 6:22 ` Qu Wenruo
2018-04-24 10:29 ` David Sterba
0 siblings, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2018-04-24 6:22 UTC (permalink / raw)
To: Nikolay Borisov, David Sterba, linux-btrfs
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 = 0
>
> 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.
Thanks,
Qu
> arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 12
>
>
>> }
>>
>> static inline void extent_buffer_get(struct extent_buffer *eb)
>>
> --
> 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] btrfs: simplify counting number of eb pages
2018-04-24 6:22 ` Qu Wenruo
@ 2018-04-24 10:29 ` David Sterba
2018-04-24 10:36 ` Qu Wenruo
0 siblings, 1 reply; 10+ messages in thread
From: David Sterba @ 2018-04-24 10:29 UTC (permalink / raw)
To: Qu Wenruo; +Cc: Nikolay Borisov, David Sterba, linux-btrfs
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 = 0
> >
> > 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] btrfs: simplify counting number of eb pages
2018-04-24 10:29 ` David Sterba
@ 2018-04-24 10:36 ` Qu Wenruo
0 siblings, 0 replies; 10+ messages in thread
From: Qu Wenruo @ 2018-04-24 10:36 UTC (permalink / raw)
To: dsterba, Nikolay Borisov, David Sterba, linux-btrfs
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] btrfs: pass only eb to num_extent_pages
2018-04-23 23:03 ` [PATCH 2/3] btrfs: pass only eb to num_extent_pages David Sterba
@ 2018-04-24 13:26 ` Nikolay Borisov
0 siblings, 0 replies; 10+ messages in thread
From: Nikolay Borisov @ 2018-04-24 13:26 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 24.04.2018 02:03, David Sterba wrote:
> Almost all callers pass the start and len as 2 arguments but this is not
> necessary, all the information is provided by the eb. By reordering the
> calls to num_extent_pages, we don't need the local variables with
> start/len.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/extent_io.c | 30 +++++++++++++++---------------
> fs/btrfs/extent_io.h | 4 ++--
> 2 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index fb32394fd830..0cc5d6ae1876 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2062,7 +2062,7 @@ int repair_eb_io_failure(struct btrfs_fs_info *fs_info,
> struct extent_buffer *eb, int mirror_num)
> {
> u64 start = eb->start;
> - unsigned long i, num_pages = num_extent_pages(eb->start, eb->len);
> + unsigned long i, num_pages = num_extent_pages(eb);
> int ret = 0;
>
> if (sb_rdonly(fs_info->sb))
> @@ -3591,7 +3591,7 @@ lock_extent_buffer_for_io(struct extent_buffer *eb,
> if (!ret)
> return ret;
>
> - num_pages = num_extent_pages(eb->start, eb->len);
> + num_pages = num_extent_pages(eb);
> for (i = 0; i < num_pages; i++) {
> struct page *p = eb->pages[i];
>
> @@ -3721,7 +3721,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
> int ret = 0;
>
> clear_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags);
> - num_pages = num_extent_pages(eb->start, eb->len);
> + num_pages = num_extent_pages(eb);
> atomic_set(&eb->io_pages, num_pages);
>
> /* set btree blocks beyond nritems with 0 to avoid stale content. */
> @@ -4654,7 +4654,7 @@ static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
>
> BUG_ON(extent_buffer_under_io(eb));
>
> - index = num_extent_pages(eb->start, eb->len);
> + index = num_extent_pages(eb);
> if (index == 0)
> return;
>
> @@ -4747,7 +4747,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
> unsigned long i;
> struct page *p;
> struct extent_buffer *new;
> - unsigned long num_pages = num_extent_pages(src->start, src->len);
> + unsigned long num_pages = num_extent_pages(src);
>
> new = __alloc_extent_buffer(src->fs_info, src->start, src->len);
> if (new == NULL)
> @@ -4779,12 +4779,11 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
> unsigned long num_pages;
> unsigned long i;
>
> - num_pages = num_extent_pages(start, len);
> -
> eb = __alloc_extent_buffer(fs_info, start, len);
> if (!eb)
> return NULL;
>
> + num_pages = num_extent_pages(eb);
> for (i = 0; i < num_pages; i++) {
> eb->pages[i] = alloc_page(GFP_NOFS);
> if (!eb->pages[i])
> @@ -4848,7 +4847,7 @@ static void mark_extent_buffer_accessed(struct extent_buffer *eb,
>
> check_buffer_tree_ref(eb);
>
> - num_pages = num_extent_pages(eb->start, eb->len);
> + num_pages = num_extent_pages(eb);
> for (i = 0; i < num_pages; i++) {
> struct page *p = eb->pages[i];
>
> @@ -4945,7 +4944,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
> u64 start)
> {
> unsigned long len = fs_info->nodesize;
> - unsigned long num_pages = num_extent_pages(start, len);
> + unsigned long num_pages;
> unsigned long i;
> unsigned long index = start >> PAGE_SHIFT;
> struct extent_buffer *eb;
> @@ -4968,6 +4967,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
> if (!eb)
> return ERR_PTR(-ENOMEM);
>
> + num_pages = num_extent_pages(eb);
> for (i = 0; i < num_pages; i++, index++) {
> p = find_or_create_page(mapping, index, GFP_NOFS|__GFP_NOFAIL);
> if (!p) {
> @@ -5164,7 +5164,7 @@ void clear_extent_buffer_dirty(struct extent_buffer *eb)
> unsigned long num_pages;
> struct page *page;
>
> - num_pages = num_extent_pages(eb->start, eb->len);
> + num_pages = num_extent_pages(eb);
>
> for (i = 0; i < num_pages; i++) {
> page = eb->pages[i];
> @@ -5198,7 +5198,7 @@ int set_extent_buffer_dirty(struct extent_buffer *eb)
>
> was_dirty = test_and_set_bit(EXTENT_BUFFER_DIRTY, &eb->bflags);
>
> - num_pages = num_extent_pages(eb->start, eb->len);
> + num_pages = num_extent_pages(eb);
> WARN_ON(atomic_read(&eb->refs) == 0);
> WARN_ON(!test_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags));
>
> @@ -5214,7 +5214,7 @@ void clear_extent_buffer_uptodate(struct extent_buffer *eb)
> unsigned long num_pages;
>
> clear_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
> - num_pages = num_extent_pages(eb->start, eb->len);
> + num_pages = num_extent_pages(eb);
> for (i = 0; i < num_pages; i++) {
> page = eb->pages[i];
> if (page)
> @@ -5229,7 +5229,7 @@ void set_extent_buffer_uptodate(struct extent_buffer *eb)
> unsigned long num_pages;
>
> set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
> - num_pages = num_extent_pages(eb->start, eb->len);
> + num_pages = num_extent_pages(eb);
> for (i = 0; i < num_pages; i++) {
> page = eb->pages[i];
> SetPageUptodate(page);
> @@ -5253,7 +5253,7 @@ int read_extent_buffer_pages(struct extent_io_tree *tree,
> if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))
> return 0;
>
> - num_pages = num_extent_pages(eb->start, eb->len);
> + num_pages = num_extent_pages(eb);
> for (i = 0; i < num_pages; i++) {
> page = eb->pages[i];
> if (wait == WAIT_NONE) {
> @@ -5581,7 +5581,7 @@ void copy_extent_buffer_full(struct extent_buffer *dst,
>
> ASSERT(dst->len == src->len);
>
> - num_pages = num_extent_pages(dst->start, dst->len);
> + num_pages = num_extent_pages(dst);
> for (i = 0; i < num_pages; i++)
> copy_page(page_address(dst->pages[i]),
> page_address(src->pages[i]));
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index ee92c1289edd..d08abc9d385e 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -452,9 +452,9 @@ int read_extent_buffer_pages(struct extent_io_tree *tree,
> int mirror_num);
> void wait_on_extent_buffer_writeback(struct extent_buffer *eb);
>
> -static inline unsigned long num_extent_pages(u64 start, u64 len)
> +static inline unsigned long num_extent_pages(const struct extent_buffer *eb)
> {
> - return len >> PAGE_SHIFT;
> + return eb->len >> PAGE_SHIFT;
> }
>
> static inline void extent_buffer_get(struct extent_buffer *eb)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] btrfs: switch types to int when counting eb pages
2018-04-23 23:03 ` [PATCH 3/3] btrfs: switch types to int when counting eb pages David Sterba
@ 2018-04-24 13:26 ` Nikolay Borisov
0 siblings, 0 replies; 10+ messages in thread
From: Nikolay Borisov @ 2018-04-24 13:26 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 24.04.2018 02:03, David Sterba wrote:
> The loops iterating eb pages use unsigned long, that's an overkill as
> we know that there are at most 16 pages (64k / 4k), and 4 by default
> (with nodesize 16k).
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/extent_io.c | 44 ++++++++++++++++++++++----------------------
> fs/btrfs/extent_io.h | 2 +-
> 2 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 0cc5d6ae1876..5bdfdb9c8777 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2062,7 +2062,7 @@ int repair_eb_io_failure(struct btrfs_fs_info *fs_info,
> struct extent_buffer *eb, int mirror_num)
> {
> u64 start = eb->start;
> - unsigned long i, num_pages = num_extent_pages(eb);
> + int i, num_pages = num_extent_pages(eb);
> int ret = 0;
>
> if (sb_rdonly(fs_info->sb))
> @@ -3541,7 +3541,7 @@ lock_extent_buffer_for_io(struct extent_buffer *eb,
> struct btrfs_fs_info *fs_info,
> struct extent_page_data *epd)
> {
> - unsigned long i, num_pages;
> + int i, num_pages;
> int flush = 0;
> int ret = 0;
>
> @@ -3715,7 +3715,7 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
> struct extent_io_tree *tree = &BTRFS_I(fs_info->btree_inode)->io_tree;
> u64 offset = eb->start;
> u32 nritems;
> - unsigned long i, num_pages;
> + int i, num_pages;
> unsigned long start, end;
> unsigned int write_flags = wbc_to_write_flags(wbc) | REQ_META;
> int ret = 0;
> @@ -4648,7 +4648,7 @@ int extent_buffer_under_io(struct extent_buffer *eb)
> */
> static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
> {
> - unsigned long index;
> + int index;
> struct page *page;
> int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
>
> @@ -4744,10 +4744,10 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
>
> struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
> {
> - unsigned long i;
> + int i;
> struct page *p;
> struct extent_buffer *new;
> - unsigned long num_pages = num_extent_pages(src);
> + int num_pages = num_extent_pages(src);
>
> new = __alloc_extent_buffer(src->fs_info, src->start, src->len);
> if (new == NULL)
> @@ -4776,8 +4776,8 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
> u64 start, unsigned long len)
> {
> struct extent_buffer *eb;
> - unsigned long num_pages;
> - unsigned long i;
> + int num_pages;
> + int i;
>
> eb = __alloc_extent_buffer(fs_info, start, len);
> if (!eb)
> @@ -4843,7 +4843,7 @@ static void check_buffer_tree_ref(struct extent_buffer *eb)
> static void mark_extent_buffer_accessed(struct extent_buffer *eb,
> struct page *accessed)
> {
> - unsigned long num_pages, i;
> + int num_pages, i;
>
> check_buffer_tree_ref(eb);
>
> @@ -4944,8 +4944,8 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
> u64 start)
> {
> unsigned long len = fs_info->nodesize;
> - unsigned long num_pages;
> - unsigned long i;
> + int num_pages;
> + int i;
> unsigned long index = start >> PAGE_SHIFT;
> struct extent_buffer *eb;
> struct extent_buffer *exists = NULL;
> @@ -5160,8 +5160,8 @@ void free_extent_buffer_stale(struct extent_buffer *eb)
>
> void clear_extent_buffer_dirty(struct extent_buffer *eb)
> {
> - unsigned long i;
> - unsigned long num_pages;
> + int i;
> + int num_pages;
> struct page *page;
>
> num_pages = num_extent_pages(eb);
> @@ -5190,8 +5190,8 @@ void clear_extent_buffer_dirty(struct extent_buffer *eb)
>
> int set_extent_buffer_dirty(struct extent_buffer *eb)
> {
> - unsigned long i;
> - unsigned long num_pages;
> + int i;
> + int num_pages;
> int was_dirty = 0;
>
> check_buffer_tree_ref(eb);
> @@ -5209,9 +5209,9 @@ int set_extent_buffer_dirty(struct extent_buffer *eb)
>
> void clear_extent_buffer_uptodate(struct extent_buffer *eb)
> {
> - unsigned long i;
> + int i;
> struct page *page;
> - unsigned long num_pages;
> + int num_pages;
>
> clear_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
> num_pages = num_extent_pages(eb);
> @@ -5224,9 +5224,9 @@ void clear_extent_buffer_uptodate(struct extent_buffer *eb)
>
> void set_extent_buffer_uptodate(struct extent_buffer *eb)
> {
> - unsigned long i;
> + int i;
> struct page *page;
> - unsigned long num_pages;
> + int num_pages;
>
> set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
> num_pages = num_extent_pages(eb);
> @@ -5239,13 +5239,13 @@ void set_extent_buffer_uptodate(struct extent_buffer *eb)
> int read_extent_buffer_pages(struct extent_io_tree *tree,
> struct extent_buffer *eb, int wait, int mirror_num)
> {
> - unsigned long i;
> + int i;
> struct page *page;
> int err;
> int ret = 0;
> int locked_pages = 0;
> int all_uptodate = 1;
> - unsigned long num_pages;
> + int num_pages;
> unsigned long num_reads = 0;
> struct bio *bio = NULL;
> unsigned long bio_flags = 0;
> @@ -5577,7 +5577,7 @@ void copy_extent_buffer_full(struct extent_buffer *dst,
> struct extent_buffer *src)
> {
> int i;
> - unsigned num_pages;
> + int num_pages;
>
> ASSERT(dst->len == src->len);
>
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index d08abc9d385e..3248553a3aa2 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -452,7 +452,7 @@ int read_extent_buffer_pages(struct extent_io_tree *tree,
> int mirror_num);
> void wait_on_extent_buffer_writeback(struct extent_buffer *eb);
>
> -static inline unsigned long num_extent_pages(const struct extent_buffer *eb)
> +static inline int num_extent_pages(const struct extent_buffer *eb)
> {
> return eb->len >> PAGE_SHIFT;
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-04-24 13:26 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-23 23:03 [PATCH 0/3] Simplify counting of extent buffer pages David Sterba
2018-04-23 23:03 ` [PATCH 1/3] btrfs: simplify counting number of eb pages David Sterba
2018-04-24 5:59 ` Nikolay Borisov
2018-04-24 6:22 ` Qu Wenruo
2018-04-24 10:29 ` David Sterba
2018-04-24 10:36 ` Qu Wenruo
2018-04-23 23:03 ` [PATCH 2/3] btrfs: pass only eb to num_extent_pages David Sterba
2018-04-24 13:26 ` Nikolay Borisov
2018-04-23 23:03 ` [PATCH 3/3] btrfs: switch types to int when counting eb pages David Sterba
2018-04-24 13:26 ` Nikolay Borisov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).