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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
