lkml.org 
[lkml]   [2015]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [git pull] more vfs bits
    From
    On Sat, Feb 21, 2015 at 4:18 PM, David Howells <dhowells@redhat.com> wrote:
    >
    > So what I want to do is:
    >
    > (1) Introduce wrappers around accesses to ->d_inode. These can be used to
    > mark such accesses as being classified into two categories:
    >
    > (A) Some accesses should only consider the dentry they're given.
    > Filesystems would fall into this category when dealing with their own
    > dentries and inodes.

    .. and this is the one that makes no sense to me.

    It's the common case, and I don't see how it *possibly* adds any
    value. The "I want the inode of this dentry" is traditionally done as
    "dentry->d_inode".

    What is the *upside* of the wrapper?

    > (B) Other accesses should fall through from the dentry they're given to
    > an underlying layer. A lot of non-filesystem accesses, including
    > those in the VFS side of pathwalk, fall into this category, as do
    > accesses to external objects made by filesystems such as overlayfs
    > and ecryptfs.

    Again, if they actually want something *else* than the "native inode",
    then at that point a wrapper makes sense.

    If you actually want to document that "this use wants the underlying
    inode", then go wild. But that is *different* from all the common uses
    inside random filesystems that just want "what's the inode of this
    dentry".

    IOW, my argument is that I cannot see *any* possible reason to wrap
    the normal "give me the inode associated with this dentry".

    I *can* see a reason to wrap - and document - the cases that are *not*
    that kind of simple thing. I *can* see the reason for saying "give me
    the inode of the lower layer", or "give me the inode, or NULL if it's
    a whiteout entry".

    But it's just that empty "fs_inode()" wrapper itself that I just don't
    see the point of.

    Every single low-level filesystem that does all the actual core
    lookup/mkdir/create/whatever operations care about the native inode.
    You seem to even kind of acknowledge that in the naming: "fs_inode()".
    And my argument is that there is never any possible reason why that
    would ever be anything but "dentry->d_inode".

    So the wrapper doesn't actually help anything, and it *does* obfuscate
    things. It makes people go "whats' the difference between "fs_inode()"
    and "fs_inode_once()". It quite possible makes people think it's an
    expensive operation. Who knows. It seems pointless.

    > (6) Add a DCACHE_WHITEOUT_TYPE type variant for dentries that will in the
    > future be recognised by the VFS as a 'negative' type, but can be used by
    > the overlay or unionmount to note a difference to an ordinary negative
    > type dentry that can also be a fallthrough (at least in unionmount if
    > this ever makes it).


    Right. And a helper/wrapper makes sense for those cases, and actually
    clarifies that "this particular code knows and cares about whiteout
    entries".

    I'm not arguing against those kinds of wrappers that actually
    _clarify_ things. I'm arguing against random wrappers that don't, and
    that really fundamentally seem like they cannot possibly ever have any
    other valid value.

    A low-level filesystem may have a real inode associated with a
    whiteout dentry, it could be a device inode with a zero major number
    or some other special inode (or it might not be an inode at alln - it
    could easily also be just a directory entry type). Such a filesystem
    would clearly care about the inode. and would actually really care
    about "dentry->d_inode".

    And I actually think that having "dentry->d_inode" is *clearer* than
    "fs_ionode(dentry)". It's clear that that is a naked native access.
    It's not some hidden abstracted case. That's *the* inode associated
    with the dentry.

    > (7) Where feasible, replace if-statements that check the positivity or
    > negativity of dentries by doing if (...->d_inode) with checks on the type
    > of the dentry.

    .. and this is again the kind of wrapper I think is *good*. It's
    abstracting some real issue.

    I don't object at all to abstracting out "ok, a dentry is negative if
    it has a NULL inode, or if it is marked as a whiteout entry". I think
    that writing

    if (d_negative(dentry)) ...

    is more readable than

    if (!dentry->d_inode || (dentry->d_flags & DCACHE_WHITEOUT)) ..

    or variations of that (I guess it's "IS_WHITEOUT(dentry->d_inode)" right now).

    So I'm not against helpers that do something meaningful.

    And there are downsides to arbitrary random wrappers/helpers. Churn.
    Harder to see what the code actually *does*. Bad naming (dentry
    operations are generally called "d_xyz()" or "dentry_xyz()"). Does it
    do soemthign else? Are there rules for calling this? All the mental
    rules you have to have, and that *change* just because you change the
    syntax.

    >> Now, that was true in the "bad old days" when we just used
    >> ACCESS_ONCE(dentry->d_inode) too, but at least in that old model we
    >> don't have some idiotic wrapper around it.
    >
    > I can't make ACCESS_ONCE(fs_inode(dentry)) work if fs_inode() is an inline
    > function. I might be able to make it work if it's a macro. I also don't want
    > to call ACCESS_ONCE() inside fs_inode().

    Oh, I see *why* you did it, given that you wanted a wrapper.

    But I also see this very much as an example of "the wrapper is
    actually causing more problems than it's solving". The fact that the
    first wrapper exists now means that you have to use *another* wrapper.

    And dammit, it's not *helping*.

    Now, if it was something like "ok, the revalidate() call is very very
    special for a low-level filesystem, in that the filesystem cannot
    trust the inode pointer, so we have a special
    "d_revalidate_inode(dentry) wrapper that does ACCESS_ONCE()", then
    such a wrapper would actually be *documentation*, and would kind of
    show something real ("revalidate is special").

    See what I'm trying to say? At that point - even if the wrapper
    doesn't *do* anything - it at least documents some rule, and migth be
    worth it for that reason.

    But what does "fs_inode_once()" document? Nothing. It's just an
    artifact of you having introduced a wrapper and done a
    search-and-replace. So now you have the extra abstraction of a
    wrapper, with all the disadvantages of abstractions, and none of the
    advantages that abstraction is supposed to actually bring us.

    > Suggest a better name that's not too long then please. fs_inode(d) is about
    > the same length as d->d_inode which means I can just script it and I don't
    > have to go in and reformat the code.

    So I didn't check, but I *think* the only valid use for the
    ACCESS_ONCE() on ->d_inode is for revalidate, which is kind of special
    in that we call it without locks held. So *if* that is true, then
    maybe "d_revalidate_inode()" would be a reasonable name. But again, I
    didn't check that semantic rule, so maybe it doesn't work.

    And yes, I'm annoyed, because I really hate the timing of this pull
    request. So it's not just about the wrapper, it's about when/how this
    all reached me.

    Linus


    \
     
     \ /
      Last update: 2015-02-22 02:21    [W:4.190 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site