| Message ID | 20171211042315.GA25236@bombadil.infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Dec 10, 2017 at 08:23:15PM -0800, Matthew Wilcox wrote: > 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; Can we avoid encoding the error to be returned into the function call? No other generic/library API does this, so this seems like a highly unusual special snowflake approach. I just don't see how creating a whole new error specification convention is justified for the case where it *might* save a line or two of error checking code in a caller? I'd much prefer that the API defines the error on failure, and let the caller handle that specific error however they need to like every other library interface we have... Cheers, Dave.
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);