[PATCH] btrfs: fiemap: preallocate ulists for btrfs_check_shared

[PATCH] btrfs: fiemap: preallocate ulists for btrfs_check_shared

[David Sterba]

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@suse.com>
---
 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);
 	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;
 }
 
-- 
2.21.0

Re: [PATCH] btrfs: fiemap: preallocate ulists for btrfs_check_shared

[Nikolay Borisov]

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@suse.com>

Looks good, one minor nit worth considering below, otherwise:


Reviewed-by: Nikolay Borisov <nborisov@suse.com>


> ---
>  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;
>  }
>  
> 

Re: [PATCH] btrfs: fiemap: preallocate ulists for btrfs_check_shared

[David Sterba]

On Wed, May 15, 2019 at 04:45:42PM +0300, Nikolay Borisov wrote:
> 
> 
> 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@suse.com>
> 
> Looks good, one minor nit worth considering below, otherwise:
> 
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> 
> > ---
> >  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)

I think I had the _reinit at the end in one of the versions, but then it
looked more clear with the explicit _init at the beginning of the
function so that it does not rely on the caller to initialize.

Re: [PATCH] btrfs: fiemap: preallocate ulists for btrfs_check_shared

[Filipe Manana]

On Wed, May 15, 2019 at 2:31 PM David Sterba <dsterba@suse.com> 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@suse.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  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);
>         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;
>  }
>
> --
> 2.21.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”