Hello!
On Oct 20, 2014, at 9:13 PM, Al Viro wrote:
> a) what protects ->d_name in ll_intent_file_open()? It copies
> ->d_name.name and ->d_name.len into local variables and proceeds to
> use those; what's to guarantee that dentry won't get hit with d_move()
> halfway through that? None of the locks that would give an exclusion
> against d_move() appear to be held…
You are right. We hit something very similar not too long ago.
>
> b) what stabilizes *dentryp->d_name in do_statahead_enter()?
>
> c) what stabilizes fdentry->d_parent->d_name in llog_lvfs_destroy()?
I don't think there could be possible d_move in llog path. Those files
are never visible in the proper namespace, are not exposed to rename
and other user-influenced actions so should be safe.
> Unless I'm missing something subtle, all three can race with d_move(),
> with obvious unpleasant results. The next bunch doesn't (the callers
> are holding ->i_mutex on parents), but it's also bloody odd - why are we
> playing these games in llite/namei.c?
This is a throwback from the past where we patched "raw" rename methods into
VFS, intent-like (I am including a big explanation below, that you can skip
if you are not into Luste archeology. TL;DR: patches are in the queue to get rid of that).
So basically we had parent inode and the child name we wanted
operation to be performed on. All of that was shipped directly to the server
and server returned the result, without any intermediate lookup RPCs that would
have happened otherwise, and a significant erformance boost ensued (at certain
cost of course, like no local SELinux enforcement and such).
After all wedecided to switch to "patchless clients" after open intents appeared
in 2.6.15, and had a pairof possible methofds here: ll_XXX_raw for the old-style
intent call and ll_XXX are the normal vfs-style calls that both in the end
did some processign and called ll_XXX_generic that talked to the server.
Since we don't need any of this for some time, we are getting rid of those things
and I am pretty sure the patches for removal of this extra wrapping is in the
queue of stuff to send.
> While we are at it, where is ll_new_inode() ever called with NULL dchild?
This is the deprecated "intent" code path explained above.
> Similar to that, where is ll_d_mountpoint() ever called with NULL dchild,
> why do you have
> if (unlikely(dchild))
Same as above.
All of that would go away once we get rid of the wrapper and unroll the
vfs ops.
Hopefully this makes at least some sense to you.
Bye,
Oleg--
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
