[PATCH 0/7] Allocate structures on stack instead of kmalloc()

[PATCH 0/7] Allocate structures on stack instead of kmalloc()

[Goldwyn Rodrigues]

Here are some structs which can be converted to allocation on stack in order
to save on post-checks on kmalloc() and kfree() each of them.

Sizes of the structures are also in the commit message in case you feel they
are too bit to be allocated on stack.

There are two more structs in ioctl.c which can be converted, but
I was not sure of them:

1. In create_snapshot(), pending_snapshot can be converted. pending_snapshot
   is used in the transaction.
2. In btrfs_ioctl_set_received_subvol_32, args64 can be converted, but args32
   cannot. All Pointers associated with memdup_user() can also be converted
   by using copy_from_user() instead. This would include many more structs.

Goldwyn Rodrigues (7):
  btrfs: Allocate walk_control on stack
  btrfs: Allocate file_ra_state on stack
  btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack
  btrfs: Allocate btrfs_ioctl_balance_args on stack
  btrfs: Allocate btrfs_ioctl_quota_rescan_args on stack
  btrfs: Allocate btrfs_ioctl_defrag_range_args on stack
  btrfs: Alloc backref_ctx on stack

 fs/btrfs/extent-tree.c      |  89 +++++++++++++-----------------
 fs/btrfs/free-space-cache.c |  12 ++---
 fs/btrfs/ioctl.c            | 105 ++++++++++++++----------------------
 fs/btrfs/send.c             |  29 ++++------
 4 files changed, 89 insertions(+), 146 deletions(-)

[PATCH 1/7] btrfs: Allocate walk_control on stack

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Instead of using kmalloc() to allocate walk_control, allocate
walk_control on stack.

No need to memset() a struct to zero if it is initialized to zero.

sizeof(walk_control) = 200

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/extent-tree.c | 89 +++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 53 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index fc3da7585fb7..a66cb2e5146f 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5484,7 +5484,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 	struct btrfs_trans_handle *trans;
 	struct btrfs_root *tree_root = fs_info->tree_root;
 	struct btrfs_root_item *root_item = &root->root_item;
-	struct walk_control *wc;
+	struct walk_control wc = {0};
 	struct btrfs_key key;
 	int err = 0;
 	int ret;
@@ -5499,13 +5499,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 		goto out;
 	}
 
-	wc = kzalloc(sizeof(*wc), GFP_NOFS);
-	if (!wc) {
-		btrfs_free_path(path);
-		err = -ENOMEM;
-		goto out;
-	}
-
 	/*
 	 * Use join to avoid potential EINTR from transaction start. See
 	 * wait_reserve_ticket and the whole reservation callchain.
@@ -5537,12 +5530,10 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 		path->nodes[level] = btrfs_lock_root_node(root);
 		path->slots[level] = 0;
 		path->locks[level] = BTRFS_WRITE_LOCK;
-		memset(&wc->update_progress, 0,
-		       sizeof(wc->update_progress));
 	} else {
 		btrfs_disk_key_to_cpu(&key, &root_item->drop_progress);
-		memcpy(&wc->update_progress, &key,
-		       sizeof(wc->update_progress));
+		memcpy(&wc.update_progress, &key,
+		       sizeof(wc.update_progress));
 
 		level = btrfs_root_drop_level(root_item);
 		BUG_ON(level == 0);
@@ -5568,62 +5559,62 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 
 			ret = btrfs_lookup_extent_info(trans, fs_info,
 						path->nodes[level]->start,
-						level, 1, &wc->refs[level],
-						&wc->flags[level]);
+						level, 1, &wc.refs[level],
+						&wc.flags[level]);
 			if (ret < 0) {
 				err = ret;
 				goto out_end_trans;
 			}
-			BUG_ON(wc->refs[level] == 0);
+			BUG_ON(wc.refs[level] == 0);
 
 			if (level == btrfs_root_drop_level(root_item))
 				break;
 
 			btrfs_tree_unlock(path->nodes[level]);
 			path->locks[level] = 0;
-			WARN_ON(wc->refs[level] != 1);
+			WARN_ON(wc.refs[level] != 1);
 			level--;
 		}
 	}
 
-	wc->restarted = test_bit(BTRFS_ROOT_DEAD_TREE, &root->state);
-	wc->level = level;
-	wc->shared_level = -1;
-	wc->stage = DROP_REFERENCE;
-	wc->update_ref = update_ref;
-	wc->keep_locks = 0;
-	wc->reada_count = BTRFS_NODEPTRS_PER_BLOCK(fs_info);
+	wc.restarted = test_bit(BTRFS_ROOT_DEAD_TREE, &root->state);
+	wc.level = level;
+	wc.shared_level = -1;
+	wc.stage = DROP_REFERENCE;
+	wc.update_ref = update_ref;
+	wc.keep_locks = 0;
+	wc.reada_count = BTRFS_NODEPTRS_PER_BLOCK(fs_info);
 
 	while (1) {
 
-		ret = walk_down_tree(trans, root, path, wc);
+		ret = walk_down_tree(trans, root, path, &wc);
 		if (ret < 0) {
 			err = ret;
 			break;
 		}
 
-		ret = walk_up_tree(trans, root, path, wc, BTRFS_MAX_LEVEL);
+		ret = walk_up_tree(trans, root, path, &wc, BTRFS_MAX_LEVEL);
 		if (ret < 0) {
 			err = ret;
 			break;
 		}
 
 		if (ret > 0) {
-			BUG_ON(wc->stage != DROP_REFERENCE);
+			BUG_ON(wc.stage != DROP_REFERENCE);
 			break;
 		}
 
-		if (wc->stage == DROP_REFERENCE) {
-			wc->drop_level = wc->level;
-			btrfs_node_key_to_cpu(path->nodes[wc->drop_level],
-					      &wc->drop_progress,
-					      path->slots[wc->drop_level]);
+		if (wc.stage == DROP_REFERENCE) {
+			wc.drop_level = wc.level;
+			btrfs_node_key_to_cpu(path->nodes[wc.drop_level],
+					      &wc.drop_progress,
+					      path->slots[wc.drop_level]);
 		}
 		btrfs_cpu_key_to_disk(&root_item->drop_progress,
-				      &wc->drop_progress);
-		btrfs_set_root_drop_level(root_item, wc->drop_level);
+				      &wc.drop_progress);
+		btrfs_set_root_drop_level(root_item, wc.drop_level);
 
-		BUG_ON(wc->level == 0);
+		BUG_ON(wc.level == 0);
 		if (btrfs_should_end_transaction(trans) ||
 		    (!for_reloc && btrfs_need_cleaner_sleep(fs_info))) {
 			ret = btrfs_update_root(trans, tree_root,
@@ -5703,7 +5694,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 out_end_trans:
 	btrfs_end_transaction_throttle(trans);
 out_free:
-	kfree(wc);
 	btrfs_free_path(path);
 out:
 	/*
@@ -5731,7 +5721,7 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_path *path;
-	struct walk_control *wc;
+	struct walk_control wc = {0};
 	int level;
 	int parent_level;
 	int ret = 0;
@@ -5743,12 +5733,6 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
 	if (!path)
 		return -ENOMEM;
 
-	wc = kzalloc(sizeof(*wc), GFP_NOFS);
-	if (!wc) {
-		btrfs_free_path(path);
-		return -ENOMEM;
-	}
-
 	btrfs_assert_tree_locked(parent);
 	parent_level = btrfs_header_level(parent);
 	atomic_inc(&parent->refs);
@@ -5761,30 +5745,29 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
 	path->slots[level] = 0;
 	path->locks[level] = BTRFS_WRITE_LOCK;
 
-	wc->refs[parent_level] = 1;
-	wc->flags[parent_level] = BTRFS_BLOCK_FLAG_FULL_BACKREF;
-	wc->level = level;
-	wc->shared_level = -1;
-	wc->stage = DROP_REFERENCE;
-	wc->update_ref = 0;
-	wc->keep_locks = 1;
-	wc->reada_count = BTRFS_NODEPTRS_PER_BLOCK(fs_info);
+	wc.refs[parent_level] = 1;
+	wc.flags[parent_level] = BTRFS_BLOCK_FLAG_FULL_BACKREF;
+	wc.level = level;
+	wc.shared_level = -1;
+	wc.stage = DROP_REFERENCE;
+	wc.update_ref = 0;
+	wc.keep_locks = 1;
+	wc.reada_count = BTRFS_NODEPTRS_PER_BLOCK(fs_info);
 
 	while (1) {
-		wret = walk_down_tree(trans, root, path, wc);
+		wret = walk_down_tree(trans, root, path, &wc);
 		if (wret < 0) {
 			ret = wret;
 			break;
 		}
 
-		wret = walk_up_tree(trans, root, path, wc, parent_level);
+		wret = walk_up_tree(trans, root, path, &wc, parent_level);
 		if (wret < 0)
 			ret = wret;
 		if (wret != 0)
 			break;
 	}
 
-	kfree(wc);
 	btrfs_free_path(path);
 	return ret;
 }
-- 
2.32.0

[PATCH 2/7] btrfs: Allocate file_ra_state on stack

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Instead of allocating file_ra_state using kmalloc, allocate on stack.
sizeof(struct readahead) = 32 bytes

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/free-space-cache.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 2131ae5b9ed7..8eeb65278ac0 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -344,19 +344,13 @@ int btrfs_truncate_free_space_cache(struct btrfs_trans_handle *trans,
 
 static void readahead_cache(struct inode *inode)
 {
-	struct file_ra_state *ra;
+	struct file_ra_state ra;
 	unsigned long last_index;
 
-	ra = kzalloc(sizeof(*ra), GFP_NOFS);
-	if (!ra)
-		return;
-
-	file_ra_state_init(ra, inode->i_mapping);
+	file_ra_state_init(&ra, inode->i_mapping);
 	last_index = (i_size_read(inode) - 1) >> PAGE_SHIFT;
 
-	page_cache_sync_readahead(inode->i_mapping, ra, NULL, 0, last_index);
-
-	kfree(ra);
+	page_cache_sync_readahead(inode->i_mapping, &ra, NULL, 0, last_index);
 }
 
 static int io_ctl_init(struct btrfs_io_ctl *io_ctl, struct inode *inode,
-- 
2.32.0

[PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Instead of using kmalloc() to allocate btrfs_ioctl_get_subvol_info_args,
allocate btrfs_ioctl_get_subvol_info_args on stack.

sizeof(btrfs_ioctl_get_subvol_info_args) = 504

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/ioctl.c | 55 +++++++++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0ba98e08a029..90b134b5a653 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2681,7 +2681,7 @@ static int btrfs_ioctl_ino_lookup_user(struct file *file, void __user *argp)
 /* Get the subvolume information in BTRFS_ROOT_ITEM and BTRFS_ROOT_BACKREF */
 static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 {
-	struct btrfs_ioctl_get_subvol_info_args *subvol_info;
+	struct btrfs_ioctl_get_subvol_info_args subvol_info = {0};
 	struct btrfs_fs_info *fs_info;
 	struct btrfs_root *root;
 	struct btrfs_path *path;
@@ -2699,12 +2699,6 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 	if (!path)
 		return -ENOMEM;
 
-	subvol_info = kzalloc(sizeof(*subvol_info), GFP_KERNEL);
-	if (!subvol_info) {
-		btrfs_free_path(path);
-		return -ENOMEM;
-	}
-
 	inode = file_inode(file);
 	fs_info = BTRFS_I(inode)->root->fs_info;
 
@@ -2717,32 +2711,32 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 	}
 	root_item = &root->root_item;
 
-	subvol_info->treeid = key.objectid;
+	subvol_info.treeid = key.objectid;
 
-	subvol_info->generation = btrfs_root_generation(root_item);
-	subvol_info->flags = btrfs_root_flags(root_item);
+	subvol_info.generation = btrfs_root_generation(root_item);
+	subvol_info.flags = btrfs_root_flags(root_item);
 
-	memcpy(subvol_info->uuid, root_item->uuid, BTRFS_UUID_SIZE);
-	memcpy(subvol_info->parent_uuid, root_item->parent_uuid,
+	memcpy(subvol_info.uuid, root_item->uuid, BTRFS_UUID_SIZE);
+	memcpy(subvol_info.parent_uuid, root_item->parent_uuid,
 						    BTRFS_UUID_SIZE);
-	memcpy(subvol_info->received_uuid, root_item->received_uuid,
+	memcpy(subvol_info.received_uuid, root_item->received_uuid,
 						    BTRFS_UUID_SIZE);
 
-	subvol_info->ctransid = btrfs_root_ctransid(root_item);
-	subvol_info->ctime.sec = btrfs_stack_timespec_sec(&root_item->ctime);
-	subvol_info->ctime.nsec = btrfs_stack_timespec_nsec(&root_item->ctime);
+	subvol_info.ctransid = btrfs_root_ctransid(root_item);
+	subvol_info.ctime.sec = btrfs_stack_timespec_sec(&root_item->ctime);
+	subvol_info.ctime.nsec = btrfs_stack_timespec_nsec(&root_item->ctime);
 
-	subvol_info->otransid = btrfs_root_otransid(root_item);
-	subvol_info->otime.sec = btrfs_stack_timespec_sec(&root_item->otime);
-	subvol_info->otime.nsec = btrfs_stack_timespec_nsec(&root_item->otime);
+	subvol_info.otransid = btrfs_root_otransid(root_item);
+	subvol_info.otime.sec = btrfs_stack_timespec_sec(&root_item->otime);
+	subvol_info.otime.nsec = btrfs_stack_timespec_nsec(&root_item->otime);
 
-	subvol_info->stransid = btrfs_root_stransid(root_item);
-	subvol_info->stime.sec = btrfs_stack_timespec_sec(&root_item->stime);
-	subvol_info->stime.nsec = btrfs_stack_timespec_nsec(&root_item->stime);
+	subvol_info.stransid = btrfs_root_stransid(root_item);
+	subvol_info.stime.sec = btrfs_stack_timespec_sec(&root_item->stime);
+	subvol_info.stime.nsec = btrfs_stack_timespec_nsec(&root_item->stime);
 
-	subvol_info->rtransid = btrfs_root_rtransid(root_item);
-	subvol_info->rtime.sec = btrfs_stack_timespec_sec(&root_item->rtime);
-	subvol_info->rtime.nsec = btrfs_stack_timespec_nsec(&root_item->rtime);
+	subvol_info.rtransid = btrfs_root_rtransid(root_item);
+	subvol_info.rtime.sec = btrfs_stack_timespec_sec(&root_item->rtime);
+	subvol_info.rtime.nsec = btrfs_stack_timespec_nsec(&root_item->rtime);
 
 	if (key.objectid != BTRFS_FS_TREE_OBJECTID) {
 		/* Search root tree for ROOT_BACKREF of this subvolume */
@@ -2765,18 +2759,18 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 		leaf = path->nodes[0];
 		slot = path->slots[0];
 		btrfs_item_key_to_cpu(leaf, &key, slot);
-		if (key.objectid == subvol_info->treeid &&
+		if (key.objectid == subvol_info.treeid &&
 		    key.type == BTRFS_ROOT_BACKREF_KEY) {
-			subvol_info->parent_id = key.offset;
+			subvol_info.parent_id = key.offset;
 
 			rref = btrfs_item_ptr(leaf, slot, struct btrfs_root_ref);
-			subvol_info->dirid = btrfs_root_ref_dirid(leaf, rref);
+			subvol_info.dirid = btrfs_root_ref_dirid(leaf, rref);
 
 			item_off = btrfs_item_ptr_offset(leaf, slot)
 					+ sizeof(struct btrfs_root_ref);
 			item_len = btrfs_item_size_nr(leaf, slot)
 					- sizeof(struct btrfs_root_ref);
-			read_extent_buffer(leaf, subvol_info->name,
+			read_extent_buffer(leaf, subvol_info.name,
 					   item_off, item_len);
 		} else {
 			ret = -ENOENT;
@@ -2784,14 +2778,13 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 		}
 	}
 
-	if (copy_to_user(argp, subvol_info, sizeof(*subvol_info)))
+	if (copy_to_user(argp, &subvol_info, sizeof(subvol_info)))
 		ret = -EFAULT;
 
 out:
 	btrfs_put_root(root);
 out_free:
 	btrfs_free_path(path);
-	kfree(subvol_info);
 	return ret;
 }
 
-- 
2.32.0

[PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args on stack

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Instead of using kmalloc() to allocate btrfs_ioctl_balance_args, allocate
btrfs_ioctl_balance_args on stack.

sizeof(btrfs_ioctl_balance_args) = 1024

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/ioctl.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 90b134b5a653..9c3acc539052 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4141,7 +4141,7 @@ static long btrfs_ioctl_balance_ctl(struct btrfs_fs_info *fs_info, int cmd)
 static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
 					 void __user *arg)
 {
-	struct btrfs_ioctl_balance_args *bargs;
+	struct btrfs_ioctl_balance_args bargs = {0};
 	int ret = 0;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -4153,18 +4153,11 @@ static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
 		goto out;
 	}
 
-	bargs = kzalloc(sizeof(*bargs), GFP_KERNEL);
-	if (!bargs) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	btrfs_update_ioctl_balance_args(fs_info, bargs);
+	btrfs_update_ioctl_balance_args(fs_info, &bargs);
 
-	if (copy_to_user(arg, bargs, sizeof(*bargs)))
+	if (copy_to_user(arg, &bargs, sizeof(bargs)))
 		ret = -EFAULT;
 
-	kfree(bargs);
 out:
 	mutex_unlock(&fs_info->balance_mutex);
 	return ret;
-- 
2.32.0

[PATCH 5/7] btrfs: Allocate btrfs_ioctl_quota_rescan_args on stack

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Instead of using kmalloc() to allocate btrfs_ioctl_quota_rescan_args,
allocate btrfs_ioctl_quota_rescan_args on stack.

sizeof(btrfs_ioctl_quota_rescan_args) = 64

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/ioctl.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9c3acc539052..291c16d8576b 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4390,25 +4390,20 @@ static long btrfs_ioctl_quota_rescan(struct file *file, void __user *arg)
 static long btrfs_ioctl_quota_rescan_status(struct btrfs_fs_info *fs_info,
 						void __user *arg)
 {
-	struct btrfs_ioctl_quota_rescan_args *qsa;
+	struct btrfs_ioctl_quota_rescan_args qsa = {0};
 	int ret = 0;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	qsa = kzalloc(sizeof(*qsa), GFP_KERNEL);
-	if (!qsa)
-		return -ENOMEM;
-
 	if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN) {
-		qsa->flags = 1;
-		qsa->progress = fs_info->qgroup_rescan_progress.objectid;
+		qsa.flags = 1;
+		qsa.progress = fs_info->qgroup_rescan_progress.objectid;
 	}
 
-	if (copy_to_user(arg, qsa, sizeof(*qsa)))
+	if (copy_to_user(arg, &qsa, sizeof(qsa)))
 		ret = -EFAULT;
 
-	kfree(qsa);
 	return ret;
 }
 
-- 
2.32.0

[PATCH 6/7] btrfs: Allocate btrfs_ioctl_defrag_range_args on stack

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Instead of using kmalloc() to allocate btrfs_ioctl_defrag_range_args,
allocate btrfs_ioctl_defrag_range_args on stack.

sizeof(btrfs_ioctl_defrag_range_args) = 48

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/ioctl.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 291c16d8576b..bc38a1af45c7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3096,7 +3096,7 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
 {
 	struct inode *inode = file_inode(file);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
-	struct btrfs_ioctl_defrag_range_args *range;
+	struct btrfs_ioctl_defrag_range_args range = {0};
 	int ret;
 
 	ret = mnt_want_write_file(file);
@@ -3128,33 +3128,25 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
 			goto out;
 		}
 
-		range = kzalloc(sizeof(*range), GFP_KERNEL);
-		if (!range) {
-			ret = -ENOMEM;
-			goto out;
-		}
-
 		if (argp) {
-			if (copy_from_user(range, argp,
-					   sizeof(*range))) {
+			if (copy_from_user(&range, argp,
+					   sizeof(range))) {
 				ret = -EFAULT;
-				kfree(range);
 				goto out;
 			}
 			/* compression requires us to start the IO */
-			if ((range->flags & BTRFS_DEFRAG_RANGE_COMPRESS)) {
-				range->flags |= BTRFS_DEFRAG_RANGE_START_IO;
-				range->extent_thresh = (u32)-1;
+			if ((range.flags & BTRFS_DEFRAG_RANGE_COMPRESS)) {
+				range.flags |= BTRFS_DEFRAG_RANGE_START_IO;
+				range.extent_thresh = (u32)-1;
 			}
 		} else {
 			/* the rest are all set to zero by kzalloc */
-			range->len = (u64)-1;
+			range.len = (u64)-1;
 		}
 		ret = btrfs_defrag_file(file_inode(file), file,
-					range, BTRFS_OLDEST_GENERATION, 0);
+					&range, BTRFS_OLDEST_GENERATION, 0);
 		if (ret > 0)
 			ret = 0;
-		kfree(range);
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.32.0

[PATCH 7/7] btrfs: Alloc backref_ctx on stack

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Instead of using kmalloc() to allocate backref_ctx, allocate backref_ctx
on stack.

sizeof(backref_ctx) = 48

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/send.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 6ac37ae6c811..e0553fa27f85 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1307,7 +1307,7 @@ static int find_extent_clone(struct send_ctx *sctx,
 	u64 flags = 0;
 	struct btrfs_file_extent_item *fi;
 	struct extent_buffer *eb = path->nodes[0];
-	struct backref_ctx *backref_ctx = NULL;
+	struct backref_ctx backref_ctx = {0};
 	struct clone_root *cur_clone_root;
 	struct btrfs_key found_key;
 	struct btrfs_path *tmp_path;
@@ -1322,12 +1322,6 @@ static int find_extent_clone(struct send_ctx *sctx,
 	/* We only use this path under the commit sem */
 	tmp_path->need_commit_sem = 0;
 
-	backref_ctx = kmalloc(sizeof(*backref_ctx), GFP_KERNEL);
-	if (!backref_ctx) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
 	if (data_offset >= ino_size) {
 		/*
 		 * There may be extents that lie behind the file's size.
@@ -1392,12 +1386,12 @@ static int find_extent_clone(struct send_ctx *sctx,
 		cur_clone_root->found_refs = 0;
 	}
 
-	backref_ctx->sctx = sctx;
-	backref_ctx->found = 0;
-	backref_ctx->cur_objectid = ino;
-	backref_ctx->cur_offset = data_offset;
-	backref_ctx->found_itself = 0;
-	backref_ctx->extent_len = num_bytes;
+	backref_ctx.sctx = sctx;
+	backref_ctx.found = 0;
+	backref_ctx.cur_objectid = ino;
+	backref_ctx.cur_offset = data_offset;
+	backref_ctx.found_itself = 0;
+	backref_ctx.extent_len = num_bytes;
 
 	/*
 	 * The last extent of a file may be too large due to page alignment.
@@ -1405,7 +1399,7 @@ static int find_extent_clone(struct send_ctx *sctx,
 	 * __iterate_backrefs work.
 	 */
 	if (data_offset + num_bytes >= ino_size)
-		backref_ctx->extent_len = ino_size - data_offset;
+		backref_ctx.extent_len = ino_size - data_offset;
 
 	/*
 	 * Now collect all backrefs.
@@ -1416,12 +1410,12 @@ static int find_extent_clone(struct send_ctx *sctx,
 		extent_item_pos = 0;
 	ret = iterate_extent_inodes(fs_info, found_key.objectid,
 				    extent_item_pos, 1, __iterate_backrefs,
-				    backref_ctx, false);
+				    &backref_ctx, false);
 
 	if (ret < 0)
 		goto out;
 
-	if (!backref_ctx->found_itself) {
+	if (!backref_ctx.found_itself) {
 		/* found a bug in backref code? */
 		ret = -EIO;
 		btrfs_err(fs_info,
@@ -1434,7 +1428,7 @@ static int find_extent_clone(struct send_ctx *sctx,
 		    "find_extent_clone: data_offset=%llu, ino=%llu, num_bytes=%llu, logical=%llu",
 		    data_offset, ino, num_bytes, logical);
 
-	if (!backref_ctx->found)
+	if (!backref_ctx.found)
 		btrfs_debug(fs_info, "no clones found");
 
 	cur_clone_root = NULL;
@@ -1458,7 +1452,6 @@ static int find_extent_clone(struct send_ctx *sctx,
 
 out:
 	btrfs_free_path(tmp_path);
-	kfree(backref_ctx);
 	return ret;
 }
 
-- 
2.32.0

Re: [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args on stack

[Darrick J. Wong]

On Tue, Jul 27, 2021 at 04:17:28PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate btrfs_ioctl_balance_args, allocate
> btrfs_ioctl_balance_args on stack.
> 
> sizeof(btrfs_ioctl_balance_args) = 1024

That's a pretty big addition to the stack frame.  Aren't some of the
kbuild robots configured to whinge about functions that eat more than
1100 bytes or so?

--D

> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/ioctl.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 90b134b5a653..9c3acc539052 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4141,7 +4141,7 @@ static long btrfs_ioctl_balance_ctl(struct btrfs_fs_info *fs_info, int cmd)
>  static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
>  					 void __user *arg)
>  {
> -	struct btrfs_ioctl_balance_args *bargs;
> +	struct btrfs_ioctl_balance_args bargs = {0};
>  	int ret = 0;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -4153,18 +4153,11 @@ static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
>  		goto out;
>  	}
>  
> -	bargs = kzalloc(sizeof(*bargs), GFP_KERNEL);
> -	if (!bargs) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -
> -	btrfs_update_ioctl_balance_args(fs_info, bargs);
> +	btrfs_update_ioctl_balance_args(fs_info, &bargs);
>  
> -	if (copy_to_user(arg, bargs, sizeof(*bargs)))
> +	if (copy_to_user(arg, &bargs, sizeof(bargs)))
>  		ret = -EFAULT;
>  
> -	kfree(bargs);
>  out:
>  	mutex_unlock(&fs_info->balance_mutex);
>  	return ret;
> -- 
> 2.32.0
> 

Re: [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args on stack

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3609 bytes --]

Hi Goldwyn,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.14-rc3 next-20210727]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Goldwyn-Rodrigues/btrfs-Allocate-walk_control-on-stack/20210728-061756
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d5153c5e6009b09ae3916c2d693a0a609ec75cac
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Goldwyn-Rodrigues/btrfs-Allocate-walk_control-on-stack/20210728-061756
        git checkout d5153c5e6009b09ae3916c2d693a0a609ec75cac
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/btrfs/ioctl.c: In function 'btrfs_ioctl_balance_progress':
>> fs/btrfs/ioctl.c:4177:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
    4177 | }
         | ^


vim +4177 fs/btrfs/ioctl.c

837d5b6e46d1a4 Ilya Dryomov      2012-01-16  4153  
2ff7e61e0d30ff Jeff Mahoney      2016-06-22  4154  static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4155  					 void __user *arg)
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4156  {
d5153c5e6009b0 Goldwyn Rodrigues 2021-07-27  4157  	struct btrfs_ioctl_balance_args bargs = {0};
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4158  	int ret = 0;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4159  
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4160  	if (!capable(CAP_SYS_ADMIN))
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4161  		return -EPERM;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4162  
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4163  	mutex_lock(&fs_info->balance_mutex);
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4164  	if (!fs_info->balance_ctl) {
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4165  		ret = -ENOTCONN;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4166  		goto out;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4167  	}
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4168  
d5153c5e6009b0 Goldwyn Rodrigues 2021-07-27  4169  	btrfs_update_ioctl_balance_args(fs_info, &bargs);
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4170  
d5153c5e6009b0 Goldwyn Rodrigues 2021-07-27  4171  	if (copy_to_user(arg, &bargs, sizeof(bargs)))
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4172  		ret = -EFAULT;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4173  
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4174  out:
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4175  	mutex_unlock(&fs_info->balance_mutex);
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4176  	return ret;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16 @4177  }
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4178  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 68196 bytes --]

Re: [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args on stack

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3678 bytes --]

Hi Goldwyn,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.14-rc3 next-20210727]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Goldwyn-Rodrigues/btrfs-Allocate-walk_control-on-stack/20210728-061756
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d5153c5e6009b09ae3916c2d693a0a609ec75cac
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Goldwyn-Rodrigues/btrfs-Allocate-walk_control-on-stack/20210728-061756
        git checkout d5153c5e6009b09ae3916c2d693a0a609ec75cac
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/btrfs/ioctl.c: In function 'btrfs_ioctl_balance_progress':
>> fs/btrfs/ioctl.c:4177:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
    4177 | }
         | ^


vim +4177 fs/btrfs/ioctl.c

837d5b6e46d1a4 Ilya Dryomov      2012-01-16  4153  
2ff7e61e0d30ff Jeff Mahoney      2016-06-22  4154  static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4155  					 void __user *arg)
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4156  {
d5153c5e6009b0 Goldwyn Rodrigues 2021-07-27  4157  	struct btrfs_ioctl_balance_args bargs = {0};
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4158  	int ret = 0;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4159  
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4160  	if (!capable(CAP_SYS_ADMIN))
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4161  		return -EPERM;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4162  
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4163  	mutex_lock(&fs_info->balance_mutex);
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4164  	if (!fs_info->balance_ctl) {
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4165  		ret = -ENOTCONN;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4166  		goto out;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4167  	}
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4168  
d5153c5e6009b0 Goldwyn Rodrigues 2021-07-27  4169  	btrfs_update_ioctl_balance_args(fs_info, &bargs);
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4170  
d5153c5e6009b0 Goldwyn Rodrigues 2021-07-27  4171  	if (copy_to_user(arg, &bargs, sizeof(bargs)))
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4172  		ret = -EFAULT;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4173  
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4174  out:
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4175  	mutex_unlock(&fs_info->balance_mutex);
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4176  	return ret;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16 @4177  }
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4178  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 68196 bytes --]

Re: [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args on stack

[Goldwyn Rodrigues]

On 17:02 27/07, Darrick J. Wong wrote:
> On Tue, Jul 27, 2021 at 04:17:28PM -0500, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > Instead of using kmalloc() to allocate btrfs_ioctl_balance_args, allocate
> > btrfs_ioctl_balance_args on stack.
> > 
> > sizeof(btrfs_ioctl_balance_args) = 1024
> 
> That's a pretty big addition to the stack frame.  Aren't some of the
> kbuild robots configured to whinge about functions that eat more than
> 1100 bytes or so?

Apparently you are faster than the bot to detect this ;)
I got the warning mail from the kbuild bot and the limit is 1024, so it
would not fit in the frame. We can reject this patch.


-- 
Goldwyn

Re: [PATCH 1/7] btrfs: Allocate walk_control on stack

[Anand Jain]

On 28/07/2021 05:17, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate walk_control, allocate
> walk_control on stack.
> 
> No need to memset() a struct to zero if it is initialized to zero.
> 
> sizeof(walk_control) = 200

  IMO it is ok.

> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

  Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

> ---
>   fs/btrfs/extent-tree.c | 89 +++++++++++++++++-------------------------
>   1 file changed, 36 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index fc3da7585fb7..a66cb2e5146f 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -5484,7 +5484,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>   	struct btrfs_trans_handle *trans;
>   	struct btrfs_root *tree_root = fs_info->tree_root;
>   	struct btrfs_root_item *root_item = &root->root_item;
> -	struct walk_control *wc;
> +	struct walk_control wc = {0};
>   	struct btrfs_key key;
>   	int err = 0;
>   	int ret;
> @@ -5499,13 +5499,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>   		goto out;
>   	}
>   
> -	wc = kzalloc(sizeof(*wc), GFP_NOFS);
> -	if (!wc) {
> -		btrfs_free_path(path);
> -		err = -ENOMEM;
> -		goto out;
> -	}
> -
>   	/*
>   	 * Use join to avoid potential EINTR from transaction start. See
>   	 * wait_reserve_ticket and the whole reservation callchain.
> @@ -5537,12 +5530,10 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>   		path->nodes[level] = btrfs_lock_root_node(root);
>   		path->slots[level] = 0;
>   		path->locks[level] = BTRFS_WRITE_LOCK;
> -		memset(&wc->update_progress, 0,
> -		       sizeof(wc->update_progress));
>   	} else {
>   		btrfs_disk_key_to_cpu(&key, &root_item->drop_progress);
> -		memcpy(&wc->update_progress, &key,
> -		       sizeof(wc->update_progress));
> +		memcpy(&wc.update_progress, &key,
> +		       sizeof(wc.update_progress));
>   
>   		level = btrfs_root_drop_level(root_item);
>   		BUG_ON(level == 0);
> @@ -5568,62 +5559,62 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>   
>   			ret = btrfs_lookup_extent_info(trans, fs_info,
>   						path->nodes[level]->start,
> -						level, 1, &wc->refs[level],
> -						&wc->flags[level]);
> +						level, 1, &wc.refs[level],
> +						&wc.flags[level]);
>   			if (ret < 0) {
>   				err = ret;
>   				goto out_end_trans;
>   			}
> -			BUG_ON(wc->refs[level] == 0);
> +			BUG_ON(wc.refs[level] == 0);
>   
>   			if (level == btrfs_root_drop_level(root_item))
>   				break;
>   
>   			btrfs_tree_unlock(path->nodes[level]);
>   			path->locks[level] = 0;
> -			WARN_ON(wc->refs[level] != 1);
> +			WARN_ON(wc.refs[level] != 1);
>   			level--;
>   		}
>   	}
>   
> -	wc->restarted = test_bit(BTRFS_ROOT_DEAD_TREE, &root->state);
> -	wc->level = level;
> -	wc->shared_level = -1;
> -	wc->stage = DROP_REFERENCE;
> -	wc->update_ref = update_ref;
> -	wc->keep_locks = 0;
> -	wc->reada_count = BTRFS_NODEPTRS_PER_BLOCK(fs_info);
> +	wc.restarted = test_bit(BTRFS_ROOT_DEAD_TREE, &root->state);
> +	wc.level = level;
> +	wc.shared_level = -1;
> +	wc.stage = DROP_REFERENCE;
> +	wc.update_ref = update_ref;
> +	wc.keep_locks = 0;
> +	wc.reada_count = BTRFS_NODEPTRS_PER_BLOCK(fs_info);
>   
>   	while (1) {
>   
> -		ret = walk_down_tree(trans, root, path, wc);
> +		ret = walk_down_tree(trans, root, path, &wc);
>   		if (ret < 0) {
>   			err = ret;
>   			break;
>   		}
>   
> -		ret = walk_up_tree(trans, root, path, wc, BTRFS_MAX_LEVEL);
> +		ret = walk_up_tree(trans, root, path, &wc, BTRFS_MAX_LEVEL);
>   		if (ret < 0) {
>   			err = ret;
>   			break;
>   		}
>   
>   		if (ret > 0) {
> -			BUG_ON(wc->stage != DROP_REFERENCE);
> +			BUG_ON(wc.stage != DROP_REFERENCE);
>   			break;
>   		}
>   
> -		if (wc->stage == DROP_REFERENCE) {
> -			wc->drop_level = wc->level;
> -			btrfs_node_key_to_cpu(path->nodes[wc->drop_level],
> -					      &wc->drop_progress,
> -					      path->slots[wc->drop_level]);
> +		if (wc.stage == DROP_REFERENCE) {
> +			wc.drop_level = wc.level;
> +			btrfs_node_key_to_cpu(path->nodes[wc.drop_level],
> +					      &wc.drop_progress,
> +					      path->slots[wc.drop_level]);
>   		}
>   		btrfs_cpu_key_to_disk(&root_item->drop_progress,
> -				      &wc->drop_progress);
> -		btrfs_set_root_drop_level(root_item, wc->drop_level);
> +				      &wc.drop_progress);
> +		btrfs_set_root_drop_level(root_item, wc.drop_level);
>   
> -		BUG_ON(wc->level == 0);
> +		BUG_ON(wc.level == 0);
>   		if (btrfs_should_end_transaction(trans) ||
>   		    (!for_reloc && btrfs_need_cleaner_sleep(fs_info))) {
>   			ret = btrfs_update_root(trans, tree_root,
> @@ -5703,7 +5694,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>   out_end_trans:
>   	btrfs_end_transaction_throttle(trans);
>   out_free:
> -	kfree(wc);
>   	btrfs_free_path(path);
>   out:
>   	/*
> @@ -5731,7 +5721,7 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
>   {
>   	struct btrfs_fs_info *fs_info = root->fs_info;
>   	struct btrfs_path *path;
> -	struct walk_control *wc;
> +	struct walk_control wc = {0};
>   	int level;
>   	int parent_level;
>   	int ret = 0;
> @@ -5743,12 +5733,6 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
>   	if (!path)
>   		return -ENOMEM;
>   
> -	wc = kzalloc(sizeof(*wc), GFP_NOFS);
> -	if (!wc) {
> -		btrfs_free_path(path);
> -		return -ENOMEM;
> -	}
> -
>   	btrfs_assert_tree_locked(parent);
>   	parent_level = btrfs_header_level(parent);
>   	atomic_inc(&parent->refs);
> @@ -5761,30 +5745,29 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
>   	path->slots[level] = 0;
>   	path->locks[level] = BTRFS_WRITE_LOCK;
>   
> -	wc->refs[parent_level] = 1;
> -	wc->flags[parent_level] = BTRFS_BLOCK_FLAG_FULL_BACKREF;
> -	wc->level = level;
> -	wc->shared_level = -1;
> -	wc->stage = DROP_REFERENCE;
> -	wc->update_ref = 0;
> -	wc->keep_locks = 1;
> -	wc->reada_count = BTRFS_NODEPTRS_PER_BLOCK(fs_info);
> +	wc.refs[parent_level] = 1;
> +	wc.flags[parent_level] = BTRFS_BLOCK_FLAG_FULL_BACKREF;
> +	wc.level = level;
> +	wc.shared_level = -1;
> +	wc.stage = DROP_REFERENCE;
> +	wc.update_ref = 0;
> +	wc.keep_locks = 1;
> +	wc.reada_count = BTRFS_NODEPTRS_PER_BLOCK(fs_info);
>   
>   	while (1) {
> -		wret = walk_down_tree(trans, root, path, wc);
> +		wret = walk_down_tree(trans, root, path, &wc);
>   		if (wret < 0) {
>   			ret = wret;
>   			break;
>   		}
>   
> -		wret = walk_up_tree(trans, root, path, wc, parent_level);
> +		wret = walk_up_tree(trans, root, path, &wc, parent_level);
>   		if (wret < 0)
>   			ret = wret;
>   		if (wret != 0)
>   			break;
>   	}
>   
> -	kfree(wc);
>   	btrfs_free_path(path);
>   	return ret;
>   }
> 

Re: [PATCH 1/7] btrfs: Allocate walk_control on stack

[Anand Jain]

Nit:

>> @@ -5537,12 +5530,10 @@ int btrfs_drop_snapshot(struct btrfs_root 
>> *root, int update_ref, int for_reloc)
>>           path->nodes[level] = btrfs_lock_root_node(root);
>>           path->slots[level] = 0;
>>           path->locks[level] = BTRFS_WRITE_LOCK;
>> -        memset(&wc->update_progress, 0,
>> -               sizeof(wc->update_progress));
>>       } else {
>>           btrfs_disk_key_to_cpu(&key, &root_item->drop_progress);
>> -        memcpy(&wc->update_progress, &key,
>> -               sizeof(wc->update_progress));


>> +        memcpy(&wc.update_progress, &key,
>> +               sizeof(wc.update_progress));


  Now, this can fit in one line.
  No need to resend. David may fix it.

Re: [PATCH 2/7] btrfs: Allocate file_ra_state on stack

[Anand Jain]

On 28/07/2021 05:17, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of allocating file_ra_state using kmalloc, allocate on stack.
> sizeof(struct readahead) = 32 bytes
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Looks good.
  Reviewed-by: Anand Jain <anand.jain@oracle.com>


> ---
>   fs/btrfs/free-space-cache.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> index 2131ae5b9ed7..8eeb65278ac0 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -344,19 +344,13 @@ int btrfs_truncate_free_space_cache(struct btrfs_trans_handle *trans,
>   
>   static void readahead_cache(struct inode *inode)
>   {
> -	struct file_ra_state *ra;
> +	struct file_ra_state ra;
>   	unsigned long last_index;
>   
> -	ra = kzalloc(sizeof(*ra), GFP_NOFS);
> -	if (!ra)
> -		return;
> -
> -	file_ra_state_init(ra, inode->i_mapping);
> +	file_ra_state_init(&ra, inode->i_mapping);
>   	last_index = (i_size_read(inode) - 1) >> PAGE_SHIFT;
>   
> -	page_cache_sync_readahead(inode->i_mapping, ra, NULL, 0, last_index);
> -
> -	kfree(ra);
> +	page_cache_sync_readahead(inode->i_mapping, &ra, NULL, 0, last_index);
>   }
>   
>   static int io_ctl_init(struct btrfs_io_ctl *io_ctl, struct inode *inode,
> 

Re: [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack

[Anand Jain]

On 28/07/2021 05:17, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate btrfs_ioctl_get_subvol_info_args,
> allocate btrfs_ioctl_get_subvol_info_args on stack.
> 
> sizeof(btrfs_ioctl_get_subvol_info_args) = 504
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>


Looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>


> ---
>   fs/btrfs/ioctl.c | 55 +++++++++++++++++++++---------------------------
>   1 file changed, 24 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 0ba98e08a029..90b134b5a653 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2681,7 +2681,7 @@ static int btrfs_ioctl_ino_lookup_user(struct file *file, void __user *argp)
>   /* Get the subvolume information in BTRFS_ROOT_ITEM and BTRFS_ROOT_BACKREF */
>   static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   {
> -	struct btrfs_ioctl_get_subvol_info_args *subvol_info;
> +	struct btrfs_ioctl_get_subvol_info_args subvol_info = {0};
>   	struct btrfs_fs_info *fs_info;
>   	struct btrfs_root *root;
>   	struct btrfs_path *path;
> @@ -2699,12 +2699,6 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   	if (!path)
>   		return -ENOMEM;
>   
> -	subvol_info = kzalloc(sizeof(*subvol_info), GFP_KERNEL);
> -	if (!subvol_info) {
> -		btrfs_free_path(path);
> -		return -ENOMEM;
> -	}
> -
>   	inode = file_inode(file);
>   	fs_info = BTRFS_I(inode)->root->fs_info;
>   
> @@ -2717,32 +2711,32 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   	}
>   	root_item = &root->root_item;
>   
> -	subvol_info->treeid = key.objectid;
> +	subvol_info.treeid = key.objectid;
>   
> -	subvol_info->generation = btrfs_root_generation(root_item);
> -	subvol_info->flags = btrfs_root_flags(root_item);
> +	subvol_info.generation = btrfs_root_generation(root_item);
> +	subvol_info.flags = btrfs_root_flags(root_item);
>   
> -	memcpy(subvol_info->uuid, root_item->uuid, BTRFS_UUID_SIZE);
> -	memcpy(subvol_info->parent_uuid, root_item->parent_uuid,
> +	memcpy(subvol_info.uuid, root_item->uuid, BTRFS_UUID_SIZE);
> +	memcpy(subvol_info.parent_uuid, root_item->parent_uuid,
>   						    BTRFS_UUID_SIZE);
> -	memcpy(subvol_info->received_uuid, root_item->received_uuid,
> +	memcpy(subvol_info.received_uuid, root_item->received_uuid,
>   						    BTRFS_UUID_SIZE);
>   
> -	subvol_info->ctransid = btrfs_root_ctransid(root_item);
> -	subvol_info->ctime.sec = btrfs_stack_timespec_sec(&root_item->ctime);
> -	subvol_info->ctime.nsec = btrfs_stack_timespec_nsec(&root_item->ctime);
> +	subvol_info.ctransid = btrfs_root_ctransid(root_item);
> +	subvol_info.ctime.sec = btrfs_stack_timespec_sec(&root_item->ctime);
> +	subvol_info.ctime.nsec = btrfs_stack_timespec_nsec(&root_item->ctime);
>   
> -	subvol_info->otransid = btrfs_root_otransid(root_item);
> -	subvol_info->otime.sec = btrfs_stack_timespec_sec(&root_item->otime);
> -	subvol_info->otime.nsec = btrfs_stack_timespec_nsec(&root_item->otime);
> +	subvol_info.otransid = btrfs_root_otransid(root_item);
> +	subvol_info.otime.sec = btrfs_stack_timespec_sec(&root_item->otime);
> +	subvol_info.otime.nsec = btrfs_stack_timespec_nsec(&root_item->otime);
>   
> -	subvol_info->stransid = btrfs_root_stransid(root_item);
> -	subvol_info->stime.sec = btrfs_stack_timespec_sec(&root_item->stime);
> -	subvol_info->stime.nsec = btrfs_stack_timespec_nsec(&root_item->stime);
> +	subvol_info.stransid = btrfs_root_stransid(root_item);
> +	subvol_info.stime.sec = btrfs_stack_timespec_sec(&root_item->stime);
> +	subvol_info.stime.nsec = btrfs_stack_timespec_nsec(&root_item->stime);
>   
> -	subvol_info->rtransid = btrfs_root_rtransid(root_item);
> -	subvol_info->rtime.sec = btrfs_stack_timespec_sec(&root_item->rtime);
> -	subvol_info->rtime.nsec = btrfs_stack_timespec_nsec(&root_item->rtime);
> +	subvol_info.rtransid = btrfs_root_rtransid(root_item);
> +	subvol_info.rtime.sec = btrfs_stack_timespec_sec(&root_item->rtime);
> +	subvol_info.rtime.nsec = btrfs_stack_timespec_nsec(&root_item->rtime);
>   
>   	if (key.objectid != BTRFS_FS_TREE_OBJECTID) {
>   		/* Search root tree for ROOT_BACKREF of this subvolume */
> @@ -2765,18 +2759,18 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   		leaf = path->nodes[0];
>   		slot = path->slots[0];
>   		btrfs_item_key_to_cpu(leaf, &key, slot);
> -		if (key.objectid == subvol_info->treeid &&
> +		if (key.objectid == subvol_info.treeid &&
>   		    key.type == BTRFS_ROOT_BACKREF_KEY) {
> -			subvol_info->parent_id = key.offset;
> +			subvol_info.parent_id = key.offset;
>   
>   			rref = btrfs_item_ptr(leaf, slot, struct btrfs_root_ref);
> -			subvol_info->dirid = btrfs_root_ref_dirid(leaf, rref);
> +			subvol_info.dirid = btrfs_root_ref_dirid(leaf, rref);
>   
>   			item_off = btrfs_item_ptr_offset(leaf, slot)
>   					+ sizeof(struct btrfs_root_ref);
>   			item_len = btrfs_item_size_nr(leaf, slot)
>   					- sizeof(struct btrfs_root_ref);
> -			read_extent_buffer(leaf, subvol_info->name,
> +			read_extent_buffer(leaf, subvol_info.name,
>   					   item_off, item_len);
>   		} else {
>   			ret = -ENOENT;
> @@ -2784,14 +2778,13 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   		}
>   	}
>   
> -	if (copy_to_user(argp, subvol_info, sizeof(*subvol_info)))
> +	if (copy_to_user(argp, &subvol_info, sizeof(subvol_info)))
>   		ret = -EFAULT;
>   
>   out:
>   	btrfs_put_root(root);
>   out_free:
>   	btrfs_free_path(path);
> -	kfree(subvol_info);
>   	return ret;
>   }
>   
> 

Re: [PATCH 5/7] btrfs: Allocate btrfs_ioctl_quota_rescan_args on stack

[Anand Jain]

On 28/07/2021 05:17, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate btrfs_ioctl_quota_rescan_args,
> allocate btrfs_ioctl_quota_rescan_args on stack.
> 
> sizeof(btrfs_ioctl_quota_rescan_args) = 64
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

> ---
>   fs/btrfs/ioctl.c | 13 ++++---------
>   1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 9c3acc539052..291c16d8576b 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4390,25 +4390,20 @@ static long btrfs_ioctl_quota_rescan(struct file *file, void __user *arg)
>   static long btrfs_ioctl_quota_rescan_status(struct btrfs_fs_info *fs_info,
>   						void __user *arg)
>   {
> -	struct btrfs_ioctl_quota_rescan_args *qsa;
> +	struct btrfs_ioctl_quota_rescan_args qsa = {0};
>   	int ret = 0;
>   
>   	if (!capable(CAP_SYS_ADMIN))
>   		return -EPERM;
>   
> -	qsa = kzalloc(sizeof(*qsa), GFP_KERNEL);
> -	if (!qsa)
> -		return -ENOMEM;
> -
>   	if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN) {
> -		qsa->flags = 1;
> -		qsa->progress = fs_info->qgroup_rescan_progress.objectid;
> +		qsa.flags = 1;
> +		qsa.progress = fs_info->qgroup_rescan_progress.objectid;
>   	}
>   
> -	if (copy_to_user(arg, qsa, sizeof(*qsa)))
> +	if (copy_to_user(arg, &qsa, sizeof(qsa)))
>   		ret = -EFAULT;
>   
> -	kfree(qsa);
>   	return ret;
>   }
>   
> 

Re: [PATCH 6/7] btrfs: Allocate btrfs_ioctl_defrag_range_args on stack

[Anand Jain]

On 28/07/2021 05:17, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate btrfs_ioctl_defrag_range_args,
> allocate btrfs_ioctl_defrag_range_args on stack.
> 
> sizeof(btrfs_ioctl_defrag_range_args) = 48
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Looks good. A nit is below.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

> ---
>   fs/btrfs/ioctl.c | 24 ++++++++----------------
>   1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 291c16d8576b..bc38a1af45c7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3096,7 +3096,7 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
>   {
>   	struct inode *inode = file_inode(file);
>   	struct btrfs_root *root = BTRFS_I(inode)->root;
> -	struct btrfs_ioctl_defrag_range_args *range;
> +	struct btrfs_ioctl_defrag_range_args range = {0};
>   	int ret;
>   
>   	ret = mnt_want_write_file(file);
> @@ -3128,33 +3128,25 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
>   			goto out;
>   		}
>   
> -		range = kzalloc(sizeof(*range), GFP_KERNEL);
> -		if (!range) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> -
>   		if (argp) {
> -			if (copy_from_user(range, argp,
> -					   sizeof(*range))) {
> +			if (copy_from_user(&range, argp,
> +					   sizeof(range))) {

Nit.
This fits in a line.


>   				ret = -EFAULT;
> -				kfree(range);
>   				goto out;
>   			}
>   			/* compression requires us to start the IO */
> -			if ((range->flags & BTRFS_DEFRAG_RANGE_COMPRESS)) {
> -				range->flags |= BTRFS_DEFRAG_RANGE_START_IO;
> -				range->extent_thresh = (u32)-1;
> +			if ((range.flags & BTRFS_DEFRAG_RANGE_COMPRESS)) {
> +				range.flags |= BTRFS_DEFRAG_RANGE_START_IO;
> +				range.extent_thresh = (u32)-1;
>   			}
>   		} else {
>   			/* the rest are all set to zero by kzalloc */
> -			range->len = (u64)-1;
> +			range.len = (u64)-1;
>   		}
>   		ret = btrfs_defrag_file(file_inode(file), file,
> -					range, BTRFS_OLDEST_GENERATION, 0);
> +					&range, BTRFS_OLDEST_GENERATION, 0);
>   		if (ret > 0)
>   			ret = 0;
> -		kfree(range);
>   		break;
>   	default:
>   		ret = -EINVAL;
> 

Re: [PATCH 7/7] btrfs: Alloc backref_ctx on stack

[Anand Jain]

On 28/07/2021 05:17, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate backref_ctx, allocate backref_ctx
> on stack.
> 
> sizeof(backref_ctx) = 48
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Looks good.
  Reviewed-by: Anand Jain <anand.jain@oracle.com>

> ---
>   fs/btrfs/send.c | 29 +++++++++++------------------
>   1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 6ac37ae6c811..e0553fa27f85 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -1307,7 +1307,7 @@ static int find_extent_clone(struct send_ctx *sctx,
>   	u64 flags = 0;
>   	struct btrfs_file_extent_item *fi;
>   	struct extent_buffer *eb = path->nodes[0];
> -	struct backref_ctx *backref_ctx = NULL;
> +	struct backref_ctx backref_ctx = {0};
>   	struct clone_root *cur_clone_root;
>   	struct btrfs_key found_key;
>   	struct btrfs_path *tmp_path;
> @@ -1322,12 +1322,6 @@ static int find_extent_clone(struct send_ctx *sctx,
>   	/* We only use this path under the commit sem */
>   	tmp_path->need_commit_sem = 0;
>   
> -	backref_ctx = kmalloc(sizeof(*backref_ctx), GFP_KERNEL);
> -	if (!backref_ctx) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -
>   	if (data_offset >= ino_size) {
>   		/*
>   		 * There may be extents that lie behind the file's size.
> @@ -1392,12 +1386,12 @@ static int find_extent_clone(struct send_ctx *sctx,
>   		cur_clone_root->found_refs = 0;
>   	}
>   
> -	backref_ctx->sctx = sctx;
> -	backref_ctx->found = 0;
> -	backref_ctx->cur_objectid = ino;
> -	backref_ctx->cur_offset = data_offset;
> -	backref_ctx->found_itself = 0;
> -	backref_ctx->extent_len = num_bytes;
> +	backref_ctx.sctx = sctx;
> +	backref_ctx.found = 0;
> +	backref_ctx.cur_objectid = ino;
> +	backref_ctx.cur_offset = data_offset;
> +	backref_ctx.found_itself = 0;
> +	backref_ctx.extent_len = num_bytes;
>   
>   	/*
>   	 * The last extent of a file may be too large due to page alignment.
> @@ -1405,7 +1399,7 @@ static int find_extent_clone(struct send_ctx *sctx,
>   	 * __iterate_backrefs work.
>   	 */
>   	if (data_offset + num_bytes >= ino_size)
> -		backref_ctx->extent_len = ino_size - data_offset;
> +		backref_ctx.extent_len = ino_size - data_offset;
>   
>   	/*
>   	 * Now collect all backrefs.
> @@ -1416,12 +1410,12 @@ static int find_extent_clone(struct send_ctx *sctx,
>   		extent_item_pos = 0;
>   	ret = iterate_extent_inodes(fs_info, found_key.objectid,
>   				    extent_item_pos, 1, __iterate_backrefs,
> -				    backref_ctx, false);
> +				    &backref_ctx, false);
>   
>   	if (ret < 0)
>   		goto out;
>   
> -	if (!backref_ctx->found_itself) {
> +	if (!backref_ctx.found_itself) {
>   		/* found a bug in backref code? */
>   		ret = -EIO;
>   		btrfs_err(fs_info,
> @@ -1434,7 +1428,7 @@ static int find_extent_clone(struct send_ctx *sctx,
>   		    "find_extent_clone: data_offset=%llu, ino=%llu, num_bytes=%llu, logical=%llu",
>   		    data_offset, ino, num_bytes, logical);
>   
> -	if (!backref_ctx->found)
> +	if (!backref_ctx.found)
>   		btrfs_debug(fs_info, "no clones found");
>   
>   	cur_clone_root = NULL;
> @@ -1458,7 +1452,6 @@ static int find_extent_clone(struct send_ctx *sctx,
>   
>   out:
>   	btrfs_free_path(tmp_path);
> -	kfree(backref_ctx);
>   	return ret;
>   }
>   
> 

Re: [PATCH 1/7] btrfs: Allocate walk_control on stack

[David Sterba]

On Tue, Jul 27, 2021 at 04:17:25PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate walk_control, allocate
> walk_control on stack.
> 
> No need to memset() a struct to zero if it is initialized to zero.
> 
> sizeof(walk_control) = 200

This is IMHO too much for on-stack variable, the function can be called
from nested contexts. Removing snapshots is a restartable operation so
this is not a critical operation where we'd consider reducing the
potential errors.

[PATCH 0/7] Change allocation from kmalloc() to stack

[Goldwyn Rodrigues]

Apparently, git send-email --compose missed to send PATCH 0/7. So
sending again and hoping it lands in the right place...

Here are some structs which can be converted to allocation on stack in order
to save on post-checks on kmalloc() and kfree() each of them.

Sizes of the structures are also in the commit message in case you feel they
are too bit to be allocated on stack.

There are two more structs in ioctl.c which can be converted, but
I was not sure of them:

1. In create_snapshot(), pending_snapshot can be converted. pending_snapshot
   is used in the transaction.
2. In btrfs_ioctl_set_received_subvol_32, args64 can be converted, but args32
   cannot. All Pointers associated with memdup_user() can also be converted
   by using copy_from_user() instead. This would include many more structs.

Goldwyn Rodrigues (7):
  btrfs: Allocate walk_control on stack
  btrfs: Allocate file_ra_state on stack
  btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack
  btrfs: Allocate btrfs_ioctl_balance_args on stack
  btrfs: Allocate btrfs_ioctl_quota_rescan_args on stack
  btrfs: Allocate btrfs_ioctl_defrag_range_args on stack
  btrfs: Alloc backref_ctx on stack

 fs/btrfs/extent-tree.c      |  89 +++++++++++++-----------------
 fs/btrfs/free-space-cache.c |  12 ++---
 fs/btrfs/ioctl.c            | 105 ++++++++++++++----------------------
 fs/btrfs/send.c             |  29 ++++------
 4 files changed, 89 insertions(+), 146 deletions(-)

-- 
Goldwyn

Re: [PATCH 0/7] Allocate structures on stack instead of kmalloc()

[David Sterba]

On Tue, Jul 27, 2021 at 04:17:24PM -0500, Goldwyn Rodrigues wrote:
> Here are some structs which can be converted to allocation on stack in order
> to save on post-checks on kmalloc() and kfree() each of them.
> 
> Sizes of the structures are also in the commit message in case you feel they
> are too bit to be allocated on stack.

Reducing the potential error failures is good and may slightly increase
performance in case the system is low on memory and allocator would have
to do some work.

We must also try to reduce stack usage, but for the ioctls it should be
safe as it's run from the process context and not from a deep call
chain. Where it could be a problem, if the call chaing becomes deep
under btrfs, ie. in any other intermediate layer like NFS, DM, block
layer and drivers.

So, I'd limit the size of on-stack variables to something like 128, that
accounts for a few nested function calls with a few varaibles (eg. 4
functions with 4 pointers each).. This is a normal pattern anywhere
else.

> There are two more structs in ioctl.c which can be converted, but
> I was not sure of them:
> 
> 1. In create_snapshot(), pending_snapshot can be converted. pending_snapshot
>    is used in the transaction.

That one is 144 bytes, arguably still ok.

> 2. In btrfs_ioctl_set_received_subvol_32, args64 can be converted, but args32
>    cannot. All Pointers associated with memdup_user() can also be converted
>    by using copy_from_user() instead. This would include many more structs.

size of btrfs_ioctl_received_subvol_args_32 is 192,
and btrfs_ioctl_received_subvol_args is 200 bytes, both in the same
function.

I'd leave this one as is, it's not something critical where performance
matters.

So, overall I'll apply the series without the two commented.

Re: [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack

[David Sterba]

On Tue, Jul 27, 2021 at 04:17:27PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate btrfs_ioctl_get_subvol_info_args,
> allocate btrfs_ioctl_get_subvol_info_args on stack.
> 
> sizeof(btrfs_ioctl_get_subvol_info_args) = 504
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/ioctl.c | 55 +++++++++++++++++++++---------------------------
>  1 file changed, 24 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 0ba98e08a029..90b134b5a653 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2681,7 +2681,7 @@ static int btrfs_ioctl_ino_lookup_user(struct file *file, void __user *argp)
>  /* Get the subvolume information in BTRFS_ROOT_ITEM and BTRFS_ROOT_BACKREF */
>  static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>  {
> -	struct btrfs_ioctl_get_subvol_info_args *subvol_info;
> +	struct btrfs_ioctl_get_subvol_info_args subvol_info = {0};

There are concerns [1] if the {0} initialization zeroes all the bytes
entirely, including padding between members and at the end, but as I
understand it, this should be safe.

[1] long thread, https://lore.kernel.org/lkml/20210727205855.411487-20-keescook@chromium.org/T/#m7e4e04af9664f204232a569390c3f3dc2e4bf907

If it turns out {0} is not sufficient, we'll have to add explicit
memset() in case the on-stack structure is then copied back with
copy_to_user.

Re: [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack

[David Sterba]

On Tue, Jul 27, 2021 at 04:17:27PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate btrfs_ioctl_get_subvol_info_args,
> allocate btrfs_ioctl_get_subvol_info_args on stack.
> 
> sizeof(btrfs_ioctl_get_subvol_info_args) = 504

I'm not sure about this one, it's a lot and it's not a perf critical
ioctl. Reading information about a subvolume can potentially trigger a
lot of reads in a cold state, thus triggering all the deep call chains.