lkml.org 
[lkml]   [2015]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [git pull] more vfs bits
    On Sat, Feb 21, 2015 at 05:34:23PM -0800, Linus Torvalds wrote:
    > On Sat, Feb 21, 2015 at 4:51 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
    > >
    > > Looking at that queue, it might make sense to hold back everything in that
    > > series past "fanotify: Fix up scripted S_ISDIR/S_ISREG/S_ISLNK conversions"
    > > for now
    >
    > Hmm. Even I'd pull just that, quite frankly, I just think it's
    > *confusing* to have those badly named "helpers", that were introduced
    > earlier in that series.
    >
    > These guys are currently all teh same thing, but even if they weren't,
    > the naming is not helpful, and not sane:
    > - fs_inode
    > - fs_inode_once
    > - dentry_inode
    > - dentry_inode_once
    >
    > Let's walk through them:
    >
    > - dentry_inode*() is supposed to be "the inode that would be used if
    > the dentry was opened"
    >
    > What part of "dentry_inode()" implies "if the dentry was opened" to
    > you? Nothing. The name is fundamentally bad.
    >
    > And what *possible* situation could make that "_once()" version ever
    > be valid? None. It's bogus. It's crap. It's insane. There is no way
    > that it is *ever* a valid question to even ask. If the dentry is so
    > unstable that you can't safely look at the inode, you had damn well
    > better never ask "ok, what would the inode be if I opened this random
    > pointer"?
    >
    > So one of them is badly named, and the other one is fundamentally
    > not a valid operation at all, as far as I can tell.
    >
    > - fs_inode*() is supposed to be "this is the inode that the native
    > filesystem uses".
    >
    > So again, I think the naming is horrible, since it doesn't really
    > follow the normal dentry helper routine names. But I'm sure we have
    > other cases where we screwed that up, so whatever..
    >
    > The "_once()" naming is doubly bad, as explained elsewhere. What
    > possible situation merits using that helper? If it's just
    > revalidate(), then make it about that.
    >
    > But more importantly, this is the one where I don't see how it
    > could ever possibly be anything but "dentry->d_inode". I'd much rather
    > just leave that.
    >
    > So of the four new helpers, I really don't see any of them as "good".
    > I think "dentry_inode()" could remain, but even there I think the name
    > should specify *what* it is ("d_opened_inode()"? I don't like that
    > name either, but at least it would try to explain what the point is,
    > rather than having to look up a comment above the function definition
    > to figure out what the point is)
    >
    > The strongest argument I've seen for them existing at all was that
    > "markers for what has been looked at". But that's something that
    > belongs in a development tree, not as a series to confuse others with.

    Hmm... ..._once() variants are trivially dropped, IMO. dentry_inode_once()
    is so bloody special that it *SHOULD* stick out; we don't have any places
    like that, anyway.

    I'm somewhat tempted to do this:
    fs_inode -> d_inode
    fs_inode_once ->d_inode_rcu (it's not quite ->d_revalidate()-only, there's
    a bit in autofs ->d_manage() as well)
    dentry_inode -> something. d_opened_inode() might do, but I'm not sure -
    still sounds a bit wrong to me. What it's about is "the actual fs object
    behind this name, maybe from upper fs, maybe showing through from underlying
    layer". It's not always opened; it's what we'd get if we opened it (and
    hadn't triggered any copyups, that is). E.g. sys_getxattr() would want to
    use that, even if nobody has opened that sucker yet, etc.
    dentry_inode_once -> RIP

    It's still greppable ([-]>d_inode\> will do it) and IMO it's better than
    fs_inode(). And yes, the churn issue remains, but IMO having a pair of
    inlined helpers (d_inode(dentry) and d_inode_rcu(dentry)) in dcache.h is
    not too horrible per se.

    Comments?


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