Hi Kiran,
> Note: This is the version 2 of the patch containing additional fixes, following the ported change by Daniel, Axtens and original change by Shantanu, Goel.
Thanks for revising this. I have now run through all the paths through
the function (I think!!) with pen and paper, keeping track of references
taken to newpage and backpage. (Assuming I haven't missed any,) I can
see that we now release all the references you take.
This exercise also convinced me that I was misguided in my earlier
concern.
So, as far as I am concerned:
Reviewed-by: Daniel Axtens <dja@xxxxxxxxxx>
Regards,
Daniel
> [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.
>
> Subject: [PATCH:v2] cachefiles: page reference leak fix when vmscan is active
>
> Signed-off-by: Shantanu Goel <sgoel01@xxxxxxxxx>
> Signed-off-by: Kiran Kumar Modukuri <kiran.modukuri@xxxxxxxxx>
> [dja: forward ported to current upstream]
> Signed-off-by: Daniel Axtens <dja@xxxxxxxxxx>
> ---
> fs/cachefiles/rdwr.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/fs/cachefiles/rdwr.c b/fs/cachefiles/rdwr.c
> index 40f7595..9dc87d4 100644
> --- a/fs/cachefiles/rdwr.c
> +++ b/fs/cachefiles/rdwr.c
> @@ -274,6 +274,8 @@ static int cachefiles_read_backing_file_one(struct cachefiles_object *object,
> goto installed_new_backing_page;
> if (ret != -EEXIST)
> goto nomem_page;
> + put_page(newpage);
> + newpage = NULL;
> }
>
> /* we've installed a new backing page, so now we need to start
> @@ -511,6 +513,8 @@ static int cachefiles_read_backing_file(struct cachefiles_object *object,
> goto installed_new_backing_page;
> if (ret != -EEXIST)
> goto nomem;
> + put_page(newpage);
> + newpage = NULL;
> }
>
> /* we've installed a new backing page, so now we need
> @@ -535,7 +539,10 @@ static int cachefiles_read_backing_file(struct cachefiles_object *object,
> netpage->index, cachefiles_gfp);
> if (ret < 0) {
> if (ret == -EEXIST) {
> + put_page(backpage);
> + backpage = NULL;
> put_page(netpage);
> + netpage = NULL;
> fscache_retrieval_complete(op, 1);
> continue;
> }
> @@ -608,6 +615,8 @@ static int cachefiles_read_backing_file(struct cachefiles_object *object,
> netpage->index, cachefiles_gfp);
> if (ret < 0) {
> if (ret == -EEXIST) {
> + put_page(backpage);
> + backpage = NULL;
> put_page(netpage);
> fscache_retrieval_complete(op, 1);
> continue;
> --
> 2.7.4
--
Linux-cachefs mailing list
Linux-cachefs@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/linux-cachefs
