On Mon, Dec 11, 2017 at 10:57:45AM +1100, Dave Chinner wrote:
> i.e. the fact the cmpxchg failed may not have anything to do with a
> race condtion - it failed because the slot wasn't empty like we
> expected it to be. There can be any number of reasons the slot isn't
> empty - the API should not "document" that the reason the insert
> failed was a race condition. It should document the case that we
> "couldn't insert because there was an existing entry in the slot".
> Let the surrounding code document the reason why that might have
> happened - it's not for the API to assume reasons for failure.
>
> i.e. this API and potential internal implementation makes much
> more sense:
>
> int
> xa_store_iff_empty(...)
> {
> curr = xa_cmpxchg(&pag->pag_ici_xa, agino, NULL, ip, GFP_NOFS);
> if (!curr)
> return 0; /* success! */
> if (!IS_ERR(curr))
> return -EEXIST; /* failed - slot not empty */
> return PTR_ERR(curr); /* failed - XA internal issue */
> }
>
> as it replaces the existing preload and insert code in the XFS code
> paths whilst letting us handle and document the "insert failed
> because slot not empty" case however we want. It implies nothing
> about *why* the slot wasn't empty, just that we couldn't do the
> insert because it wasn't empty.
Yeah, OK. So, over the top of the recent changes I'm looking at this:
I'm not in love with xa_store_empty() as a name. I almost want
xa_store_weak(), but after my MAP_FIXED_WEAK proposed name got shot
down, I'm leery of it. "empty" is at least a concept we already have
in the API with the comment for xa_init() talking about an empty array
and xa_empty(). I also considered xa_store_null and xa_overwrite_null
and xa_replace_null(). Naming remains hard.
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 941f38bb94a4..586b43836905 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -451,7 +451,7 @@ xfs_iget_cache_miss(
int flags,
int lock_flags)
{
- struct xfs_inode *ip, *curr;
+ struct xfs_inode *ip;
int error;
xfs_agino_t agino = XFS_INO_TO_AGINO(mp, ino);
int iflags;
@@ -498,8 +498,7 @@ xfs_iget_cache_miss(
xfs_iflags_set(ip, iflags);
/* insert the new inode */
- curr = xa_cmpxchg(&pag->pag_ici_xa, agino, NULL, ip, GFP_NOFS);
- error = __xa_race(curr, -EAGAIN);
+ error = xa_store_empty(&pag->pag_ici_xa, agino, ip, GFP_NOFS, -EAGAIN);
if (error)
goto out_unlock;
diff --git a/include/linux/xarray.h b/include/linux/xarray.h
index 5792b6dbb040..cc7cc5253a67 100644
--- a/include/linux/xarray.h
+++ b/include/linux/xarray.h
@@ -271,43 +271,30 @@ static inline int xa_err(void *entry)
}
/**
- * __xa_race() - Turn a cmpxchg result into an errno.
- * @entry: Result from calling an XArray function.
- * @errno: Error number to return if we lost the race.
+ * xa_store_empty() - Store this entry in the XArray unless another entry is
+ * already present.
+ * @xa: XArray.
+ * @index: Index into array.
+ * @entry: New entry.
+ * @gfp: Memory allocation flags.
+ * @rc: Number to return if another entry was present.
*
- * Like xa_race(), but returns the error number of your choice. Calling
- * __xa_race(entry, 0) has the same result (but is less efficient) as
- * calling xa_err().
+ * Like xa_store(), but will fail and return the supplied error number if
+ * the existing entry at @index is not %NULL.
*
* Return: A negative errno or 0.
*/
-static inline int __xa_race(void *entry, int errno)
+static inline int xa_store_empty(struct xarray *xa, unsigned long index,
+ void *entry, gfp_t gfp, int errno)
{
- if (!entry)
+ void *curr = xa_cmpxchg(xa, index, NULL, entry, gfp);
+ if (!curr)
return 0;
- if (xa_is_err(entry))
- return (long)entry >> 2;
+ if (xa_is_err(curr))
+ return xa_err(curr);
return errno;
}
-/**
- * xa_race() - Turn a cmpxchg result into an errno.
- * @entry: Result from calling an XArray function.
- *
- * It is common to use xa_cmpxchg() to ensure that only one thread assigns
- * a value to an index. Pass the result from xa_cmpxchg() to xa_race() to
- * get an errno back. This function also handles any other error which
- * may have been returned by xa_cmpxchg() such as ENOMEM.
- *
- * If you don't care that you lost the race, you can use xa_err() instead.
- *
- * Return: A negative errno or 0.
- */
-static inline int xa_race(void *entry)
-{
- return __xa_race(entry, -EEXIST);
-}
-
/* Everything below here is the Advanced API. Proceed with caution. */
#define xa_trylock(xa) spin_trylock(&(xa)->xa_lock)
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 85d1bc963ab6..87ed55af823e 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -614,8 +614,8 @@ static int cgwb_create(struct backing_dev_info *bdi,
spin_lock_irqsave(&cgwb_lock, flags);
if (test_bit(WB_registered, &bdi->wb.state) &&
blkcg_cgwb_list->next && memcg_cgwb_list->next) {
- ret = xa_race(xa_cmpxchg(&bdi->cgwb_xa, memcg_css->id, NULL,
- wb, GFP_ATOMIC));
+ ret = xa_store_empty(&bdi->cgwb_xa, memcg_css->id, wb,
+ GFP_ATOMIC, -EEXIST);
if (!ret) {
list_add_tail_rcu(&wb->bdi_node, &bdi->wb_list);
list_add(&wb->memcg_node, memcg_cgwb_list);
