On 15.05.19 г. 16:31 ч., David Sterba wrote:
> btrfs_check_shared looks up parents of a given extent and uses ulists
> for that. These are allocated and freed repeatedly. Preallocation in the
> caller will avoid the overhead and also allow us to use the GFP_KERNEL
> as it is happens before the extent locks are taken.
>
> Signed-off-by: David Sterba <dsterba@xxxxxxxx>
Looks good, one minor nit worth considering below, otherwise:
Reviewed-by: Nikolay Borisov <nborisov@xxxxxxxx>
> ---
> fs/btrfs/backref.c | 17 ++++++-----------
> fs/btrfs/backref.h | 3 ++-
> fs/btrfs/extent_io.c | 15 +++++++++++++--
> 3 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 982152d3f920..89116afda7a2 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -1465,12 +1465,11 @@ int btrfs_find_all_roots(struct btrfs_trans_handle *trans,
> *
> * Return: 0 if extent is not shared, 1 if it is shared, < 0 on error.
> */
> -int btrfs_check_shared(struct btrfs_root *root, u64 inum, u64 bytenr)
> +int btrfs_check_shared(struct btrfs_root *root, u64 inum, u64 bytenr,
> + struct ulist *roots, struct ulist *tmp)
> {
> struct btrfs_fs_info *fs_info = root->fs_info;
> struct btrfs_trans_handle *trans;
> - struct ulist *tmp = NULL;
> - struct ulist *roots = NULL;
> struct ulist_iterator uiter;
> struct ulist_node *node;
> struct seq_list elem = SEQ_LIST_INIT(elem);
> @@ -1481,12 +1480,8 @@ int btrfs_check_shared(struct btrfs_root *root, u64 inum, u64 bytenr)
> .share_count = 0,
> };
>
> - tmp = ulist_alloc(GFP_NOFS);
> - roots = ulist_alloc(GFP_NOFS);
> - if (!tmp || !roots) {
> - ret = -ENOMEM;
> - goto out;
> - }
> + ulist_init(roots);
> + ulist_init(tmp);
>
> trans = btrfs_attach_transaction(root);
> if (IS_ERR(trans)) {
> @@ -1527,8 +1522,8 @@ int btrfs_check_shared(struct btrfs_root *root, u64 inum, u64 bytenr)
> up_read(&fs_info->commit_root_sem);
> }
> out:
> - ulist_free(tmp);
> - ulist_free(roots);
> + ulist_release(roots);
> + ulist_release(tmp);
nit: If we turn these into ulist_reinit there is no need to do ulit_init
at the beginning. Having said that, the only difference between
ulist_release/init is that the latter also does ulist->nnode=0 (apart
form the memory freeing). So ulist_release can really boil down to:
list_for_each_entry_safe() {
kfree}
ulist_init(ulist)
> return ret;
> }
>
> diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
> index 54d58988483a..777f61dc081e 100644
> --- a/fs/btrfs/backref.h
> +++ b/fs/btrfs/backref.h
> @@ -57,7 +57,8 @@ int btrfs_find_one_extref(struct btrfs_root *root, u64 inode_objectid,
> u64 start_off, struct btrfs_path *path,
> struct btrfs_inode_extref **ret_extref,
> u64 *found_off);
> -int btrfs_check_shared(struct btrfs_root *root, u64 inum, u64 bytenr);
> +int btrfs_check_shared(struct btrfs_root *root, u64 inum, u64 bytenr,
> + struct ulist *roots, struct ulist *tmp_ulist);
>
> int __init btrfs_prelim_ref_init(void);
> void __cold btrfs_prelim_ref_exit(void);
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 13fca7bfc1f2..d70a602a5d48 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4557,6 +4557,13 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> return -ENOMEM;
> path->leave_spinning = 1;
>
> + roots = ulist_alloc(GFP_KERNEL);
> + tmp_ulist = ulist_alloc(GFP_KERNEL);
> + if (!roots || !tmp_ulist) {
> + ret = -ENOMEM;
> + goto out_free_ulist;
> + }
> +
> start = round_down(start, btrfs_inode_sectorsize(inode));
> len = round_up(max, btrfs_inode_sectorsize(inode)) - start;
>
> @@ -4568,7 +4575,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> btrfs_ino(BTRFS_I(inode)), -1, 0);
> if (ret < 0) {
> btrfs_free_path(path);
> - return ret;
> + goto out_free_ulist;
> } else {
> WARN_ON(!ret);
> if (ret == 1)
> @@ -4677,7 +4684,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> */
> ret = btrfs_check_shared(root,
> btrfs_ino(BTRFS_I(inode)),
> - bytenr);
> + bytenr, roots, tmp_ulist);
> if (ret < 0)
> goto out_free;
> if (ret)
> @@ -4723,6 +4730,10 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> btrfs_free_path(path);
> unlock_extent_cached(&BTRFS_I(inode)->io_tree, start, start + len - 1,
> &cached_state);
> +
> +out_free_ulist:
> + ulist_free(roots);
> + ulist_free(tmp_ulist);
> return ret;
> }
>
>
