Re: [PATCH 8/9 v8] sunrpc: New helper cache_delete_entry for deleting cache_head directly

[Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 7/29/2015 10:29, NeilBrown wrote:
> On Mon, 27 Jul 2015 11:10:45 +0800 Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 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.

Yes, that's right.
I will drop the checking of cache_detail here.

> 
> 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 again for your comments.

thanks,
Kinglong Mee

> 
> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html