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

[Boris Burkov <boris@bur.io>]

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