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. Linus -- 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