Skip to contentSkip to conversation

nfs: remove redundant tests on reading new pages

PATCH In review Wu Fengguang 1 participant 2 messages Dec 1, 2008

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 nfs: remove redundant tests on reading new pages

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

fs/nfs/read.c +0 -6
@@ -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);
#1 lore raw
Quoted 24 lines · 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 <[email protected]>
---
 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.

Quoted 8 lines

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

Quoted 15 lines
fs/nfs/read.c +0 -6
@@ -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);