lkml.org 
[lkml]   [2021]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a new helper
    On Wed, May 19, 2021 at 11:07:04AM +0300, Andy Shevchenko wrote:
    > On Wed, May 19, 2021 at 12:48:59AM +0000, Al Viro wrote:
    > > ... and leave the rename_lock/mount_lock handling in prepend_path()
    > > itself
    >
    > ...
    >
    > > + if (!IS_ERR_OR_NULL(mnt_ns) && !is_anon_ns(mnt_ns))
    > > + return 1; // absolute root
    > > + else
    > > + return 2; // detached or not attached yet
    >
    > Would it be slightly better to read
    >
    > if (IS_ERR_OR_NULL(mnt_ns) || is_anon_ns(mnt_ns))
    > return 2; // detached or not attached yet
    > else
    > return 1; // absolute root
    >
    > ?
    >
    > Oh, I have noticed that it's in the original piece of code (perhaps separate
    > change if we ever need it?).

    The real readability problem here is not the negations. There are 4 possible
    states for vfsmount encoded via ->mnt_ns:
    1) not attached to any tree, kept alive by refcount alone.
    ->mnt_ns == NULL.
    2) long-term unattached. Not a part of any mount tree, but we have
    a known holder for it and until that's gone (making ->mnt_ns NULL), refcount
    is guaranteed to remain positive. pipe_mnt is an example of such.
    ->mnt_ns == MNT_NS_INTERNAL, which is encoded as ERR_PTR(-1), thus the use of
    IS_ERR_OR_NULL here (something I'd normally taken out and shot - use of that
    primitive is a sign of lousy API or of a cargo-culted "defensive programming").
    3) part of a temporary mount tree; not in anyone's namespace.
    ->mnt_ns points the tree in question, ->mnt_ns->seq == 0.
    4) belongs to someone's namespace. ->mnt_ns points to that,
    ->mnt_ns->seq != 0. That's what we are looking for here.

    It's kludges all the way down ;-/ Note that temporary tree can't become
    a normal one or vice versa - mounts can get transferred to normal namespace,
    but they will see ->mnt_ns reassigned to that. IOW, ->mnt_ns->seq can't
    get changed without a change to ->mnt_ns. I suspect that the right way
    to handle that would be to have that state stored as explicit flags.

    All mounts are created (and destroyed) in state (1); state changes:
    commit_tree() - (1) or (3) to (3) or (4)
    umount_tree() - (3) or (4) to (1)
    clone_private_mount() - (1) to (2)
    open_detached_copy() - (1) to (3)
    copy_mnt_ns() - (1) to (4)
    mount_subtree() - (1) to (3)
    fsmount() - (1) to (3)
    init_mount_tree() - (1) to (4)
    kern_mount() - (1) to (2)
    kern_unmount{,_array}() - (2) to (1)

    commit_tree() has a pathological call chain that has it
    attach stuff to temporary tree; that's basically automount by lookup in
    temporary namespace. It can distinguish it from the usual (adding to
    normal namespace) by looking at the state of mountpoint we are attaching
    to - or simply describe all cases as "(1) or (3) to whatever state the
    mountpoint is".

    One really hot path where we check (1) vs. (2,3,4) is
    mntput_no_expire(), which is the initial reason behind the current
    representation. However, read from ->mnt_flags is just as cheap as
    that from ->mnt_ns and the same reasons that make READ_ONCE()
    legitimate there would apply to ->mnt_flags as well.

    We can't reuse MNT_INTERNAL for that, more's the pity -
    it's used to mark the mounts (kern_mount()-created, mostly) that
    need to destroyed synchronously on the final mntput(), with no
    task_work_add() allowed (think of module_init() failing halfway through,
    with kern_unmount() done to destroy the internal mounts already created;
    we *really* don't want to delay that filesystem shutdown until insmod(2)
    heads out to userland). Another headache is in LSM shite, as usual...

    Anyway, sorting that out is definitely a separate story.

    \
     
     \ /
      Last update: 2021-05-19 17:56    [W:2.986 / U:0.832 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site