Re: [PATCH RFC 3/3] nfsd: convert DRC code to use list_lru

[Christoph Hellwig <hch@infradead.org>]

>  static void
> -nfsd_reply_cache_free_locked(struct svc_cacherep *rp)
> +__nfsd_reply_cache_free_locked(struct svc_cacherep *rp)
>  {
>  	if (rp->c_type == RC_REPLBUFF && rp->c_replvec.iov_base) {
>  		drc_mem_usage -= rp->c_replvec.iov_len;
> @@ -140,13 +141,26 @@ nfsd_reply_cache_free_locked(struct svc_cacherep *rp)
>  	}
>  	if (!hlist_unhashed(&rp->c_hash))
>  		hlist_del(&rp->c_hash);
> -	list_del(&rp->c_lru);
>  	--num_drc_entries;
>  	drc_mem_usage -= sizeof(*rp);
>  	kmem_cache_free(drc_slab, rp);

I would be better to move the hash list deletion out of this and
keep this as a plain nfsd_reply_cache_free().  E.g. in the lookup
cache hit case we've never linked any item and would want this low-level
function.

>  }
>  
>  static void
> +nfsd_reply_cache_free_locked(struct svc_cacherep *rp)
> +{
> +	list_lru_del(&lru_head, &rp->c_lru);
> +	__nfsd_reply_cache_free_locked(rp);
> +}
> +
> +static void
> +nfsd_reply_cache_free_isolate(struct svc_cacherep *rp)
> +{
> +	list_del(&rp->c_lru);
> +	__nfsd_reply_cache_free_locked(rp);
> +}

Should be merged into the only caller.

> +
> +static void
>  nfsd_reply_cache_free(struct svc_cacherep *rp)
>  {
>  	spin_lock(&cache_lock);
> @@ -156,50 +170,66 @@ nfsd_reply_cache_free(struct svc_cacherep *rp)
>  

> +static enum lru_status
> +nfsd_purge_lru_entry(struct list_head *item, spinlock_t *lock, void *cb_arg)
>  {
> -	struct svc_cacherep	*rp;
> +	struct svc_cacherep *rp = list_entry(item, struct svc_cacherep, c_lru);
>  
> +	nfsd_reply_cache_free_locked(rp);
> +	return LRU_REMOVED;
> +}
> +
> +void nfsd_reply_cache_shutdown(void)
> +{
>  	unregister_shrinker(&nfsd_reply_cache_shrinker);
>  	cancel_delayed_work_sync(&cache_cleaner);
>  
> -	while (!list_empty(&lru_head)) {
> -		rp = list_entry(lru_head.next, struct svc_cacherep, c_lru);
> -		nfsd_reply_cache_free_locked(rp);
> -	}
> +	/* In principle, nothing should be altering the list now, but... */
> +	spin_lock(&cache_lock);
> +	list_lru_walk(&lru_head, nfsd_purge_lru_entry, NULL, ULONG_MAX);
> +	spin_unlock(&cache_lock);

This needs the version that does the list_del internally to, doesn't
it?  I'd suggest to just use sd_purge_expired_entry with a forced
flag in the argument.

> +	memset(&lru_head, 0, sizeof(lru_head));

don't think there's much of a point.


All together doesn't seem to helpful as long as the DRC keeps
it's own timing based purge and similar bits unfortunatel.