[regression] NFS readdir change break fully cached nfs root

[regression] NFS readdir change break fully cached nfs root

[Andi Kleen]

My NFS root test systems stopped booting with 2.6.37-rc1. It just
hangs after entering user space.

The NFS root setup uses a slightly fancy parameter setup to minimize 
network traffic:

nfsroot=10.23.204.1:/home/nfsroot/edwin,nocto,acregmin=86400,acregmax=86400,acdirmin=86400,acdirmax=86400 root=/dev/nfs

I bisected it down to this commit from Bryan.

Unfortunately it's not trivially revertable because that conflicts
with later patches.

commit d1bacf9eb2fd0e7ef870acf84b9e3b157dcfa7dc
Author: Bryan Schumaker <bjschuma@netapp.com>
Date:   Fri Sep 24 14:48:42 2010 -0400

    NFS: add readdir cache array


-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [regression] NFS readdir change break fully cached nfs root

[Trond Myklebust]

On Tue, 2010-11-02 at 19:00 +0100, Andi Kleen wrote:
> My NFS root test systems stopped booting with 2.6.37-rc1. It just
> hangs after entering user space.
> 
> The NFS root setup uses a slightly fancy parameter setup to minimize 
> network traffic:
> 
> nfsroot=10.23.204.1:/home/nfsroot/edwin,nocto,acregmin=86400,acregmax=86400,acdirmin=86400,acdirmax=86400 root=/dev/nfs
> 
> I bisected it down to this commit from Bryan.
> 
> Unfortunately it's not trivially revertable because that conflicts
> with later patches.

Does the following patch help?

Cheers
  Trond
------------------------------------------------------------------------
NFS: Fix a couple of regressions in readdir.

From: Trond Myklebust <Trond.Myklebust@netapp.com>

Fix up the issue that array->eof_index needs to be able to be set
even if array->size == 0.

Ensure that we catch all important memory allocation error conditions
and/or kmap() failures.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/nfs/dir.c |   73 ++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 48 insertions(+), 25 deletions(-)


diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 07ac384..8c22d60 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -194,9 +194,13 @@ typedef struct {
 static
 struct nfs_cache_array *nfs_readdir_get_array(struct page *page)
 {
+	void *ptr;
 	if (page == NULL)
 		return ERR_PTR(-EIO);
-	return (struct nfs_cache_array *)kmap(page);
+	ptr = kmap(page);
+	if (ptr == NULL)
+		return ERR_PTR(-ENOMEM);
+	return ptr;
 }
 
 static
@@ -213,6 +217,9 @@ int nfs_readdir_clear_array(struct page *page, gfp_t mask)
 {
 	struct nfs_cache_array *array = nfs_readdir_get_array(page);
 	int i;
+
+	if (IS_ERR(array))
+		return PTR_ERR(array);
 	for (i = 0; i < array->size; i++)
 		kfree(array->array[i].string.name);
 	nfs_readdir_release_array(page);
@@ -244,7 +251,7 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
 
 	if (IS_ERR(array))
 		return PTR_ERR(array);
-	ret = -EIO;
+	ret = -ENOSPC;
 	if (array->size >= MAX_READDIR_ARRAY)
 		goto out;
 
@@ -255,9 +262,9 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
 	if (ret)
 		goto out;
 	array->last_cookie = entry->cookie;
+	array->size++;
 	if (entry->eof == 1)
 		array->eof_index = array->size;
-	array->size++;
 out:
 	nfs_readdir_release_array(page);
 	return ret;
@@ -272,7 +279,7 @@ int nfs_readdir_search_for_pos(struct nfs_cache_array *array, nfs_readdir_descri
 	if (diff < 0)
 		goto out_eof;
 	if (diff >= array->size) {
-		if (array->eof_index > 0)
+		if (array->eof_index >= 0)
 			goto out_eof;
 		desc->current_index += array->size;
 		return -EAGAIN;
@@ -281,8 +288,6 @@ int nfs_readdir_search_for_pos(struct nfs_cache_array *array, nfs_readdir_descri
 	index = (unsigned int)diff;
 	*desc->dir_cookie = array->array[index].cookie;
 	desc->cache_entry_index = index;
-	if (index == array->eof_index)
-		desc->eof = 1;
 	return 0;
 out_eof:
 	desc->eof = 1;
@@ -296,17 +301,17 @@ int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_des
 	int status = -EAGAIN;
 
 	for (i = 0; i < array->size; i++) {
-		if (i == array->eof_index) {
-			desc->eof = 1;
-			status = -EBADCOOKIE;
-		}
 		if (array->array[i].cookie == *desc->dir_cookie) {
 			desc->cache_entry_index = i;
 			status = 0;
-			break;
+			goto out;
 		}
 	}
-
+	if (i == array->eof_index) {
+		desc->eof = 1;
+		status = -EBADCOOKIE;
+	}
+out:
 	return status;
 }
 
@@ -449,7 +454,7 @@ out:
 
 /* Perform conversion from xdr to cache array */
 static
-void nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *entry,
+int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *entry,
 				void *xdr_page, struct page *page, unsigned int buflen)
 {
 	struct xdr_stream stream;
@@ -471,21 +476,29 @@ void nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *e
 
 	do {
 		status = xdr_decode(desc, entry, &stream);
-		if (status != 0)
+		if (status != 0) {
+			if (status == -EAGAIN)
+				status = 0;
 			break;
+		}
 
-		if (nfs_readdir_add_to_array(entry, page) == -1)
-			break;
 		if (desc->plus == 1)
 			nfs_prime_dcache(desc->file->f_path.dentry, entry);
+
+		status = nfs_readdir_add_to_array(entry, page);
+		if (status != 0)
+			break;
 	} while (!entry->eof);
 
 	if (status == -EBADCOOKIE && entry->eof) {
 		array = nfs_readdir_get_array(page);
-		array->eof_index = array->size - 1;
-		status = 0;
-		nfs_readdir_release_array(page);
+		if (!IS_ERR(array)) {
+			array->eof_index = array->size;
+			status = 0;
+			nfs_readdir_release_array(page);
+		}
 	}
+	return status;
 }
 
 static
@@ -549,9 +562,14 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
 		goto out;
 
 	array = nfs_readdir_get_array(page);
+	if (IS_ERR(array)) {
+		status = PTR_ERR(array);
+		goto out;
+	}
 	memset(array, 0, sizeof(struct nfs_cache_array));
 	array->eof_index = -1;
 
+	status = -ENOMEM;
 	pages_ptr = nfs_readdir_large_page(pages, array_size);
 	if (!pages_ptr)
 		goto out_release_array;
@@ -560,8 +578,13 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
 
 		if (status < 0)
 			break;
-		nfs_readdir_page_filler(desc, &entry, pages_ptr, page, array_size * PAGE_SIZE);
-	} while (array->eof_index < 0 && array->size < MAX_READDIR_ARRAY);
+		status = nfs_readdir_page_filler(desc, &entry, pages_ptr, page, array_size * PAGE_SIZE);
+		if (status < 0) {
+			if (status == -ENOSPC)
+				status = 0;
+			break;
+		}
+	} while (array->eof_index < 0);
 
 	nfs_readdir_free_large_page(pages_ptr, pages, array_size);
 out_release_array:
@@ -670,6 +693,8 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *dirent,
 	struct dentry *dentry = NULL;
 
 	array = nfs_readdir_get_array(desc->page);
+	if (IS_ERR(array))
+		return PTR_ERR(array);
 
 	for (i = desc->cache_entry_index; i < array->size; i++) {
 		d_type = DT_UNKNOWN;
@@ -685,11 +710,9 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *dirent,
 			*desc->dir_cookie = array->array[i+1].cookie;
 		else
 			*desc->dir_cookie = array->last_cookie;
-		if (i == array->eof_index) {
-			desc->eof = 1;
-			break;
-		}
 	}
+	if (i == array->eof_index)
+		desc->eof = 1;
 
 	nfs_readdir_release_array(desc->page);
 	cache_page_release(desc);

Re: [regression] NFS readdir change break fully cached nfs root

[Andi Kleen]

On Tue, Nov 02, 2010 at 05:05:30PM -0400, Trond Myklebust wrote:
> On Tue, 2010-11-02 at 19:00 +0100, Andi Kleen wrote:
> > My NFS root test systems stopped booting with 2.6.37-rc1. It just
> > hangs after entering user space.
> > 
> > The NFS root setup uses a slightly fancy parameter setup to minimize 
> > network traffic:
> > 
> > nfsroot=10.23.204.1:/home/nfsroot/edwin,nocto,acregmin=86400,acregmax=86400,acdirmin=86400,acdirmax=86400 root=/dev/nfs
> > 
> > I bisected it down to this commit from Bryan.
> > 
> > Unfortunately it's not trivially revertable because that conflicts
> > with later patches.
> 
> Does the following patch help?

Yes that fixes it. Thanks. Test system boots again.
-Andi

Re: [regression] NFS readdir change break fully cached nfs root

[Benny Halevy]

On 2010-11-02 23:05, Trond Myklebust wrote:
> On Tue, 2010-11-02 at 19:00 +0100, Andi Kleen wrote:
>> My NFS root test systems stopped booting with 2.6.37-rc1. It just
>> hangs after entering user space.
>>
>> The NFS root setup uses a slightly fancy parameter setup to minimize 
>> network traffic:
>>
>> nfsroot=10.23.204.1:/home/nfsroot/edwin,nocto,acregmin=86400,acregmax=86400,acdirmin=86400,acdirmax=86400 root=/dev/nfs
>>
>> I bisected it down to this commit from Bryan.
>>
>> Unfortunately it's not trivially revertable because that conflicts
>> with later patches.
> 
> Does the following patch help?

This patch helps me too with an infinite readdir loop I've seen
with 2.6.37-rc1.

I'll queue it up in the linux-pnfs tree until it hits upstream.

Benny

> 
> Cheers
>   Trond
> ------------------------------------------------------------------------
> NFS: Fix a couple of regressions in readdir.
> 
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> 
> Fix up the issue that array->eof_index needs to be able to be set
> even if array->size == 0.
> 
> Ensure that we catch all important memory allocation error conditions
> and/or kmap() failures.
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> 
>  fs/nfs/dir.c |   73 ++++++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 48 insertions(+), 25 deletions(-)
> 
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 07ac384..8c22d60 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -194,9 +194,13 @@ typedef struct {
>  static
>  struct nfs_cache_array *nfs_readdir_get_array(struct page *page)
>  {
> +	void *ptr;
>  	if (page == NULL)
>  		return ERR_PTR(-EIO);
> -	return (struct nfs_cache_array *)kmap(page);
> +	ptr = kmap(page);
> +	if (ptr == NULL)
> +		return ERR_PTR(-ENOMEM);
> +	return ptr;
>  }
>  
>  static
> @@ -213,6 +217,9 @@ int nfs_readdir_clear_array(struct page *page, gfp_t mask)
>  {
>  	struct nfs_cache_array *array = nfs_readdir_get_array(page);
>  	int i;
> +
> +	if (IS_ERR(array))
> +		return PTR_ERR(array);
>  	for (i = 0; i < array->size; i++)
>  		kfree(array->array[i].string.name);
>  	nfs_readdir_release_array(page);
> @@ -244,7 +251,7 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
>  
>  	if (IS_ERR(array))
>  		return PTR_ERR(array);
> -	ret = -EIO;
> +	ret = -ENOSPC;
>  	if (array->size >= MAX_READDIR_ARRAY)
>  		goto out;
>  
> @@ -255,9 +262,9 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
>  	if (ret)
>  		goto out;
>  	array->last_cookie = entry->cookie;
> +	array->size++;
>  	if (entry->eof == 1)
>  		array->eof_index = array->size;
> -	array->size++;
>  out:
>  	nfs_readdir_release_array(page);
>  	return ret;
> @@ -272,7 +279,7 @@ int nfs_readdir_search_for_pos(struct nfs_cache_array *array, nfs_readdir_descri
>  	if (diff < 0)
>  		goto out_eof;
>  	if (diff >= array->size) {
> -		if (array->eof_index > 0)
> +		if (array->eof_index >= 0)
>  			goto out_eof;
>  		desc->current_index += array->size;
>  		return -EAGAIN;
> @@ -281,8 +288,6 @@ int nfs_readdir_search_for_pos(struct nfs_cache_array *array, nfs_readdir_descri
>  	index = (unsigned int)diff;
>  	*desc->dir_cookie = array->array[index].cookie;
>  	desc->cache_entry_index = index;
> -	if (index == array->eof_index)
> -		desc->eof = 1;
>  	return 0;
>  out_eof:
>  	desc->eof = 1;
> @@ -296,17 +301,17 @@ int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_des
>  	int status = -EAGAIN;
>  
>  	for (i = 0; i < array->size; i++) {
> -		if (i == array->eof_index) {
> -			desc->eof = 1;
> -			status = -EBADCOOKIE;
> -		}
>  		if (array->array[i].cookie == *desc->dir_cookie) {
>  			desc->cache_entry_index = i;
>  			status = 0;
> -			break;
> +			goto out;
>  		}
>  	}
> -
> +	if (i == array->eof_index) {
> +		desc->eof = 1;
> +		status = -EBADCOOKIE;
> +	}
> +out:
>  	return status;
>  }
>  
> @@ -449,7 +454,7 @@ out:
>  
>  /* Perform conversion from xdr to cache array */
>  static
> -void nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *entry,
> +int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *entry,
>  				void *xdr_page, struct page *page, unsigned int buflen)
>  {
>  	struct xdr_stream stream;
> @@ -471,21 +476,29 @@ void nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *e
>  
>  	do {
>  		status = xdr_decode(desc, entry, &stream);
> -		if (status != 0)
> +		if (status != 0) {
> +			if (status == -EAGAIN)
> +				status = 0;
>  			break;
> +		}
>  
> -		if (nfs_readdir_add_to_array(entry, page) == -1)
> -			break;
>  		if (desc->plus == 1)
>  			nfs_prime_dcache(desc->file->f_path.dentry, entry);
> +
> +		status = nfs_readdir_add_to_array(entry, page);
> +		if (status != 0)
> +			break;
>  	} while (!entry->eof);
>  
>  	if (status == -EBADCOOKIE && entry->eof) {
>  		array = nfs_readdir_get_array(page);
> -		array->eof_index = array->size - 1;
> -		status = 0;
> -		nfs_readdir_release_array(page);
> +		if (!IS_ERR(array)) {
> +			array->eof_index = array->size;
> +			status = 0;
> +			nfs_readdir_release_array(page);
> +		}
>  	}
> +	return status;
>  }
>  
>  static
> @@ -549,9 +562,14 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
>  		goto out;
>  
>  	array = nfs_readdir_get_array(page);
> +	if (IS_ERR(array)) {
> +		status = PTR_ERR(array);
> +		goto out;
> +	}
>  	memset(array, 0, sizeof(struct nfs_cache_array));
>  	array->eof_index = -1;
>  
> +	status = -ENOMEM;
>  	pages_ptr = nfs_readdir_large_page(pages, array_size);
>  	if (!pages_ptr)
>  		goto out_release_array;
> @@ -560,8 +578,13 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
>  
>  		if (status < 0)
>  			break;
> -		nfs_readdir_page_filler(desc, &entry, pages_ptr, page, array_size * PAGE_SIZE);
> -	} while (array->eof_index < 0 && array->size < MAX_READDIR_ARRAY);
> +		status = nfs_readdir_page_filler(desc, &entry, pages_ptr, page, array_size * PAGE_SIZE);
> +		if (status < 0) {
> +			if (status == -ENOSPC)
> +				status = 0;
> +			break;
> +		}
> +	} while (array->eof_index < 0);
>  
>  	nfs_readdir_free_large_page(pages_ptr, pages, array_size);
>  out_release_array:
> @@ -670,6 +693,8 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *dirent,
>  	struct dentry *dentry = NULL;
>  
>  	array = nfs_readdir_get_array(desc->page);
> +	if (IS_ERR(array))
> +		return PTR_ERR(array);
>  
>  	for (i = desc->cache_entry_index; i < array->size; i++) {
>  		d_type = DT_UNKNOWN;
> @@ -685,11 +710,9 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *dirent,
>  			*desc->dir_cookie = array->array[i+1].cookie;
>  		else
>  			*desc->dir_cookie = array->last_cookie;
> -		if (i == array->eof_index) {
> -			desc->eof = 1;
> -			break;
> -		}
>  	}
> +	if (i == array->eof_index)
> +		desc->eof = 1;
>  
>  	nfs_readdir_release_array(desc->page);
>  	cache_page_release(desc);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [regression] NFS readdir change break fully cached nfs root

[Nick Bowler]

On 2010-11-02 17:05 -0400, Trond Myklebust wrote:
> On Tue, 2010-11-02 at 19:00 +0100, Andi Kleen wrote:
> > My NFS root test systems stopped booting with 2.6.37-rc1. It just
> > hangs after entering user space.
[...]
> Does the following patch help?
[...]
> NFS: Fix a couple of regressions in readdir.

I just ran into this issue with git today, where some operations locked
up permanently (seemed to be an infinite readdir loop as described
elsewhere in this thread).

I applied this patch, and it solves the lockups, but there's a new
problem: the directory list sometimes comes back _empty_!

Here's the test script I used:

  #!/bin/sh -e
  
  mkdir gittest
  cd gittest
  git init
  
  for i in `seq 500`
  do
  	printf 'Hello\nWorld!\n' > $i.txt
  done
  
  git add ./*.txt
  git commit -m 'Initial Commit'
  sed -i 1d ./*.txt
  git stash
  git stash pop
  
  echo 'Directory listing:'
  ls

the 'ls' at the end prints nothing when the directory actually contains
500 files.  If we insert an 'ls' just before the 'git stash pop', that
one prints the full listing.  Touching the directory afterwards makes
the listings work again.

The script works fine on 2.6.36.

-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Re: [regression] NFS readdir change break fully cached nfs root

[Trond Myklebust]

On Wed, 2010-11-03 at 12:24 -0400, Nick Bowler wrote:
> I applied this patch, and it solves the lockups, but there's a new
> problem: the directory list sometimes comes back _empty_!
> 
> Here's the test script I used:
> 
>   #!/bin/sh -e
>   
>   mkdir gittest
>   cd gittest
>   git init
>   
>   for i in `seq 500`
>   do
>   	printf 'Hello\nWorld!\n' > $i.txt
>   done
>   
>   git add ./*.txt
>   git commit -m 'Initial Commit'
>   sed -i 1d ./*.txt
>   git stash
>   git stash pop
>   
>   echo 'Directory listing:'
>   ls
> 
> the 'ls' at the end prints nothing when the directory actually contains
> 500 files.  If we insert an 'ls' just before the 'git stash pop', that
> one prints the full listing.  Touching the directory afterwards makes
> the listings work again.
> 
> The script works fine on 2.6.36.

I found a couple more issues. Can you try this revision of the same
patch?

Cheers
  Trond
-------------------------------------------------------------------------------------------
NFS: Fix a couple of regressions in readdir.

From: Trond Myklebust <Trond.Myklebust@netapp.com>

Fix up the issue that array->eof_index needs to be able to be set
even if array->size == 0.

Ensure that we catch all important memory allocation error conditions
and/or kmap() failures.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/nfs/dir.c |   90 ++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 56 insertions(+), 34 deletions(-)


diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 07ac384..4c6968b 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -194,9 +194,13 @@ typedef struct {
 static
 struct nfs_cache_array *nfs_readdir_get_array(struct page *page)
 {
+	void *ptr;
 	if (page == NULL)
 		return ERR_PTR(-EIO);
-	return (struct nfs_cache_array *)kmap(page);
+	ptr = kmap(page);
+	if (ptr == NULL)
+		return ERR_PTR(-ENOMEM);
+	return ptr;
 }
 
 static
@@ -213,6 +217,9 @@ int nfs_readdir_clear_array(struct page *page, gfp_t mask)
 {
 	struct nfs_cache_array *array = nfs_readdir_get_array(page);
 	int i;
+
+	if (IS_ERR(array))
+		return PTR_ERR(array);
 	for (i = 0; i < array->size; i++)
 		kfree(array->array[i].string.name);
 	nfs_readdir_release_array(page);
@@ -244,7 +251,7 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
 
 	if (IS_ERR(array))
 		return PTR_ERR(array);
-	ret = -EIO;
+	ret = -ENOSPC;
 	if (array->size >= MAX_READDIR_ARRAY)
 		goto out;
 
@@ -255,9 +262,9 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
 	if (ret)
 		goto out;
 	array->last_cookie = entry->cookie;
+	array->size++;
 	if (entry->eof == 1)
 		array->eof_index = array->size;
-	array->size++;
 out:
 	nfs_readdir_release_array(page);
 	return ret;
@@ -272,7 +279,7 @@ int nfs_readdir_search_for_pos(struct nfs_cache_array *array, nfs_readdir_descri
 	if (diff < 0)
 		goto out_eof;
 	if (diff >= array->size) {
-		if (array->eof_index > 0)
+		if (array->eof_index >= 0)
 			goto out_eof;
 		desc->current_index += array->size;
 		return -EAGAIN;
@@ -281,8 +288,6 @@ int nfs_readdir_search_for_pos(struct nfs_cache_array *array, nfs_readdir_descri
 	index = (unsigned int)diff;
 	*desc->dir_cookie = array->array[index].cookie;
 	desc->cache_entry_index = index;
-	if (index == array->eof_index)
-		desc->eof = 1;
 	return 0;
 out_eof:
 	desc->eof = 1;
@@ -296,17 +301,17 @@ int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_des
 	int status = -EAGAIN;
 
 	for (i = 0; i < array->size; i++) {
-		if (i == array->eof_index) {
-			desc->eof = 1;
-			status = -EBADCOOKIE;
-		}
 		if (array->array[i].cookie == *desc->dir_cookie) {
 			desc->cache_entry_index = i;
 			status = 0;
-			break;
+			goto out;
 		}
 	}
-
+	if (i == array->eof_index) {
+		desc->eof = 1;
+		status = -EBADCOOKIE;
+	}
+out:
 	return status;
 }
 
@@ -449,7 +454,7 @@ out:
 
 /* Perform conversion from xdr to cache array */
 static
-void nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *entry,
+int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *entry,
 				void *xdr_page, struct page *page, unsigned int buflen)
 {
 	struct xdr_stream stream;
@@ -471,21 +476,29 @@ void nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *e
 
 	do {
 		status = xdr_decode(desc, entry, &stream);
-		if (status != 0)
+		if (status != 0) {
+			if (status == -EAGAIN)
+				status = 0;
 			break;
+		}
 
-		if (nfs_readdir_add_to_array(entry, page) == -1)
-			break;
 		if (desc->plus == 1)
 			nfs_prime_dcache(desc->file->f_path.dentry, entry);
+
+		status = nfs_readdir_add_to_array(entry, page);
+		if (status != 0)
+			break;
 	} while (!entry->eof);
 
 	if (status == -EBADCOOKIE && entry->eof) {
 		array = nfs_readdir_get_array(page);
-		array->eof_index = array->size - 1;
-		status = 0;
-		nfs_readdir_release_array(page);
+		if (!IS_ERR(array)) {
+			array->eof_index = array->size;
+			status = 0;
+			nfs_readdir_release_array(page);
+		}
 	}
+	return status;
 }
 
 static
@@ -537,7 +550,7 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
 	struct nfs_entry entry;
 	struct file	*file = desc->file;
 	struct nfs_cache_array *array;
-	int status = 0;
+	int status = -ENOMEM;
 	unsigned int array_size = ARRAY_SIZE(pages);
 
 	entry.prev_cookie = 0;
@@ -549,6 +562,10 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
 		goto out;
 
 	array = nfs_readdir_get_array(page);
+	if (IS_ERR(array)) {
+		status = PTR_ERR(array);
+		goto out;
+	}
 	memset(array, 0, sizeof(struct nfs_cache_array));
 	array->eof_index = -1;
 
@@ -560,8 +577,13 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
 
 		if (status < 0)
 			break;
-		nfs_readdir_page_filler(desc, &entry, pages_ptr, page, array_size * PAGE_SIZE);
-	} while (array->eof_index < 0 && array->size < MAX_READDIR_ARRAY);
+		status = nfs_readdir_page_filler(desc, &entry, pages_ptr, page, array_size * PAGE_SIZE);
+		if (status < 0) {
+			if (status == -ENOSPC)
+				status = 0;
+			break;
+		}
+	} while (array->eof_index < 0);
 
 	nfs_readdir_free_large_page(pages_ptr, pages, array_size);
 out_release_array:
@@ -582,8 +604,10 @@ static
 int nfs_readdir_filler(nfs_readdir_descriptor_t *desc, struct page* page)
 {
 	struct inode	*inode = desc->file->f_path.dentry->d_inode;
+	int ret;
 
-	if (nfs_readdir_xdr_to_array(desc, page, inode) < 0)
+	ret = nfs_readdir_xdr_to_array(desc, page, inode);
+	if (ret < 0)
 		goto error;
 	SetPageUptodate(page);
 
@@ -595,7 +619,7 @@ int nfs_readdir_filler(nfs_readdir_descriptor_t *desc, struct page* page)
 	return 0;
  error:
 	unlock_page(page);
-	return -EIO;
+	return ret;
 }
 
 static
@@ -608,12 +632,8 @@ void cache_page_release(nfs_readdir_descriptor_t *desc)
 static
 struct page *get_cache_page(nfs_readdir_descriptor_t *desc)
 {
-	struct page *page;
-	page = read_cache_page(desc->file->f_path.dentry->d_inode->i_mapping,
+	return read_cache_page(desc->file->f_path.dentry->d_inode->i_mapping,
 			desc->page_index, (filler_t *)nfs_readdir_filler, desc);
-	if (IS_ERR(page))
-		desc->eof = 1;
-	return page;
 }
 
 /*
@@ -639,8 +659,10 @@ int find_cache_page(nfs_readdir_descriptor_t *desc)
 static inline
 int readdir_search_pagecache(nfs_readdir_descriptor_t *desc)
 {
-	int res = -EAGAIN;
+	int res;
 
+	if (desc->page_index == 0)
+		desc->current_index = 0;
 	while (1) {
 		res = find_cache_page(desc);
 		if (res != -EAGAIN)
@@ -670,6 +692,8 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *dirent,
 	struct dentry *dentry = NULL;
 
 	array = nfs_readdir_get_array(desc->page);
+	if (IS_ERR(array))
+		return PTR_ERR(array);
 
 	for (i = desc->cache_entry_index; i < array->size; i++) {
 		d_type = DT_UNKNOWN;
@@ -685,11 +709,9 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc, void *dirent,
 			*desc->dir_cookie = array->array[i+1].cookie;
 		else
 			*desc->dir_cookie = array->last_cookie;
-		if (i == array->eof_index) {
-			desc->eof = 1;
-			break;
-		}
 	}
+	if (i == array->eof_index)
+		desc->eof = 1;
 
 	nfs_readdir_release_array(desc->page);
 	cache_page_release(desc);

Re: [regression] NFS readdir change break fully cached nfs root

[Nick Bowler]

On 2010-11-04 12:06 -0400, Trond Myklebust wrote:
> On Wed, 2010-11-03 at 12:24 -0400, Nick Bowler wrote:
> > I applied this patch, and it solves the lockups, but there's a new
> > problem: the directory list sometimes comes back _empty_!
[...]
> I found a couple more issues. Can you try this revision of the same
> patch?
[...]
> NFS: Fix a couple of regressions in readdir.

Unfortunately, I still have the same problem (listing comes back empty
after running the script) with the new patch.

-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Re: [regression] NFS readdir change break fully cached nfs root

[Andi Kleen]

Trond, I noticed that this is still broken in Linus' current tree,
somewhat disrupting testing here. Could you please submit the fixes
for this ASAP?

Thanks,
-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.