diff mbox

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

Message ID 55B5A135.9050800@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kinglong Mee July 27, 2015, 3:10 a.m. UTC
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.

Because pin_kill is not many times, so the influence of performance
is accepted.

v8, same as v6.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 include/linux/sunrpc/cache.h |  1 +
 net/sunrpc/cache.c           | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

Comments

NeilBrown July 29, 2015, 2:29 a.m. UTC | #1
On Mon, 27 Jul 2015 11:10:45 +0800 Kinglong Mee <kinglongmee@gmail.com>
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@gmail.com>
> ---
>  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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kinglong Mee July 30, 2015, 1:14 p.m. UTC | #2
On 7/29/2015 10:29, NeilBrown wrote:
> On Mon, 27 Jul 2015 11:10:45 +0800 Kinglong Mee <kinglongmee@gmail.com>
> 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@gmail.com>
>> ---
>>  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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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 ...
  */