[PATCH v3 0/3] btrfs path auto free

[PATCH v3 0/3] btrfs path auto free

[Leo Martins]

The DEFINE_FREE macro defines a wrapper function for a given memory
cleanup function which takes a pointer as an argument and calls the
cleanup function with the value of the pointer. The __free macro adds
a scoped-based cleanup to a variable, using the __cleanup attribute
to specify the cleanup function that should be called when the variable
goes out of scope.

Using this cleanup code pattern ensures that memory is properly freed
when it's no longer needed, preventing memory leaks and reducing the
risk of crashes or other issues caused by incorrect memory management.
Even if the code is already memory safe, using this pattern reduces
the risk of introducing memory-related bugs in the future

In this series of patches I've added a DEFINE_FREE for btrfs_free_path
and created a macro BTRFS_PATH_AUTO_FREE to clearly identify path
declarations that will be automatically freed.

I've included some simple examples of where this pattern can be used.
The trivial examples are ones where there is one exit path and the only
cleanup performed is a call to btrfs_free_path.

There appear to be around 130 instances that would be a pretty simple to
convert to this pattern. Is it worth going back and updating
all trivial instances or would it be better to leave them and use the pattern
in new code? Another option is to have all path declarations declared
with BTRFS_PATH_AUTO_FREE and not remove any btrfs_free_path instances.
In theory this would not change the functionality as it is fine to call
btrfs_free_path on an already freed path.

Leo Martins (3):
  btrfs: DEFINE_FREE for btrfs_free_path
  btrfs: BTRFS_PATH_AUTO_FREE in zoned.c
  btrfs: BTRFS_PATH_AUTO_FREE in orphan.c

 fs/btrfs/ctree.c  |  2 +-
 fs/btrfs/ctree.h  |  4 ++++
 fs/btrfs/orphan.c | 19 ++++++-------------
 fs/btrfs/zoned.c  | 34 +++++++++++-----------------------
 4 files changed, 22 insertions(+), 37 deletions(-)

-- 
2.43.5

[PATCH v3 1/3] btrfs: DEFINE_FREE for btrfs_free_path

[Leo Martins]

Add a DEFINE_FREE for btrfs_free_path. This defines a function that can
be called using the __free attribute. Defined a macro
BTRFS_PATH_AUTO_FREE to make the declaration of an auto freeing path
very clear.

Signed-off-by: Leo Martins <loemra.dev@gmail.com>
---
 fs/btrfs/ctree.c | 2 +-
 fs/btrfs/ctree.h | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 451203055bbfb..f0bdea206d672 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -196,7 +196,7 @@ struct btrfs_path *btrfs_alloc_path(void)
 /* this also releases the path */
 void btrfs_free_path(struct btrfs_path *p)
 {
-	if (!p)
+	if (IS_ERR_OR_NULL(p))
 		return;
 	btrfs_release_path(p);
 	kmem_cache_free(btrfs_path_cachep, p);
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 75fa563e4cacb..fbf1f8644faea 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -6,6 +6,7 @@
 #ifndef BTRFS_CTREE_H
 #define BTRFS_CTREE_H
 
+#include "linux/cleanup.h"
 #include <linux/pagemap.h>
 #include <linux/spinlock.h>
 #include <linux/rbtree.h>
@@ -599,6 +600,9 @@ int btrfs_search_slot_for_read(struct btrfs_root *root,
 void btrfs_release_path(struct btrfs_path *p);
 struct btrfs_path *btrfs_alloc_path(void);
 void btrfs_free_path(struct btrfs_path *p);
+DEFINE_FREE(btrfs_free_path, struct btrfs_path *, btrfs_free_path(_T))
+#define BTRFS_PATH_AUTO_FREE(path_name) \
+	struct btrfs_path *path_name __free(btrfs_free_path) = NULL;
 
 int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		   struct btrfs_path *path, int slot, int nr);
-- 
2.43.5

[PATCH v3 2/3] btrfs: BTRFS_PATH_AUTO_FREE in zoned.c

[Leo Martins]

Add automatic path freeing in zoned.c, the examples here are both fairly
trivial with one exit path and only btrfs_free_path cleanup.

Signed-off-by: Leo Martins <loemra.dev@gmail.com>
---
 fs/btrfs/zoned.c | 34 +++++++++++-----------------------
 1 file changed, 11 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 66f63e82af793..158bb0b708805 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -287,7 +287,7 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
 /* The emulated zone size is determined from the size of device extent */
 static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info)
 {
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_root *root = fs_info->dev_root;
 	struct btrfs_key key;
 	struct extent_buffer *leaf;
@@ -304,28 +304,21 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info)
 
 	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
 		ret = btrfs_next_leaf(root, path);
 		if (ret < 0)
-			goto out;
+			return ret;
 		/* No dev extents at all? Not good */
-		if (ret > 0) {
-			ret = -EUCLEAN;
-			goto out;
-		}
+		if (ret > 0)
+			return -EUCLEAN;
 	}
 
 	leaf = path->nodes[0];
 	dext = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_dev_extent);
 	fs_info->zone_size = btrfs_dev_extent_length(leaf, dext);
-	ret = 0;
-
-out:
-	btrfs_free_path(path);
-
-	return ret;
+	return 0;
 }
 
 int btrfs_get_dev_zone_info_all_devices(struct btrfs_fs_info *fs_info)
@@ -1211,7 +1204,7 @@ static int calculate_alloc_pointer(struct btrfs_block_group *cache,
 {
 	struct btrfs_fs_info *fs_info = cache->fs_info;
 	struct btrfs_root *root;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_key key;
 	struct btrfs_key found_key;
 	int ret;
@@ -1246,7 +1239,7 @@ static int calculate_alloc_pointer(struct btrfs_block_group *cache,
 	if (!ret)
 		ret = -EUCLEAN;
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	ret = btrfs_previous_extent_item(root, path, cache->start);
 	if (ret) {
@@ -1254,7 +1247,7 @@ static int calculate_alloc_pointer(struct btrfs_block_group *cache,
 			ret = 0;
 			*offset_ret = 0;
 		}
-		goto out;
+		return ret;
 	}
 
 	btrfs_item_key_to_cpu(path->nodes[0], &found_key, path->slots[0]);
@@ -1266,15 +1259,10 @@ static int calculate_alloc_pointer(struct btrfs_block_group *cache,
 
 	if (!(found_key.objectid >= cache->start &&
 	       found_key.objectid + length <= cache->start + cache->length)) {
-		ret = -EUCLEAN;
-		goto out;
+		return -EUCLEAN;
 	}
 	*offset_ret = found_key.objectid + length - cache->start;
-	ret = 0;
-
-out:
-	btrfs_free_path(path);
-	return ret;
+	return 0;
 }
 
 struct zone_info {
-- 
2.43.5

[PATCH v3 3/3] btrfs: BTRFS_PATH_AUTO_FREE in orphan.c

[Leo Martins]

Add automatic path freeing in orphan.c, the examples here are both fairly
trivial with one exit path and only btrfs_free_path cleanup.

Signed-off-by: Leo Martins <loemra.dev@gmail.com>
---
 fs/btrfs/orphan.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/orphan.c b/fs/btrfs/orphan.c
index 6195a2215b8fe..696dbaf26af52 100644
--- a/fs/btrfs/orphan.c
+++ b/fs/btrfs/orphan.c
@@ -9,7 +9,7 @@
 int btrfs_insert_orphan_item(struct btrfs_trans_handle *trans,
 			     struct btrfs_root *root, u64 offset)
 {
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_key key;
 	int ret = 0;
 
@@ -23,14 +23,13 @@ int btrfs_insert_orphan_item(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_insert_empty_item(trans, root, path, &key, 0);
 
-	btrfs_free_path(path);
 	return ret;
 }
 
 int btrfs_del_orphan_item(struct btrfs_trans_handle *trans,
 			  struct btrfs_root *root, u64 offset)
 {
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_key key;
 	int ret = 0;
 
@@ -44,15 +43,9 @@ int btrfs_del_orphan_item(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
 	if (ret < 0)
-		goto out;
-	if (ret) { /* JDM: Really? */
-		ret = -ENOENT;
-		goto out;
-	}
+		return ret;
+	if (ret)
+		return -ENOENT;
 
-	ret = btrfs_del_item(trans, root, path);
-
-out:
-	btrfs_free_path(path);
-	return ret;
+	return btrfs_del_item(trans, root, path);
 }
-- 
2.43.5

Re: [PATCH v3 0/3] btrfs path auto free

[David Sterba]

On Tue, Aug 27, 2024 at 03:41:30PM -0700, Leo Martins wrote:
> The DEFINE_FREE macro defines a wrapper function for a given memory
> cleanup function which takes a pointer as an argument and calls the
> cleanup function with the value of the pointer. The __free macro adds
> a scoped-based cleanup to a variable, using the __cleanup attribute
> to specify the cleanup function that should be called when the variable
> goes out of scope.
> 
> Using this cleanup code pattern ensures that memory is properly freed
> when it's no longer needed, preventing memory leaks and reducing the
> risk of crashes or other issues caused by incorrect memory management.
> Even if the code is already memory safe, using this pattern reduces
> the risk of introducing memory-related bugs in the future
> 
> In this series of patches I've added a DEFINE_FREE for btrfs_free_path
> and created a macro BTRFS_PATH_AUTO_FREE to clearly identify path
> declarations that will be automatically freed.
> 
> I've included some simple examples of where this pattern can be used.
> The trivial examples are ones where there is one exit path and the only
> cleanup performed is a call to btrfs_free_path.
> 
> There appear to be around 130 instances that would be a pretty simple to
> convert to this pattern.

I'm not sure what exactly are you counting, the number seems too much.

> Is it worth going back and updating
> all trivial instances or would it be better to leave them and use the pattern
> in new code?

Using it in new code sounds like a better option for the start so we
don't cause too much churn in code that hasn't otherwise changed for a
long time.

> Another option is to have all path declarations declared
> with BTRFS_PATH_AUTO_FREE and not remove any btrfs_free_path instances.
> In theory this would not change the functionality as it is fine to call
> btrfs_free_path on an already freed path.

That would require adding path = NULL after each btrfs_free_path() so
the auto free does not free it twice.

Changing all instances of path declarations may not be feasible, the
pattern of path freeing right before return is not that widespread,
sometimes mixed with btrfs_release_path.

I've sent some comments for v2 that I don't see fixed in v3. Also please
for patchset iterations write a short list of changes between.

Re: [PATCH v3 0/3] btrfs path auto free

[Leo Martins]

I think the only feedback I haven't addressed in this patchset was
moving the declaration to be next to the btrfs_path struct.
However, I don't think moving the declaration makes sense because
the DEFINE_FREE references btrfs_free_path which itself is only
declared after the structure. The other examples I've looked at also
have DEFINE_FREE next to the freeing function as opposed to the
structure being freed.

On Tue, 27 Aug 2024 15:41, Leo Martins <loemra.dev@gmail.com> wrote:
>The DEFINE_FREE macro defines a wrapper function for a given memory
>cleanup function which takes a pointer as an argument and calls the
>cleanup function with the value of the pointer. The __free macro adds
>a scoped-based cleanup to a variable, using the __cleanup attribute
>to specify the cleanup function that should be called when the variable
>goes out of scope.
>
>Using this cleanup code pattern ensures that memory is properly freed
>when it's no longer needed, preventing memory leaks and reducing the
>risk of crashes or other issues caused by incorrect memory management.
>Even if the code is already memory safe, using this pattern reduces
>the risk of introducing memory-related bugs in the future
>
>In this series of patches I've added a DEFINE_FREE for btrfs_free_path
>and created a macro BTRFS_PATH_AUTO_FREE to clearly identify path
>declarations that will be automatically freed.
>
>I've included some simple examples of where this pattern can be used.
>The trivial examples are ones where there is one exit path and the only
>cleanup performed is a call to btrfs_free_path.
>
>There appear to be around 130 instances that would be a pretty simple to
>convert to this pattern. Is it worth going back and updating
>all trivial instances or would it be better to leave them and use the pattern
>in new code? Another option is to have all path declarations declared
>with BTRFS_PATH_AUTO_FREE and not remove any btrfs_free_path instances.
>In theory this would not change the functionality as it is fine to call
>btrfs_free_path on an already freed path.
>
>Leo Martins (3):
>  btrfs: DEFINE_FREE for btrfs_free_path
>  btrfs: BTRFS_PATH_AUTO_FREE in zoned.c
>  btrfs: BTRFS_PATH_AUTO_FREE in orphan.c
>
> fs/btrfs/ctree.c  |  2 +-
> fs/btrfs/ctree.h  |  4 ++++
> fs/btrfs/orphan.c | 19 ++++++-------------
> fs/btrfs/zoned.c  | 34 +++++++++++-----------------------
> 4 files changed, 22 insertions(+), 37 deletions(-)
>
>-- 
>2.43.5
>

Re: [PATCH v3 0/3] btrfs path auto free

[David Sterba]

On Fri, Aug 30, 2024 at 01:46:59PM -0700, Leo Martins wrote:
> I think the only feedback I haven't addressed in this patchset was
> moving the declaration to be next to the btrfs_path struct.
> However, I don't think moving the declaration makes sense because
> the DEFINE_FREE references btrfs_free_path which itself is only
> declared after the structure.

As long as the definition of btrfs_free_path() is not needed to
compile, because it's a macro, I'd really want to keep the auto freeing
defininion next to the structure because it's closely related to the
structure. So if I ever go to the definition of any structure I want to
see right away if it does or does not support the auto free.

> The other examples I've looked at also
> have DEFINE_FREE next to the freeing function as opposed to the
> structure being freed.

Re: [PATCH v3 0/3] btrfs path auto free

[David Sterba]

On Tue, Sep 03, 2024 at 01:43:46AM +0200, David Sterba wrote:
> On Fri, Aug 30, 2024 at 01:46:59PM -0700, Leo Martins wrote:
> > I think the only feedback I haven't addressed in this patchset was
> > moving the declaration to be next to the btrfs_path struct.
> > However, I don't think moving the declaration makes sense because
> > the DEFINE_FREE references btrfs_free_path which itself is only
> > declared after the structure.
> 
> As long as the definition of btrfs_free_path() is not needed to
> compile, because it's a macro, I'd really want to keep the auto freeing
> defininion next to the structure because it's closely related to the
> structure. So if I ever go to the definition of any structure I want to
> see right away if it does or does not support the auto free.

This diff demonstrates the idea:

--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -196,7 +196,7 @@ struct btrfs_path *btrfs_alloc_path(void)
 /* this also releases the path */
 void btrfs_free_path(struct btrfs_path *p)
 {
-       if (IS_ERR_OR_NULL(p))
+       if (!p)
                return;
        btrfs_release_path(p);
        kmem_cache_free(btrfs_path_cachep, p);
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index fbf1f8644fae..fd5474770862 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -85,6 +85,9 @@ struct btrfs_path {
        unsigned int nowait:1;
 };
 
+#define BTRFS_PATH_AUTO_FREE(path_name) \
+       struct btrfs_path *path_name __free(btrfs_free_path) = NULL;
+
 /*
  * The state of btrfs root
  */
@@ -601,8 +604,6 @@ void btrfs_release_path(struct btrfs_path *p);
 struct btrfs_path *btrfs_alloc_path(void);
 void btrfs_free_path(struct btrfs_path *p);
 DEFINE_FREE(btrfs_free_path, struct btrfs_path *, btrfs_free_path(_T))
-#define BTRFS_PATH_AUTO_FREE(path_name) \
-       struct btrfs_path *path_name __free(btrfs_free_path) = NULL;
 
 int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
                   struct btrfs_path *path, int slot, int nr);
---

So, it's moving defintion of auto free capability next to the structure, while
keeping the DEFINE_FREE next to the function forward declaration, or eventually
definition if needed.