[PATCH v2 0/3] btrfs path auto free

[PATCH v2 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 v2 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.
---
 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..babc86af564a2 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 *, if (!IS_ERR_OR_NULL(_T)) 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 v2 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 the only cleanup is btrfs_free_path.
---
 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 v2 3/3] btrfs: BTRFS_PATH_AUTO_FREE in orphan.c

[Leo Martins]

Add automatic path freeing in orphan.c. This is the original example I
sent with only one exit point that just frees the path.
---
 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 v2 1/3] btrfs: DEFINE_FREE for btrfs_free_path

[Josef Bacik]

On Tue, Aug 27, 2024 at 12:08:43PM -0700, Leo Martins wrote:
> 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.
> ---
>  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..babc86af564a2 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 *, if (!IS_ERR_OR_NULL(_T)) btrfs_free_path(_T))

Remember to run "git commit -s" so you get your signed-off-by automatically
added.

You don't need the extra IS_ERR_OR_NULL bit if the free has it, so you can keep
the change above and just have btrfs_free_path(_T) here.  Thanks,

Josef

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

[Josef Bacik]

On Tue, Aug 27, 2024 at 12:08:45PM -0700, Leo Martins wrote:
> Add automatic path freeing in orphan.c. This is the original example I
> sent with only one exit point that just frees the path.

We don't have context to what your other postings were when we're looking
through changelogs, simply write

Add automatic path freeing in orphan.c.  We only allocate a path twice in this
file and they're both simple with a single exit point.

Or something like that.  Thanks,

Josef

Re: [PATCH v2 1/3] btrfs: DEFINE_FREE for btrfs_free_path

[David Sterba]

On Tue, Aug 27, 2024 at 04:30:58PM -0400, Josef Bacik wrote:
> On Tue, Aug 27, 2024 at 12:08:43PM -0700, Leo Martins wrote:
> > 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.
> > ---
> >  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..babc86af564a2 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 *, if (!IS_ERR_OR_NULL(_T)) btrfs_free_path(_T))
> 
> Remember to run "git commit -s" so you get your signed-off-by automatically
> added.
> 
> You don't need the extra IS_ERR_OR_NULL bit if the free has it, so you can keep
> the change above and just have btrfs_free_path(_T) here.  Thanks,

The pattern I'd suggest is to keep the special NULL checks in the
functions so it's obvious that it's done, the macro wrapping the cleanup
functil will be a simple call to the freeing function.

Re: [PATCH v2 1/3] btrfs: DEFINE_FREE for btrfs_free_path

[David Sterba]

On Tue, Aug 27, 2024 at 12:08:43PM -0700, Leo Martins wrote:
> 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.
> ---
>  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..babc86af564a2 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 *, if (!IS_ERR_OR_NULL(_T)) btrfs_free_path(_T))
> +#define BTRFS_PATH_AUTO_FREE(path_name) \
> +	struct btrfs_path *path_name __free(btrfs_free_path) = NULL;

This would be better defined next to the structure, btrfs_path in this
case, so we know which ones already have it.

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

[David Sterba]

On Tue, Aug 27, 2024 at 12:08:44PM -0700, Leo Martins wrote:
> Add automatic path freeing in zoned.c, the examples here are both fairly
> trivial with one exit path and the only cleanup is btrfs_free_path.
> ---
>  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);

This looks good, I've tested that applied in the code and looked at the
function how it feels in the code, I think we can settle on that.

So if you're going to send more such updates, you can use a template
changelog like:

"All cleanup paths lead to btrfs_path_free so we can define path with the
 automatic free callback."

It describes why it can be used and roughly what it does if somebody
sees it for the first time.

Re: [PATCH v2 1/3] btrfs: DEFINE_FREE for btrfs_free_path

[Boris Burkov]

On Wed, Aug 28, 2024 at 02:16:01AM +0200, David Sterba wrote:
> On Tue, Aug 27, 2024 at 04:30:58PM -0400, Josef Bacik wrote:
> > On Tue, Aug 27, 2024 at 12:08:43PM -0700, Leo Martins wrote:
> > > 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.
> > > ---
> > >  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..babc86af564a2 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 *, if (!IS_ERR_OR_NULL(_T)) btrfs_free_path(_T))
> > 
> > Remember to run "git commit -s" so you get your signed-off-by automatically
> > added.
> > 
> > You don't need the extra IS_ERR_OR_NULL bit if the free has it, so you can keep
> > the change above and just have btrfs_free_path(_T) here.  Thanks,
> 
> The pattern I'd suggest is to keep the special NULL checks in the
> functions so it's obvious that it's done, the macro wrapping the cleanup
> functil will be a simple call to the freeing function.

This pattern came from the cleanup.h documentation:
https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L11

As far as I can tell, it will only be relevant if we end up using the
'return_ptr' functionality in the cleanup library, which seems
relatively unlikely for btrfs_path.

But there is also not much harm in using the common documented pattern,
and seeds future uses in btrfs that are likely to copy this one. For
example, if we do it for allocating something we do have a "factory"
function for.

Thanks,
Boris

Re: [PATCH v2 1/3] btrfs: DEFINE_FREE for btrfs_free_path

[David Sterba]

On Wed, Aug 28, 2024 at 10:09:12AM -0700, Boris Burkov wrote:
> On Wed, Aug 28, 2024 at 02:16:01AM +0200, David Sterba wrote:
> > On Tue, Aug 27, 2024 at 04:30:58PM -0400, Josef Bacik wrote:
> > > On Tue, Aug 27, 2024 at 12:08:43PM -0700, Leo Martins wrote:
> > > > 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.
> > > > ---
> > > >  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..babc86af564a2 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 *, if (!IS_ERR_OR_NULL(_T)) btrfs_free_path(_T))
> > > 
> > > Remember to run "git commit -s" so you get your signed-off-by automatically
> > > added.
> > > 
> > > You don't need the extra IS_ERR_OR_NULL bit if the free has it, so you can keep
> > > the change above and just have btrfs_free_path(_T) here.  Thanks,
> > 
> > The pattern I'd suggest is to keep the special NULL checks in the
> > functions so it's obvious that it's done, the macro wrapping the cleanup
> > functil will be a simple call to the freeing function.
> 
> This pattern came from the cleanup.h documentation:
> https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L11

[...] @free should typically include a NULL test before calling a function

Typically yes, but we don't need to do it twice.

> As far as I can tell, it will only be relevant if we end up using the
> 'return_ptr' functionality in the cleanup library, which seems
> relatively unlikely for btrfs_path.

So return_ptr undoes __free, in that case we shouldn't use it at all,
the macros obscure what the code does, this is IMHO taking it too far.

Re: [PATCH v2 1/3] btrfs: DEFINE_FREE for btrfs_free_path

[Boris Burkov]

On Wed, Aug 28, 2024 at 07:54:19PM +0200, David Sterba wrote:
> On Wed, Aug 28, 2024 at 10:09:12AM -0700, Boris Burkov wrote:
> > On Wed, Aug 28, 2024 at 02:16:01AM +0200, David Sterba wrote:
> > > On Tue, Aug 27, 2024 at 04:30:58PM -0400, Josef Bacik wrote:
> > > > On Tue, Aug 27, 2024 at 12:08:43PM -0700, Leo Martins wrote:
> > > > > 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.
> > > > > ---
> > > > >  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..babc86af564a2 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 *, if (!IS_ERR_OR_NULL(_T)) btrfs_free_path(_T))
> > > > 
> > > > Remember to run "git commit -s" so you get your signed-off-by automatically
> > > > added.
> > > > 
> > > > You don't need the extra IS_ERR_OR_NULL bit if the free has it, so you can keep
> > > > the change above and just have btrfs_free_path(_T) here.  Thanks,
> > > 
> > > The pattern I'd suggest is to keep the special NULL checks in the
> > > functions so it's obvious that it's done, the macro wrapping the cleanup
> > > functil will be a simple call to the freeing function.
> > 
> > This pattern came from the cleanup.h documentation:
> > https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L11
> 
> [...] @free should typically include a NULL test before calling a function
> 
> Typically yes, but we don't need to do it twice.
> 

I believe we do if we want to get the desired compiler behavior in the
release case. Whether or not the resource-freeing function we call
checks NULL is not relevant to the point of checking it here in the
macro.

> > As far as I can tell, it will only be relevant if we end up using the
> > 'return_ptr' functionality in the cleanup library, which seems
> > relatively unlikely for btrfs_path.
> 
> So return_ptr undoes __free, in that case we shouldn't use it at all,
> the macros obscure what the code does, this is IMHO taking it too far.

This may well be taking it too far, but it is a common and valid
pattern of RAII: auto freeing the half-initialized parts of structure
automatically on the error exit paths in the initialization, while
releasing the local cleanup responsibility on success.

Look at their alloc_obj example again:
https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L31
and the explanation that acknowledges kfree handles NULL:
https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L43

Suppose we were initializing some object that owned a path (and cleaned
it up itself on death), then initializing that object we would create a
__free owned path (to cleanup on every error path), but then once we were
sure we were done, we would release the auto free and let the owning object
take over before returning it to the caller. Freeing the path in this case
would be a bug, as the owning object would have it freed under it.

That's almost certainly nonsense for btrfs_path and will never happen,
but it isn't in general, so if we do add a __free, it makes sense to me
to do it by the book. If we really want to avoid this double check, then
we should add a comment saying that btrfs_path will never be released,
so it doesn't make sense to support that pattern.

Re: [PATCH v2 1/3] btrfs: DEFINE_FREE for btrfs_free_path

[David Sterba]

On Wed, Aug 28, 2024 at 11:54:42AM -0700, Boris Burkov wrote:
> > > This pattern came from the cleanup.h documentation:
> > > https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L11
> > 
> > [...] @free should typically include a NULL test before calling a function
> > 
> > Typically yes, but we don't need to do it twice.
> 
> I believe we do if we want to get the desired compiler behavior in the
> release case. Whether or not the resource-freeing function we call
> checks NULL is not relevant to the point of checking it here in the
> macro.

I'm trying to understand why we're discussing that, maybe I'm missing
some aspect that makes it important to stick to the recommended use.
I've been reading the macros and looking for potential use, from that
POV no "if(NULL)" check is needed when it's done in the freeing
function.

There will be few cases that we will review when using the macros and
then we can forget about the details and it will work.

> > > As far as I can tell, it will only be relevant if we end up using the
> > > 'return_ptr' functionality in the cleanup library, which seems
> > > relatively unlikely for btrfs_path.
> > 
> > So return_ptr undoes __free, in that case we shouldn't use it at all,
> > the macros obscure what the code does, this is IMHO taking it too far.
> 
> This may well be taking it too far, but it is a common and valid
> pattern of RAII: auto freeing the half-initialized parts of structure
> automatically on the error exit paths in the initialization, while
> releasing the local cleanup responsibility on success.
> 
> Look at their alloc_obj example again:
> https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L31
> and the explanation that acknowledges kfree handles NULL:
> https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L43
> 
> Suppose we were initializing some object that owned a path (and cleaned
> it up itself on death), then initializing that object we would create a
> __free owned path (to cleanup on every error path), but then once we were
> sure we were done, we would release the auto free and let the owning object
> take over before returning it to the caller. Freeing the path in this case
> would be a bug, as the owning object would have it freed under it.
> 
> That's almost certainly nonsense for btrfs_path and will never happen,
> but it isn't in general,

You got me worried in the previous paragraph, I agree it will never
happen so I'm less inclined to prepare for such cases.

> so if we do add a __free, it makes sense to me
> to do it by the book. If we really want to avoid this double check, then
> we should add a comment saying that btrfs_path will never be released,
> so it doesn't make sense to support that pattern.

Sorry I don't understand this, can you please provide pseudo-code
examples? Why wouldn't be btrfs_path released?

Re: [PATCH v2 1/3] btrfs: DEFINE_FREE for btrfs_free_path

[Boris Burkov]

On Thu, Aug 29, 2024 at 07:36:56PM +0200, David Sterba wrote:
> On Wed, Aug 28, 2024 at 11:54:42AM -0700, Boris Burkov wrote:
> > > > This pattern came from the cleanup.h documentation:
> > > > https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L11
> > > 
> > > [...] @free should typically include a NULL test before calling a function
> > > 
> > > Typically yes, but we don't need to do it twice.
> > 
> > I believe we do if we want to get the desired compiler behavior in the
> > release case. Whether or not the resource-freeing function we call
> > checks NULL is not relevant to the point of checking it here in the
> > macro.
> 
> I'm trying to understand why we're discussing that, maybe I'm missing
> some aspect that makes it important to stick to the recommended use.
> I've been reading the macros and looking for potential use, from that
> POV no "if(NULL)" check is needed when it's done in the freeing
> function.

For the record, I I agree that there is no risk for ever doing something
bad to a btrfs_path, no matter what we do with this extra check in
DEFINE_FREE

> 
> There will be few cases that we will review when using the macros and
> then we can forget about the details and it will work.
> 
> > > > As far as I can tell, it will only be relevant if we end up using the
> > > > 'return_ptr' functionality in the cleanup library, which seems
> > > > relatively unlikely for btrfs_path.
> > > 
> > > So return_ptr undoes __free, in that case we shouldn't use it at all,
> > > the macros obscure what the code does, this is IMHO taking it too far.
> > 
> > This may well be taking it too far, but it is a common and valid
> > pattern of RAII: auto freeing the half-initialized parts of structure
> > automatically on the error exit paths in the initialization, while
> > releasing the local cleanup responsibility on success.
> > 
> > Look at their alloc_obj example again:
> > https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L31
> > and the explanation that acknowledges kfree handles NULL:
> > https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L43
> > 
> > Suppose we were initializing some object that owned a path (and cleaned
> > it up itself on death), then initializing that object we would create a
> > __free owned path (to cleanup on every error path), but then once we were
> > sure we were done, we would release the auto free and let the owning object
> > take over before returning it to the caller. Freeing the path in this case
> > would be a bug, as the owning object would have it freed under it.
> > 
> > That's almost certainly nonsense for btrfs_path and will never happen,
> > but it isn't in general,
> 
> You got me worried in the previous paragraph, I agree it will never
> happen so I'm less inclined to prepare for such cases.
> 

That is fair enough, and I have no problem with that judgement.

> > so if we do add a __free, it makes sense to me
> > to do it by the book. If we really want to avoid this double check, then
> > we should add a comment saying that btrfs_path will never be released,
> > so it doesn't make sense to support that pattern.
> 
> Sorry I don't understand this, can you please provide pseudo-code
> examples? Why wouldn't be btrfs_path released?

I think this is just me not having a good terminology for "we used
return_ptr or no_free_ptr". Perhaps "gave up ownership" or "gave up
responsibility" or "canceled free" as "released" is too similar to the
actual __free action :)

I don't have any pseudocode materially different from the example in
cleanup.h with "btrfs_path" substituted in.

All I'm trying to accomplish in this whole discussion is emphasize that
the extra IS_ERR_OR_NULL check is not about ensuring that btrfs_path is
valid, but is about following the best practice for supporting the code
reduction in the "gave up ownership" happy path. In fact, just "if (_T)"
would be sufficient in Leo's DEFINE_FREE, for that reason.

My taste preference here is to explicitly acknowledge that we plan to
never give up ownership of an auto-free btrfs_path, and thus document
that we are intentionally not including the extra NULL check. Since the
authors of the library bothered to explicitly call it out as a pattern
users should follow.

Thanks for entertaining this discussion, I enjoyed learning more about
cleanup.h.

Boris

Re: [PATCH v2 1/3] btrfs: DEFINE_FREE for btrfs_free_path

[David Sterba]

On Thu, Aug 29, 2024 at 11:40:41AM -0700, Boris Burkov wrote:
> > > so if we do add a __free, it makes sense to me
> > > to do it by the book. If we really want to avoid this double check, then
> > > we should add a comment saying that btrfs_path will never be released,
> > > so it doesn't make sense to support that pattern.
> > 
> > Sorry I don't understand this, can you please provide pseudo-code
> > examples? Why wouldn't be btrfs_path released?
> 
> I think this is just me not having a good terminology for "we used
> return_ptr or no_free_ptr". Perhaps "gave up ownership" or "gave up
> responsibility" or "canceled free" as "released" is too similar to the
> actual __free action :)
> 
> I don't have any pseudocode materially different from the example in
> cleanup.h with "btrfs_path" substituted in.
> 
> All I'm trying to accomplish in this whole discussion is emphasize that
> the extra IS_ERR_OR_NULL check is not about ensuring that btrfs_path is
> valid, but is about following the best practice for supporting the code
> reduction in the "gave up ownership" happy path. In fact, just "if (_T)"
> would be sufficient in Leo's DEFINE_FREE, for that reason.

I guess I'm reading that with my "counting instructions and branch
prediction efficiency" hat on, if there's such thing. I understand
following best practice, OTOH I can't simply ignore the practical side
of the implementation.

The ERR_PTR will never be stored to path, or there may be counter
examples. What I remember is that path is often allocated at the
beginning of the function so it usually returns -ENOMEM before anything
happens. Also path is I think never a return value but a separate
variable so mixing ERR_PTR does not happen. Please provide examples
where this does not happen, this is something that can cause problems
but I'd rather get rid of that than to take it as an eventual case to
handle.

> My taste preference here is to explicitly acknowledge that we plan to
> never give up ownership of an auto-free btrfs_path, and thus document
> that we are intentionally not including the extra NULL check. Since the
> authors of the library bothered to explicitly call it out as a pattern
> users should follow.
> 
> Thanks for entertaining this discussion, I enjoyed learning more about
> cleanup.h.

Same here, thanks. It's a new pattern and it may help us to simplify the
code, but I'd really want to avoid cases where we bend the code just to
fit the convenience macros, at the cost of making the code less obvious
due to the hidden semantics behind definitons or things like
"return_ptr".