Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/4] Readdir fixes
@ 2020-02-02 22:53 Trond Myklebust
  2020-02-02 22:53 ` [PATCH 1/4] NFS: Fix memory leaks and corruption in readdir Trond Myklebust
  0 siblings, 1 reply; 16+ messages in thread
From: Trond Myklebust @ 2020-02-02 22:53 UTC (permalink / raw)
  To: Anna Schumaker; +Cc: Benjamin Coddington, Dai Ngo, linux-nfs

Two stable patches that fix memory corruption and a memory leak in the
readdir code. Fixing those issues together with the patch by Dai Ngo
then allows us to switch NFS over to the iterate_shared() interface
to enable parallel access to readdir() for the same directory.

Trond Myklebust (4):
  NFS: Fix memory leaks and corruption in readdir
  NFS: Directory page cache pages need to be locked when read
  NFS: Use kmemdup_nul() in nfs_readdir_make_qstr()
  NFS: Switch readdir to using iterate_shared()

 fs/nfs/dir.c | 51 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 15 deletions(-)

-- 
2.24.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/4] NFS: Fix memory leaks and corruption in readdir
  2020-02-02 22:53 [PATCH 0/4] Readdir fixes Trond Myklebust
@ 2020-02-02 22:53 ` Trond Myklebust
  2020-02-02 22:53   ` [PATCH 2/4] NFS: Directory page cache pages need to be locked when read Trond Myklebust
  2020-02-03 13:44   ` [PATCH 1/4] NFS: Fix memory leaks and corruption in readdir Benjamin Coddington
  0 siblings, 2 replies; 16+ messages in thread
From: Trond Myklebust @ 2020-02-02 22:53 UTC (permalink / raw)
  To: Anna Schumaker; +Cc: Benjamin Coddington, Dai Ngo, linux-nfs

nfs_readdir_xdr_to_array() must not exit without having initialised
the array, so that the page cache deletion routines can safely
call nfs_readdir_clear_array().
Furthermore, we should ensure that if we exit nfs_readdir_filler()
with an error, we free up any page contents to prevent a leak
if we try to fill the page again.

Fixes: 11de3b11e08c ("NFS: Fix a memory leak in nfs_readdir")
Cc: stable@vger.kernel.org # v2.6.37+
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/dir.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 76404f53cf21..ba0d55930e8a 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -163,6 +163,17 @@ typedef struct {
 	bool eof;
 } nfs_readdir_descriptor_t;
 
+static
+void nfs_readdir_init_array(struct page *page)
+{
+	struct nfs_cache_array *array;
+
+	array = kmap_atomic(page);
+	memset(array, 0, sizeof(struct nfs_cache_array));
+	array->eof_index = -1;
+	kunmap_atomic(array);
+}
+
 /*
  * we are freeing strings created by nfs_add_to_readdir_array()
  */
@@ -175,6 +186,7 @@ void nfs_readdir_clear_array(struct page *page)
 	array = kmap_atomic(page);
 	for (i = 0; i < array->size; i++)
 		kfree(array->array[i].string.name);
+	array->size = 0;
 	kunmap_atomic(array);
 }
 
@@ -613,6 +625,8 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
 	int status = -ENOMEM;
 	unsigned int array_size = ARRAY_SIZE(pages);
 
+	nfs_readdir_init_array(page);
+
 	entry.prev_cookie = 0;
 	entry.cookie = desc->last_cookie;
 	entry.eof = 0;
@@ -629,8 +643,6 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
 	}
 
 	array = kmap(page);
-	memset(array, 0, sizeof(struct nfs_cache_array));
-	array->eof_index = -1;
 
 	status = nfs_readdir_alloc_pages(pages, array_size);
 	if (status < 0)
@@ -685,6 +697,7 @@ int nfs_readdir_filler(void *data, struct page* page)
 	unlock_page(page);
 	return 0;
  error:
+	nfs_readdir_clear_array(page);
 	unlock_page(page);
 	return ret;
 }
-- 
2.24.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/4] NFS: Directory page cache pages need to be locked when read
  2020-02-02 22:53 ` [PATCH 1/4] NFS: Fix memory leaks and corruption in readdir Trond Myklebust
@ 2020-02-02 22:53   ` Trond Myklebust
  2020-02-02 22:53     ` [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr() Trond Myklebust
                       ` (2 more replies)
  2020-02-03 13:44   ` [PATCH 1/4] NFS: Fix memory leaks and corruption in readdir Benjamin Coddington
  1 sibling, 3 replies; 16+ messages in thread
From: Trond Myklebust @ 2020-02-02 22:53 UTC (permalink / raw)
  To: Anna Schumaker; +Cc: Benjamin Coddington, Dai Ngo, linux-nfs

When a NFS directory page cache page is removed from the page cache,
its contents are freed through a call to nfs_readdir_clear_array().
To prevent the removal of the page cache entry until after we've
finished reading it, we must take the page lock.

Fixes: 11de3b11e08c ("NFS: Fix a memory leak in nfs_readdir")
Cc: stable@vger.kernel.org # v2.6.37+
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/dir.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index ba0d55930e8a..90467b44ec13 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -705,8 +705,6 @@ int nfs_readdir_filler(void *data, struct page* page)
 static
 void cache_page_release(nfs_readdir_descriptor_t *desc)
 {
-	if (!desc->page->mapping)
-		nfs_readdir_clear_array(desc->page);
 	put_page(desc->page);
 	desc->page = NULL;
 }
@@ -720,19 +718,28 @@ struct page *get_cache_page(nfs_readdir_descriptor_t *desc)
 
 /*
  * Returns 0 if desc->dir_cookie was found on page desc->page_index
+ * and locks the page to prevent removal from the page cache.
  */
 static
-int find_cache_page(nfs_readdir_descriptor_t *desc)
+int find_and_lock_cache_page(nfs_readdir_descriptor_t *desc)
 {
 	int res;
 
 	desc->page = get_cache_page(desc);
 	if (IS_ERR(desc->page))
 		return PTR_ERR(desc->page);
-
-	res = nfs_readdir_search_array(desc);
+	res = lock_page_killable(desc->page);
 	if (res != 0)
-		cache_page_release(desc);
+		goto error;
+	res = -EAGAIN;
+	if (desc->page->mapping != NULL) {
+		res = nfs_readdir_search_array(desc);
+		if (res == 0)
+			return 0;
+	}
+	unlock_page(desc->page);
+error:
+	cache_page_release(desc);
 	return res;
 }
 
@@ -747,7 +754,7 @@ int readdir_search_pagecache(nfs_readdir_descriptor_t *desc)
 		desc->last_cookie = 0;
 	}
 	do {
-		res = find_cache_page(desc);
+		res = find_and_lock_cache_page(desc);
 	} while (res == -EAGAIN);
 	return res;
 }
@@ -786,7 +793,6 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc)
 		desc->eof = true;
 
 	kunmap(desc->page);
-	cache_page_release(desc);
 	dfprintk(DIRCACHE, "NFS: nfs_do_filldir() filling ended @ cookie %Lu; returning = %d\n",
 			(unsigned long long)*desc->dir_cookie, res);
 	return res;
@@ -832,13 +838,13 @@ int uncached_readdir(nfs_readdir_descriptor_t *desc)
 
 	status = nfs_do_filldir(desc);
 
+ out_release:
+	nfs_readdir_clear_array(desc->page);
+	cache_page_release(desc);
  out:
 	dfprintk(DIRCACHE, "NFS: %s: returns %d\n",
 			__func__, status);
 	return status;
- out_release:
-	cache_page_release(desc);
-	goto out;
 }
 
 /* The file offset position represents the dirent entry number.  A
@@ -903,6 +909,8 @@ static int nfs_readdir(struct file *file, struct dir_context *ctx)
 			break;
 
 		res = nfs_do_filldir(desc);
+		unlock_page(desc->page);
+		cache_page_release(desc);
 		if (res < 0)
 			break;
 	} while (!desc->eof);
-- 
2.24.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr()
  2020-02-02 22:53   ` [PATCH 2/4] NFS: Directory page cache pages need to be locked when read Trond Myklebust
@ 2020-02-02 22:53     ` Trond Myklebust
  2020-02-02 22:53       ` [PATCH 4/4] NFS: Switch readdir to using iterate_shared() Trond Myklebust
  2020-02-03 14:22       ` [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr() Benjamin Coddington
  2020-02-03 14:21     ` [PATCH 2/4] NFS: Directory page cache pages need to be locked when read Benjamin Coddington
  2020-02-03 20:31     ` Schumaker, Anna
  2 siblings, 2 replies; 16+ messages in thread
From: Trond Myklebust @ 2020-02-02 22:53 UTC (permalink / raw)
  To: Anna Schumaker; +Cc: Benjamin Coddington, Dai Ngo, linux-nfs

The directory strings stored in the readdir cache may be used with
printk(), so it is better to ensure they are nul-terminated.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 90467b44ec13..60387dec9032 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -199,7 +199,7 @@ static
 int nfs_readdir_make_qstr(struct qstr *string, const char *name, unsigned int len)
 {
 	string->len = len;
-	string->name = kmemdup(name, len, GFP_KERNEL);
+	string->name = kmemdup_nul(name, len, GFP_KERNEL);
 	if (string->name == NULL)
 		return -ENOMEM;
 	/*
-- 
2.24.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 4/4] NFS: Switch readdir to using iterate_shared()
  2020-02-02 22:53     ` [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr() Trond Myklebust
@ 2020-02-02 22:53       ` Trond Myklebust
  2020-02-03 14:41         ` Benjamin Coddington
  2020-02-03 14:22       ` [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr() Benjamin Coddington
  1 sibling, 1 reply; 16+ messages in thread
From: Trond Myklebust @ 2020-02-02 22:53 UTC (permalink / raw)
  To: Anna Schumaker; +Cc: Benjamin Coddington, Dai Ngo, linux-nfs

Now that the page cache locking is repaired, we should be able to
switch to using iterate_shared() for improved concurrency when
doing readdir().

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 60387dec9032..803e6fec0266 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -58,7 +58,7 @@ static void nfs_readdir_clear_array(struct page*);
 const struct file_operations nfs_dir_operations = {
 	.llseek		= nfs_llseek_dir,
 	.read		= generic_read_dir,
-	.iterate	= nfs_readdir,
+	.iterate_shared	= nfs_readdir,
 	.open		= nfs_opendir,
 	.release	= nfs_closedir,
 	.fsync		= nfs_fsync_dir,
-- 
2.24.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] NFS: Fix memory leaks and corruption in readdir
  2020-02-02 22:53 ` [PATCH 1/4] NFS: Fix memory leaks and corruption in readdir Trond Myklebust
  2020-02-02 22:53   ` [PATCH 2/4] NFS: Directory page cache pages need to be locked when read Trond Myklebust
@ 2020-02-03 13:44   ` Benjamin Coddington
  1 sibling, 0 replies; 16+ messages in thread
From: Benjamin Coddington @ 2020-02-03 13:44 UTC (permalink / raw)
  To: Trond Myklebust; +Cc: Anna Schumaker, Dai Ngo, linux-nfs

On 2 Feb 2020, at 17:53, Trond Myklebust wrote:

> nfs_readdir_xdr_to_array() must not exit without having initialised
> the array, so that the page cache deletion routines can safely
> call nfs_readdir_clear_array().
> Furthermore, we should ensure that if we exit nfs_readdir_filler()
> with an error, we free up any page contents to prevent a leak
> if we try to fill the page again.
>
> Fixes: 11de3b11e08c ("NFS: Fix a memory leak in nfs_readdir")
> Cc: stable@vger.kernel.org # v2.6.37+
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>

Looks good to me.

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben

> ---
>  fs/nfs/dir.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 76404f53cf21..ba0d55930e8a 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -163,6 +163,17 @@ typedef struct {
>  	bool eof;
>  } nfs_readdir_descriptor_t;
>
> +static
> +void nfs_readdir_init_array(struct page *page)
> +{
> +	struct nfs_cache_array *array;
> +
> +	array = kmap_atomic(page);
> +	memset(array, 0, sizeof(struct nfs_cache_array));
> +	array->eof_index = -1;
> +	kunmap_atomic(array);
> +}
> +
>  /*
>   * we are freeing strings created by nfs_add_to_readdir_array()
>   */
> @@ -175,6 +186,7 @@ void nfs_readdir_clear_array(struct page *page)
>  	array = kmap_atomic(page);
>  	for (i = 0; i < array->size; i++)
>  		kfree(array->array[i].string.name);
> +	array->size = 0;
>  	kunmap_atomic(array);
>  }
>
> @@ -613,6 +625,8 @@ int 
> nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page 
> *page,
>  	int status = -ENOMEM;
>  	unsigned int array_size = ARRAY_SIZE(pages);
>
> +	nfs_readdir_init_array(page);
> +
>  	entry.prev_cookie = 0;
>  	entry.cookie = desc->last_cookie;
>  	entry.eof = 0;
> @@ -629,8 +643,6 @@ int 
> nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page 
> *page,
>  	}
>
>  	array = kmap(page);
> -	memset(array, 0, sizeof(struct nfs_cache_array));
> -	array->eof_index = -1;
>
>  	status = nfs_readdir_alloc_pages(pages, array_size);
>  	if (status < 0)
> @@ -685,6 +697,7 @@ int nfs_readdir_filler(void *data, struct page* 
> page)
>  	unlock_page(page);
>  	return 0;
>   error:
> +	nfs_readdir_clear_array(page);
>  	unlock_page(page);
>  	return ret;
>  }
> -- 
> 2.24.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] NFS: Directory page cache pages need to be locked when read
  2020-02-02 22:53   ` [PATCH 2/4] NFS: Directory page cache pages need to be locked when read Trond Myklebust
  2020-02-02 22:53     ` [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr() Trond Myklebust
@ 2020-02-03 14:21     ` Benjamin Coddington
  2020-02-03 20:31     ` Schumaker, Anna
  2 siblings, 0 replies; 16+ messages in thread
From: Benjamin Coddington @ 2020-02-03 14:21 UTC (permalink / raw)
  To: Trond Myklebust; +Cc: Anna Schumaker, Dai Ngo, linux-nfs

On 2 Feb 2020, at 17:53, Trond Myklebust wrote:

> When a NFS directory page cache page is removed from the page cache,
> its contents are freed through a call to nfs_readdir_clear_array().
> To prevent the removal of the page cache entry until after we've
> finished reading it, we must take the page lock.
>
> Fixes: 11de3b11e08c ("NFS: Fix a memory leak in nfs_readdir")
> Cc: stable@vger.kernel.org # v2.6.37+
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr()
  2020-02-02 22:53     ` [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr() Trond Myklebust
  2020-02-02 22:53       ` [PATCH 4/4] NFS: Switch readdir to using iterate_shared() Trond Myklebust
@ 2020-02-03 14:22       ` Benjamin Coddington
  1 sibling, 0 replies; 16+ messages in thread
From: Benjamin Coddington @ 2020-02-03 14:22 UTC (permalink / raw)
  To: Trond Myklebust; +Cc: Anna Schumaker, Dai Ngo, linux-nfs

On 2 Feb 2020, at 17:53, Trond Myklebust wrote:

> The directory strings stored in the readdir cache may be used with
> printk(), so it is better to ensure they are nul-terminated.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/dir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 90467b44ec13..60387dec9032 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -199,7 +199,7 @@ static
>  int nfs_readdir_make_qstr(struct qstr *string, const char *name, 
> unsigned int len)
>  {
>  	string->len = len;
> -	string->name = kmemdup(name, len, GFP_KERNEL);
> +	string->name = kmemdup_nul(name, len, GFP_KERNEL);
>  	if (string->name == NULL)
>  		return -ENOMEM;
>  	/*

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/4] NFS: Switch readdir to using iterate_shared()
  2020-02-02 22:53       ` [PATCH 4/4] NFS: Switch readdir to using iterate_shared() Trond Myklebust
@ 2020-02-03 14:41         ` Benjamin Coddington
  0 siblings, 0 replies; 16+ messages in thread
From: Benjamin Coddington @ 2020-02-03 14:41 UTC (permalink / raw)
  To: Trond Myklebust; +Cc: Anna Schumaker, Dai Ngo, linux-nfs

On 2 Feb 2020, at 17:53, Trond Myklebust wrote:

> Now that the page cache locking is repaired, we should be able to
> switch to using iterate_shared() for improved concurrency when
> doing readdir().
>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/dir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 60387dec9032..803e6fec0266 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -58,7 +58,7 @@ static void nfs_readdir_clear_array(struct page*);
>  const struct file_operations nfs_dir_operations = {
>  	.llseek		= nfs_llseek_dir,
>  	.read		= generic_read_dir,
> -	.iterate	= nfs_readdir,
> +	.iterate_shared	= nfs_readdir,
>  	.open		= nfs_opendir,
>  	.release	= nfs_closedir,
>  	.fsync		= nfs_fsync_dir,

Jeez, it makes a lot of sense just to use the page lock.  Wish I could have
thought of that a long time ago.  Thanks for this, we'll send it through
some testing, but it looks good to me.

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] NFS: Directory page cache pages need to be locked when read
  2020-02-02 22:53   ` [PATCH 2/4] NFS: Directory page cache pages need to be locked when read Trond Myklebust
  2020-02-02 22:53     ` [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr() Trond Myklebust
  2020-02-03 14:21     ` [PATCH 2/4] NFS: Directory page cache pages need to be locked when read Benjamin Coddington
@ 2020-02-03 20:31     ` Schumaker, Anna
  2020-02-03 21:18       ` Trond Myklebust
  2 siblings, 1 reply; 16+ messages in thread
From: Schumaker, Anna @ 2020-02-03 20:31 UTC (permalink / raw)
  To: trondmy; +Cc: dai.ngo, linux-nfs, bcodding

Hi Trond,

On Sun, 2020-02-02 at 17:53 -0500, Trond Myklebust wrote:
> When a NFS directory page cache page is removed from the page cache,
> its contents are freed through a call to nfs_readdir_clear_array().
> To prevent the removal of the page cache entry until after we've
> finished reading it, we must take the page lock.
> 
> Fixes: 11de3b11e08c ("NFS: Fix a memory leak in nfs_readdir")
> Cc: stable@vger.kernel.org # v2.6.37+
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/dir.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index ba0d55930e8a..90467b44ec13 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -705,8 +705,6 @@ int nfs_readdir_filler(void *data, struct page* page)
>  static
>  void cache_page_release(nfs_readdir_descriptor_t *desc)
>  {
> -	if (!desc->page->mapping)
> -		nfs_readdir_clear_array(desc->page);
>  	put_page(desc->page);
>  	desc->page = NULL;
>  }
> @@ -720,19 +718,28 @@ struct page *get_cache_page(nfs_readdir_descriptor_t
> *desc)
>  
>  /*
>   * Returns 0 if desc->dir_cookie was found on page desc->page_index
> + * and locks the page to prevent removal from the page cache.
>   */
>  static
> -int find_cache_page(nfs_readdir_descriptor_t *desc)
> +int find_and_lock_cache_page(nfs_readdir_descriptor_t *desc)
>  {
>  	int res;
>  
>  	desc->page = get_cache_page(desc);
>  	if (IS_ERR(desc->page))
>  		return PTR_ERR(desc->page);
> -
> -	res = nfs_readdir_search_array(desc);
> +	res = lock_page_killable(desc->page);
>  	if (res != 0)
> -		cache_page_release(desc);
> +		goto error;
> +	res = -EAGAIN;
> +	if (desc->page->mapping != NULL) {
> +		res = nfs_readdir_search_array(desc);
> +		if (res == 0)
> +			return 0;
> +	}
> +	unlock_page(desc->page);
> +error:
> +	cache_page_release(desc);
>  	return res;
>  }
>  

Can you give me some guidance on how to apply this on top of Dai's v2 patch from
January 23? Right now I have the nfsi->page_index getting set before the
unlock_page():

@@ -732,15 +733,24 @@ int find_cache_page(nfs_readdir_descriptor_t *desc)
 	if (IS_ERR(desc->page))
 		return PTR_ERR(desc->page);
 
-	res = nfs_readdir_search_array(desc);
+	res = lock_page_killable(desc->page);
 	if (res != 0)
 		cache_page_release(desc);
+		goto error;
+	res = -EAGAIN;
+	if (desc->page->mapping != NULL) {
+		res = nfs_readdir_search_array(desc);
+		if (res == 0)
+			return 0;
+	}
 
 	dentry = file_dentry(desc->file);
 	inode = d_inode(dentry);
 	nfsi = NFS_I(inode);
 	nfsi->page_index = desc->page_index;
-
+	unlock_page(desc->page);
+error:
+	cache_page_release(desc);
 	return res;
 }
 

Please let me know if there is a better way to handle the conflict!

Anna


> @@ -747,7 +754,7 @@ int readdir_search_pagecache(nfs_readdir_descriptor_t
> *desc)
>  		desc->last_cookie = 0;
>  	}
>  	do {
> -		res = find_cache_page(desc);
> +		res = find_and_lock_cache_page(desc);
>  	} while (res == -EAGAIN);
>  	return res;
>  }
> @@ -786,7 +793,6 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc)
>  		desc->eof = true;
>  
>  	kunmap(desc->page);
> -	cache_page_release(desc);
>  	dfprintk(DIRCACHE, "NFS: nfs_do_filldir() filling ended @ cookie %Lu;
> returning = %d\n",
>  			(unsigned long long)*desc->dir_cookie, res);
>  	return res;
> @@ -832,13 +838,13 @@ int uncached_readdir(nfs_readdir_descriptor_t *desc)
>  
>  	status = nfs_do_filldir(desc);
>  
> + out_release:
> +	nfs_readdir_clear_array(desc->page);
> +	cache_page_release(desc);
>   out:
>  	dfprintk(DIRCACHE, "NFS: %s: returns %d\n",
>  			__func__, status);
>  	return status;
> - out_release:
> -	cache_page_release(desc);
> -	goto out;
>  }
>  
>  /* The file offset position represents the dirent entry number.  A
> @@ -903,6 +909,8 @@ static int nfs_readdir(struct file *file, struct
> dir_context *ctx)
>  			break;
>  
>  		res = nfs_do_filldir(desc);
> +		unlock_page(desc->page);
> +		cache_page_release(desc);
>  		if (res < 0)
>  			break;
>  	} while (!desc->eof);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] NFS: Directory page cache pages need to be locked when read
  2020-02-03 20:31     ` Schumaker, Anna
@ 2020-02-03 21:18       ` Trond Myklebust
  2020-02-03 21:21         ` Schumaker, Anna
  2020-02-05  0:44         ` Dai Ngo
  0 siblings, 2 replies; 16+ messages in thread
From: Trond Myklebust @ 2020-02-03 21:18 UTC (permalink / raw)
  To: Schumaker, Anna; +Cc: dai.ngo, linux-nfs, bcodding

On Mon, 2020-02-03 at 20:31 +0000, Schumaker, Anna wrote:
> Hi Trond,
> 
> On Sun, 2020-02-02 at 17:53 -0500, Trond Myklebust wrote:
> > When a NFS directory page cache page is removed from the page
> > cache,
> > its contents are freed through a call to nfs_readdir_clear_array().
> > To prevent the removal of the page cache entry until after we've
> > finished reading it, we must take the page lock.
> > 
> > Fixes: 11de3b11e08c ("NFS: Fix a memory leak in nfs_readdir")
> > Cc: stable@vger.kernel.org # v2.6.37+
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/dir.c | 30 +++++++++++++++++++-----------
> >  1 file changed, 19 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index ba0d55930e8a..90467b44ec13 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -705,8 +705,6 @@ int nfs_readdir_filler(void *data, struct page*
> > page)
> >  static
> >  void cache_page_release(nfs_readdir_descriptor_t *desc)
> >  {
> > -	if (!desc->page->mapping)
> > -		nfs_readdir_clear_array(desc->page);
> >  	put_page(desc->page);
> >  	desc->page = NULL;
> >  }
> > @@ -720,19 +718,28 @@ struct page
> > *get_cache_page(nfs_readdir_descriptor_t
> > *desc)
> >  
> >  /*
> >   * Returns 0 if desc->dir_cookie was found on page desc-
> > >page_index
> > + * and locks the page to prevent removal from the page cache.
> >   */
> >  static
> > -int find_cache_page(nfs_readdir_descriptor_t *desc)
> > +int find_and_lock_cache_page(nfs_readdir_descriptor_t *desc)
> >  {
> >  	int res;
> >  
> >  	desc->page = get_cache_page(desc);
> >  	if (IS_ERR(desc->page))
> >  		return PTR_ERR(desc->page);
> > -
> > -	res = nfs_readdir_search_array(desc);
> > +	res = lock_page_killable(desc->page);
> >  	if (res != 0)
> > -		cache_page_release(desc);
> > +		goto error;
> > +	res = -EAGAIN;
> > +	if (desc->page->mapping != NULL) {
> > +		res = nfs_readdir_search_array(desc);
> > +		if (res == 0)
> > +			return 0;
> > +	}
> > +	unlock_page(desc->page);
> > +error:
> > +	cache_page_release(desc);
> >  	return res;
> >  }
> >  
> 
> Can you give me some guidance on how to apply this on top of Dai's v2
> patch from
> January 23? Right now I have the nfsi->page_index getting set before
> the
> unlock_page():

Since this needs to be a stable patch, it would be preferable to apply
them in the opposite order to avoid an extra dependency on Dai's patch.

That said, since the nfsi->page_index is not a per-page variable, there
is no need to put it under the page lock.


> @@ -732,15 +733,24 @@ int find_cache_page(nfs_readdir_descriptor_t
> *desc)
>  	if (IS_ERR(desc->page))
>  		return PTR_ERR(desc->page);
>  
> -	res = nfs_readdir_search_array(desc);
> +	res = lock_page_killable(desc->page);
>  	if (res != 0)
>  		cache_page_release(desc);
> +		goto error;
> +	res = -EAGAIN;
> +	if (desc->page->mapping != NULL) {
> +		res = nfs_readdir_search_array(desc);
> +		if (res == 0)
> +			return 0;
> +	}
>  
>  	dentry = file_dentry(desc->file);
>  	inode = d_inode(dentry);
>  	nfsi = NFS_I(inode);
>  	nfsi->page_index = desc->page_index;
> -
> +	unlock_page(desc->page);
> +error:
> +	cache_page_release(desc);
>  	return res;
>  }
>  
> 
> Please let me know if there is a better way to handle the conflict!
> 
> Anna
> 
> 
> > @@ -747,7 +754,7 @@ int
> > readdir_search_pagecache(nfs_readdir_descriptor_t
> > *desc)
> >  		desc->last_cookie = 0;
> >  	}
> >  	do {
> > -		res = find_cache_page(desc);
> > +		res = find_and_lock_cache_page(desc);
> >  	} while (res == -EAGAIN);
> >  	return res;
> >  }
> > @@ -786,7 +793,6 @@ int nfs_do_filldir(nfs_readdir_descriptor_t
> > *desc)
> >  		desc->eof = true;
> >  
> >  	kunmap(desc->page);
> > -	cache_page_release(desc);
> >  	dfprintk(DIRCACHE, "NFS: nfs_do_filldir() filling ended @
> > cookie %Lu;
> > returning = %d\n",
> >  			(unsigned long long)*desc->dir_cookie, res);
> >  	return res;
> > @@ -832,13 +838,13 @@ int uncached_readdir(nfs_readdir_descriptor_t
> > *desc)
> >  
> >  	status = nfs_do_filldir(desc);
> >  
> > + out_release:
> > +	nfs_readdir_clear_array(desc->page);
> > +	cache_page_release(desc);
> >   out:
> >  	dfprintk(DIRCACHE, "NFS: %s: returns %d\n",
> >  			__func__, status);
> >  	return status;
> > - out_release:
> > -	cache_page_release(desc);
> > -	goto out;
> >  }
> >  
> >  /* The file offset position represents the dirent entry number.  A
> > @@ -903,6 +909,8 @@ static int nfs_readdir(struct file *file,
> > struct
> > dir_context *ctx)
> >  			break;
> >  
> >  		res = nfs_do_filldir(desc);
> > +		unlock_page(desc->page);
> > +		cache_page_release(desc);
> >  		if (res < 0)
> >  			break;
> >  	} while (!desc->eof);


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] NFS: Directory page cache pages need to be locked when read
  2020-02-03 21:18       ` Trond Myklebust
@ 2020-02-03 21:21         ` Schumaker, Anna
  2020-02-03 22:45           ` Trond Myklebust
  2020-02-05  0:44         ` Dai Ngo
  1 sibling, 1 reply; 16+ messages in thread
From: Schumaker, Anna @ 2020-02-03 21:21 UTC (permalink / raw)
  To: trondmy; +Cc: dai.ngo, linux-nfs, bcodding

On Mon, 2020-02-03 at 16:18 -0500, Trond Myklebust wrote:
> On Mon, 2020-02-03 at 20:31 +0000, Schumaker, Anna wrote:
> > Hi Trond,
> > 
> > On Sun, 2020-02-02 at 17:53 -0500, Trond Myklebust wrote:
> > > When a NFS directory page cache page is removed from the page
> > > cache,
> > > its contents are freed through a call to nfs_readdir_clear_array().
> > > To prevent the removal of the page cache entry until after we've
> > > finished reading it, we must take the page lock.
> > > 
> > > Fixes: 11de3b11e08c ("NFS: Fix a memory leak in nfs_readdir")
> > > Cc: stable@vger.kernel.org # v2.6.37+
> > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > ---
> > >  fs/nfs/dir.c | 30 +++++++++++++++++++-----------
> > >  1 file changed, 19 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > index ba0d55930e8a..90467b44ec13 100644
> > > --- a/fs/nfs/dir.c
> > > +++ b/fs/nfs/dir.c
> > > @@ -705,8 +705,6 @@ int nfs_readdir_filler(void *data, struct page*
> > > page)
> > >  static
> > >  void cache_page_release(nfs_readdir_descriptor_t *desc)
> > >  {
> > > -	if (!desc->page->mapping)
> > > -		nfs_readdir_clear_array(desc->page);
> > >  	put_page(desc->page);
> > >  	desc->page = NULL;
> > >  }
> > > @@ -720,19 +718,28 @@ struct page
> > > *get_cache_page(nfs_readdir_descriptor_t
> > > *desc)
> > >  
> > >  /*
> > >   * Returns 0 if desc->dir_cookie was found on page desc-
> > > > page_index
> > > + * and locks the page to prevent removal from the page cache.
> > >   */
> > >  static
> > > -int find_cache_page(nfs_readdir_descriptor_t *desc)
> > > +int find_and_lock_cache_page(nfs_readdir_descriptor_t *desc)
> > >  {
> > >  	int res;
> > >  
> > >  	desc->page = get_cache_page(desc);
> > >  	if (IS_ERR(desc->page))
> > >  		return PTR_ERR(desc->page);
> > > -
> > > -	res = nfs_readdir_search_array(desc);
> > > +	res = lock_page_killable(desc->page);
> > >  	if (res != 0)
> > > -		cache_page_release(desc);
> > > +		goto error;
> > > +	res = -EAGAIN;
> > > +	if (desc->page->mapping != NULL) {
> > > +		res = nfs_readdir_search_array(desc);
> > > +		if (res == 0)
> > > +			return 0;
> > > +	}
> > > +	unlock_page(desc->page);
> > > +error:
> > > +	cache_page_release(desc);
> > >  	return res;
> > >  }
> > >  
> > 
> > Can you give me some guidance on how to apply this on top of Dai's v2
> > patch from
> > January 23? Right now I have the nfsi->page_index getting set before
> > the
> > unlock_page():
> 
> Since this needs to be a stable patch, it would be preferable to apply
> them in the opposite order to avoid an extra dependency on Dai's patch.

That makes sense.

> 
> That said, since the nfsi->page_index is not a per-page variable, there
> is no need to put it under the page lock.

Got it. I'll swap the order of everything, and put the page_index outside of the
page lock when resolving the conflict.

Thanks!
Anna

> 
> 
> > @@ -732,15 +733,24 @@ int find_cache_page(nfs_readdir_descriptor_t
> > *desc)
> >  	if (IS_ERR(desc->page))
> >  		return PTR_ERR(desc->page);
> >  
> > -	res = nfs_readdir_search_array(desc);
> > +	res = lock_page_killable(desc->page);
> >  	if (res != 0)
> >  		cache_page_release(desc);
> > +		goto error;
> > +	res = -EAGAIN;
> > +	if (desc->page->mapping != NULL) {
> > +		res = nfs_readdir_search_array(desc);
> > +		if (res == 0)
> > +			return 0;
> > +	}
> >  
> >  	dentry = file_dentry(desc->file);
> >  	inode = d_inode(dentry);
> >  	nfsi = NFS_I(inode);
> >  	nfsi->page_index = desc->page_index;
> > -
> > +	unlock_page(desc->page);
> > +error:
> > +	cache_page_release(desc);
> >  	return res;
> >  }
> >  
> > 
> > Please let me know if there is a better way to handle the conflict!
> > 
> > Anna
> > 
> > 
> > > @@ -747,7 +754,7 @@ int
> > > readdir_search_pagecache(nfs_readdir_descriptor_t
> > > *desc)
> > >  		desc->last_cookie = 0;
> > >  	}
> > >  	do {
> > > -		res = find_cache_page(desc);
> > > +		res = find_and_lock_cache_page(desc);
> > >  	} while (res == -EAGAIN);
> > >  	return res;
> > >  }
> > > @@ -786,7 +793,6 @@ int nfs_do_filldir(nfs_readdir_descriptor_t
> > > *desc)
> > >  		desc->eof = true;
> > >  
> > >  	kunmap(desc->page);
> > > -	cache_page_release(desc);
> > >  	dfprintk(DIRCACHE, "NFS: nfs_do_filldir() filling ended @
> > > cookie %Lu;
> > > returning = %d\n",
> > >  			(unsigned long long)*desc->dir_cookie, res);
> > >  	return res;
> > > @@ -832,13 +838,13 @@ int uncached_readdir(nfs_readdir_descriptor_t
> > > *desc)
> > >  
> > >  	status = nfs_do_filldir(desc);
> > >  
> > > + out_release:
> > > +	nfs_readdir_clear_array(desc->page);
> > > +	cache_page_release(desc);
> > >   out:
> > >  	dfprintk(DIRCACHE, "NFS: %s: returns %d\n",
> > >  			__func__, status);
> > >  	return status;
> > > - out_release:
> > > -	cache_page_release(desc);
> > > -	goto out;
> > >  }
> > >  
> > >  /* The file offset position represents the dirent entry number.  A
> > > @@ -903,6 +909,8 @@ static int nfs_readdir(struct file *file,
> > > struct
> > > dir_context *ctx)
> > >  			break;
> > >  
> > >  		res = nfs_do_filldir(desc);
> > > +		unlock_page(desc->page);
> > > +		cache_page_release(desc);
> > >  		if (res < 0)
> > >  			break;
> > >  	} while (!desc->eof);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] NFS: Directory page cache pages need to be locked when read
  2020-02-03 21:21         ` Schumaker, Anna
@ 2020-02-03 22:45           ` Trond Myklebust
  2020-02-03 22:50             ` Trond Myklebust
  0 siblings, 1 reply; 16+ messages in thread
From: Trond Myklebust @ 2020-02-03 22:45 UTC (permalink / raw)
  To: Anna.Schumaker; +Cc: dai.ngo, linux-nfs, bcodding

On Mon, 2020-02-03 at 21:21 +0000, Schumaker, Anna wrote:
> On Mon, 2020-02-03 at 16:18 -0500, Trond Myklebust wrote:
> > On Mon, 2020-02-03 at 20:31 +0000, Schumaker, Anna wrote:
> > > Hi Trond,
> > > 
> > > On Sun, 2020-02-02 at 17:53 -0500, Trond Myklebust wrote:
> > > > When a NFS directory page cache page is removed from the page
> > > > cache,
> > > > its contents are freed through a call to
> > > > nfs_readdir_clear_array().
> > > > To prevent the removal of the page cache entry until after
> > > > we've
> > > > finished reading it, we must take the page lock.
> > > > 
> > > > Fixes: 11de3b11e08c ("NFS: Fix a memory leak in nfs_readdir")
> > > > Cc: stable@vger.kernel.org # v2.6.37+
> > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com
> > > > >
> > > > ---
> > > >  fs/nfs/dir.c | 30 +++++++++++++++++++-----------
> > > >  1 file changed, 19 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > > index ba0d55930e8a..90467b44ec13 100644
> > > > --- a/fs/nfs/dir.c
> > > > +++ b/fs/nfs/dir.c
> > > > @@ -705,8 +705,6 @@ int nfs_readdir_filler(void *data, struct
> > > > page*
> > > > page)
> > > >  static
> > > >  void cache_page_release(nfs_readdir_descriptor_t *desc)
> > > >  {
> > > > -	if (!desc->page->mapping)
> > > > -		nfs_readdir_clear_array(desc->page);
> > > >  	put_page(desc->page);
> > > >  	desc->page = NULL;
> > > >  }
> > > > @@ -720,19 +718,28 @@ struct page
> > > > *get_cache_page(nfs_readdir_descriptor_t
> > > > *desc)
> > > >  
> > > >  /*
> > > >   * Returns 0 if desc->dir_cookie was found on page desc-
> > > > > page_index
> > > > + * and locks the page to prevent removal from the page cache.
> > > >   */
> > > >  static
> > > > -int find_cache_page(nfs_readdir_descriptor_t *desc)
> > > > +int find_and_lock_cache_page(nfs_readdir_descriptor_t *desc)
> > > >  {
> > > >  	int res;
> > > >  
> > > >  	desc->page = get_cache_page(desc);
> > > >  	if (IS_ERR(desc->page))
> > > >  		return PTR_ERR(desc->page);
> > > > -
> > > > -	res = nfs_readdir_search_array(desc);
> > > > +	res = lock_page_killable(desc->page);
> > > >  	if (res != 0)
> > > > -		cache_page_release(desc);
> > > > +		goto error;
> > > > +	res = -EAGAIN;
> > > > +	if (desc->page->mapping != NULL) {
> > > > +		res = nfs_readdir_search_array(desc);
> > > > +		if (res == 0)
> > > > +			return 0;
> > > > +	}
> > > > +	unlock_page(desc->page);
> > > > +error:
> > > > +	cache_page_release(desc);
> > > >  	return res;
> > > >  }
> > > >  
> > > 
> > > Can you give me some guidance on how to apply this on top of
> > > Dai's v2
> > > patch from
> > > January 23? Right now I have the nfsi->page_index getting set
> > > before
> > > the
> > > unlock_page():
> > 
> > Since this needs to be a stable patch, it would be preferable to
> > apply
> > them in the opposite order to avoid an extra dependency on Dai's
> > patch.
> 
> That makes sense.
> 
> > That said, since the nfsi->page_index is not a per-page variable,
> > there
> > is no need to put it under the page lock.
> 
> Got it. I'll swap the order of everything, and put the page_index
> outside of the
> page lock when resolving the conflict.
> 

Oops... Actually Dai's code needs to go in the 'return 0' path above
(i.e. after a successful call to nfs_readdir_search_array()).
It shouldn't go in the error path.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] NFS: Directory page cache pages need to be locked when read
  2020-02-03 22:45           ` Trond Myklebust
@ 2020-02-03 22:50             ` Trond Myklebust
  2020-02-03 22:55               ` Trond Myklebust
  0 siblings, 1 reply; 16+ messages in thread
From: Trond Myklebust @ 2020-02-03 22:50 UTC (permalink / raw)
  To: Anna.Schumaker; +Cc: dai.ngo, linux-nfs, bcodding

On Mon, 2020-02-03 at 22:45 +0000, Trond Myklebust wrote:
> On Mon, 2020-02-03 at 21:21 +0000, Schumaker, Anna wrote:
> > On Mon, 2020-02-03 at 16:18 -0500, Trond Myklebust wrote:
> > > On Mon, 2020-02-03 at 20:31 +0000, Schumaker, Anna wrote:
> > > > Hi Trond,
> > > > 
> > > > On Sun, 2020-02-02 at 17:53 -0500, Trond Myklebust wrote:
> > > > > When a NFS directory page cache page is removed from the page
> > > > > cache,
> > > > > its contents are freed through a call to
> > > > > nfs_readdir_clear_array().
> > > > > To prevent the removal of the page cache entry until after
> > > > > we've
> > > > > finished reading it, we must take the page lock.
> > > > > 
> > > > > Fixes: 11de3b11e08c ("NFS: Fix a memory leak in nfs_readdir")
> > > > > Cc: stable@vger.kernel.org # v2.6.37+
> > > > > Signed-off-by: Trond Myklebust <
> > > > > trond.myklebust@hammerspace.com
> > > > > ---
> > > > >  fs/nfs/dir.c | 30 +++++++++++++++++++-----------
> > > > >  1 file changed, 19 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > > > index ba0d55930e8a..90467b44ec13 100644
> > > > > --- a/fs/nfs/dir.c
> > > > > +++ b/fs/nfs/dir.c
> > > > > @@ -705,8 +705,6 @@ int nfs_readdir_filler(void *data, struct
> > > > > page*
> > > > > page)
> > > > >  static
> > > > >  void cache_page_release(nfs_readdir_descriptor_t *desc)
> > > > >  {
> > > > > -	if (!desc->page->mapping)
> > > > > -		nfs_readdir_clear_array(desc->page);
> > > > >  	put_page(desc->page);
> > > > >  	desc->page = NULL;
> > > > >  }
> > > > > @@ -720,19 +718,28 @@ struct page
> > > > > *get_cache_page(nfs_readdir_descriptor_t
> > > > > *desc)
> > > > >  
> > > > >  /*
> > > > >   * Returns 0 if desc->dir_cookie was found on page desc-
> > > > > > page_index
> > > > > + * and locks the page to prevent removal from the page
> > > > > cache.
> > > > >   */
> > > > >  static
> > > > > -int find_cache_page(nfs_readdir_descriptor_t *desc)
> > > > > +int find_and_lock_cache_page(nfs_readdir_descriptor_t *desc)
> > > > >  {
> > > > >  	int res;
> > > > >  
> > > > >  	desc->page = get_cache_page(desc);
> > > > >  	if (IS_ERR(desc->page))
> > > > >  		return PTR_ERR(desc->page);
> > > > > -
> > > > > -	res = nfs_readdir_search_array(desc);
> > > > > +	res = lock_page_killable(desc->page);
> > > > >  	if (res != 0)
> > > > > -		cache_page_release(desc);
> > > > > +		goto error;
> > > > > +	res = -EAGAIN;
> > > > > +	if (desc->page->mapping != NULL) {
> > > > > +		res = nfs_readdir_search_array(desc);
> > > > > +		if (res == 0)
> > > > > +			return 0;
> > > > > +	}
> > > > > +	unlock_page(desc->page);
> > > > > +error:
> > > > > +	cache_page_release(desc);
> > > > >  	return res;
> > > > >  }
> > > > >  
> > > > 
> > > > Can you give me some guidance on how to apply this on top of
> > > > Dai's v2
> > > > patch from
> > > > January 23? Right now I have the nfsi->page_index getting set
> > > > before
> > > > the
> > > > unlock_page():
> > > 
> > > Since this needs to be a stable patch, it would be preferable to
> > > apply
> > > them in the opposite order to avoid an extra dependency on Dai's
> > > patch.
> > 
> > That makes sense.
> > 
> > > That said, since the nfsi->page_index is not a per-page variable,
> > > there
> > > is no need to put it under the page lock.
> > 
> > Got it. I'll swap the order of everything, and put the page_index
> > outside of the
> > page lock when resolving the conflict.
> > 
> 
> Oops... Actually Dai's code needs to go in the 'return 0' path above
> (i.e. after a successful call to nfs_readdir_search_array()).
> It shouldn't go in the error path.


While moving the code, could you also add in a small micro-
optimisation? If we use file_inode(desc->file) instead of
d_inode(file_dentry(desc->file)) then we avoid at least one pointer
indirection.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] NFS: Directory page cache pages need to be locked when read
  2020-02-03 22:50             ` Trond Myklebust
@ 2020-02-03 22:55               ` Trond Myklebust
  0 siblings, 0 replies; 16+ messages in thread
From: Trond Myklebust @ 2020-02-03 22:55 UTC (permalink / raw)
  To: Anna.Schumaker; +Cc: dai.ngo, linux-nfs, bcodding

On Mon, 2020-02-03 at 22:50 +0000, Trond Myklebust wrote:
> On Mon, 2020-02-03 at 22:45 +0000, Trond Myklebust wrote:
> > On Mon, 2020-02-03 at 21:21 +0000, Schumaker, Anna wrote:
> > > On Mon, 2020-02-03 at 16:18 -0500, Trond Myklebust wrote:
> > > > On Mon, 2020-02-03 at 20:31 +0000, Schumaker, Anna wrote:
> > > > > Hi Trond,
> > > > > 
> > > > > On Sun, 2020-02-02 at 17:53 -0500, Trond Myklebust wrote:
> > > > > > When a NFS directory page cache page is removed from the
> > > > > > page
> > > > > > cache,
> > > > > > its contents are freed through a call to
> > > > > > nfs_readdir_clear_array().
> > > > > > To prevent the removal of the page cache entry until after
> > > > > > we've
> > > > > > finished reading it, we must take the page lock.
> > > > > > 
> > > > > > Fixes: 11de3b11e08c ("NFS: Fix a memory leak in
> > > > > > nfs_readdir")
> > > > > > Cc: stable@vger.kernel.org # v2.6.37+
> > > > > > Signed-off-by: Trond Myklebust <
> > > > > > trond.myklebust@hammerspace.com
> > > > > > ---
> > > > > >  fs/nfs/dir.c | 30 +++++++++++++++++++-----------
> > > > > >  1 file changed, 19 insertions(+), 11 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > > > > index ba0d55930e8a..90467b44ec13 100644
> > > > > > --- a/fs/nfs/dir.c
> > > > > > +++ b/fs/nfs/dir.c
> > > > > > @@ -705,8 +705,6 @@ int nfs_readdir_filler(void *data,
> > > > > > struct
> > > > > > page*
> > > > > > page)
> > > > > >  static
> > > > > >  void cache_page_release(nfs_readdir_descriptor_t *desc)
> > > > > >  {
> > > > > > -	if (!desc->page->mapping)
> > > > > > -		nfs_readdir_clear_array(desc->page);
> > > > > >  	put_page(desc->page);
> > > > > >  	desc->page = NULL;
> > > > > >  }
> > > > > > @@ -720,19 +718,28 @@ struct page
> > > > > > *get_cache_page(nfs_readdir_descriptor_t
> > > > > > *desc)
> > > > > >  
> > > > > >  /*
> > > > > >   * Returns 0 if desc->dir_cookie was found on page desc-
> > > > > > > page_index
> > > > > > + * and locks the page to prevent removal from the page
> > > > > > cache.
> > > > > >   */
> > > > > >  static
> > > > > > -int find_cache_page(nfs_readdir_descriptor_t *desc)
> > > > > > +int find_and_lock_cache_page(nfs_readdir_descriptor_t
> > > > > > *desc)
> > > > > >  {
> > > > > >  	int res;
> > > > > >  
> > > > > >  	desc->page = get_cache_page(desc);
> > > > > >  	if (IS_ERR(desc->page))
> > > > > >  		return PTR_ERR(desc->page);
> > > > > > -
> > > > > > -	res = nfs_readdir_search_array(desc);
> > > > > > +	res = lock_page_killable(desc->page);
> > > > > >  	if (res != 0)
> > > > > > -		cache_page_release(desc);
> > > > > > +		goto error;
> > > > > > +	res = -EAGAIN;
> > > > > > +	if (desc->page->mapping != NULL) {
> > > > > > +		res = nfs_readdir_search_array(desc);
> > > > > > +		if (res == 0)
> > > > > > +			return 0;
> > > > > > +	}
> > > > > > +	unlock_page(desc->page);
> > > > > > +error:
> > > > > > +	cache_page_release(desc);
> > > > > >  	return res;
> > > > > >  }
> > > > > >  
> > > > > 
> > > > > Can you give me some guidance on how to apply this on top of
> > > > > Dai's v2
> > > > > patch from
> > > > > January 23? Right now I have the nfsi->page_index getting set
> > > > > before
> > > > > the
> > > > > unlock_page():
> > > > 
> > > > Since this needs to be a stable patch, it would be preferable
> > > > to
> > > > apply
> > > > them in the opposite order to avoid an extra dependency on
> > > > Dai's
> > > > patch.
> > > 
> > > That makes sense.
> > > 
> > > > That said, since the nfsi->page_index is not a per-page
> > > > variable,
> > > > there
> > > > is no need to put it under the page lock.
> > > 
> > > Got it. I'll swap the order of everything, and put the page_index
> > > outside of the
> > > page lock when resolving the conflict.
> > > 
> > 
> > Oops... Actually Dai's code needs to go in the 'return 0' path
> > above
> > (i.e. after a successful call to nfs_readdir_search_array()).
> > It shouldn't go in the error path.
> 
> While moving the code, could you also add in a small micro-
> optimisation? If we use file_inode(desc->file) instead of
> d_inode(file_dentry(desc->file)) then we avoid at least one pointer
> indirection.
> 

Oh, and please remove the call to nfs_zap_mapping(dir, dir->i_mapping)
in nfs_for_use_readdirplus(). We don't need that when we have the call
to invalidate_mapping_pages().
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] NFS: Directory page cache pages need to be locked when read
  2020-02-03 21:18       ` Trond Myklebust
  2020-02-03 21:21         ` Schumaker, Anna
@ 2020-02-05  0:44         ` Dai Ngo
  1 sibling, 0 replies; 16+ messages in thread
From: Dai Ngo @ 2020-02-05  0:44 UTC (permalink / raw)
  To: Trond Myklebust, Schumaker, Anna; +Cc: linux-nfs, bcodding


On 2/3/20 1:18 PM, Trond Myklebust wrote:
> On Mon, 2020-02-03 at 20:31 +0000, Schumaker, Anna wrote:
>> Hi Trond,
>>
>> On Sun, 2020-02-02 at 17:53 -0500, Trond Myklebust wrote:
>>> When a NFS directory page cache page is removed from the page
>>> cache,
>>> its contents are freed through a call to nfs_readdir_clear_array().
>>> To prevent the removal of the page cache entry until after we've
>>> finished reading it, we must take the page lock.
>>>
>>> Fixes: 11de3b11e08c ("NFS: Fix a memory leak in nfs_readdir")
>>> Cc: stable@vger.kernel.org # v2.6.37+
>>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> ---
>>>   fs/nfs/dir.c | 30 +++++++++++++++++++-----------
>>>   1 file changed, 19 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>>> index ba0d55930e8a..90467b44ec13 100644
>>> --- a/fs/nfs/dir.c
>>> +++ b/fs/nfs/dir.c
>>> @@ -705,8 +705,6 @@ int nfs_readdir_filler(void *data, struct page*
>>> page)
>>>   static
>>>   void cache_page_release(nfs_readdir_descriptor_t *desc)
>>>   {
>>> -	if (!desc->page->mapping)
>>> -		nfs_readdir_clear_array(desc->page);
>>>   	put_page(desc->page);
>>>   	desc->page = NULL;
>>>   }
>>> @@ -720,19 +718,28 @@ struct page
>>> *get_cache_page(nfs_readdir_descriptor_t
>>> *desc)
>>>   
>>>   /*
>>>    * Returns 0 if desc->dir_cookie was found on page desc-
>>>> page_index
>>> + * and locks the page to prevent removal from the page cache.
>>>    */
>>>   static
>>> -int find_cache_page(nfs_readdir_descriptor_t *desc)
>>> +int find_and_lock_cache_page(nfs_readdir_descriptor_t *desc)
>>>   {
>>>   	int res;
>>>   
>>>   	desc->page = get_cache_page(desc);
>>>   	if (IS_ERR(desc->page))
>>>   		return PTR_ERR(desc->page);
>>> -
>>> -	res = nfs_readdir_search_array(desc);
>>> +	res = lock_page_killable(desc->page);
>>>   	if (res != 0)
>>> -		cache_page_release(desc);
>>> +		goto error;
>>> +	res = -EAGAIN;
>>> +	if (desc->page->mapping != NULL) {
>>> +		res = nfs_readdir_search_array(desc);
>>> +		if (res == 0)
>>> +			return 0;
>>> +	}
>>> +	unlock_page(desc->page);
>>> +error:
>>> +	cache_page_release(desc);
>>>   	return res;
>>>   }
>>>   
>> Can you give me some guidance on how to apply this on top of Dai's v2
>> patch from
>> January 23? Right now I have the nfsi->page_index getting set before
>> the
>> unlock_page():
> Since this needs to be a stable patch, it would be preferable to apply
> them in the opposite order to avoid an extra dependency on Dai's patch.

Hi Trond, does this mean my patch won't make it to the stable backport
this time? This patch is not just a performance enhancement, but fixes
real bug, which in some cases can cause readdir to take very long time
to complete.

Thanks,
-Dai

>
> That said, since the nfsi->page_index is not a per-page variable, there
> is no need to put it under the page lock.
>
>
>> @@ -732,15 +733,24 @@ int find_cache_page(nfs_readdir_descriptor_t
>> *desc)
>>   	if (IS_ERR(desc->page))
>>   		return PTR_ERR(desc->page);
>>   
>> -	res = nfs_readdir_search_array(desc);
>> +	res = lock_page_killable(desc->page);
>>   	if (res != 0)
>>   		cache_page_release(desc);
>> +		goto error;
>> +	res = -EAGAIN;
>> +	if (desc->page->mapping != NULL) {
>> +		res = nfs_readdir_search_array(desc);
>> +		if (res == 0)
>> +			return 0;
>> +	}
>>   
>>   	dentry = file_dentry(desc->file);
>>   	inode = d_inode(dentry);
>>   	nfsi = NFS_I(inode);
>>   	nfsi->page_index = desc->page_index;
>> -
>> +	unlock_page(desc->page);
>> +error:
>> +	cache_page_release(desc);
>>   	return res;
>>   }
>>   
>>
>> Please let me know if there is a better way to handle the conflict!
>>
>> Anna
>>
>>
>>> @@ -747,7 +754,7 @@ int
>>> readdir_search_pagecache(nfs_readdir_descriptor_t
>>> *desc)
>>>   		desc->last_cookie = 0;
>>>   	}
>>>   	do {
>>> -		res = find_cache_page(desc);
>>> +		res = find_and_lock_cache_page(desc);
>>>   	} while (res == -EAGAIN);
>>>   	return res;
>>>   }
>>> @@ -786,7 +793,6 @@ int nfs_do_filldir(nfs_readdir_descriptor_t
>>> *desc)
>>>   		desc->eof = true;
>>>   
>>>   	kunmap(desc->page);
>>> -	cache_page_release(desc);
>>>   	dfprintk(DIRCACHE, "NFS: nfs_do_filldir() filling ended @
>>> cookie %Lu;
>>> returning = %d\n",
>>>   			(unsigned long long)*desc->dir_cookie, res);
>>>   	return res;
>>> @@ -832,13 +838,13 @@ int uncached_readdir(nfs_readdir_descriptor_t
>>> *desc)
>>>   
>>>   	status = nfs_do_filldir(desc);
>>>   
>>> + out_release:
>>> +	nfs_readdir_clear_array(desc->page);
>>> +	cache_page_release(desc);
>>>    out:
>>>   	dfprintk(DIRCACHE, "NFS: %s: returns %d\n",
>>>   			__func__, status);
>>>   	return status;
>>> - out_release:
>>> -	cache_page_release(desc);
>>> -	goto out;
>>>   }
>>>   
>>>   /* The file offset position represents the dirent entry number.  A
>>> @@ -903,6 +909,8 @@ static int nfs_readdir(struct file *file,
>>> struct
>>> dir_context *ctx)
>>>   			break;
>>>   
>>>   		res = nfs_do_filldir(desc);
>>> +		unlock_page(desc->page);
>>> +		cache_page_release(desc);
>>>   		if (res < 0)
>>>   			break;
>>>   	} while (!desc->eof);

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-02 22:53 [PATCH 0/4] Readdir fixes Trond Myklebust
2020-02-02 22:53 ` [PATCH 1/4] NFS: Fix memory leaks and corruption in readdir Trond Myklebust
2020-02-02 22:53   ` [PATCH 2/4] NFS: Directory page cache pages need to be locked when read Trond Myklebust
2020-02-02 22:53     ` [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr() Trond Myklebust
2020-02-02 22:53       ` [PATCH 4/4] NFS: Switch readdir to using iterate_shared() Trond Myklebust
2020-02-03 14:41         ` Benjamin Coddington
2020-02-03 14:22       ` [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr() Benjamin Coddington
2020-02-03 14:21     ` [PATCH 2/4] NFS: Directory page cache pages need to be locked when read Benjamin Coddington
2020-02-03 20:31     ` Schumaker, Anna
2020-02-03 21:18       ` Trond Myklebust
2020-02-03 21:21         ` Schumaker, Anna
2020-02-03 22:45           ` Trond Myklebust
2020-02-03 22:50             ` Trond Myklebust
2020-02-03 22:55               ` Trond Myklebust
2020-02-05  0:44         ` Dai Ngo
2020-02-03 13:44   ` [PATCH 1/4] NFS: Fix memory leaks and corruption in readdir Benjamin Coddington

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git