[v2] btrfs: move subpage preallocation out of the loop

Commit Message

Qu Wenruo July 9, 2023, 7:08 a.m. UTC

Initially we preallocate btrfs_subpage structure in the main loop of
alloc_extent_buffer().

But later commit fbca46eb46ec ("btrfs: make nodesize >= PAGE_SIZE case
to reuse the non-subpage routine") has make sure we only go subpage
routine if our nodesize is smaller than PAGE_SIZE.

This means for that case, we only need to allocate the subpage structure
once anyway.

So this patch would make the preallocation out of the main loop.

This would slightly reduce the workload when we hold the page lock, and
make code a little easier to read.

Signed-off-by: Qu Wenruo <[e-mail]>
---
Changelog:
v2:
- Fix a possible (not that possible though) memory leak
  If find_or_create_page() failed (which calls with GFP_NOFAIL), we
  still need to release the preallocated memory.
---
 fs/btrfs/extent_io.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

Comments

David Sterba July 11, 2023, 9:10 p.m. UTC | #1

On Sun, Jul 09, 2023 at 03:08:18PM +0800, Qu Wenruo wrote:
> Initially we preallocate btrfs_subpage structure in the main loop of
> alloc_extent_buffer().
> 
> But later commit fbca46eb46ec ("btrfs: make nodesize >= PAGE_SIZE case
> to reuse the non-subpage routine") has make sure we only go subpage
> routine if our nodesize is smaller than PAGE_SIZE.
> 
> This means for that case, we only need to allocate the subpage structure
> once anyway.
> 
> So this patch would make the preallocation out of the main loop.
> 
> This would slightly reduce the workload when we hold the page lock, and
> make code a little easier to read.
> 
> Signed-off-by: Qu Wenruo <[e-mail]>
> ---
> Changelog:
> v2:
> - Fix a possible (not that possible though) memory leak
>   If find_or_create_page() failed (which calls with GFP_NOFAIL), we
>   still need to release the preallocated memory.

Added to misc-next, thanks.

Patch

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index f2b3007803be..5851bb05897d 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3529,6 +3529,7 @@  struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 	struct extent_buffer *exists = NULL;
 	struct page *p;
 	struct address_space *mapping = fs_info->btree_inode->i_mapping;
+	struct btrfs_subpage *prealloc = NULL;
 	u64 lockdep_owner = owner_root;
 	bool pages_contig = true;
 	int uptodate = 1;
@@ -3566,36 +3567,30 @@  struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 	btrfs_set_buffer_lockdep_class(lockdep_owner, eb, level);
 
 	num_pages = num_extent_pages(eb);
-	for (i = 0; i < num_pages; i++, index++) {
-		struct btrfs_subpage *prealloc = NULL;
 
+	/*
+	 * Preallocate page->private for subpage case, so that we won't
+	 * allocate memory with private_lock nor page lock hold.
+	 *
+	 * The memory will be freed by attach_extent_buffer_page() or freed
+	 * manually if we exit earlier.
+	 */
+	if (fs_info->nodesize < PAGE_SIZE) {
+		prealloc = btrfs_alloc_subpage(fs_info, BTRFS_SUBPAGE_METADATA);
+		if (IS_ERR(prealloc)) {
+			exists = ERR_CAST(prealloc);
+			goto free_eb;
+		}
+	}
+
+	for (i = 0; i < num_pages; i++, index++) {
 		p = find_or_create_page(mapping, index, GFP_NOFS|__GFP_NOFAIL);
 		if (!p) {
 			exists = ERR_PTR(-ENOMEM);
+			btrfs_free_subpage(prealloc);
 			goto free_eb;
 		}
 
-		/*
-		 * Preallocate page->private for subpage case, so that we won't
-		 * allocate memory with private_lock hold.  The memory will be
-		 * freed by attach_extent_buffer_page() or freed manually if
-		 * we exit earlier.
-		 *
-		 * Although we have ensured one subpage eb can only have one
-		 * page, but it may change in the future for 16K page size
-		 * support, so we still preallocate the memory in the loop.
-		 */
-		if (fs_info->nodesize < PAGE_SIZE) {
-			prealloc = btrfs_alloc_subpage(fs_info, BTRFS_SUBPAGE_METADATA);
-			if (IS_ERR(prealloc)) {
-				ret = PTR_ERR(prealloc);
-				unlock_page(p);
-				put_page(p);
-				exists = ERR_PTR(ret);
-				goto free_eb;
-			}
-		}
-
 		spin_lock(&mapping->private_lock);
 		exists = grab_extent_buffer(fs_info, p);
 		if (exists) {