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@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)
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.
