Re: [PATCH 0/5] Convert seed devices to proper list API

David Sterba
2020-08-21T14:34:29Z

On Thu, Jul 23, 2020 at 11:02:14AM +0300, Nikolay Borisov wrote:
> Regarding patch 5 - I don't know what made you think there is a
> tree-like structure involved. Simply looking at the old way seeds are
> iterated:
>
> while (seed_devices) {
> fs_devices = seed_devices;
> seed_devices = fs_devices->seed;
> close_fs_devices(fs_devices);
> free_fs_devices(fs_devices);
> }
>
> There's no conditional deciding if we should go left/right of the tree.

A tree structure that has only one direction arrows, from leaves to the
root. Where the root is the seed fs and the leaves are the sprouts.

fs1 ---> seed
^ ^
fs2 -----| |
|
fs3 -------|

The while loop in each fs goes by the pointers up to the seeding device
and always removes it's references.

I'm not sure about a deeper tree structure here, so the fs3 -> fs2 would
be possible.

> Or let's take for example deleting from a list of seed devices:
>
> tmp_fs_devices = fs_info->fs_devices;
> while (tmp_fs_devices) {
> if (tmp_fs_devices->seed == fs_devices) {
> tmp_fs_devices->seed = fs_devices->seed;
> break;
> }
> tmp_fs_devices = tmp_fs_devices->seed;
> }
>
> Here a simple linear search is performed and when a member of the seed
> list matches our fs_devices it simply pointed 1 past our fs_devices
>
> When the if inside the loop matches we get the following situation:
>
> [tmp_fs_devices]->[fs_devices]->[fs_devices->seeds]
>
> Then we perform [tmp_fs_devices] -> [fs_devices->seed]
>
> So a simple deletion, just the fact you were confused shows the old code
> is rather wonky.

Well, I still am after reading the above, the missing part is about the
device cloning and that each fs has it's own copy. The tree structure
implies sharing and would need reference counting, but this does not
seem to be so.

I'll add the series to for-next so we have a base for the other seeding
patches but I haven't reviewed it to the point where I'm convinced it's
correct.