Re: [git pull] more vfs bits

[Al Viro]

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@xxxxxxxxxxxxxxxxxx> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/