2010-11-02 18:00:53

by Andi Kleen

[permalink] [raw]
Subject: [regression] NFS readdir change break fully cached nfs root


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 <[email protected]>
Date: Fri Sep 24 14:48:42 2010 -0400

NFS: add readdir cache array


-Andi

--
[email protected] -- Speaking for myself only.


2010-11-03 07:31:18

by Benny Halevy

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

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 <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2010-11-04 16:07:46

by Myklebust, Trond

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

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 <[email protected]>

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 <[email protected]>
---

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);




2010-11-04 18:58:05

by Nick Bowler

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

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/)

2010-11-02 21:05:36

by Myklebust, Trond

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

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 <[email protected]>

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 <[email protected]>
---

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);


2010-11-03 16:24:46

by Nick Bowler

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

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/)

2010-11-11 19:08:02

by Andi Kleen

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


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
--
[email protected] -- Speaking for myself only.

2010-11-02 21:16:03

by Andi Kleen

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

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