[PATCH 1/2] btrfs: remove superfluous chunk_tree argument from btrfs_alloc_dev_extent

[PATCH 1/2] btrfs: remove superfluous chunk_tree argument from btrfs_alloc_dev_extent

[Nikolay Borisov]

Currently this function is always called with the object id of the root key of
the chunk_tree, which is always BTRFS_CHUNK_TREE_OBJECTID. So let's subsume it
straight into the function itself. No functional change.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1c0a5fff0e2f..5a1913956f20 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1578,8 +1578,7 @@ static int btrfs_free_dev_extent(struct btrfs_trans_handle *trans,
 
 static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 				  struct btrfs_device *device,
-				  u64 chunk_tree, u64 chunk_offset, u64 start,
-				  u64 num_bytes)
+				  u64 chunk_offset, u64 start, u64 num_bytes)
 {
 	int ret;
 	struct btrfs_path *path;
@@ -1606,7 +1605,8 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 	leaf = path->nodes[0];
 	extent = btrfs_item_ptr(leaf, path->slots[0],
 				struct btrfs_dev_extent);
-	btrfs_set_dev_extent_chunk_tree(leaf, extent, chunk_tree);
+	btrfs_set_dev_extent_chunk_tree(leaf, extent,
+					BTRFS_CHUNK_TREE_OBJECTID);
 	btrfs_set_dev_extent_chunk_objectid(leaf, extent,
 					    BTRFS_FIRST_CHUNK_TREE_OBJECTID);
 	btrfs_set_dev_extent_chunk_offset(leaf, extent, chunk_offset);
@@ -4931,10 +4931,8 @@ int btrfs_finish_chunk_alloc(struct btrfs_trans_handle *trans,
 		ret = btrfs_update_device(trans, device);
 		if (ret)
 			break;
-		ret = btrfs_alloc_dev_extent(trans, device,
-					     chunk_root->root_key.objectid,
-					     chunk_offset, dev_offset,
-					     stripe_size);
+		ret = btrfs_alloc_dev_extent(trans, device, chunk_offset,
+					     dev_offset, stripe_size);
 		if (ret)
 			break;
 	}
-- 
2.7.4

[PATCH 2/2] btrfs: Simplify btrfs_alloc_dev_extent

[Nikolay Borisov]

Currently btrfs_alloc_dev_extent essentially open codes btrfs_insert_item. So
let's remove the superfluous code, leaving only the important bits, namely
initialising the device extent and just calling btrfs_insert_item. So first add
definition for the stack-based set/get function. And then use them.
Additionally, remove the code which sets the uuid of the block header, since
this is something which is already handled by the core btree code.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h   |  8 ++++++++
 fs/btrfs/volumes.c | 34 ++++++++++++----------------------
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index cd9497bcdb1e..567fbf186257 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1740,6 +1740,14 @@ BTRFS_SETGET_FUNCS(dev_extent_chunk_objectid, struct btrfs_dev_extent,
 BTRFS_SETGET_FUNCS(dev_extent_chunk_offset, struct btrfs_dev_extent,
 		   chunk_offset, 64);
 BTRFS_SETGET_FUNCS(dev_extent_length, struct btrfs_dev_extent, length, 64);
+BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_tree, struct btrfs_dev_extent,
+			 chunk_tree, 64);
+BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_objectid,
+			 struct btrfs_dev_extent, chunk_objectid, 64);
+BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_offset, struct btrfs_dev_extent,
+			 chunk_offset, 64);
+BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_length, struct btrfs_dev_extent,
+			 length, 64);
 
 static inline unsigned long btrfs_dev_extent_chunk_tree_uuid(struct btrfs_dev_extent *dev)
 {
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5a1913956f20..94e98261dbd0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1581,42 +1581,32 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 				  u64 chunk_offset, u64 start, u64 num_bytes)
 {
 	int ret;
-	struct btrfs_path *path;
-	struct btrfs_fs_info *fs_info = device->fs_info;
-	struct btrfs_root *root = fs_info->dev_root;
+	struct btrfs_root *root = device->fs_info->dev_root;
 	struct btrfs_dev_extent *extent;
-	struct extent_buffer *leaf;
 	struct btrfs_key key;
 
 	WARN_ON(!device->in_fs_metadata);
 	WARN_ON(device->is_tgtdev_for_dev_replace);
-	path = btrfs_alloc_path();
-	if (!path)
+
+	extent = kzalloc(sizeof(*extent), GFP_NOFS);
+	if (!extent)
 		return -ENOMEM;
 
 	key.objectid = device->devid;
 	key.offset = start;
 	key.type = BTRFS_DEV_EXTENT_KEY;
-	ret = btrfs_insert_empty_item(trans, root, path, &key,
-				      sizeof(*extent));
-	if (ret)
-		goto out;
 
-	leaf = path->nodes[0];
-	extent = btrfs_item_ptr(leaf, path->slots[0],
-				struct btrfs_dev_extent);
-	btrfs_set_dev_extent_chunk_tree(leaf, extent,
-					BTRFS_CHUNK_TREE_OBJECTID);
-	btrfs_set_dev_extent_chunk_objectid(leaf, extent,
+	btrfs_set_stack_dev_extent_chunk_tree(extent,
+					      BTRFS_CHUNK_TREE_OBJECTID);
+	btrfs_set_stack_dev_extent_chunk_objectid(extent,
 					    BTRFS_FIRST_CHUNK_TREE_OBJECTID);
-	btrfs_set_dev_extent_chunk_offset(leaf, extent, chunk_offset);
+	btrfs_set_stack_dev_extent_chunk_offset(extent, chunk_offset);
+	btrfs_set_stack_dev_extent_length(extent, num_bytes);
 
-	write_extent_buffer_chunk_tree_uuid(leaf, fs_info->chunk_tree_uuid);
+	ret = btrfs_insert_item(trans, root, &key, extent, sizeof(*extent));
+	if (ret)
+		kfree(extent);
 
-	btrfs_set_dev_extent_length(leaf, extent, num_bytes);
-	btrfs_mark_buffer_dirty(leaf);
-out:
-	btrfs_free_path(path);
 	return ret;
 }
 
-- 
2.7.4

Re: [PATCH 2/2] btrfs: Simplify btrfs_alloc_dev_extent

[Filipe Manana]

On Thu, Jul 27, 2017 at 6:36 PM, Nikolay Borisov <nborisov@suse.com> wrote:
> Currently btrfs_alloc_dev_extent essentially open codes btrfs_insert_item. So
> let's remove the superfluous code, leaving only the important bits, namely
> initialising the device extent and just calling btrfs_insert_item. So first add
> definition for the stack-based set/get function. And then use them.
> Additionally, remove the code which sets the uuid of the block header, since
> this is something which is already handled by the core btree code.

Quite honestly, I don't see the value of this change at all.
It doesn't make things simpler nor more readable nor nothing.
We have many, really many places using btrfs_insert_empty_item()
instead of calling btrfs_insert_item(), are you planning on sending a
patch to do the replacement for each of them? What's the point?

Plus you are introducing now a memory leak. See below.

>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ctree.h   |  8 ++++++++
>  fs/btrfs/volumes.c | 34 ++++++++++++----------------------
>  2 files changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index cd9497bcdb1e..567fbf186257 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1740,6 +1740,14 @@ BTRFS_SETGET_FUNCS(dev_extent_chunk_objectid, struct btrfs_dev_extent,
>  BTRFS_SETGET_FUNCS(dev_extent_chunk_offset, struct btrfs_dev_extent,
>                    chunk_offset, 64);
>  BTRFS_SETGET_FUNCS(dev_extent_length, struct btrfs_dev_extent, length, 64);
> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_tree, struct btrfs_dev_extent,
> +                        chunk_tree, 64);
> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_objectid,
> +                        struct btrfs_dev_extent, chunk_objectid, 64);
> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_offset, struct btrfs_dev_extent,
> +                        chunk_offset, 64);
> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_length, struct btrfs_dev_extent,
> +                        length, 64);
>
>  static inline unsigned long btrfs_dev_extent_chunk_tree_uuid(struct btrfs_dev_extent *dev)
>  {
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 5a1913956f20..94e98261dbd0 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1581,42 +1581,32 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>                                   u64 chunk_offset, u64 start, u64 num_bytes)
>  {
>         int ret;
> -       struct btrfs_path *path;
> -       struct btrfs_fs_info *fs_info = device->fs_info;
> -       struct btrfs_root *root = fs_info->dev_root;
> +       struct btrfs_root *root = device->fs_info->dev_root;
>         struct btrfs_dev_extent *extent;
> -       struct extent_buffer *leaf;
>         struct btrfs_key key;
>
>         WARN_ON(!device->in_fs_metadata);
>         WARN_ON(device->is_tgtdev_for_dev_replace);
> -       path = btrfs_alloc_path();
> -       if (!path)
> +
> +       extent = kzalloc(sizeof(*extent), GFP_NOFS);
> +       if (!extent)
>                 return -ENOMEM;
>
>         key.objectid = device->devid;
>         key.offset = start;
>         key.type = BTRFS_DEV_EXTENT_KEY;
> -       ret = btrfs_insert_empty_item(trans, root, path, &key,
> -                                     sizeof(*extent));
> -       if (ret)
> -               goto out;
>
> -       leaf = path->nodes[0];
> -       extent = btrfs_item_ptr(leaf, path->slots[0],
> -                               struct btrfs_dev_extent);
> -       btrfs_set_dev_extent_chunk_tree(leaf, extent,
> -                                       BTRFS_CHUNK_TREE_OBJECTID);
> -       btrfs_set_dev_extent_chunk_objectid(leaf, extent,
> +       btrfs_set_stack_dev_extent_chunk_tree(extent,
> +                                             BTRFS_CHUNK_TREE_OBJECTID);
> +       btrfs_set_stack_dev_extent_chunk_objectid(extent,
>                                             BTRFS_FIRST_CHUNK_TREE_OBJECTID);
> -       btrfs_set_dev_extent_chunk_offset(leaf, extent, chunk_offset);
> +       btrfs_set_stack_dev_extent_chunk_offset(extent, chunk_offset);
> +       btrfs_set_stack_dev_extent_length(extent, num_bytes);
>
> -       write_extent_buffer_chunk_tree_uuid(leaf, fs_info->chunk_tree_uuid);
> +       ret = btrfs_insert_item(trans, root, &key, extent, sizeof(*extent));
> +       if (ret)
> +               kfree(extent);

Even if ret is 0 (success), you need to kfree(extent).

thanks

>
> -       btrfs_set_dev_extent_length(leaf, extent, num_bytes);
> -       btrfs_mark_buffer_dirty(leaf);
> -out:
> -       btrfs_free_path(path);
>         return ret;
>  }
>
> --
> 2.7.4
>
> --
> 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-info.html



-- 
Filipe David Manana,

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

Re: [PATCH 2/2] btrfs: Simplify btrfs_alloc_dev_extent

[Nikolay Borisov]

On 27.07.2017 20:57, Filipe Manana wrote:
> On Thu, Jul 27, 2017 at 6:36 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>> Currently btrfs_alloc_dev_extent essentially open codes btrfs_insert_item. So
>> let's remove the superfluous code, leaving only the important bits, namely
>> initialising the device extent and just calling btrfs_insert_item. So first add
>> definition for the stack-based set/get function. And then use them.
>> Additionally, remove the code which sets the uuid of the block header, since
>> this is something which is already handled by the core btree code.
> 
> Quite honestly, I don't see the value of this change at all.
> It doesn't make things simpler nor more readable nor nothing.
> We have many, really many places using btrfs_insert_empty_item()
> instead of calling btrfs_insert_item(), are you planning on sending a
> patch to do the replacement for each of them? What's the point?

I beg you to differ. Some of the code in btrfs is a mess, it's working
but it's messy. There is constant violation of abstractions (as is the
case in this function, heck the uuid setting of the block header
function doesn't even belong here). All of this hampers reading
comprehension of the code for newcomers. You are experienced in the code
and likely this doesn't apply to you but since I'm someone relatively
new to the code this has been my experience. And I believe any effort to
actually simplify the code, make it more coherent and succinct is a win
long-term.

I will wait for other feedback, if people feel patches like that are
just bikeshedding then I will refrain from such cleanups in the future.

> 
> Plus you are introducing now a memory leak. See below.

Will fix it.

> 
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/ctree.h   |  8 ++++++++
>>  fs/btrfs/volumes.c | 34 ++++++++++++----------------------
>>  2 files changed, 20 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index cd9497bcdb1e..567fbf186257 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -1740,6 +1740,14 @@ BTRFS_SETGET_FUNCS(dev_extent_chunk_objectid, struct btrfs_dev_extent,
>>  BTRFS_SETGET_FUNCS(dev_extent_chunk_offset, struct btrfs_dev_extent,
>>                    chunk_offset, 64);
>>  BTRFS_SETGET_FUNCS(dev_extent_length, struct btrfs_dev_extent, length, 64);
>> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_tree, struct btrfs_dev_extent,
>> +                        chunk_tree, 64);
>> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_objectid,
>> +                        struct btrfs_dev_extent, chunk_objectid, 64);
>> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_offset, struct btrfs_dev_extent,
>> +                        chunk_offset, 64);
>> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_length, struct btrfs_dev_extent,
>> +                        length, 64);
>>
>>  static inline unsigned long btrfs_dev_extent_chunk_tree_uuid(struct btrfs_dev_extent *dev)
>>  {
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 5a1913956f20..94e98261dbd0 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1581,42 +1581,32 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>>                                   u64 chunk_offset, u64 start, u64 num_bytes)
>>  {
>>         int ret;
>> -       struct btrfs_path *path;
>> -       struct btrfs_fs_info *fs_info = device->fs_info;
>> -       struct btrfs_root *root = fs_info->dev_root;
>> +       struct btrfs_root *root = device->fs_info->dev_root;
>>         struct btrfs_dev_extent *extent;
>> -       struct extent_buffer *leaf;
>>         struct btrfs_key key;
>>
>>         WARN_ON(!device->in_fs_metadata);
>>         WARN_ON(device->is_tgtdev_for_dev_replace);
>> -       path = btrfs_alloc_path();
>> -       if (!path)
>> +
>> +       extent = kzalloc(sizeof(*extent), GFP_NOFS);
>> +       if (!extent)
>>                 return -ENOMEM;
>>
>>         key.objectid = device->devid;
>>         key.offset = start;
>>         key.type = BTRFS_DEV_EXTENT_KEY;
>> -       ret = btrfs_insert_empty_item(trans, root, path, &key,
>> -                                     sizeof(*extent));
>> -       if (ret)
>> -               goto out;
>>
>> -       leaf = path->nodes[0];
>> -       extent = btrfs_item_ptr(leaf, path->slots[0],
>> -                               struct btrfs_dev_extent);
>> -       btrfs_set_dev_extent_chunk_tree(leaf, extent,
>> -                                       BTRFS_CHUNK_TREE_OBJECTID);
>> -       btrfs_set_dev_extent_chunk_objectid(leaf, extent,
>> +       btrfs_set_stack_dev_extent_chunk_tree(extent,
>> +                                             BTRFS_CHUNK_TREE_OBJECTID);
>> +       btrfs_set_stack_dev_extent_chunk_objectid(extent,
>>                                             BTRFS_FIRST_CHUNK_TREE_OBJECTID);
>> -       btrfs_set_dev_extent_chunk_offset(leaf, extent, chunk_offset);
>> +       btrfs_set_stack_dev_extent_chunk_offset(extent, chunk_offset);
>> +       btrfs_set_stack_dev_extent_length(extent, num_bytes);
>>
>> -       write_extent_buffer_chunk_tree_uuid(leaf, fs_info->chunk_tree_uuid);
>> +       ret = btrfs_insert_item(trans, root, &key, extent, sizeof(*extent));
>> +       if (ret)
>> +               kfree(extent);
> 
> Even if ret is 0 (success), you need to kfree(extent).
> 
> thanks
> 
>>
>> -       btrfs_set_dev_extent_length(leaf, extent, num_bytes);
>> -       btrfs_mark_buffer_dirty(leaf);
>> -out:
>> -       btrfs_free_path(path);
>>         return ret;
>>  }
>>
>> --
>> 2.7.4
>>
>> --
>> 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-info.html
> 
> 
> 

Re: [PATCH 2/2] btrfs: Simplify btrfs_alloc_dev_extent

[Filipe Manana]

On Fri, Jul 28, 2017 at 6:59 AM, Nikolay Borisov <nborisov@suse.com> wrote:
>
>
> On 27.07.2017 20:57, Filipe Manana wrote:
>> On Thu, Jul 27, 2017 at 6:36 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>>> Currently btrfs_alloc_dev_extent essentially open codes btrfs_insert_item. So
>>> let's remove the superfluous code, leaving only the important bits, namely
>>> initialising the device extent and just calling btrfs_insert_item. So first add
>>> definition for the stack-based set/get function. And then use them.
>>> Additionally, remove the code which sets the uuid of the block header, since
>>> this is something which is already handled by the core btree code.
>>
>> Quite honestly, I don't see the value of this change at all.
>> It doesn't make things simpler nor more readable nor nothing.
>> We have many, really many places using btrfs_insert_empty_item()
>> instead of calling btrfs_insert_item(), are you planning on sending a
>> patch to do the replacement for each of them? What's the point?
>
> I beg you to differ. Some of the code in btrfs is a mess, it's working
> but it's messy. There is constant violation of abstractions (as is the
> case in this function, heck the uuid setting of the block header
> function doesn't even belong here).

The uuid setting is a different thing (and that's fine to go away),
unrelated to using insert_empty_item() vs insert_item(), which is what
I was referring to in my previous reply.

> All of this hampers reading
> comprehension of the code for newcomers. You are experienced in the code
> and likely this doesn't apply to you but since I'm someone relatively
> new to the code this has been my experience. And I believe any effort to
> actually simplify the code, make it more coherent and succinct is a win
> long-term.

Well, this hasn't prevented me, or others that have started
contributing to btrfs after I did, from being able to understand the
code and do useful changes (otherwise such kind of patches would have
landed long time ago). This kind of change won't save anyone's time
understanding the code.

Plus, if I want to go a bit more nitpick, this change of using
btrfs_insert_item() is from a performance/efficiency point of view,
worse as it requires an additional memory allocation/free (the device
extent).

>
> I will wait for other feedback, if people feel patches like that are
> just bikeshedding then I will refrain from such cleanups in the future.
>
>>
>> Plus you are introducing now a memory leak. See below.
>
> Will fix it.
>
>>
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>>  fs/btrfs/ctree.h   |  8 ++++++++
>>>  fs/btrfs/volumes.c | 34 ++++++++++++----------------------
>>>  2 files changed, 20 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>> index cd9497bcdb1e..567fbf186257 100644
>>> --- a/fs/btrfs/ctree.h
>>> +++ b/fs/btrfs/ctree.h
>>> @@ -1740,6 +1740,14 @@ BTRFS_SETGET_FUNCS(dev_extent_chunk_objectid, struct btrfs_dev_extent,
>>>  BTRFS_SETGET_FUNCS(dev_extent_chunk_offset, struct btrfs_dev_extent,
>>>                    chunk_offset, 64);
>>>  BTRFS_SETGET_FUNCS(dev_extent_length, struct btrfs_dev_extent, length, 64);
>>> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_tree, struct btrfs_dev_extent,
>>> +                        chunk_tree, 64);
>>> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_objectid,
>>> +                        struct btrfs_dev_extent, chunk_objectid, 64);
>>> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_chunk_offset, struct btrfs_dev_extent,
>>> +                        chunk_offset, 64);
>>> +BTRFS_SETGET_STACK_FUNCS(stack_dev_extent_length, struct btrfs_dev_extent,
>>> +                        length, 64);
>>>
>>>  static inline unsigned long btrfs_dev_extent_chunk_tree_uuid(struct btrfs_dev_extent *dev)
>>>  {
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 5a1913956f20..94e98261dbd0 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -1581,42 +1581,32 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>>>                                   u64 chunk_offset, u64 start, u64 num_bytes)
>>>  {
>>>         int ret;
>>> -       struct btrfs_path *path;
>>> -       struct btrfs_fs_info *fs_info = device->fs_info;
>>> -       struct btrfs_root *root = fs_info->dev_root;
>>> +       struct btrfs_root *root = device->fs_info->dev_root;
>>>         struct btrfs_dev_extent *extent;
>>> -       struct extent_buffer *leaf;
>>>         struct btrfs_key key;
>>>
>>>         WARN_ON(!device->in_fs_metadata);
>>>         WARN_ON(device->is_tgtdev_for_dev_replace);
>>> -       path = btrfs_alloc_path();
>>> -       if (!path)
>>> +
>>> +       extent = kzalloc(sizeof(*extent), GFP_NOFS);
>>> +       if (!extent)
>>>                 return -ENOMEM;
>>>
>>>         key.objectid = device->devid;
>>>         key.offset = start;
>>>         key.type = BTRFS_DEV_EXTENT_KEY;
>>> -       ret = btrfs_insert_empty_item(trans, root, path, &key,
>>> -                                     sizeof(*extent));
>>> -       if (ret)
>>> -               goto out;
>>>
>>> -       leaf = path->nodes[0];
>>> -       extent = btrfs_item_ptr(leaf, path->slots[0],
>>> -                               struct btrfs_dev_extent);
>>> -       btrfs_set_dev_extent_chunk_tree(leaf, extent,
>>> -                                       BTRFS_CHUNK_TREE_OBJECTID);
>>> -       btrfs_set_dev_extent_chunk_objectid(leaf, extent,
>>> +       btrfs_set_stack_dev_extent_chunk_tree(extent,
>>> +                                             BTRFS_CHUNK_TREE_OBJECTID);
>>> +       btrfs_set_stack_dev_extent_chunk_objectid(extent,
>>>                                             BTRFS_FIRST_CHUNK_TREE_OBJECTID);
>>> -       btrfs_set_dev_extent_chunk_offset(leaf, extent, chunk_offset);
>>> +       btrfs_set_stack_dev_extent_chunk_offset(extent, chunk_offset);
>>> +       btrfs_set_stack_dev_extent_length(extent, num_bytes);
>>>
>>> -       write_extent_buffer_chunk_tree_uuid(leaf, fs_info->chunk_tree_uuid);
>>> +       ret = btrfs_insert_item(trans, root, &key, extent, sizeof(*extent));
>>> +       if (ret)
>>> +               kfree(extent);
>>
>> Even if ret is 0 (success), you need to kfree(extent).
>>
>> thanks
>>
>>>
>>> -       btrfs_set_dev_extent_length(leaf, extent, num_bytes);
>>> -       btrfs_mark_buffer_dirty(leaf);
>>> -out:
>>> -       btrfs_free_path(path);
>>>         return ret;
>>>  }
>>>
>>> --
>>> 2.7.4
>>>
>>> --
>>> 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-info.html
>>
>>
>>



-- 
Filipe David Manana,

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

Re: [PATCH 2/2] btrfs: Simplify btrfs_alloc_dev_extent

[David Sterba]

On Fri, Jul 28, 2017 at 08:59:12AM +0300, Nikolay Borisov wrote:
> 
> 
> On 27.07.2017 20:57, Filipe Manana wrote:
> > On Thu, Jul 27, 2017 at 6:36 PM, Nikolay Borisov <nborisov@suse.com> wrote:
> >> Currently btrfs_alloc_dev_extent essentially open codes btrfs_insert_item. So
> >> let's remove the superfluous code, leaving only the important bits, namely
> >> initialising the device extent and just calling btrfs_insert_item. So first add
> >> definition for the stack-based set/get function. And then use them.
> >> Additionally, remove the code which sets the uuid of the block header, since
> >> this is something which is already handled by the core btree code.
> > 
> > Quite honestly, I don't see the value of this change at all.
> > It doesn't make things simpler nor more readable nor nothing.
> > We have many, really many places using btrfs_insert_empty_item()
> > instead of calling btrfs_insert_item(), are you planning on sending a
> > patch to do the replacement for each of them? What's the point?
> 
> I beg you to differ. Some of the code in btrfs is a mess, it's working
> but it's messy. There is constant violation of abstractions (as is the
> case in this function, heck the uuid setting of the block header
> function doesn't even belong here). All of this hampers reading
> comprehension of the code for newcomers. You are experienced in the code
> and likely this doesn't apply to you but since I'm someone relatively
> new to the code this has been my experience. And I believe any effort to
> actually simplify the code, make it more coherent and succinct is a win
> long-term.
> 
> I will wait for other feedback, if people feel patches like that are
> just bikeshedding then I will refrain from such cleanups in the future.

What I don't like about this patch, also pointed out by Filipe, is the
additional memory allocation. Just for the sake of code beauty, this is
not the direction the cleanups should go. The function is IMHO readable
and below my current threshold of cleanup need.

Re: [PATCH 1/2] btrfs: remove superfluous chunk_tree argument from btrfs_alloc_dev_extent

[David Sterba]

On Thu, Jul 27, 2017 at 08:36:36PM +0300, Nikolay Borisov wrote:
> Currently this function is always called with the object id of the root key of
> the chunk_tree, which is always BTRFS_CHUNK_TREE_OBJECTID. So let's subsume it
> straight into the function itself. No functional change.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/volumes.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1c0a5fff0e2f..5a1913956f20 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1578,8 +1578,7 @@ static int btrfs_free_dev_extent(struct btrfs_trans_handle *trans,
>  
>  static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>  				  struct btrfs_device *device,
> -				  u64 chunk_tree, u64 chunk_offset, u64 start,
> -				  u64 num_bytes)
> +				  u64 chunk_offset, u64 start, u64 num_bytes)

This hunk does not apply, the context in git contains u64 chunk_objectid
and I haven't found any patch that would remove or rename it. Please
refresh and resend.

[PATCH 1/2] btrfs: Remove chunk_objectid parameter of btrfs_alloc_dev_extent

[Nikolay Borisov]

THe function is always called with chunk_objectid set to
BTRFS_FIRST_CHUNK_TREE_OBJECTID. Let's collapse the parameter in the function
itself. No functional changes

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a37a31ba6843..63608c5f4487 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1571,8 +1571,8 @@ static int btrfs_free_dev_extent(struct btrfs_trans_handle *trans,
 
 static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 				  struct btrfs_device *device,
-				  u64 chunk_tree, u64 chunk_objectid,
-				  u64 chunk_offset, u64 start, u64 num_bytes)
+				  u64 chunk_tree, u64 chunk_offset, u64 start,
+				  u64 num_bytes)
 {
 	int ret;
 	struct btrfs_path *path;
@@ -1600,7 +1600,8 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 	extent = btrfs_item_ptr(leaf, path->slots[0],
 				struct btrfs_dev_extent);
 	btrfs_set_dev_extent_chunk_tree(leaf, extent, chunk_tree);
-	btrfs_set_dev_extent_chunk_objectid(leaf, extent, chunk_objectid);
+	btrfs_set_dev_extent_chunk_objectid(leaf, extent,
+					    BTRFS_FIRST_CHUNK_TREE_OBJECTID);
 	btrfs_set_dev_extent_chunk_offset(leaf, extent, chunk_offset);
 
 	btrfs_set_dev_extent_length(leaf, extent, num_bytes);
@@ -4904,7 +4905,6 @@ int btrfs_finish_chunk_alloc(struct btrfs_trans_handle *trans,
 			break;
 		ret = btrfs_alloc_dev_extent(trans, device,
 					     chunk_root->root_key.objectid,
-					     BTRFS_FIRST_CHUNK_TREE_OBJECTID,
 					     chunk_offset, dev_offset,
 					     stripe_size);
 		if (ret)
-- 
2.7.4

[PATCH 2/2] btrfs: remove superfluous chunk_tree argument from btrfs_alloc_dev_extent

[Nikolay Borisov]

Currently this function is always called with the object id of the root key of
the chunk_tree, which is always BTRFS_CHUNK_TREE_OBJECTID. So let's subsume it
straight into the function itself. No functional change.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 63608c5f4487..d024f1b07282 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1571,8 +1571,7 @@ static int btrfs_free_dev_extent(struct btrfs_trans_handle *trans,
 
 static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 				  struct btrfs_device *device,
-				  u64 chunk_tree, u64 chunk_offset, u64 start,
-				  u64 num_bytes)
+				  u64 chunk_offset, u64 start, u64 num_bytes)
 {
 	int ret;
 	struct btrfs_path *path;
@@ -1599,7 +1598,8 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 	leaf = path->nodes[0];
 	extent = btrfs_item_ptr(leaf, path->slots[0],
 				struct btrfs_dev_extent);
-	btrfs_set_dev_extent_chunk_tree(leaf, extent, chunk_tree);
+	btrfs_set_dev_extent_chunk_tree(leaf, extent,
+					BTRFS_CHUNK_TREE_OBJECTID);
 	btrfs_set_dev_extent_chunk_objectid(leaf, extent,
 					    BTRFS_FIRST_CHUNK_TREE_OBJECTID);
 	btrfs_set_dev_extent_chunk_offset(leaf, extent, chunk_offset);
@@ -4903,10 +4903,8 @@ int btrfs_finish_chunk_alloc(struct btrfs_trans_handle *trans,
 		ret = btrfs_update_device(trans, device);
 		if (ret)
 			break;
-		ret = btrfs_alloc_dev_extent(trans, device,
-					     chunk_root->root_key.objectid,
-					     chunk_offset, dev_offset,
-					     stripe_size);
+		ret = btrfs_alloc_dev_extent(trans, device, chunk_offset,
+					     dev_offset, stripe_size);
 		if (ret)
 			break;
 	}
-- 
2.7.4

Re: [PATCH 2/2] btrfs: remove superfluous chunk_tree argument from btrfs_alloc_dev_extent

[David Sterba]

On Fri, Aug 18, 2017 at 05:58:23PM +0300, Nikolay Borisov wrote:
> Currently this function is always called with the object id of the root key of
> the chunk_tree, which is always BTRFS_CHUNK_TREE_OBJECTID. So let's subsume it
> straight into the function itself. No functional change.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 1/2] btrfs: Remove chunk_objectid parameter of btrfs_alloc_dev_extent

[David Sterba]

On Fri, Aug 18, 2017 at 05:58:22PM +0300, Nikolay Borisov wrote:
> THe function is always called with chunk_objectid set to
> BTRFS_FIRST_CHUNK_TREE_OBJECTID. Let's collapse the parameter in the function
> itself. No functional changes
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>