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