Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

[Dave Chinner]

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