* [PATCH 0/2] Stack usage reduction
@ 2016-04-11 18:04 David Sterba
2016-04-11 18:04 ` [PATCH 1/2] btrfs: use dynamic allocation for root item in create_subvol David Sterba
2016-04-11 18:04 ` [PATCH 2/2] btrfs: reuse existing variable in scrub_stripe, reduce stack usage David Sterba
0 siblings, 2 replies; 6+ messages in thread
From: David Sterba @ 2016-04-11 18:04 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
Hi,
using the gcc option -fstack-usage I measured the stack usage and tried to get
rid of the worst offenders. The best improvement was in create_subvol where we
stored a 400B+ structure on stack, but otherwise it's not always clear why the
stack usage is that high. Most functions consume less than 300 bytes, and this
number can account for inlined functions or other invisible compiler
optimization magic.
A few samples of what's left:
scrub.c:3056:31:scrub_stripe 568 static
volumes.c:3984:12:btrfs_uuid_scan_kthread 536 static
scrub.c:2834:31:scrub_raid56_parity 384 static
ioctl.c:5225:12:btrfs_ioctl_set_fslabel 304 static
ioctl.c:1261:5:btrfs_defrag_file 304 static
tree-log.c:3593:21:copy_items 288 dynamic,bounded
ioctl.c:5202:12:btrfs_ioctl_get_fslabel 288 static
ioctl.c:3457:12:btrfs_clone 288 static
extent_io.c:2873:12:__do_readpage 288 static
file.c:691:5:__btrfs_drop_extents 272 static
file.c:2646:13:btrfs_fallocate 272 static
extent-tree.c:2469:21:__btrfs_run_delayed_refs 272 static
extent_io.c:3779:5:btree_write_cache_pages 272 static
extent_io.c:1730:6:extent_clear_unlock_delalloc 272 static
tree-log.c:4432:12:btrfs_log_inode 264 dynamic,bounded
tree-log.c:578:21:replay_one_extent 256 static
transaction.c:1322:21:create_pending_snapshot 256 static
ioctl.c:434:21:create_subvol 256 static
inode.c:1221:21:run_delalloc_nocow 256 static
extent_io.c:4145:5:extent_readpages 256 static
tree-log.c:4126:12:btrfs_log_changed_extents 248 dynamic,bounded
relocation.c:680:22:build_backref_tree 248 static
inode.c:4287:5:btrfs_truncate_inode_items 248 static
extent_io.c:3312:31:__extent_writepage_io 248 static
volumes.c:2948:12:insert_balance_item 240 static
relocation.c:1762:5:replace_path 240 static
...
More detailed info would be needed to decide whether it's worth to reshuffle
the stack variables, from what I've seen it would make the code readability
worse, so I've stopped.
----------------------------------------------------------------
The following changes since commit bb7ab3b92e46da06b580c6f83abe7894dc449cca:
btrfs: Fix misspellings in comments. (2016-03-14 15:05:02 +0100)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git dev/stack-reduce
for you to fetch changes up to 355ef15a6eafc0cafd49b049d7173040adb44f61:
btrfs: reuse existing variable in scrub_stripe, reduce stack usage (2016-03-24 18:06:34 +0100)
----------------------------------------------------------------
David Sterba (2):
btrfs: use dynamic allocation for root item in create_subvol
btrfs: reuse existing variable in scrub_stripe, reduce stack usage
fs/btrfs/ioctl.c | 49 ++++++++++++++++++++++++++-----------------------
fs/btrfs/scrub.c | 19 +++++++++----------
2 files changed, 35 insertions(+), 33 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] btrfs: use dynamic allocation for root item in create_subvol
2016-04-11 18:04 [PATCH 0/2] Stack usage reduction David Sterba
@ 2016-04-11 18:04 ` David Sterba
2016-04-12 0:15 ` Tsutomu Itoh
2016-04-25 11:18 ` [PATCH v2] " David Sterba
2016-04-11 18:04 ` [PATCH 2/2] btrfs: reuse existing variable in scrub_stripe, reduce stack usage David Sterba
1 sibling, 2 replies; 6+ messages in thread
From: David Sterba @ 2016-04-11 18:04 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
The size of root item is more than 400 bytes, which is quite a lot of
stack space. As we do IO from inside the subvolume ioctls, we should
keep the stack usage low in case the filesystem is on top of other
layers (NFS, device mapper, iscsi, etc).
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/ioctl.c | 49 ++++++++++++++++++++++++++-----------------------
1 file changed, 26 insertions(+), 23 deletions(-)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 053e677839fe..0be13b9c53d9 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -439,7 +439,7 @@ static noinline int create_subvol(struct inode *dir,
{
struct btrfs_trans_handle *trans;
struct btrfs_key key;
- struct btrfs_root_item root_item;
+ struct btrfs_root_item *root_item;
struct btrfs_inode_item *inode_item;
struct extent_buffer *leaf;
struct btrfs_root *root = BTRFS_I(dir)->root;
@@ -455,6 +455,10 @@ static noinline int create_subvol(struct inode *dir,
u64 qgroup_reserved;
uuid_le new_uuid;
+ root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
+ if (!root_item)
+ return -ENOMEM;
+
ret = btrfs_find_free_objectid(root->fs_info->tree_root, &objectid);
if (ret)
return ret;
@@ -509,47 +513,45 @@ static noinline int create_subvol(struct inode *dir,
BTRFS_UUID_SIZE);
btrfs_mark_buffer_dirty(leaf);
- memset(&root_item, 0, sizeof(root_item));
-
- inode_item = &root_item.inode;
+ inode_item = &root_item->inode;
btrfs_set_stack_inode_generation(inode_item, 1);
btrfs_set_stack_inode_size(inode_item, 3);
btrfs_set_stack_inode_nlink(inode_item, 1);
btrfs_set_stack_inode_nbytes(inode_item, root->nodesize);
btrfs_set_stack_inode_mode(inode_item, S_IFDIR | 0755);
- btrfs_set_root_flags(&root_item, 0);
- btrfs_set_root_limit(&root_item, 0);
+ btrfs_set_root_flags(root_item, 0);
+ btrfs_set_root_limit(root_item, 0);
btrfs_set_stack_inode_flags(inode_item, BTRFS_INODE_ROOT_ITEM_INIT);
- btrfs_set_root_bytenr(&root_item, leaf->start);
- btrfs_set_root_generation(&root_item, trans->transid);
- btrfs_set_root_level(&root_item, 0);
- btrfs_set_root_refs(&root_item, 1);
- btrfs_set_root_used(&root_item, leaf->len);
- btrfs_set_root_last_snapshot(&root_item, 0);
+ btrfs_set_root_bytenr(root_item, leaf->start);
+ btrfs_set_root_generation(root_item, trans->transid);
+ btrfs_set_root_level(root_item, 0);
+ btrfs_set_root_refs(root_item, 1);
+ btrfs_set_root_used(root_item, leaf->len);
+ btrfs_set_root_last_snapshot(root_item, 0);
- btrfs_set_root_generation_v2(&root_item,
- btrfs_root_generation(&root_item));
+ btrfs_set_root_generation_v2(root_item,
+ btrfs_root_generation(root_item));
uuid_le_gen(&new_uuid);
- memcpy(root_item.uuid, new_uuid.b, BTRFS_UUID_SIZE);
- btrfs_set_stack_timespec_sec(&root_item.otime, cur_time.tv_sec);
- btrfs_set_stack_timespec_nsec(&root_item.otime, cur_time.tv_nsec);
- root_item.ctime = root_item.otime;
- btrfs_set_root_ctransid(&root_item, trans->transid);
- btrfs_set_root_otransid(&root_item, trans->transid);
+ memcpy(root_item->uuid, new_uuid.b, BTRFS_UUID_SIZE);
+ btrfs_set_stack_timespec_sec(&root_item->otime, cur_time.tv_sec);
+ btrfs_set_stack_timespec_nsec(&root_item->otime, cur_time.tv_nsec);
+ root_item->ctime = root_item->otime;
+ btrfs_set_root_ctransid(root_item, trans->transid);
+ btrfs_set_root_otransid(root_item, trans->transid);
btrfs_tree_unlock(leaf);
free_extent_buffer(leaf);
leaf = NULL;
- btrfs_set_root_dirid(&root_item, new_dirid);
+ btrfs_set_root_dirid(root_item, new_dirid);
key.objectid = objectid;
key.offset = 0;
key.type = BTRFS_ROOT_ITEM_KEY;
ret = btrfs_insert_root(trans, root->fs_info->tree_root, &key,
- &root_item);
+ root_item);
if (ret)
goto fail;
@@ -601,12 +603,13 @@ static noinline int create_subvol(struct inode *dir,
BUG_ON(ret);
ret = btrfs_uuid_tree_add(trans, root->fs_info->uuid_root,
- root_item.uuid, BTRFS_UUID_KEY_SUBVOL,
+ root_item->uuid, BTRFS_UUID_KEY_SUBVOL,
objectid);
if (ret)
btrfs_abort_transaction(trans, root, ret);
fail:
+ kfree(root_item);
trans->block_rsv = NULL;
trans->bytes_reserved = 0;
btrfs_subvolume_release_metadata(root, &block_rsv, qgroup_reserved);
--
2.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] btrfs: reuse existing variable in scrub_stripe, reduce stack usage
2016-04-11 18:04 [PATCH 0/2] Stack usage reduction David Sterba
2016-04-11 18:04 ` [PATCH 1/2] btrfs: use dynamic allocation for root item in create_subvol David Sterba
@ 2016-04-11 18:04 ` David Sterba
1 sibling, 0 replies; 6+ messages in thread
From: David Sterba @ 2016-04-11 18:04 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
The key variable occupies 17 bytes, the key_start is used once, we can
simply reuse existing 'key' for that purpose. As the key is not a simple
type, compiler doest not do it on itself.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/scrub.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 39dbdcbf4d13..07db452e4a15 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3070,7 +3070,6 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
int slot;
u64 nstripes;
struct extent_buffer *l;
- struct btrfs_key key;
u64 physical;
u64 logical;
u64 logic_end;
@@ -3079,7 +3078,7 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
int mirror_num;
struct reada_control *reada1;
struct reada_control *reada2;
- struct btrfs_key key_start;
+ struct btrfs_key key;
struct btrfs_key key_end;
u64 increment = map->stripe_len;
u64 offset;
@@ -3158,21 +3157,21 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
scrub_blocked_if_needed(fs_info);
/* FIXME it might be better to start readahead at commit root */
- key_start.objectid = logical;
- key_start.type = BTRFS_EXTENT_ITEM_KEY;
- key_start.offset = (u64)0;
+ key.objectid = logical;
+ key.type = BTRFS_EXTENT_ITEM_KEY;
+ key.offset = (u64)0;
key_end.objectid = logic_end;
key_end.type = BTRFS_METADATA_ITEM_KEY;
key_end.offset = (u64)-1;
- reada1 = btrfs_reada_add(root, &key_start, &key_end);
+ reada1 = btrfs_reada_add(root, &key, &key_end);
- key_start.objectid = BTRFS_EXTENT_CSUM_OBJECTID;
- key_start.type = BTRFS_EXTENT_CSUM_KEY;
- key_start.offset = logical;
+ key.objectid = BTRFS_EXTENT_CSUM_OBJECTID;
+ key.type = BTRFS_EXTENT_CSUM_KEY;
+ key.offset = logical;
key_end.objectid = BTRFS_EXTENT_CSUM_OBJECTID;
key_end.type = BTRFS_EXTENT_CSUM_KEY;
key_end.offset = logic_end;
- reada2 = btrfs_reada_add(csum_root, &key_start, &key_end);
+ reada2 = btrfs_reada_add(csum_root, &key, &key_end);
if (!IS_ERR(reada1))
btrfs_reada_wait(reada1);
--
2.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] btrfs: use dynamic allocation for root item in create_subvol
2016-04-11 18:04 ` [PATCH 1/2] btrfs: use dynamic allocation for root item in create_subvol David Sterba
@ 2016-04-12 0:15 ` Tsutomu Itoh
2016-04-25 11:18 ` [PATCH v2] " David Sterba
1 sibling, 0 replies; 6+ messages in thread
From: Tsutomu Itoh @ 2016-04-12 0:15 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
On 2016/04/12 3:04, David Sterba wrote:
> The size of root item is more than 400 bytes, which is quite a lot of
> stack space. As we do IO from inside the subvolume ioctls, we should
> keep the stack usage low in case the filesystem is on top of other
> layers (NFS, device mapper, iscsi, etc).
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/ioctl.c | 49 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 26 insertions(+), 23 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 053e677839fe..0be13b9c53d9 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -439,7 +439,7 @@ static noinline int create_subvol(struct inode *dir,
> {
> struct btrfs_trans_handle *trans;
> struct btrfs_key key;
> - struct btrfs_root_item root_item;
> + struct btrfs_root_item *root_item;
> struct btrfs_inode_item *inode_item;
> struct extent_buffer *leaf;
> struct btrfs_root *root = BTRFS_I(dir)->root;
> @@ -455,6 +455,10 @@ static noinline int create_subvol(struct inode *dir,
> u64 qgroup_reserved;
> uuid_le new_uuid;
>
> + root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
> + if (!root_item)
> + return -ENOMEM;
> +
> ret = btrfs_find_free_objectid(root->fs_info->tree_root, &objectid);
> if (ret)
> return ret;
'kfree(root_item)' is necessary here and other 'return'.
Thanks,
Tsutomu
> @@ -509,47 +513,45 @@ static noinline int create_subvol(struct inode *dir,
> BTRFS_UUID_SIZE);
> btrfs_mark_buffer_dirty(leaf);
>
> - memset(&root_item, 0, sizeof(root_item));
> -
> - inode_item = &root_item.inode;
> + inode_item = &root_item->inode;
> btrfs_set_stack_inode_generation(inode_item, 1);
> btrfs_set_stack_inode_size(inode_item, 3);
> btrfs_set_stack_inode_nlink(inode_item, 1);
> btrfs_set_stack_inode_nbytes(inode_item, root->nodesize);
> btrfs_set_stack_inode_mode(inode_item, S_IFDIR | 0755);
>
> - btrfs_set_root_flags(&root_item, 0);
> - btrfs_set_root_limit(&root_item, 0);
> + btrfs_set_root_flags(root_item, 0);
> + btrfs_set_root_limit(root_item, 0);
> btrfs_set_stack_inode_flags(inode_item, BTRFS_INODE_ROOT_ITEM_INIT);
>
> - btrfs_set_root_bytenr(&root_item, leaf->start);
> - btrfs_set_root_generation(&root_item, trans->transid);
> - btrfs_set_root_level(&root_item, 0);
> - btrfs_set_root_refs(&root_item, 1);
> - btrfs_set_root_used(&root_item, leaf->len);
> - btrfs_set_root_last_snapshot(&root_item, 0);
> + btrfs_set_root_bytenr(root_item, leaf->start);
> + btrfs_set_root_generation(root_item, trans->transid);
> + btrfs_set_root_level(root_item, 0);
> + btrfs_set_root_refs(root_item, 1);
> + btrfs_set_root_used(root_item, leaf->len);
> + btrfs_set_root_last_snapshot(root_item, 0);
>
> - btrfs_set_root_generation_v2(&root_item,
> - btrfs_root_generation(&root_item));
> + btrfs_set_root_generation_v2(root_item,
> + btrfs_root_generation(root_item));
> uuid_le_gen(&new_uuid);
> - memcpy(root_item.uuid, new_uuid.b, BTRFS_UUID_SIZE);
> - btrfs_set_stack_timespec_sec(&root_item.otime, cur_time.tv_sec);
> - btrfs_set_stack_timespec_nsec(&root_item.otime, cur_time.tv_nsec);
> - root_item.ctime = root_item.otime;
> - btrfs_set_root_ctransid(&root_item, trans->transid);
> - btrfs_set_root_otransid(&root_item, trans->transid);
> + memcpy(root_item->uuid, new_uuid.b, BTRFS_UUID_SIZE);
> + btrfs_set_stack_timespec_sec(&root_item->otime, cur_time.tv_sec);
> + btrfs_set_stack_timespec_nsec(&root_item->otime, cur_time.tv_nsec);
> + root_item->ctime = root_item->otime;
> + btrfs_set_root_ctransid(root_item, trans->transid);
> + btrfs_set_root_otransid(root_item, trans->transid);
>
> btrfs_tree_unlock(leaf);
> free_extent_buffer(leaf);
> leaf = NULL;
>
> - btrfs_set_root_dirid(&root_item, new_dirid);
> + btrfs_set_root_dirid(root_item, new_dirid);
>
> key.objectid = objectid;
> key.offset = 0;
> key.type = BTRFS_ROOT_ITEM_KEY;
> ret = btrfs_insert_root(trans, root->fs_info->tree_root, &key,
> - &root_item);
> + root_item);
> if (ret)
> goto fail;
>
> @@ -601,12 +603,13 @@ static noinline int create_subvol(struct inode *dir,
> BUG_ON(ret);
>
> ret = btrfs_uuid_tree_add(trans, root->fs_info->uuid_root,
> - root_item.uuid, BTRFS_UUID_KEY_SUBVOL,
> + root_item->uuid, BTRFS_UUID_KEY_SUBVOL,
> objectid);
> if (ret)
> btrfs_abort_transaction(trans, root, ret);
>
> fail:
> + kfree(root_item);
> trans->block_rsv = NULL;
> trans->bytes_reserved = 0;
> btrfs_subvolume_release_metadata(root, &block_rsv, qgroup_reserved);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] btrfs: use dynamic allocation for root item in create_subvol
2016-04-11 18:04 ` [PATCH 1/2] btrfs: use dynamic allocation for root item in create_subvol David Sterba
2016-04-12 0:15 ` Tsutomu Itoh
@ 2016-04-25 11:18 ` David Sterba
2016-04-26 0:18 ` Tsutomu Itoh
1 sibling, 1 reply; 6+ messages in thread
From: David Sterba @ 2016-04-25 11:18 UTC (permalink / raw)
To: linux-btrfs; +Cc: t-itoh, David Sterba
The size of root item is more than 400 bytes, which is quite a lot of
stack space. As we do IO from inside the subvolume ioctls, we should
keep the stack usage low in case the filesystem is on top of other
layers (NFS, device mapper, iscsi, etc).
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/ioctl.c | 65 ++++++++++++++++++++++++++++++++------------------------
1 file changed, 37 insertions(+), 28 deletions(-)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 053e677839fe..9a63fe07bc2e 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -439,7 +439,7 @@ static noinline int create_subvol(struct inode *dir,
{
struct btrfs_trans_handle *trans;
struct btrfs_key key;
- struct btrfs_root_item root_item;
+ struct btrfs_root_item *root_item;
struct btrfs_inode_item *inode_item;
struct extent_buffer *leaf;
struct btrfs_root *root = BTRFS_I(dir)->root;
@@ -455,16 +455,22 @@ static noinline int create_subvol(struct inode *dir,
u64 qgroup_reserved;
uuid_le new_uuid;
+ root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
+ if (!root_item)
+ return -ENOMEM;
+
ret = btrfs_find_free_objectid(root->fs_info->tree_root, &objectid);
if (ret)
- return ret;
+ goto fail_free;
/*
* Don't create subvolume whose level is not zero. Or qgroup will be
* screwed up since it assume subvolme qgroup's level to be 0.
*/
- if (btrfs_qgroup_level(objectid))
- return -ENOSPC;
+ if (btrfs_qgroup_level(objectid)) {
+ ret = -ENOSPC;
+ goto fail_free;
+ }
btrfs_init_block_rsv(&block_rsv, BTRFS_BLOCK_RSV_TEMP);
/*
@@ -474,14 +480,14 @@ static noinline int create_subvol(struct inode *dir,
ret = btrfs_subvolume_reserve_metadata(root, &block_rsv,
8, &qgroup_reserved, false);
if (ret)
- return ret;
+ goto fail_free;
trans = btrfs_start_transaction(root, 0);
if (IS_ERR(trans)) {
ret = PTR_ERR(trans);
btrfs_subvolume_release_metadata(root, &block_rsv,
qgroup_reserved);
- return ret;
+ goto fail_free;
}
trans->block_rsv = &block_rsv;
trans->bytes_reserved = block_rsv.size;
@@ -509,47 +515,45 @@ static noinline int create_subvol(struct inode *dir,
BTRFS_UUID_SIZE);
btrfs_mark_buffer_dirty(leaf);
- memset(&root_item, 0, sizeof(root_item));
-
- inode_item = &root_item.inode;
+ inode_item = &root_item->inode;
btrfs_set_stack_inode_generation(inode_item, 1);
btrfs_set_stack_inode_size(inode_item, 3);
btrfs_set_stack_inode_nlink(inode_item, 1);
btrfs_set_stack_inode_nbytes(inode_item, root->nodesize);
btrfs_set_stack_inode_mode(inode_item, S_IFDIR | 0755);
- btrfs_set_root_flags(&root_item, 0);
- btrfs_set_root_limit(&root_item, 0);
+ btrfs_set_root_flags(root_item, 0);
+ btrfs_set_root_limit(root_item, 0);
btrfs_set_stack_inode_flags(inode_item, BTRFS_INODE_ROOT_ITEM_INIT);
- btrfs_set_root_bytenr(&root_item, leaf->start);
- btrfs_set_root_generation(&root_item, trans->transid);
- btrfs_set_root_level(&root_item, 0);
- btrfs_set_root_refs(&root_item, 1);
- btrfs_set_root_used(&root_item, leaf->len);
- btrfs_set_root_last_snapshot(&root_item, 0);
+ btrfs_set_root_bytenr(root_item, leaf->start);
+ btrfs_set_root_generation(root_item, trans->transid);
+ btrfs_set_root_level(root_item, 0);
+ btrfs_set_root_refs(root_item, 1);
+ btrfs_set_root_used(root_item, leaf->len);
+ btrfs_set_root_last_snapshot(root_item, 0);
- btrfs_set_root_generation_v2(&root_item,
- btrfs_root_generation(&root_item));
+ btrfs_set_root_generation_v2(root_item,
+ btrfs_root_generation(root_item));
uuid_le_gen(&new_uuid);
- memcpy(root_item.uuid, new_uuid.b, BTRFS_UUID_SIZE);
- btrfs_set_stack_timespec_sec(&root_item.otime, cur_time.tv_sec);
- btrfs_set_stack_timespec_nsec(&root_item.otime, cur_time.tv_nsec);
- root_item.ctime = root_item.otime;
- btrfs_set_root_ctransid(&root_item, trans->transid);
- btrfs_set_root_otransid(&root_item, trans->transid);
+ memcpy(root_item->uuid, new_uuid.b, BTRFS_UUID_SIZE);
+ btrfs_set_stack_timespec_sec(&root_item->otime, cur_time.tv_sec);
+ btrfs_set_stack_timespec_nsec(&root_item->otime, cur_time.tv_nsec);
+ root_item->ctime = root_item->otime;
+ btrfs_set_root_ctransid(root_item, trans->transid);
+ btrfs_set_root_otransid(root_item, trans->transid);
btrfs_tree_unlock(leaf);
free_extent_buffer(leaf);
leaf = NULL;
- btrfs_set_root_dirid(&root_item, new_dirid);
+ btrfs_set_root_dirid(root_item, new_dirid);
key.objectid = objectid;
key.offset = 0;
key.type = BTRFS_ROOT_ITEM_KEY;
ret = btrfs_insert_root(trans, root->fs_info->tree_root, &key,
- &root_item);
+ root_item);
if (ret)
goto fail;
@@ -601,12 +605,13 @@ static noinline int create_subvol(struct inode *dir,
BUG_ON(ret);
ret = btrfs_uuid_tree_add(trans, root->fs_info->uuid_root,
- root_item.uuid, BTRFS_UUID_KEY_SUBVOL,
+ root_item->uuid, BTRFS_UUID_KEY_SUBVOL,
objectid);
if (ret)
btrfs_abort_transaction(trans, root, ret);
fail:
+ kfree(root_item);
trans->block_rsv = NULL;
trans->bytes_reserved = 0;
btrfs_subvolume_release_metadata(root, &block_rsv, qgroup_reserved);
@@ -629,6 +634,10 @@ static noinline int create_subvol(struct inode *dir,
d_instantiate(dentry, inode);
}
return ret;
+
+fail_free:
+ kfree(root_item);
+ return ret;
}
static void btrfs_wait_for_no_snapshoting_writes(struct btrfs_root *root)
--
2.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] btrfs: use dynamic allocation for root item in create_subvol
2016-04-25 11:18 ` [PATCH v2] " David Sterba
@ 2016-04-26 0:18 ` Tsutomu Itoh
0 siblings, 0 replies; 6+ messages in thread
From: Tsutomu Itoh @ 2016-04-26 0:18 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
On 2016/04/25 20:18, David Sterba wrote:
> The size of root item is more than 400 bytes, which is quite a lot of
> stack space. As we do IO from inside the subvolume ioctls, we should
> keep the stack usage low in case the filesystem is on top of other
> layers (NFS, device mapper, iscsi, etc).
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Looks good to me.
Reviewed-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
> ---
> fs/btrfs/ioctl.c | 65 ++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 37 insertions(+), 28 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 053e677839fe..9a63fe07bc2e 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -439,7 +439,7 @@ static noinline int create_subvol(struct inode *dir,
> {
> struct btrfs_trans_handle *trans;
> struct btrfs_key key;
> - struct btrfs_root_item root_item;
> + struct btrfs_root_item *root_item;
> struct btrfs_inode_item *inode_item;
> struct extent_buffer *leaf;
> struct btrfs_root *root = BTRFS_I(dir)->root;
> @@ -455,16 +455,22 @@ static noinline int create_subvol(struct inode *dir,
> u64 qgroup_reserved;
> uuid_le new_uuid;
>
> + root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
> + if (!root_item)
> + return -ENOMEM;
> +
> ret = btrfs_find_free_objectid(root->fs_info->tree_root, &objectid);
> if (ret)
> - return ret;
> + goto fail_free;
>
> /*
> * Don't create subvolume whose level is not zero. Or qgroup will be
> * screwed up since it assume subvolme qgroup's level to be 0.
> */
> - if (btrfs_qgroup_level(objectid))
> - return -ENOSPC;
> + if (btrfs_qgroup_level(objectid)) {
> + ret = -ENOSPC;
> + goto fail_free;
> + }
>
> btrfs_init_block_rsv(&block_rsv, BTRFS_BLOCK_RSV_TEMP);
> /*
> @@ -474,14 +480,14 @@ static noinline int create_subvol(struct inode *dir,
> ret = btrfs_subvolume_reserve_metadata(root, &block_rsv,
> 8, &qgroup_reserved, false);
> if (ret)
> - return ret;
> + goto fail_free;
>
> trans = btrfs_start_transaction(root, 0);
> if (IS_ERR(trans)) {
> ret = PTR_ERR(trans);
> btrfs_subvolume_release_metadata(root, &block_rsv,
> qgroup_reserved);
> - return ret;
> + goto fail_free;
> }
> trans->block_rsv = &block_rsv;
> trans->bytes_reserved = block_rsv.size;
> @@ -509,47 +515,45 @@ static noinline int create_subvol(struct inode *dir,
> BTRFS_UUID_SIZE);
> btrfs_mark_buffer_dirty(leaf);
>
> - memset(&root_item, 0, sizeof(root_item));
> -
> - inode_item = &root_item.inode;
> + inode_item = &root_item->inode;
> btrfs_set_stack_inode_generation(inode_item, 1);
> btrfs_set_stack_inode_size(inode_item, 3);
> btrfs_set_stack_inode_nlink(inode_item, 1);
> btrfs_set_stack_inode_nbytes(inode_item, root->nodesize);
> btrfs_set_stack_inode_mode(inode_item, S_IFDIR | 0755);
>
> - btrfs_set_root_flags(&root_item, 0);
> - btrfs_set_root_limit(&root_item, 0);
> + btrfs_set_root_flags(root_item, 0);
> + btrfs_set_root_limit(root_item, 0);
> btrfs_set_stack_inode_flags(inode_item, BTRFS_INODE_ROOT_ITEM_INIT);
>
> - btrfs_set_root_bytenr(&root_item, leaf->start);
> - btrfs_set_root_generation(&root_item, trans->transid);
> - btrfs_set_root_level(&root_item, 0);
> - btrfs_set_root_refs(&root_item, 1);
> - btrfs_set_root_used(&root_item, leaf->len);
> - btrfs_set_root_last_snapshot(&root_item, 0);
> + btrfs_set_root_bytenr(root_item, leaf->start);
> + btrfs_set_root_generation(root_item, trans->transid);
> + btrfs_set_root_level(root_item, 0);
> + btrfs_set_root_refs(root_item, 1);
> + btrfs_set_root_used(root_item, leaf->len);
> + btrfs_set_root_last_snapshot(root_item, 0);
>
> - btrfs_set_root_generation_v2(&root_item,
> - btrfs_root_generation(&root_item));
> + btrfs_set_root_generation_v2(root_item,
> + btrfs_root_generation(root_item));
> uuid_le_gen(&new_uuid);
> - memcpy(root_item.uuid, new_uuid.b, BTRFS_UUID_SIZE);
> - btrfs_set_stack_timespec_sec(&root_item.otime, cur_time.tv_sec);
> - btrfs_set_stack_timespec_nsec(&root_item.otime, cur_time.tv_nsec);
> - root_item.ctime = root_item.otime;
> - btrfs_set_root_ctransid(&root_item, trans->transid);
> - btrfs_set_root_otransid(&root_item, trans->transid);
> + memcpy(root_item->uuid, new_uuid.b, BTRFS_UUID_SIZE);
> + btrfs_set_stack_timespec_sec(&root_item->otime, cur_time.tv_sec);
> + btrfs_set_stack_timespec_nsec(&root_item->otime, cur_time.tv_nsec);
> + root_item->ctime = root_item->otime;
> + btrfs_set_root_ctransid(root_item, trans->transid);
> + btrfs_set_root_otransid(root_item, trans->transid);
>
> btrfs_tree_unlock(leaf);
> free_extent_buffer(leaf);
> leaf = NULL;
>
> - btrfs_set_root_dirid(&root_item, new_dirid);
> + btrfs_set_root_dirid(root_item, new_dirid);
>
> key.objectid = objectid;
> key.offset = 0;
> key.type = BTRFS_ROOT_ITEM_KEY;
> ret = btrfs_insert_root(trans, root->fs_info->tree_root, &key,
> - &root_item);
> + root_item);
> if (ret)
> goto fail;
>
> @@ -601,12 +605,13 @@ static noinline int create_subvol(struct inode *dir,
> BUG_ON(ret);
>
> ret = btrfs_uuid_tree_add(trans, root->fs_info->uuid_root,
> - root_item.uuid, BTRFS_UUID_KEY_SUBVOL,
> + root_item->uuid, BTRFS_UUID_KEY_SUBVOL,
> objectid);
> if (ret)
> btrfs_abort_transaction(trans, root, ret);
>
> fail:
> + kfree(root_item);
> trans->block_rsv = NULL;
> trans->bytes_reserved = 0;
> btrfs_subvolume_release_metadata(root, &block_rsv, qgroup_reserved);
> @@ -629,6 +634,10 @@ static noinline int create_subvol(struct inode *dir,
> d_instantiate(dentry, inode);
> }
> return ret;
> +
> +fail_free:
> + kfree(root_item);
> + return ret;
> }
>
> static void btrfs_wait_for_no_snapshoting_writes(struct btrfs_root *root)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-04-26 0:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-11 18:04 [PATCH 0/2] Stack usage reduction David Sterba
2016-04-11 18:04 ` [PATCH 1/2] btrfs: use dynamic allocation for root item in create_subvol David Sterba
2016-04-12 0:15 ` Tsutomu Itoh
2016-04-25 11:18 ` [PATCH v2] " David Sterba
2016-04-26 0:18 ` Tsutomu Itoh
2016-04-11 18:04 ` [PATCH 2/2] btrfs: reuse existing variable in scrub_stripe, reduce stack usage David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.