* [PATCH 1/7] btrfs: Allocate walk_control on stack
2021-07-28 12:51 [PATCH 0/7] Change allocation from kmalloc() to stack Goldwyn Rodrigues
@ 2021-07-27 21:17 ` Goldwyn Rodrigues
2021-07-28 5:11 ` Anand Jain
2021-07-28 11:08 ` David Sterba
2021-07-27 21:17 ` [PATCH 2/7] btrfs: Allocate file_ra_state " Goldwyn Rodrigues
` (5 subsequent siblings)
6 siblings, 2 replies; 24+ messages in thread
From: Goldwyn Rodrigues @ 2021-07-27 21:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/7] btrfs: Allocate walk_control on stack
2021-07-27 21:17 ` [PATCH 1/7] btrfs: Allocate walk_control on stack Goldwyn Rodrigues
@ 2021-07-28 5:11 ` Anand Jain
2021-07-28 5:25 ` Anand Jain
2021-07-28 11:08 ` David Sterba
1 sibling, 1 reply; 24+ messages in thread
From: Anand Jain @ 2021-07-28 5:11 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs; +Cc: Goldwyn Rodrigues
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;
> }
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/7] btrfs: Allocate walk_control on stack
2021-07-28 5:11 ` Anand Jain
@ 2021-07-28 5:25 ` Anand Jain
0 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2021-07-28 5:25 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs; +Cc: Goldwyn Rodrigues
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/7] btrfs: Allocate walk_control on stack
2021-07-27 21:17 ` [PATCH 1/7] btrfs: Allocate walk_control on stack Goldwyn Rodrigues
2021-07-28 5:11 ` Anand Jain
@ 2021-07-28 11:08 ` David Sterba
1 sibling, 0 replies; 24+ messages in thread
From: David Sterba @ 2021-07-28 11:08 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/7] btrfs: Allocate file_ra_state on stack
2021-07-28 12:51 [PATCH 0/7] Change allocation from kmalloc() to stack Goldwyn Rodrigues
2021-07-27 21:17 ` [PATCH 1/7] btrfs: Allocate walk_control on stack Goldwyn Rodrigues
@ 2021-07-27 21:17 ` Goldwyn Rodrigues
2021-07-28 5:29 ` Anand Jain
2021-07-27 21:17 ` [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args " Goldwyn Rodrigues
` (4 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Goldwyn Rodrigues @ 2021-07-27 21:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/7] btrfs: Allocate file_ra_state on stack
2021-07-27 21:17 ` [PATCH 2/7] btrfs: Allocate file_ra_state " Goldwyn Rodrigues
@ 2021-07-28 5:29 ` Anand Jain
0 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2021-07-28 5:29 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs; +Cc: Goldwyn Rodrigues
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,
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack
2021-07-28 12:51 [PATCH 0/7] Change allocation from kmalloc() to stack Goldwyn Rodrigues
2021-07-27 21:17 ` [PATCH 1/7] btrfs: Allocate walk_control on stack Goldwyn Rodrigues
2021-07-27 21:17 ` [PATCH 2/7] btrfs: Allocate file_ra_state " Goldwyn Rodrigues
@ 2021-07-27 21:17 ` Goldwyn Rodrigues
2021-07-28 5:59 ` Anand Jain
` (2 more replies)
2021-07-27 21:17 ` [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args " Goldwyn Rodrigues
` (3 subsequent siblings)
6 siblings, 3 replies; 24+ messages in thread
From: Goldwyn Rodrigues @ 2021-07-27 21:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack
2021-07-27 21:17 ` [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args " Goldwyn Rodrigues
@ 2021-07-28 5:59 ` Anand Jain
2021-07-29 17:08 ` David Sterba
2021-07-29 17:22 ` David Sterba
2 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2021-07-28 5:59 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs; +Cc: Goldwyn Rodrigues
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;
> }
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack
2021-07-27 21:17 ` [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args " Goldwyn Rodrigues
2021-07-28 5:59 ` Anand Jain
@ 2021-07-29 17:08 ` David Sterba
2021-07-29 17:22 ` David Sterba
2 siblings, 0 replies; 24+ messages in thread
From: David Sterba @ 2021-07-29 17:08 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack
2021-07-27 21:17 ` [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args " Goldwyn Rodrigues
2021-07-28 5:59 ` Anand Jain
2021-07-29 17:08 ` David Sterba
@ 2021-07-29 17:22 ` David Sterba
2 siblings, 0 replies; 24+ messages in thread
From: David Sterba @ 2021-07-29 17:22 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args on stack
2021-07-28 12:51 [PATCH 0/7] Change allocation from kmalloc() to stack Goldwyn Rodrigues
` (2 preceding siblings ...)
2021-07-27 21:17 ` [PATCH 3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args " Goldwyn Rodrigues
@ 2021-07-27 21:17 ` Goldwyn Rodrigues
2021-07-28 0:02 ` Darrick J. Wong
2021-07-28 1:19 ` kernel test robot
2021-07-27 21:17 ` [PATCH 5/7] btrfs: Allocate btrfs_ioctl_quota_rescan_args " Goldwyn Rodrigues
` (2 subsequent siblings)
6 siblings, 2 replies; 24+ messages in thread
From: Goldwyn Rodrigues @ 2021-07-27 21:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args on stack
2021-07-27 21:17 ` [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args " Goldwyn Rodrigues
@ 2021-07-28 0:02 ` Darrick J. Wong
2021-07-28 2:04 ` Goldwyn Rodrigues
2021-07-28 1:19 ` kernel test robot
1 sibling, 1 reply; 24+ messages in thread
From: Darrick J. Wong @ 2021-07-28 0:02 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args on stack
2021-07-28 0:02 ` Darrick J. Wong
@ 2021-07-28 2:04 ` Goldwyn Rodrigues
0 siblings, 0 replies; 24+ messages in thread
From: Goldwyn Rodrigues @ 2021-07-28 2:04 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args on stack
2021-07-27 21:17 ` [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args " Goldwyn Rodrigues
@ 2021-07-28 1:19 ` kernel test robot
2021-07-28 1:19 ` kernel test robot
1 sibling, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-07-28 1:19 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs; +Cc: kbuild-all, Goldwyn Rodrigues
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args on stack
@ 2021-07-28 1:19 ` kernel test robot
0 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-07-28 1:19 UTC (permalink / raw)
To: kbuild-all
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 5/7] btrfs: Allocate btrfs_ioctl_quota_rescan_args on stack
2021-07-28 12:51 [PATCH 0/7] Change allocation from kmalloc() to stack Goldwyn Rodrigues
` (3 preceding siblings ...)
2021-07-27 21:17 ` [PATCH 4/7] btrfs: Allocate btrfs_ioctl_balance_args " Goldwyn Rodrigues
@ 2021-07-27 21:17 ` Goldwyn Rodrigues
2021-07-28 6:01 ` Anand Jain
2021-07-27 21:17 ` [PATCH 6/7] btrfs: Allocate btrfs_ioctl_defrag_range_args " Goldwyn Rodrigues
2021-07-27 21:17 ` [PATCH 7/7] btrfs: Alloc backref_ctx " Goldwyn Rodrigues
6 siblings, 1 reply; 24+ messages in thread
From: Goldwyn Rodrigues @ 2021-07-27 21:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 5/7] btrfs: Allocate btrfs_ioctl_quota_rescan_args on stack
2021-07-27 21:17 ` [PATCH 5/7] btrfs: Allocate btrfs_ioctl_quota_rescan_args " Goldwyn Rodrigues
@ 2021-07-28 6:01 ` Anand Jain
0 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2021-07-28 6:01 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs; +Cc: Goldwyn Rodrigues
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;
> }
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 6/7] btrfs: Allocate btrfs_ioctl_defrag_range_args on stack
2021-07-28 12:51 [PATCH 0/7] Change allocation from kmalloc() to stack Goldwyn Rodrigues
` (4 preceding siblings ...)
2021-07-27 21:17 ` [PATCH 5/7] btrfs: Allocate btrfs_ioctl_quota_rescan_args " Goldwyn Rodrigues
@ 2021-07-27 21:17 ` Goldwyn Rodrigues
2021-07-28 6:27 ` Anand Jain
2021-07-27 21:17 ` [PATCH 7/7] btrfs: Alloc backref_ctx " Goldwyn Rodrigues
6 siblings, 1 reply; 24+ messages in thread
From: Goldwyn Rodrigues @ 2021-07-27 21:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 6/7] btrfs: Allocate btrfs_ioctl_defrag_range_args on stack
2021-07-27 21:17 ` [PATCH 6/7] btrfs: Allocate btrfs_ioctl_defrag_range_args " Goldwyn Rodrigues
@ 2021-07-28 6:27 ` Anand Jain
0 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2021-07-28 6:27 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs; +Cc: Goldwyn Rodrigues
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;
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 7/7] btrfs: Alloc backref_ctx on stack
2021-07-28 12:51 [PATCH 0/7] Change allocation from kmalloc() to stack Goldwyn Rodrigues
` (5 preceding siblings ...)
2021-07-27 21:17 ` [PATCH 6/7] btrfs: Allocate btrfs_ioctl_defrag_range_args " Goldwyn Rodrigues
@ 2021-07-27 21:17 ` Goldwyn Rodrigues
2021-07-28 6:30 ` Anand Jain
6 siblings, 1 reply; 24+ messages in thread
From: Goldwyn Rodrigues @ 2021-07-27 21:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 7/7] btrfs: Alloc backref_ctx on stack
2021-07-27 21:17 ` [PATCH 7/7] btrfs: Alloc backref_ctx " Goldwyn Rodrigues
@ 2021-07-28 6:30 ` Anand Jain
0 siblings, 0 replies; 24+ messages in thread
From: Anand Jain @ 2021-07-28 6:30 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs; +Cc: Goldwyn Rodrigues
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;
> }
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread