| Message ID | cover.1724792563.git.loemra.dev@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs path auto free | expand |
On Tue, Aug 27, 2024 at 03:41:30PM -0700, Leo Martins wrote: > The DEFINE_FREE macro defines a wrapper function for a given memory > cleanup function which takes a pointer as an argument and calls the > cleanup function with the value of the pointer. The __free macro adds > a scoped-based cleanup to a variable, using the __cleanup attribute > to specify the cleanup function that should be called when the variable > goes out of scope. > > Using this cleanup code pattern ensures that memory is properly freed > when it's no longer needed, preventing memory leaks and reducing the > risk of crashes or other issues caused by incorrect memory management. > Even if the code is already memory safe, using this pattern reduces > the risk of introducing memory-related bugs in the future > > In this series of patches I've added a DEFINE_FREE for btrfs_free_path > and created a macro BTRFS_PATH_AUTO_FREE to clearly identify path > declarations that will be automatically freed. > > I've included some simple examples of where this pattern can be used. > The trivial examples are ones where there is one exit path and the only > cleanup performed is a call to btrfs_free_path. > > There appear to be around 130 instances that would be a pretty simple to > convert to this pattern. I'm not sure what exactly are you counting, the number seems too much. > Is it worth going back and updating > all trivial instances or would it be better to leave them and use the pattern > in new code? Using it in new code sounds like a better option for the start so we don't cause too much churn in code that hasn't otherwise changed for a long time. > Another option is to have all path declarations declared > with BTRFS_PATH_AUTO_FREE and not remove any btrfs_free_path instances. > In theory this would not change the functionality as it is fine to call > btrfs_free_path on an already freed path. That would require adding path = NULL after each btrfs_free_path() so the auto free does not free it twice. Changing all instances of path declarations may not be feasible, the pattern of path freeing right before return is not that widespread, sometimes mixed with btrfs_release_path. I've sent some comments for v2 that I don't see fixed in v3. Also please for patchset iterations write a short list of changes between.
I think the only feedback I haven't addressed in this patchset was moving the declaration to be next to the btrfs_path struct. However, I don't think moving the declaration makes sense because the DEFINE_FREE references btrfs_free_path which itself is only declared after the structure. The other examples I've looked at also have DEFINE_FREE next to the freeing function as opposed to the structure being freed. On Tue, 27 Aug 2024 15:41, Leo Martins <loemra.dev@gmail.com> wrote: >The DEFINE_FREE macro defines a wrapper function for a given memory >cleanup function which takes a pointer as an argument and calls the >cleanup function with the value of the pointer. The __free macro adds >a scoped-based cleanup to a variable, using the __cleanup attribute >to specify the cleanup function that should be called when the variable >goes out of scope. > >Using this cleanup code pattern ensures that memory is properly freed >when it's no longer needed, preventing memory leaks and reducing the >risk of crashes or other issues caused by incorrect memory management. >Even if the code is already memory safe, using this pattern reduces >the risk of introducing memory-related bugs in the future > >In this series of patches I've added a DEFINE_FREE for btrfs_free_path >and created a macro BTRFS_PATH_AUTO_FREE to clearly identify path >declarations that will be automatically freed. > >I've included some simple examples of where this pattern can be used. >The trivial examples are ones where there is one exit path and the only >cleanup performed is a call to btrfs_free_path. > >There appear to be around 130 instances that would be a pretty simple to >convert to this pattern. Is it worth going back and updating >all trivial instances or would it be better to leave them and use the pattern >in new code? Another option is to have all path declarations declared >with BTRFS_PATH_AUTO_FREE and not remove any btrfs_free_path instances. >In theory this would not change the functionality as it is fine to call >btrfs_free_path on an already freed path. > >Leo Martins (3): > btrfs: DEFINE_FREE for btrfs_free_path > btrfs: BTRFS_PATH_AUTO_FREE in zoned.c > btrfs: BTRFS_PATH_AUTO_FREE in orphan.c > > fs/btrfs/ctree.c | 2 +- > fs/btrfs/ctree.h | 4 ++++ > fs/btrfs/orphan.c | 19 ++++++------------- > fs/btrfs/zoned.c | 34 +++++++++++----------------------- > 4 files changed, 22 insertions(+), 37 deletions(-) > >-- >2.43.5 >
On Fri, Aug 30, 2024 at 01:46:59PM -0700, Leo Martins wrote: > I think the only feedback I haven't addressed in this patchset was > moving the declaration to be next to the btrfs_path struct. > However, I don't think moving the declaration makes sense because > the DEFINE_FREE references btrfs_free_path which itself is only > declared after the structure. As long as the definition of btrfs_free_path() is not needed to compile, because it's a macro, I'd really want to keep the auto freeing defininion next to the structure because it's closely related to the structure. So if I ever go to the definition of any structure I want to see right away if it does or does not support the auto free. > The other examples I've looked at also > have DEFINE_FREE next to the freeing function as opposed to the > structure being freed.
On Tue, Sep 03, 2024 at 01:43:46AM +0200, David Sterba wrote: > On Fri, Aug 30, 2024 at 01:46:59PM -0700, Leo Martins wrote: > > I think the only feedback I haven't addressed in this patchset was > > moving the declaration to be next to the btrfs_path struct. > > However, I don't think moving the declaration makes sense because > > the DEFINE_FREE references btrfs_free_path which itself is only > > declared after the structure. > > As long as the definition of btrfs_free_path() is not needed to > compile, because it's a macro, I'd really want to keep the auto freeing > defininion next to the structure because it's closely related to the > structure. So if I ever go to the definition of any structure I want to > see right away if it does or does not support the auto free. This diff demonstrates the idea: --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -196,7 +196,7 @@ struct btrfs_path *btrfs_alloc_path(void) /* this also releases the path */ void btrfs_free_path(struct btrfs_path *p) { - if (IS_ERR_OR_NULL(p)) + if (!p) return; btrfs_release_path(p); kmem_cache_free(btrfs_path_cachep, p); diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index fbf1f8644fae..fd5474770862 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -85,6 +85,9 @@ struct btrfs_path { unsigned int nowait:1; }; +#define BTRFS_PATH_AUTO_FREE(path_name) \ + struct btrfs_path *path_name __free(btrfs_free_path) = NULL; + /* * The state of btrfs root */ @@ -601,8 +604,6 @@ void btrfs_release_path(struct btrfs_path *p); struct btrfs_path *btrfs_alloc_path(void); void btrfs_free_path(struct btrfs_path *p); DEFINE_FREE(btrfs_free_path, struct btrfs_path *, btrfs_free_path(_T)) -#define BTRFS_PATH_AUTO_FREE(path_name) \ - struct btrfs_path *path_name __free(btrfs_free_path) = NULL; int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root, struct btrfs_path *path, int slot, int nr); --- So, it's moving defintion of auto free capability next to the structure, while keeping the DEFINE_FREE next to the function forward declaration, or eventually definition if needed.