Simplify counting of extent buffer 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 +0 -0

index a53009694b16..ee92c1289edd 100644

fs/btrfs/extent_io.h +0 -0

 }
 
 static inline void extent_buffer_get(struct extent_buffer *eb)

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

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

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.

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

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

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)

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;
 }