ACK: [SRU X][PATCH 0/6] NFS FSCache Fixes: LP: #1774336, #1776277, #1776254

[Khaled Elmously]

On 2018-08-02 14:18:04 , Daniel Axtens wrote:
> Hi all,
> 
> This is a series of patches that solves a set of bugs experienced when
> a NFS share with a FSCache/CacheFiles backend experiences heavy
> load. There are 3 relevant LP bugs:
> 
> LP: #1774336 - FS-Cache: Assertion failed: FS-Cache: 6 == 5 is false -
> We included a sauce patch to fix this because upstream hadn't done
> anything in months. Then upstream acted, and they took a different
> patch to the one I proposed. Revert that patch (#1), and bring in the
> upstream replacement (#2, #3)
> 
> LP: #1776277 - error when tearing down a NFS share clashes with an
> attempt to use a fscache object.  This is fixed by #4.
> 
> LP: #1776254 - race where a new cookie for an object could clash with
> a cookie in the process of being destroyed. Fixed in #5 and #6.
> 
> SRU Justifications:
>  - LP: #1774336 - inlined below.
>  - LP: #1776277 - inlined in patch 4
>  - LP: #1776254 - inlined in patch 5
> 
> Apologies that this is a bit messy. I didn't want to split the series
> because it went in to upstream as one series, but I can split it to
> match LP bugs and submit it piecewise if anyone would prefer.
> 
> == SRU Justification (LP: #177336) ==
> 
> [Impact]
> Oops during heavy NFS + FSCache use:
> 
> [81738.886634] FS-Cache:
> [81738.888281] FS-Cache: Assertion failed
> [81738.889461] FS-Cache: 6 == 5 is false
> [81738.890625] ------------[ cut here ]------------
> [81738.891706] kernel BUG at /build/linux-hVVhWi/linux-4.4.0/fs/fscache/operation.c:494!
> 
> 6 == 5 represents an operation being DEAD when it was not expected to be.
> 
> [Cause]
> There is a race in fscache and cachefiles.
> 
> One thread is in cachefiles_read_waiter:
>  1) object->work_lock is taken.
>  2) the operation is added to the to_do list.
>  3) the work lock is dropped.
>  4) fscache_enqueue_retrieval is called, which takes a reference.
> 
> Another thread is in cachefiles_read_copier:
>  1) object->work_lock is taken
>  2) an item is popped off the to_do list.
>  3) object->work_lock is dropped.
>  4) some processing is done on the item, and fscache_put_retrieval() is called, dropping a reference.
> 
> Now if the this process in cachefiles_read_copier takes place *between* steps 3 and 4 in cachefiles_read_waiter, a reference will be dropped before it is taken, which leads to the objects reference count hitting zero, which leads to lifecycle events for the object happening too soon, leading to the assertion failure later on.
> 
> (This is simplified and clarified from the original upstream analysis for this patch at https://www.redhat.com/archives/linux-cachefs/2018-February/msg00001.html and from a similar patch with a different approach to fixing the bug at https://www.redhat.com/archives/linux-cachefs/2017-June/msg00002.html)
> 
> [Fix]
> 
>  (Old sauce patch being reverted) Move fscache_enqueue_retrieval under the lock in cachefiles_read_waiter. This means that the object cannot be popped off the to_do list until it is in a fully consistent state with the reference taken.
> 
>  (New upstream patch) Explicitly take a reference to the object while it is being enqueued. Adjust another part of the code to deal with the greater range of object states this exposes.
> 
> [Testcase]
> A user has run ~100 hours of NFS stress tests and not seen this bug recur.
> 
> [Regression Potential]
>  - Limited to fscache/cachefiles.
>  - The change makes things more conservative (taking more references) so that's reassuring.
>  - There may be performance impacts but none have been observed so far.
> 
> =================
> 
> Daniel Axtens (1):
>   Revert "UBUNTU: SAUCE: CacheFiles: fix a read_waiter/read_copier race"
> 
> Kiran Kumar Modukuri (5):
>   fscache: Allow cancelled operations to be enqueued
>   cachefiles: Fix refcounting bug in backing-file read monitoring
>   fscache: Fix reference overput in fscache_attach_object() error
>     handling
>   cachefiles: Fix missing clear of the CACHEFILES_OBJECT_ACTIVE flag
>   cachefiles: Wait rather than BUG'ing on "Unexpected object collision"
> 
>  fs/cachefiles/bind.c   |  3 ++-
>  fs/cachefiles/namei.c  |  4 ++--
>  fs/cachefiles/rdwr.c   | 17 ++++++++++++-----
>  fs/fscache/cache.c     |  2 +-
>  fs/fscache/cookie.c    |  7 ++++---
>  fs/fscache/object.c    |  1 +
>  fs/fscache/operation.c |  6 ++++--
>  7 files changed, 26 insertions(+), 14 deletions(-)
> 

Acked-by: Khalid Elmously <khalid.elmously at canonical.com>