| Message ID | cover.1723245033.git.loemra.dev@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: add __free attribute and improve xattr cleanup | expand |
On Fri, Aug 09, 2024 at 04:11:47PM -0700, Leo Martins wrote: > The first patch introduces the __free attribute to the btrfs code, allowing > for automatic memory management of certain variables. This attribute enables > the kernel to automatically call a specified function (in this case, > btrfs_free_path()) on a variable when it goes out of scope, ensuring proper > memory release and preventing potential memory leaks. > > The second patch applies the __free attribute to the path variable in the > btrfs_getxattr(), btrfs_setxattr(), and btrfs_listxattr() functions, ensuring > that the memory allocated for this variable is properly released when it goes > out of scope. This improves the memory management of xattr operations in > btrfs, reducing the risk of memory-related bugs and improving overall system > stability. > > As a next step, I want to extend the use of the __free attribute to other > instances where btrfs_free_path is being manually called. Hold on. Adding the automatic memory management can be done but in the example patches you sent it's IMHO making things worse on the code level. The btrfs_free_path (or btrfs_release_path for that matter) are not simple free helpers but also part of the b-tree locking primitives, pairing with btrfs_search_slot and nontrivial semantics depending on the various setting flags. Dropping the explicit marker from the code is obscuring where the locked section is. Another problem is that this will make any backports less obviously correct from releases that use the __free attribue to older kernels. In the second patch in btrfs_setxattr() you removed btrfs_free_path() but there's still some code after that. In this case it's harmless and only slightly extending the section covered by path, ie. just by a few instructions, but this won't be always possible. In some cases the placement of freeing the path unlocks the tree so it has a strong reason to be there. Overall, we could the automatic memory management, although for kernel, for me, it's on the same level as trying to use other fancy C++ features. We could start using __free in new structures so it's used consistently from the beginning and not mixing two styles namely when not all instances of btrfs_path can use it. In justified cases the auto freeing may make sense but not at the cost of making the code confusing about the pairing free or extending the locked section unnecessarily. The btrfs_path is not a good example where to start with that.
On Tue, 13 Aug 2024 14:29, David Sterba <dsterba@suse.cz> wrote: >On Fri, Aug 09, 2024 at 04:11:47PM -0700, Leo Martins wrote: >> The first patch introduces the __free attribute to the btrfs code, allowing >> for automatic memory management of certain variables. This attribute enables >> the kernel to automatically call a specified function (in this case, >> btrfs_free_path()) on a variable when it goes out of scope, ensuring proper >> memory release and preventing potential memory leaks. >> >> The second patch applies the __free attribute to the path variable in the >> btrfs_getxattr(), btrfs_setxattr(), and btrfs_listxattr() functions, ensuring >> that the memory allocated for this variable is properly released when it goes >> out of scope. This improves the memory management of xattr operations in >> btrfs, reducing the risk of memory-related bugs and improving overall system >> stability. >> >> As a next step, I want to extend the use of the __free attribute to other >> instances where btrfs_free_path is being manually called. > >Hold on. Adding the automatic memory management can be done but in the >example patches you sent it's IMHO making things worse on the code >level. > >The btrfs_free_path (or btrfs_release_path for that matter) are not >simple free helpers but also part of the b-tree locking primitives, >pairing with btrfs_search_slot and nontrivial semantics depending on the >various setting flags. > >Dropping the explicit marker from the code is obscuring where the >locked section is. > >Another problem is that this will make any backports less obviously >correct from releases that use the __free attribue to older kernels. > >In the second patch in btrfs_setxattr() you removed btrfs_free_path() >but there's still some code after that. In this case it's harmless and >only slightly extending the section covered by path, ie. just by a few >instructions, but this won't be always possible. > >In some cases the placement of freeing the path unlocks the tree so it >has a strong reason to be there. > >Overall, we could the automatic memory management, although for kernel, >for me, it's on the same level as trying to use other fancy C++ >features. We could start using __free in new structures so it's used >consistently from the beginning and not mixing two styles namely when >not all instances of btrfs_path can use it. > >In justified cases the auto freeing may make sense but not at the cost >of making the code confusing about the pairing free or extending the >locked section unnecessarily. The btrfs_path is not a good example where >to start with that. This makes sense, I will drop the xattr patch. Do you think there would be any benefit in using the __free pattern in situations where it is clear that btrfs_free_path is the last thing called before returning? For example: int btrfs_del_orphan_item(struct btrfs_trans_handle *trans, struct btrfs_root *root, u64 offset) { struct btrfs_path *path; struct btrfs_key key; int ret = 0; key.objectid = BTRFS_ORPHAN_OBJECTID; key.type = BTRFS_ORPHAN_ITEM_KEY; key.offset = offset; path = btrfs_alloc_path(); if (!path) return -ENOMEM; ret = btrfs_search_slot(trans, root, &key, path, -1, 1); if (ret < 0) goto out; if (ret) { /* JDM: Really? */ ret = -ENOENT; goto out; } ret = btrfs_del_item(trans, root, path); out: btrfs_free_path(path); return ret; } In this code the behavior would be the same except it would eliminate the need for goto out as the path is freed automatically on exit.
On Thu, Aug 15, 2024 at 10:38:24AM -0700, Leo Martins wrote: > On Tue, 13 Aug 2024 14:29, David Sterba <dsterba@suse.cz> wrote: > >On Fri, Aug 09, 2024 at 04:11:47PM -0700, Leo Martins wrote: > This makes sense, I will drop the xattr patch. Do you think there would > be any benefit in using the __free pattern in situations where it > is clear that btrfs_free_path is the last thing called before returning? > For example: > > > int btrfs_del_orphan_item(struct btrfs_trans_handle *trans, > struct btrfs_root *root, u64 offset) > { > struct btrfs_path *path; > struct btrfs_key key; > int ret = 0; > > key.objectid = BTRFS_ORPHAN_OBJECTID; > key.type = BTRFS_ORPHAN_ITEM_KEY; > key.offset = offset; > > path = btrfs_alloc_path(); > if (!path) > return -ENOMEM; > > ret = btrfs_search_slot(trans, root, &key, path, -1, 1); > if (ret < 0) > goto out; > if (ret) { /* JDM: Really? */ > ret = -ENOENT; > goto out; > } > > ret = btrfs_del_item(trans, root, path); > > out: > btrfs_free_path(path); > return ret; > } > > > In this code the behavior would be the same except it would eliminate > the need for goto out as the path is freed automatically on exit. Yes, this is where I coudl be used, basically it's a pattern where the lock/allocation is done early at the beginning of a function (after some initial checks) and then right before a return and all exit paths lead there. For that I'd suggest to make it clear that the function uses the automatic unlock/deletion so the declaration of the variable would be done like BTRFS_PATH_AUTOCLEAN(name); that declares it with the proper __free callback and initializes it to NULL. There's another thing that's a common pattern in btrfs and other kernel, code, the single exit block. The __free callback allows to do a return anywhere which is the opposite of that. As this is new we should look up good examples that will be the patterns to follow or exceptions to avoidd so we can declare it current best practice and recommended coding style.
The first patch introduces the __free attribute to the btrfs code, allowing for automatic memory management of certain variables. This attribute enables the kernel to automatically call a specified function (in this case, btrfs_free_path()) on a variable when it goes out of scope, ensuring proper memory release and preventing potential memory leaks. The second patch applies the __free attribute to the path variable in the btrfs_getxattr(), btrfs_setxattr(), and btrfs_listxattr() functions, ensuring that the memory allocated for this variable is properly released when it goes out of scope. This improves the memory management of xattr operations in btrfs, reducing the risk of memory-related bugs and improving overall system stability. As a next step, I want to extend the use of the __free attribute to other instances where btrfs_free_path is being manually called. Leo Martins (2): btrfs: use __free in linux/cleanup.h to reduce btrfs_free_path boilerplate btrfs: use __free to automatically free btrfs_path on exit fs/btrfs/ctree.c | 3 ++- fs/btrfs/ctree.h | 1 + fs/btrfs/xattr.c | 28 ++++++++-------------------- 3 files changed, 11 insertions(+), 21 deletions(-)