Re: REVIEW: Improve caching in libxfs

[Christoph Hellwig <hch@infradead.org>]

>  		hash = cache->c_hash + node->cn_hashidx;
> -		if (node->cn_count > 0 ||
> -				pthread_mutex_trylock(&hash->ch_mutex) != 0) {
> +		if (pthread_mutex_trylock(&hash->ch_mutex) != 0) {
>  			pthread_mutex_unlock(&node->cn_mutex);
>  			continue;
>  		}
> +		ASSERT(node->cn_count == 0);

Remove code to check if it's reference but instead assert that it isn't
because it's not added to the list in that case.  Makes sense.

>  			/*
> -			* node found, bump node's reference count, move it to the
> -			* top of its MRU list, and update stats.
> +			* node found, bump node's reference count, remove it
> +			* from its MRU list, and update stats.
>  			*/

The comment formatting is still b0rked, all the * should line up.

>  			pthread_mutex_lock(&node->cn_mutex);
> -			node->cn_count++;
>
> -			mru = &cache->c_mrus[node->cn_priority];
> -			pthread_mutex_lock(&mru->cm_mutex);
> -			list_move(&node->cn_mru, &mru->cm_list);
> -			pthread_mutex_unlock(&mru->cm_mutex);
> +			if (node->cn_count == 0) {
> +				ASSERT(node->cn_priority >= 0);
> +				ASSERT(!list_empty(&node->cn_mru));
> +				mru = &cache->c_mrus[node->cn_priority];
> +				pthread_mutex_lock(&mru->cm_mutex);
> +				mru->cm_count--;
> +				list_del_init(&node->cn_mru);
> +				pthread_mutex_unlock(&mru->cm_mutex);
> +			}
> +			node->cn_count++;

Instead of moving it around, remove it from the list whe it was
cn_count == 0 and now is rferenced.  Makes sense.

> -	/* add new node to appropriate hash and lowest priority MRU */
> -	mru = &cache->c_mrus[0];
> -	pthread_mutex_lock(&mru->cm_mutex);
> +	/* add new node to appropriate hash */
>  	pthread_mutex_lock(&hash->ch_mutex);
>  	hash->ch_count++;
> -	mru->cm_count++;
>  	list_add(&node->cn_hash, &hash->ch_list);
> -	list_add(&node->cn_mru, &mru->cm_list);
>  	pthread_mutex_unlock(&hash->ch_mutex);
> -	pthread_mutex_unlock(&mru->cm_mutex);

Don't add it to the mru list in cache_Get - makes sense.

>  void
>  cache_node_put(
> +	struct cache *		cache,
>  	struct cache_node *	node)
>  {
> +	struct cache_mru *	mru;
> +
>  	pthread_mutex_lock(&node->cn_mutex);
>  #ifdef CACHE_DEBUG
>  	if (node->cn_count < 1) {
> @@ -368,8 +372,23 @@ cache_node_put(
>  				__FUNCTION__, node->cn_count, node);
>  		cache_abort();
>  	}
> +	if (!list_empty(&node->cn_mru)) {
> +		fprintf(stderr, "%s: node put on node (%p) in MRU list\n",
> +				__FUNCTION__, node);
> +		cache_abort();
> +	}

Assert that we don't put a node that's already on the mru list, okay.
Shouldn't this be ASSERT?

>  #endif
>  	node->cn_count--;
> +
> +	if (node->cn_count == 0) {
> +		/* add unreferenced node to appropriate MRU for shaker */
> +		mru = &cache->c_mrus[node->cn_priority];
> +		pthread_mutex_lock(&mru->cm_mutex);
> +		mru->cm_count++;
> +		list_add(&node->cn_mru, &mru->cm_list);
> +		pthread_mutex_unlock(&mru->cm_mutex);
> +	}

And add it to the list, good.

> @@ -379,33 +398,14 @@ cache_node_set_priority(
>  	struct cache_node *	node,
>  	int			priority)
>  {
> -	struct cache_mru *	mru;
> -
>  	if (priority < 0)
>  		priority = 0;
>  	else if (priority > CACHE_MAX_PRIORITY)
>  		priority = CACHE_MAX_PRIORITY;
>
>  	pthread_mutex_lock(&node->cn_mutex);
> -
>  	ASSERT(node->cn_count > 0);
> -	if (priority == node->cn_priority) {
> -		pthread_mutex_unlock(&node->cn_mutex);
> -		return;
> -	}
> -	mru = &cache->c_mrus[node->cn_priority];
> -	pthread_mutex_lock(&mru->cm_mutex);
> -	list_del_init(&node->cn_mru);
> -	mru->cm_count--;
> -	pthread_mutex_unlock(&mru->cm_mutex);
> -
> -	mru = &cache->c_mrus[priority];
> -	pthread_mutex_lock(&mru->cm_mutex);
> -	list_add(&node->cn_mru, &mru->cm_list);
>  	node->cn_priority = priority;
> -	mru->cm_count++;
> -	pthread_mutex_unlock(&mru->cm_mutex);
> -
>  	pthread_mutex_unlock(&node->cn_mutex);

Set priority now simply sets the priority and doesn't fuzz with the mru
list, good.

>  		cache_node_set_priority(libxfs_bcache, (struct cache_node *)bp,
> -			cache_node_get_priority((struct cache_node *)bp) - 4);
> +			cache_node_get_priority((struct cache_node *)bp) - 8);

???

>  #define B_DIR_BMAP	15
> -#define B_DIR_META_2	13	/* metadata in secondary queue */
> -#define B_DIR_META_H	11	/* metadata fetched for PF_META_ONLY */
> -#define B_DIR_META_S	9	/* single block of metadata */
> -#define B_DIR_META	7
> -#define B_DIR_INODE	6
> -#define B_BMAP		5
> -#define B_INODE		4
> +#define B_DIR_META_2	14	/* metadata in secondary queue */
> +#define B_DIR_META_H	13	/* metadata fetched for PF_META_ONLY */
> +#define B_DIR_META_S	12	/* single block of metadata */
> +#define B_DIR_META	11
> +#define B_DIR_INODE	10
> +#define B_BMAP		9
> +#define B_INODE		8

Changing priorities, this could use some explanation in the
patch description..

> -#define B_IS_INODE(b)	(((b) & 1) == 0)
> -#define B_IS_META(b)	(((b) & 1) != 0)
> +#define B_IS_INODE(f)	(((f) & 5) == 0)
> +#define B_IS_META(f)	(((f) & 5) != 0)

And these two macros really want some comments describing them.