diff mbox series

SUNRPC/cache: don't allow invalid entries to be flushed

Message ID 20200327155309.GA135601@pick.fieldses.org (mailing list archive)
State New, archived
Headers show
Series SUNRPC/cache: don't allow invalid entries to be flushed | expand

Commit Message

Bruce Fields March 27, 2020, 3:53 p.m. UTC 
From: "J. Bruce Fields" <bfields@redhat.com>

Trond points out in 277f27e2f277 that we allow invalid cache entries to
persist indefinitely.  That fix, however, reintroduces the problem fixed
by Kinglong Mee's d6fc8821c2d2 "SUNRPC/Cache: Always treat the invalid
cache as unexpired", where an invalid cache entry is immediately removed
by a flush before mountd responds to it.  The result is that the server
thread that should be waiting for mountd to fill in that entry instead
gets an -ETIMEDOUT return from cache_check().  Symptoms are the server
becoming unresponsive after a restart, reproduceable by running pynfs
4.1 test REBT5.

Instead, take a compromise approach: allow invalid cache entries to be
removed after they expire, but not to be removed by a cache flush.

Fixes: 277f27e2f277 "SUNRPC/cache: Allow garbage collection..."
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 include/linux/sunrpc/cache.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Chuck Lever March 27, 2020, 4:15 p.m. UTC | #1 
> On Mar 27, 2020, at 11:53 AM, J. Bruce Fields <bfields@redhat.com> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> Trond points out in 277f27e2f277 that we allow invalid cache entries to
> persist indefinitely.  That fix, however, reintroduces the problem fixed
> by Kinglong Mee's d6fc8821c2d2 "SUNRPC/Cache: Always treat the invalid
> cache as unexpired", where an invalid cache entry is immediately removed
> by a flush before mountd responds to it.  The result is that the server
> thread that should be waiting for mountd to fill in that entry instead
> gets an -ETIMEDOUT return from cache_check().  Symptoms are the server
> becoming unresponsive after a restart, reproduceable by running pynfs
> 4.1 test REBT5.
> 
> Instead, take a compromise approach: allow invalid cache entries to be
> removed after they expire, but not to be removed by a cache flush.
> 
> Fixes: 277f27e2f277 "SUNRPC/cache: Allow garbage collection..."
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>

Thanks, Bruce. checkpatch.pl is complaining loudly about the style of
the short commit descriptions. I'll fix those up before applying it to
nfsd-5.7.


> ---
> include/linux/sunrpc/cache.h | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
> index 532cdbda43da..10891b70fc7b 100644
> --- a/include/linux/sunrpc/cache.h
> +++ b/include/linux/sunrpc/cache.h
> @@ -209,8 +209,11 @@ static inline void cache_put(struct cache_head *h, struct cache_detail *cd)
> 
> static inline bool cache_is_expired(struct cache_detail *detail, struct cache_head *h)
> {
> -	return  (h->expiry_time < seconds_since_boot()) ||
> -		(detail->flush_time >= h->last_refresh);
> +	if (h->expiry_time < seconds_since_boot())
> +		return true;
> +	if (!test_bit(CACHE_VALID, &h->flags))
> +		return false;
> +	return detail->flush_time >= h->last_refresh;
> }
> 
> extern int cache_check(struct cache_detail *detail,
> -- 
> 2.25.1
> 

--
Chuck Lever
diff mbox series

Patch

diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
index 532cdbda43da..10891b70fc7b 100644
--- a/include/linux/sunrpc/cache.h
+++ b/include/linux/sunrpc/cache.h
@@ -209,8 +209,11 @@  static inline void cache_put(struct cache_head *h, struct cache_detail *cd)
 
 static inline bool cache_is_expired(struct cache_detail *detail, struct cache_head *h)
 {
-	return  (h->expiry_time < seconds_since_boot()) ||
-		(detail->flush_time >= h->last_refresh);
+	if (h->expiry_time < seconds_since_boot())
+		return true;
+	if (!test_bit(CACHE_VALID, &h->flags))
+		return false;
+	return detail->flush_time >= h->last_refresh;
 }
 
 extern int cache_check(struct cache_detail *detail,