readdir patch for 2.2.19 (was [NFS] readdir() not fixed with glibc 2.2?)
Trond Myklebust
> Indeed it was. I think I found 2 problems in fs/nfs/dir.c.
> Without the patch a linux 2.2.19 client running bonnie++
> against a Solaris 2.8 server over NFSv2 UDP fails every time
> during "Delete files in sequential order...". With the patch
> it runs to completion.
> 1. uncached_readdir() does a READDIR then loads the first entry
> of the result into desc->entry to pass to nfs_do_filldir().
> But it sets desc->page_offset to 0, instead of incrementing
> it past the entry it has already loaded. As a result,
> nfs_do_filldir stuffs the passed in desc->entry into user
> space then gets the next entry starting at offset 0, and the
> first entry gets stuffed again.
No. This should not make a difference, and in fact your patch is wrong
here.
uncached_readdir() reads in a new page, and then tries to use the data
returned to fill in part of the getdents structures. However, the page
is never added to the inode's address space, hence it is completely
discarded once we return from uncached_readdir(). Any offset within
that page is therefore bogus.
Instead we should be relying on desc->target to point to the cookie of
the next record.
BTW: this 'offsets are bogus' is in fact a general feature, even for
pages that are added to inode->i_pages. Once we call
page_cache_release() we can never be sure that somebody else won't
throw the page out of cache from beneath us. Thus the next time we
look up the page, we shouldn't rely on the old offset into the page
being still valid.
> 2. uncached_readdir() is called from nfs_readdir() when the target
> cookie is not found because the file has been deleted. But
> if the target was the last file in the directory,
> uncached_readdir() will return EBADCOOKIE just like
> readdir_search_pagecache() did before it. In that case the
> EBADCOOKIE error was being returned to the user. Instead 0
> should be returned to indicate the end of the directory.
This looks like a reasonable point. My only concern is that you're
discarding any error that the RPC call may have returned. How do you
feel about the following rewrite of that section of your patch?
Cheers,
Trond
--- linux-2.2.19/fs/nfs/dir.c.orig Sun Mar 25 18:37:38 2001
+++ linux-2.2.19/fs/nfs/dir.c Thu May 10 14:41:57 2001
@@ -357,15 +357,16 @@
}
page = page_cache_entry(cache_page);
p = (u32 *)page_address(page);
- status = NFS_PROTO(inode)->readdir(inode, cred, desc->target, p,
- NFS_SERVER(inode)->dtsize, 0);
- if (status >= 0) {
+ desc->error = NFS_PROTO(inode)->readdir(inode, cred, desc->target, p,
+ NFS_SERVER(inode)->dtsize, 0);
+ if (desc->error >= 0) {
p = desc->decode(p, desc->entry, 0);
if (IS_ERR(p))
status = PTR_ERR(p);
else
desc->entry->prev_cookie = desc->target;
- }
+ } else
+ status = -EIO;
if (status < 0)
goto out_release;
@@ -421,16 +422,15 @@
res = readdir_search_pagecache(desc);
if (res == -EBADCOOKIE) {
/* This means either end of directory */
- if (desc->entry->cookie == desc->target) {
- res = 0;
- break;
+ if (!desc->entry->eof) {
+ /* Or that the server has 'lost' a cookie */
+ res = uncached_readdir(desc, dirent, filldir);
+ if (res >= 0)
+ continue;
}
- /* Or that the server has 'lost' a cookie */
- res = uncached_readdir(desc, dirent, filldir);
- if (res >= 0)
- continue;
- }
- if (res < 0)
+ res = 0;
+ break;
+ } else if (res < 0)
break;
res = nfs_do_filldir(desc, dirent, filldir);
_______________________________________________
NFS maillist - N...@lists.sourceforge.net
http://lists.sourceforge.net/lists/listinfo/nfs
Trond Myklebust
> So here's my original patch plus the rewrites to save RPC error
> codes. Look okay?
Almost. I managed to corrupt your patch again 8-(...
The patch fails the Connectathon telldir test due to the test for
if (!desc->entry->eof)
after we've tested for EBADCOOKIE...
That flag will usually have been set, since the
readdir_search_pagecache() always traverses the entire page cache when
it gets an unknown cookie...
The old test for
if (desc->entry->cookie == desc->target)
is therefore the correct one. Here's your patch attached with the 1
line corrected...
Cheers,
Trond
--- linux-2.2.19/fs/nfs/dir.c.orig Sun Mar 25 09:37:38 2001
+++ linux-2.2.19/fs/nfs/dir.c Thu May 10 10:56:54 2001
@@ -335,48 +335,49 @@
int uncached_readdir(nfs_readdir_descriptor_t *desc, void *dirent,
filldir_t filldir)
{
struct file *file = desc->file;
struct dentry *dir = file->f_dentry;
struct inode *inode = dir->d_inode;
struct rpc_cred *cred = nfs_file_cred(file);
struct page *page = NULL;
unsigned long cache_page;
- u32 *p;
+ u32 *start, *p;
int status = -EIO;
dfprintk(VFS, "NFS: uncached_readdir() searching for cookie %Lu\n",
(long long)desc->target);
if (desc->page) {
page_cache_release(desc->page);
desc->page = NULL;
}
cache_page = page_cache_alloc();
if (!cache_page) {
status = -ENOMEM;
goto out;
}
page = page_cache_entry(cache_page);
- p = (u32 *)page_address(page);
- status = NFS_PROTO(inode)->readdir(inode, cred, desc->target, p,
- NFS_SERVER(inode)->dtsize, 0);
- if (status >= 0) {
- p = desc->decode(p, desc->entry, 0);
+ start = (u32 *)page_address(page);
+ desc->error = NFS_PROTO(inode)->readdir(inode, cred, desc->target,
+ start, NFS_SERVER(inode)->dtsize, 0);
+ if (desc->error >= 0) {
+ p = desc->decode(start, desc->entry, 0);
if (IS_ERR(p))
status = PTR_ERR(p);
else
desc->entry->prev_cookie = desc->target;
- }
+ } else
+ status = -EIO;
if (status < 0)
goto out_release;
desc->page_index = 0;
- desc->page_offset = 0;
+ desc->page_offset = (char *)p - (char *)start;
desc->page = page;
status = nfs_do_filldir(desc, dirent, filldir);
/* Reset read descriptor so it searches the page cache from
* the start upon the next call to readdir_search_pagecache() */
desc->page_index = 0;
desc->page_offset = 0;
memset(desc->entry, 0, sizeof(*desc->entry));
out:
@@ -415,28 +416,27 @@
desc->file = filp;
desc->target = filp->f_pos;
desc->entry = &my_entry;
desc->decode = NFS_PROTO(inode)->decode_dirent;
while(!desc->entry->eof) {
res = readdir_search_pagecache(desc);
if (res == -EBADCOOKIE) {
/* This means either end of directory */
- if (desc->entry->cookie == desc->target) {
- res = 0;
- break;
+ if (desc->entry->cookie != desc->target) {
+ /* Or that the server has 'lost' a cookie */
+ res = uncached_readdir(desc, dirent, filldir);
+ if (res >= 0)
+ continue;
}
- /* Or that the server has 'lost' a cookie */
- res = uncached_readdir(desc, dirent, filldir);
- if (res >= 0)
- continue;
- }
- if (res < 0)
+ res = 0;
+ break;
+ } else if (res < 0)
break;
res = nfs_do_filldir(desc, dirent, filldir);
if (res < 0) {
res = 0;
break;
}
}
if (desc->page)
Jim Castleberry
>>>>>> " " == Jim Castleberry <jca...@in-system.com> writes:
> > >No. This should not make a difference, and in fact your patch is wrong
> > >here.
> >
> > >uncached_readdir() reads in a new page, and then tries to use the data
> > >returned to fill in part of the getdents structures. However, the page
> > >is never added to the inode's address space, hence it is completely
> > >discarded once we return from uncached_readdir(). Any offset within
> > >that page is therefore bogus.
>
> > desc->entry containing the first entry from the page and
> > desc->page_offset==0, which causes nfs_do_filldir() to pass the
> > first entry to filldir() twice. The page may be discarded once
> > uncached_readdir() returns, but the damage has already been
> > done by then.
>
>Oh bugger: Now I see what you mean. That's indeed a bug...
>
>Actually, that whole page_offset thing seems silly. I can see why I
>did it namely the kmap() & friends on 2.4.x, but on 2.2.x at least, we
>might as well just cache the pointer. We can do that on 2.4.x as well
>if we're clever about it...
So here's my original patch plus the rewrites to save RPC error
codes. Look okay?
jcastle
-------
+ if (!desc->entry->eof) {