Re: [RFC][CFT][PATCH] Rewrite of propagate_umount() (was Re: [BUG] propagate_umount() breakage)

[Linus Torvalds <torvalds@linux-foundation.org>]

;


On Thu, 15 May 2025 at 22:21, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> With some cleaning the notes up, see
>
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git propagate_umount

Delayed response because this isn't really my area, and it all
somewhat confuses me. Eric - mind taking a look?

But the new code certainly looks sensible, even if I can't really
claim I understand it.

The one reaction I had was almost entirely syntactic - I wish it
didn't use those global lists.

Yes, yes, they are protected by global locks, and that part is
unavoidable, but I'm looking at that code that gathers up the umount
lists, and I actually find the data flow a bit confusing just because
it ends up hiding the state "elsewhere" (that being the global lists).

It just makes the state propagation from gather_candidates() -> users a bit odd.

Not _new_ odd, mind you, but when I was looking at your patch with

    git show --new FETCH_HEAD fs/

I had to go back and forth in the patch just to see where the source
of some the lists came from.

So I think it would make things a bit more explicit if you did something like

    struct umount_state {
        struct list_head umount,candidates;
    };

and made that a local variable in propagate_umount() with

    struct umount_state state = {
            .umount = LIST_HEAD_INIT(state.umount),
            .candidates = LIST_HEAD_INIT(state.candidates),
    };

and then passed that state pointer along an argument.

Although now that I write out that initializer from hell, I am
certainly not impressed by the clarity of that.

So maybe my reaction is bogus. It's certainly not the important part
of the patch.

Also, I realize that I'm only listing your new state globals. The
above is equally true of the existing state that is also done that
way, and your two new lists are actually less confusing than some of
the old things that I think should *also* be part of that umount
propagation state.

(The one I look at in particular is the imaginatively named "list" variable:

    static struct hlist_head *list;

which really is the least descriptive name ever).

Another thing that is either purely syntactic, or shows that I
*really* don't understand your patch. Why do you do this odd thing:

        // reduce the set until it's non-shifting
        for (m = first_candidate(); m; m = trim_one(m))
                ;

which seems to just walk the candidates list in a very non-obvious
manner (this is one of those "I had to go back and forth to see what
first_candidate() did and what lists it used" cases).

It *seems* to be the same as

        list_for_each_entry_safe(m, tmp, &candidates, mnt_umounting)
                trim_one(m);

because if I read that code right, 'trim_one()' will just always
return the next entry in that candidate list.

But again - I might be missing something important and maybe that list
gets modified in other ways while trimming (again, with those globals
it's kind of hard to see the data flow).

So I may have just misread this *completely*.

                   Linus