[PATCH] nfs: remove redundant tests on reading new pages

[PATCH] nfs: remove redundant tests on reading new pages

[Wu Fengguang]

aops->readpages() and its NFS helper readpage_async_filler() will only
be called to do readahead I/O for newly allocated pages. So it's not
necessary to test for the always 0 dirty/uptodate page flags.

The removal of nfs_wb_page() call also fixes a readahead bug: the NFS
readahead has been synchronous since 2.6.23, because that call will
clear PG_readahead, which is the reminder for asynchronous readahead.

More background: the PG_readahead page flag is shared with PG_reclaim,
one for read path and the other for write path. clear_page_dirty_for_io()
unconditionally clears PG_readahead to prevent possible readahead residuals,
assuming itself to be always called in the write path. However, NFS is one
and the only exception in that it _always_ calls clear_page_dirty_for_io()
in the read path, i.e. for readpages()/readpage().

Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
Signed-off-by: Wu Fengguang <wfg@linux.intel.com>
---
 fs/nfs/read.c |    6 ------
 1 file changed, 6 deletions(-)

This bug was found when playing with my readahead tracing module posted at
http://lkml.org/lkml/2008/11/27/373 and confirmed to work.

The NFS readahead traces are now

[   59.972526] readahead-initial0(pid=3646(dd), dev=00:0f(0:f), ino=293189(1G), req=0+25, ra=0+60-30, async=0) = 60
[   59.992734] readahead-subsequent(pid=3646(dd), dev=00:0f(0:f), ino=293189(1G), req=30+20, ra=60+30-30, async=1) = 30
[   60.030900] readahead-subsequent(pid=3646(dd), dev=00:0f(0:f), ino=293189(1G), req=60+15, ra=90+30-30, async=1) = 30
[   60.031687] readahead-subsequent(pid=3646(dd), dev=00:0f(0:f), ino=293189(1G), req=90+10, ra=120+30-30, async=1) = 30

The async field was always 0 before this patch.

--- linux-2.6.orig/fs/nfs/read.c
+++ linux-2.6/fs/nfs/read.c
@@ -533,12 +533,6 @@ readpage_async_filler(void *data, struct
 	unsigned int len;
 	int error;
 
-	error = nfs_wb_page(inode, page);
-	if (error)
-		goto out_unlock;
-	if (PageUptodate(page))
-		goto out_unlock;
-
 	len = nfs_page_length(page);
 	if (len == 0)
 		return nfs_return_empty_page(page);

Re: [PATCH] nfs: remove redundant tests on reading new pages

[Wu Fengguang]

On Mon, Dec 01, 2008 at 11:19:22AM +0800, Wu Fengguang wrote:
> aops->readpages() and its NFS helper readpage_async_filler() will only
> be called to do readahead I/O for newly allocated pages. So it's not
> necessary to test for the always 0 dirty/uptodate page flags.
> 
> The removal of nfs_wb_page() call also fixes a readahead bug: the NFS
> readahead has been synchronous since 2.6.23, because that call will
> clear PG_readahead, which is the reminder for asynchronous readahead.
> 
> More background: the PG_readahead page flag is shared with PG_reclaim,
> one for read path and the other for write path. clear_page_dirty_for_io()
> unconditionally clears PG_readahead to prevent possible readahead residuals,
> assuming itself to be always called in the write path. However, NFS is one
> and the only exception in that it _always_ calls clear_page_dirty_for_io()
> in the read path, i.e. for readpages()/readpage().
> 
> Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
> Signed-off-by: Wu Fengguang <wfg@linux.intel.com>
> ---
>  fs/nfs/read.c |    6 ------
>  1 file changed, 6 deletions(-)
> 
> This bug was found when playing with my readahead tracing module posted at
> http://lkml.org/lkml/2008/11/27/373 and confirmed to work.
> 
> The NFS readahead traces are now
> 
> [   59.972526] readahead-initial0(pid=3646(dd), dev=00:0f(0:f), ino=293189(1G), req=0+25, ra=0+60-30, async=0) = 60
> [   59.992734] readahead-subsequent(pid=3646(dd), dev=00:0f(0:f), ino=293189(1G), req=30+20, ra=60+30-30, async=1) = 30
> [   60.030900] readahead-subsequent(pid=3646(dd), dev=00:0f(0:f), ino=293189(1G), req=60+15, ra=90+30-30, async=1) = 30
> [   60.031687] readahead-subsequent(pid=3646(dd), dev=00:0f(0:f), ino=293189(1G), req=90+10, ra=120+30-30, async=1) = 30
> 
> The async field was always 0 before this patch.

On the trace messages: they are generated by the command
'dd if=/tmp/sparse/1G of=/dev/null bs=100k count=9' with readahead_size=120k.

The printk for generating the messages is:

        printk(KERN_DEBUG "readahead-%s(pid=%d(%s), dev=%02x:%02x(%s), "
                "ino=%lu(%s), req=%lu+%lu, ra=%lu+%d-%d, async=%d) = %d\n",
                ra_pattern_names[pattern],
                current->pid, current->comm,
                MAJOR(mapping->host->i_sb->s_dev),
                MINOR(mapping->host->i_sb->s_dev),
                mapping->host->i_sb->s_id,
                mapping->host->i_ino,
                filp->f_path.dentry->d_name.name,
                offset, req_size,
                ra->start, ra->size, ra->async_size,
                async,
                actual);

Thanks,
Fengguang

> --- linux-2.6.orig/fs/nfs/read.c
> +++ linux-2.6/fs/nfs/read.c
> @@ -533,12 +533,6 @@ readpage_async_filler(void *data, struct
>  	unsigned int len;
>  	int error;
>  
> -	error = nfs_wb_page(inode, page);
> -	if (error)
> -		goto out_unlock;
> -	if (PageUptodate(page))
> -		goto out_unlock;
> -
>  	len = nfs_page_length(page);
>  	if (len == 0)
>  		return nfs_return_empty_page(page);