| Message ID | 6951e579322f1389bcc02de692a696880edb2a7e.1724785204.git.loemra.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs path auto free | expand |
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
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.
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.
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
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.
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.
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?
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
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 --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);