On Wed, Mar 23 2016, Jan Kara wrote:
> On Wed 23-03-16 08:10:42, NeilBrown wrote:
>> On Sat, Mar 19 2016, Jan Kara wrote:
>> >
>> > Actually, after some thought I don't think the wakeup is needed except for
>> > dax_pfn_mkwrite(). In the other cases we know there is no radix tree
>> > exceptional entry and thus there can be no waiters for its lock...
>> >
>>
>> I think that is fragile logic - though it may be correct at present.
>>
>> A radix tree slot can transition from "Locked exception" to "unlocked
>> exception" to "deleted" to "struct page".
>
> Yes.
>
>> So it is absolutely certain that a thread cannot go to sleep after
>> finding a "locked exception" and wake up to find a "struct page" ??
>
> With current implementation this should not happen but I agree entry
> locking code should not rely on this.
>
>> How about a much simpler change.
>> - new local variable "slept" in lookup_unlocked_mapping_entry() which
>> is set if prepare_to_wait_exclusive() gets called.
>> - if after __radix_tree_lookup() returns:
>> (ret==NULL || !radix_tree_exceptional_entry(ret)) && slept
>> then it calls wakeup immediately - because if it was waiting,
>> something else might be to.
>>
>> That would cover all vaguely possible cases except dax_pfn_mkwrite()
>
> But how does this really help? If lookup_unlocked_mapping_entry() finds
> there is no entry (and it was there before), the process deleting the entry
> (or replacing it with something else) is responsible for waking up
> everybody.
"everybody" - yes. But it doesn't wake everybody does it? It just
wakes one.
+ __wake_up(wq, TASK_NORMAL, 1, &key);
^one!
Or am I misunderstanding how exclusive waiting works?
Thanks,
NeilBrown
> So your change would only duplicate what
> dax_delete_mapping_entry() does. The potential for breakage is that callers
> of lookup_unlocked_mapping_entry() are responsible for waking up other
> waiters *even if* they do not lock or delete the entry in the end. Maybe
> I'll rename lookup_unlocked_mapping_entry() to get_unlocked_mapping_entry()
> so that it is clearer that one must call either put_unlocked_mapping_entry()
> or put_locked_mapping_entry() on it.
>
> Honza
>
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
Attachment:
signature.asc
Description: PGP signature
