On Thu, Dec 07, 2017 at 11:38:43AM +1100, Dave Chinner wrote: > > > cmpxchg is for replacing a known object in a store - it's not really > > > intended for doing initial inserts after a lookup tells us there is > > > nothing in the store. The radix tree "insert only if empty" makes > > > sense here, because it naturally takes care of lookup/insert races > > > via the -EEXIST mechanism. > > > > > > I think that providing xa_store_excl() (which would return -EEXIST > > > if the entry is not empty) would be a better interface here, because > > > it matches the semantics of lookup cache population used all over > > > the kernel.... > > > > I'm not thrilled with xa_store_excl(), but I need to think about that > > a bit more. > > Not fussed about the name - I just think we need a function that > matches the insert semantics of the code.... I think I have something that works better for you than returning -EEXIST (because you don't actually want -EEXIST, you want -EAGAIN): /* insert the new inode */ - spin_lock(&pag->pag_ici_lock); - error = radix_tree_insert(&pag->pag_ici_root, agino, ip); - if (unlikely(error)) { - WARN_ON(error != -EEXIST); - XFS_STATS_INC(mp, xs_ig_dup); - error = -EAGAIN; - goto out_preload_end; - } - spin_unlock(&pag->pag_ici_lock); - radix_tree_preload_end(); + curr = xa_cmpxchg(&pag->pag_ici_xa, agino, NULL, ip, GFP_NOFS); + error = __xa_race(curr, -EAGAIN); + if (error) + goto out_unlock; ... -out_preload_end: - spin_unlock(&pag->pag_ici_lock); - radix_tree_preload_end(); +out_unlock: + if (error == -EAGAIN) + XFS_STATS_INC(mp, xs_ig_dup); I've changed the behaviour slightly in that returning an -ENOMEM used to hit a WARN_ON, and I don't think that's the right response -- GFP_NOFS returning -ENOMEM probably gets you a nice warning already from the mm code. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html