On Mar 8, 2016, at 4:11 PM, Al Viro wrote:
> On Tue, Mar 08, 2016 at 08:44:24PM +0000, Drokin, Oleg wrote:
>
>> The links are hardlinks, right? (because otherwise they would not be
>> parallel due to parent dir i_mutex held).
>
> Yes.
>
>> Hm, The race is a "safe" one, since if the alias have appeared, it does not break
>> anything other than using up some RAM, I think?
>> In fact what's to stop one appearing after we released the locking leading to the
>> same situation?
>
> Kinda-sorta. It's safe unless a rename on server gets you see the same
> directory in two places. d_splice_alias() would've coped with that, this
> code won't.
Rename on server cannot get us to see the same directory in two places, or what's
the scenario?
In Lustre:
thread1: lookup a directory on the server.
get a lock back
instantiate a dentry (guarded by the lock)
make a lock revocable (ll_lookup_finish_locks() in ll_lookup_it())
thread2: rename the directory moving it somewhere else
server attempts to revoke the lock from thread1
node that runs thread1 drops the lock and marks all dentries for that
inode invalid
server completes rename and releases the lock it holds
thread3: lookup the directory under new name on the server
this can only return from server when the rename has completed and the
dentry from thread1 marked invalid.
>>> If so, how about adding d_hash_exact_alias(dentry) that would try to find
>>> and hash an exact unhashed alias, so that this thing would
>>> * call d_hash_exact_alias(dentry), if found - just return it
>>> * ll_d_init(dentry)
>>> * return d_splice_alias() ?: dentry
>>> Do you see any problems with that? Parent directory is locked, so no new
>>> unhashed exact aliases should have a chance to appear and d_splice_alias()
>>> would take care of everything else (correctness and detached ones).
>>
>> This sounds fine on the surface. I think I remember there were some other
>> complications with d_splice_alias.
>> Andreas, do ou remember?
>
> FWIW, a patch in my queue kills d_add_unique(), replacing it with
> d_exact_alias() and d_splice_alias(); it could be used to implement
> ll_splice_alias() as well (with code duplication gone *and* capable
> of dealing with directories moving around), except for the odd
> rules re inode refcount on error; it would've been easier if ll_splice_alias()
Ok, let me try my hand at that. Hopefully whatever complications are there would
show themselves right away too.
> would always either inserted inode reference into a new dentry or dropped it.
> I'm still trying to trace what does iput() in case of error in your current
> code; I understand the one in do_statahead_enter(), but what does it in
> ll_lookup_it_finish()?
You mean when ll_d_init() fails in ll_splice_alias()?
Hm… It appears that we are indeed leaking the inode in that case, thanks.
I'll try to address that too.
Hm, in fact this was almost noticed, but I guess nobody retested it after
fixing the initial crash we had with 7486bc06ab2c46d6957f0211d09bc549aaf9cc87
Thanks.
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
