sdimitro requested changes on this pull request.
Thank you for porting this. I only have one question/suggestion but other than
that it looks good to me.
> for (indirect_split_t *is = list_head(&iv->iv_splits);
- is != NULL; is = list_next(&iv->iv_splits, is))
- segments++;
+ is != NULL; is = list_next(&iv->iv_splits, is)) {
+ uint64_t is_copies = 0;
+
+ for (int i = 0; i < is->is_children; i++) {
I have a question which may lead a small optimization although I may be
completely wrong.
In this loop we are going through each is_child:
If it has no data we skip it. If it has data we are looking at the rest of the
is_child to see if any of them has the same data as our original is_child and
mark them as duplicate, and finally we increment is_copies which we will use to
get the number of combinations.
The above makes sense with the rest of the logic in this function but to me it
sounds that what we call "accurate combinations" is not exactly accurate as the
combinations themselves are not unique.
The variable `is_copies` (that is later used to calculate `combinations`) tells
us how many of the is_children vdevs have their `ic_data` field populated (e.g.
not NULL). So that got me wondering, what if:
[1] we changed the name of `is_copies` to something like `is_split_versions`
[2] in this loop instead of just skipping when `ic_data == NULL` we also skip
when `ic_duplicate != -1` (meaning we visited this `ic_child` before [in the
inner loop] and we know that it has not a unique version of `ic_data`)
[3] Later the outer loop (right after the inner loop is done) we increment
`is_split_versions` and we use it exactly like `is_copies`.
This should give us the number of unique combinations. Now I know that this
won't work exactly with the rest of the logic in this function but what if the
struct `indirect_split_t` had a field `list_t is_unique_versions` (or some
similar name) and every time `is_split_versions` is incremented in this loop we
add the current `is_child` to this list.
Then later in this function we can use this new list without having to care, do
checks, and skip duplicates because they are never visited. Thoughts?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/625#pullrequestreview-113309423
------------------------------------------
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/discussions/Tf29308eb1ca3d052-Me9367b749c94a5faf1071273
Delivery options: https://openzfs.topicbox.com/groups
