On Tue, Dec 05, 2017 at 04:41:58PM -0800, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@xxxxxxxxxxxxx>
>
> This eliminates a call to radix_tree_preload().
.....
> void
> @@ -431,24 +424,24 @@ xfs_mru_cache_insert(
> unsigned long key,
> struct xfs_mru_cache_elem *elem)
> {
> + XA_STATE(xas, &mru->store, key);
> int error;
>
> ASSERT(mru && mru->lists);
> if (!mru || !mru->lists)
> return -EINVAL;
>
> - if (radix_tree_preload(GFP_NOFS))
> - return -ENOMEM;
> -
> INIT_LIST_HEAD(&elem->list_node);
> elem->key = key;
>
> - spin_lock(&mru->lock);
> - error = radix_tree_insert(&mru->store, key, elem);
> - radix_tree_preload_end();
> - if (!error)
> - _xfs_mru_cache_list_insert(mru, elem);
> - spin_unlock(&mru->lock);
> + do {
> + xas_lock(&xas);
> + xas_store(&xas, elem);
> + error = xas_error(&xas);
> + if (!error)
> + _xfs_mru_cache_list_insert(mru, elem);
> + xas_unlock(&xas);
> + } while (xas_nomem(&xas, GFP_NOFS));
Ok, so why does this have a retry loop on ENOMEM despite the
existing code handling that error? And why put such a loop in this
code and not any of the other XFS code that used
radix_tree_preload() and is arguably much more important to avoid
ENOMEM on insert (e.g. the inode cache)?
Also, I really don't like the pattern of using xa_lock()/xa_unlock()
to protect access to an external structure. i.e. the mru->lock
context is protecting multiple fields and operations in the MRU
structure, not just the radix tree operations. Turning that around
so that a larger XFS structure and algorithm is now protected by an
opaque internal lock from generic storage structure the forms part
of the larger structure seems like a bad design pattern to me...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
