Simplify counting of extent buffer pages

PATCH In review David Sterba 4 participants 10 messages Apr 23, 2018 – Apr 24, 2018 Btrfs file system

lore

Share & tools

lore

David Sterba

author maintainer

2018-04-23 23:06 UTC #0 lore raw

cover Simplify counting of extent buffer pages

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(-)

Signature · 1 line

2.16.2

David Sterba

author maintainer

2018-04-23 23:06 UTC #1 lore raw

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

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 <[email protected]>

---
 fs/btrfs/extent_io.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

fs/btrfs/extent_io.h +1 -2

index a53009694b16..ee92c1289edd 100644
@@ -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)

Signature · 1 line

2.16.2

Nikolay Borisov

2018-04-24 05:59 UTC #2 lore raw

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

Quoted 14 lines · 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 <[email protected]>
---
 fs/btrfs/extent_io.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
fs/btrfs/extent_io.h +0 -0
index a53009694b16..ee92c1289edd 100644

fs/btrfs/extent_io.h void wait_on_extent_buffer_writeback(struct extent_buffer *eb); · line 457

Quoted 6 lines

fs/btrfs/extent_io.h +1 -2

@@ -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

Quoted 4 lines

fs/btrfs/extent_io.h +0 -0

 }
 
 static inline void extent_buffer_get(struct extent_buffer *eb)

Qu Wenruo

2018-04-24 06:22 UTC #3 lore raw

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

Quoted 28 lines · Nikolay Borisov

Quoted 14 lines · 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 <[email protected]>
---
 fs/btrfs/extent_io.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
fs/btrfs/extent_io.h +0 -0
index a53009694b16..ee92c1289edd 100644
fs/btrfs/extent_io.h void wait_on_extent_buffer_writeback(struct extent_buffer *eb); · line 457
Quoted 6 lines
fs/btrfs/extent_io.h +1 -2
@@ -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:

Quoted 7 lines

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

Quoted 12 lines

arch/powerpc/include/asm/page.h:#define PAGE_SHIFT 12
fs/btrfs/extent_io.h void wait_on_extent_buffer_writeback(struct extent_buffer *eb); · line 460
Quoted 4 lines
fs/btrfs/extent_io.h +0 -0
 }
 
 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Sterba

2018-04-24 10:31 UTC #4 lore raw

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

Quoted 39 lines · Qu Wenruo

Quoted 28 lines · Nikolay Borisov
Quoted 14 lines · 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 <[email protected]>
---
 fs/btrfs/extent_io.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
fs/btrfs/extent_io.h +0 -0
index a53009694b16..ee92c1289edd 100644
fs/btrfs/extent_io.h void wait_on_extent_buffer_writeback(struct extent_buffer *eb); · line 457
Quoted 6 lines
fs/btrfs/extent_io.h +1 -2
@@ -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:
Quoted 7 lines
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

Quoted 6 lines

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.

Qu Wenruo

2018-04-24 10:36 UTC #5 lore raw

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

Quoted 44 lines · David Sterba

Quoted 43 lines · Qu Wenruo
Quoted 28 lines · Nikolay Borisov
Quoted 14 lines · 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 <[email protected]>
---
 fs/btrfs/extent_io.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
fs/btrfs/extent_io.h +0 -0
index a53009694b16..ee92c1289edd 100644
fs/btrfs/extent_io.h void wait_on_extent_buffer_writeback(struct extent_buffer *eb); · line 457
Quoted 6 lines
fs/btrfs/extent_io.h +1 -2
@@ -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:
Quoted 7 lines
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.

Quoted 5 lines

Quoted 2 lines
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

Quoted 5 lines

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

David Sterba

author maintainer

2018-04-23 23:06 UTC #6 lore raw

PATCH 2/3 btrfs: pass only eb to num_extent_pages

In reply to 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 <[email protected]>

---
 fs/btrfs/extent_io.c | 30 +++++++++++++++---------------
 fs/btrfs/extent_io.h |  4 ++--
 2 files changed, 17 insertions(+), 17 deletions(-)

fs/btrfs/extent_io.c +15 ~~-15~~

index fb32394fd830..0cc5d6ae1876 100644
@@ -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]));

fs/btrfs/extent_io.h +2 -2

index ee92c1289edd..d08abc9d385e 100644
@@ -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)

Signature · 1 line

2.16.2

Nikolay Borisov

Reviewed-by on v1 2/3

2018-04-24 13:26 UTC #7 lore raw

Re: [PATCH 2/3] btrfs: pass only eb to num_extent_pages

Quoted 6 lines · 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 <[email protected]>

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 165 lines

---
 fs/btrfs/extent_io.c | 30 +++++++++++++++---------------
 fs/btrfs/extent_io.h |  4 ++--
 2 files changed, 17 insertions(+), 17 deletions(-)

fs/btrfs/extent_io.c +15 ~~-15~~

index fb32394fd830..0cc5d6ae1876 100644
@@ -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]));

fs/btrfs/extent_io.h +2 -2

index ee92c1289edd..d08abc9d385e 100644
@@ -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)

David Sterba

author maintainer

2018-04-23 23:06 UTC #8 lore raw

PATCH 3/3 btrfs: switch types to int when counting eb pages

In reply to 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 <[email protected]>

---
 fs/btrfs/extent_io.c | 44 ++++++++++++++++++++++----------------------
 fs/btrfs/extent_io.h |  2 +-
 2 files changed, 23 insertions(+), 23 deletions(-)

fs/btrfs/extent_io.c +22 ~~-22~~

index 0cc5d6ae1876..5bdfdb9c8777 100644
@@ -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);

fs/btrfs/extent_io.h +1 -1

index d08abc9d385e..3248553a3aa2 100644
@@ -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;
 }

Signature · 1 line

2.16.2

Nikolay Borisov

Reviewed-by on v1 3/3

2018-04-24 13:26 UTC #9 lore raw

Re: [PATCH 3/3] btrfs: switch types to int when counting eb pages

Quoted 5 lines · 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 <[email protected]>

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 174 lines

---
 fs/btrfs/extent_io.c | 44 ++++++++++++++++++++++----------------------
 fs/btrfs/extent_io.h |  2 +-
 2 files changed, 23 insertions(+), 23 deletions(-)

fs/btrfs/extent_io.c +22 ~~-22~~

index 0cc5d6ae1876..5bdfdb9c8777 100644
@@ -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);

fs/btrfs/extent_io.h +1 -1

index d08abc9d385e..3248553a3aa2 100644
@@ -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;
 }