On Mon, 27 Jul 2015 11:10:45 +0800 Kinglong Mee <kinglongmee@xxxxxxxxx>
wrote:
> A new helper cache_delete_entry() for delete cache_head from
> cache_detail directly.
>
> It will be used by pin_kill, so make sure the cache_detail is valid
> before deleting is needed.
I cannot see any justification for validating the cache_detail.
When this gets called, the cache_head has not yet been freed (though it
probably will be soon) so the cache_detail must still be around.
However it is possible for this to race with cache_clean() which could
have already removed the cache_head from the list (and decremented
->entries), but which hasn't called cache_put() yet.
The use of cache_list_lock is not enough to protect against that race.
So I think you should drop the use of cache_list_lock, drop the check
that detail is still in the list, and after getting ->hash_lock, check
hlist_unhash() and only unhash if that failed.
Thanks,
NeilBrown
>
> Because pin_kill is not many times, so the influence of performance
> is accepted.
>
> v8, same as v6.
>
> Signed-off-by: Kinglong Mee <kinglongmee@xxxxxxxxx>
> ---
> include/linux/sunrpc/cache.h | 1 +
> net/sunrpc/cache.c | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 31 insertions(+)
>
> diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
> index 03d3b4c..2824db5 100644
> --- a/include/linux/sunrpc/cache.h
> +++ b/include/linux/sunrpc/cache.h
> @@ -210,6 +210,7 @@ extern int cache_check(struct cache_detail *detail,
> struct cache_head *h, struct cache_req *rqstp);
> extern void cache_flush(void);
> extern void cache_purge(struct cache_detail *detail);
> +extern void cache_delete_entry(struct cache_detail *cd, struct cache_head *h);
> #define NEVER (0x7FFFFFFF)
> extern void __init cache_initialize(void);
> extern int cache_register_net(struct cache_detail *cd, struct net *net);
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index 4a2340a..b722aea 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -454,6 +454,36 @@ static int cache_clean(void)
> return rv;
> }
>
> +void cache_delete_entry(struct cache_detail *detail, struct cache_head *h)
> +{
> + struct cache_detail *tmp;
> +
> + if (!detail || !h)
> + return;
> +
> + spin_lock(&cache_list_lock);
> + list_for_each_entry(tmp, &cache_list, others) {
> + if (tmp == detail)
> + goto found;
> + }
> + spin_unlock(&cache_list_lock);
> + printk(KERN_WARNING "%s: Deleted cache detail %p\n", __func__, detail);
> + return ;
> +
> +found:
> + write_lock(&detail->hash_lock);
> +
> + hlist_del_init(&h->cache_list);
> + detail->entries--;
> + set_bit(CACHE_CLEANED, &h->flags);
> +
> + write_unlock(&detail->hash_lock);
> + spin_unlock(&cache_list_lock);
> +
> + cache_put(h, detail);
> +}
> +EXPORT_SYMBOL_GPL(cache_delete_entry);
> +
> /*
> * We want to regularly clean the cache, so we need to schedule some work ...
> */
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
