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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 1 Dec 2008 11:31:09 +0800
From:	Wu Fengguang <fengguang.wu@...el.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-nfs@...r.kernel.org,
	Trond Myklebust <Trond.Myklebust@...app.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"stable@...nel.org" <stable@...nel.org>
Subject: Re: [PATCH] nfs: remove redundant tests on reading new pages

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@...app.com>
> Signed-off-by: Wu Fengguang <wfg@...ux.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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/