ACK/Cmnt: [SRU X][PATCH v2 1/1] UBUNTU: SAUCE: cachefiles: Page leaking in cachefiles_read_backing_file while vmscan is active

[Stefan Bader]

On 24.09.2018 04:20, Daniel Axtens wrote:
> From: Kiran Kumar Modukuri <kiran.modukuri at gmail.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1793430
> 
> [Description]
> In a heavily loaded system where the system pagecache is nearing memory limits and fscache is enabled,
> pages can be leaked by fscache while trying read pages from cachefiles backend.
> This can happen because two applications can be reading same page from a single mount,
> two threads can be trying to read the backing page at same time. This results in one of the thread
> finding that a page for the backing file or netfs file is already in the radix tree. During the error
> handling cachefiles does not cleanup the reference on backing page, leading to page leak.
> 
> [Fix]
> The fix is straightforward, to decrement the reference when error is encounterd.
> 
> [Testing]
> I have tested the fix using following method for 12+ hrs.
> 
> 1) mkdir -p /mnt/nfs ; mount -o vers=3,fsc <server_ip>:/export /mnt/nfs
> 2) create 10000 files of 2.8MB in a NFS mount.
> 3) start a thread to simulate heavy VM presssure
>    (while true ; do echo 3 > /proc/sys/vm/drop_caches ; sleep 1 ; done)&
> 4) start multiple parallel reader for data set at same time
>    find /mnt/nfs -type f | xargs -P 80 cat > /dev/null &
>    find /mnt/nfs -type f | xargs -P 80 cat > /dev/null &
>    find /mnt/nfs -type f | xargs -P 80 cat > /dev/null &
>    ..
>    ..
>    find /mnt/nfs -type f | xargs -P 80 cat > /dev/null &
>    find /mnt/nfs -type f | xargs -P 80 cat > /dev/null &
> 5) finally check using cat /proc/fs/fscache/stats | grep -i pages ;
>    free -h , cat /proc/meminfo and page-types -r -b lru
>    to ensure all pages are freed.
> 
> Reviewed-by: Daniel Axtens <dja at axtens.net>
> Signed-off-by: Shantanu Goel <sgoel01 at yahoo.com>
> Signed-off-by: Kiran Kumar Modukuri <kiran.modukuri at gmail.com>
> [dja: forward ported to current upstream]
> Signed-off-by: Daniel Axtens <dja at axtens.net>
> [backported from
>  https://www.redhat.com/archives/linux-cachefs/2018-September/msg00002.html
>  This is v3 of the patch. v2 has sat on the list for weeks without
>  any response or forward progress. v1 was first posted in 2014 and
>  was reposted this August.]
> Signed-off-by: Daniel Axtens <daniel.axtens at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> 
> ---

Looks sensible so far and claims to have well testing. Would be slightly more
nice if we had something upstream to compare agains, but meh.

-Stefan

> 
> Canonical v2/Upstream v3: Clean up per Seth's feedback.
> ---
>  fs/cachefiles/rdwr.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/cachefiles/rdwr.c b/fs/cachefiles/rdwr.c
> index 5b68cf526887..98e2b89dc47e 100644
> --- a/fs/cachefiles/rdwr.c
> +++ b/fs/cachefiles/rdwr.c
> @@ -513,6 +513,8 @@ static int cachefiles_read_backing_file(struct cachefiles_object *object,
>  				goto installed_new_backing_page;
>  			if (ret != -EEXIST)
>  				goto nomem;
> +			page_cache_release(newpage);
> +			newpage = NULL;
>  		}
>  
>  		/* we've installed a new backing page, so now we need
> @@ -537,7 +539,10 @@ static int cachefiles_read_backing_file(struct cachefiles_object *object,
>  					    netpage->index, cachefiles_gfp);
>  		if (ret < 0) {
>  			if (ret == -EEXIST) {
> +				page_cache_release(backpage);
> +				backpage = NULL;
>  				page_cache_release(netpage);
> +				netpage = NULL;
>  				fscache_retrieval_complete(op, 1);
>  				continue;
>  			}
> @@ -610,7 +615,10 @@ static int cachefiles_read_backing_file(struct cachefiles_object *object,
>  					    netpage->index, cachefiles_gfp);
>  		if (ret < 0) {
>  			if (ret == -EEXIST) {
> +				page_cache_release(backpage);
> +				backpage = NULL;
>  				page_cache_release(netpage);
> +				netpage = NULL;
>  				fscache_retrieval_complete(op, 1);
>  				continue;
>  			}
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180927/02f175d5/attachment.sig>