| Message ID | 20220407153855.21089-1-gniebler@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Turn delayed_nodes_tree into XArray and adjust usages | expand |
On Thu, Apr 07, 2022 at 05:38:48PM +0200, Gabriel Niebler wrote: > XArrays offer a somewhat nicer API than radix trees and were implemented > specifically to replace the latter. They utilize the exact same underlying > data structure, but their API is notionally easier to use, as it provides > array semantics to the user of radix trees. The higher level API also > takes care of locking, adding even more ease of use. > > The btrfs code uses radix trees directly in several places, such as for the > `delayed_nodes_radix` member of the btrfs_root struct. > > This patchset converts `delayed_nodes_radix` into an XArray, renames it > accordingly and adjusts all usages of this object to the XArray API. > It survived a complete fstests run. So it converts just one structure, it would be better do it in one patch otherwise it leaves the conversion half way and it's confusing to see xarray structure in the radix-tree API. > Gabriel Niebler (6): > Turn delayed_nodes_tree into delayed_nodes_xarray in btrfs_root > btrfs_get_delayed_node: convert to using XArray API > btrfs_get_or_create_delayed_node: convert to using XArray API > __btrfs_release_delayed_node: convert to using XArray API > btrfs_kill_all_delayed_nodes: convert to using XArray API > __setup_root: convert to using XArray API for delayed_nodes_xarray The subject(s) should start with "btrfs: ..."
Am 07.04.22 um 18:44 schrieb David Sterba: > On Thu, Apr 07, 2022 at 05:38:48PM +0200, Gabriel Niebler wrote: >> [...] >> This patchset converts `delayed_nodes_radix` into an XArray, renames it >> accordingly and adjusts all usages of this object to the XArray API. >> It survived a complete fstests run. > > So it converts just one structure, it would be better do it in one > patch otherwise it leaves the conversion half way and it's confusing to > see xarray structure in the radix-tree API. Yes, I figured that converting each structure separately is easier to achieve for me, since the changes in this patch set are done, but changes to other structures are not (yet). As for splitting the changes, as I did: My thought was that it's easier to review this way, patch-by-patch and that the important thing is that the conversion be done by the end of the patch set. But I do see the point that - even in a patch set - each patch should stand on its own and that does leave the code in an inconsistent (albeit working) state. Folding it all into one commit is not a problem and I will do that for resubmission. > >> Gabriel Niebler (6): >> Turn delayed_nodes_tree into delayed_nodes_xarray in btrfs_root >> btrfs_get_delayed_node: convert to using XArray API >> btrfs_get_or_create_delayed_node: convert to using XArray API >> __btrfs_release_delayed_node: convert to using XArray API >> btrfs_kill_all_delayed_nodes: convert to using XArray API >> __setup_root: convert to using XArray API for delayed_nodes_xarray > > The subject(s) should start with "btrfs: ..." > OK, will fix. I will also take the suggestions from the response to patch 1/6 on board. Best, gabe
On Mon, Apr 11, 2022 at 09:41:54AM +0200, Gabriel Niebler wrote: > Am 07.04.22 um 18:44 schrieb David Sterba: > > On Thu, Apr 07, 2022 at 05:38:48PM +0200, Gabriel Niebler wrote: > >> [...] > >> This patchset converts `delayed_nodes_radix` into an XArray, renames it > >> accordingly and adjusts all usages of this object to the XArray API. > >> It survived a complete fstests run. > > > > So it converts just one structure, it would be better do it in one > > patch otherwise it leaves the conversion half way and it's confusing to > > see xarray structure in the radix-tree API. > > Yes, I figured that converting each structure separately is easier to > achieve for me, since the changes in this patch set are done, but > changes to other structures are not (yet). > > As for splitting the changes, as I did: My thought was that it's easier > to review this way, patch-by-patch and that the important thing is that > the conversion be done by the end of the patch set. The change splitting should be on the logical level and with focus on a selected context, but there's no universal advice. In this case the scope is one structure per patch, all actions with the structure are the context that often remains same for all the API calls, splitting that per patch does not help the review. Switching all structures in one patch would mean that each call site is using a different structure so the context is changing too often and that's adding to the congnitive load. Which leads to oversights and review quality goes down.