diff mbox series

[v2,1/3] btrfs: DEFINE_FREE for btrfs_free_path

Message ID 6951e579322f1389bcc02de692a696880edb2a7e.1724785204.git.loemra.dev@gmail.com (mailing list archive)
State New, archived
Headers show
Series btrfs path auto free | expand

Commit Message

Leo Martins Aug. 27, 2024, 7:08 p.m. UTC
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(-)

Comments

Josef Bacik Aug. 27, 2024, 8:30 p.m. UTC | #1
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
David Sterba Aug. 28, 2024, 12:16 a.m. UTC | #2
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.
David Sterba Aug. 28, 2024, 12:17 a.m. UTC | #3
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.
Boris Burkov Aug. 28, 2024, 5:09 p.m. UTC | #4
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
David Sterba Aug. 28, 2024, 5:54 p.m. UTC | #5
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.
Boris Burkov Aug. 28, 2024, 6:54 p.m. UTC | #6
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.
David Sterba Aug. 29, 2024, 5:36 p.m. UTC | #7
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?
Boris Burkov Aug. 29, 2024, 6:40 p.m. UTC | #8
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
David Sterba Aug. 29, 2024, 11:20 p.m. UTC | #9
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".
diff mbox series

Patch

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);