[PATCH] 2.4 read ahead never reads the last page in a file

[PATCH] 2.4 read ahead never reads the last page in a file

[Chuck Lever]

hi marcelo-

i posted this a while back on fsdevel for comments, but never heard
anything.  i'd like this patch to be included in 2.4.24, as it improves
NFS client read performance by a significant margin, and doesn't appear to
have any negative impact (see fsdevel archives for benchmarks).  this is
against 2.4.23.

generic_file_readahead never reads the last page of a file.  this means
the last page is always read synchronously by do_generic_file_read.
normally this is not an issue, as most local disk reads are fast.
however, this means that the NFS client is never given the opportunity to
coalesce the last page of a file, which must be read in a separate
synchronous read operation.

this is especially honerous if the network round trip time is long, and/or
the workload consists of reading many uncached small files.

i also explored changing the way that generic_file_readahead computes the
index of the last page of the file.  the fix below appears to be the best
solution.



diff -X /home/cel/src/linux/dont-diff -Naurp 00-stock/mm/filemap.c 01-readahead1/mm/filemap.c
--- 00-stock/mm/filemap.c	2003-11-28 13:26:21.000000000 -0500
+++ 01-readahead1/mm/filemap.c	2003-12-03 16:46:11.000000000 -0500
@@ -1299,11 +1299,14 @@ static void generic_file_readahead(int r
  */
 	ahead = 0;
 	while (ahead < max_ahead) {
-		ahead ++;
-		if ((raend + ahead) >= end_index)
+		unsigned long ra_index = raend + ahead + 1;
+
+		if (ra_index > end_index)
 			break;
-		if (page_cache_read(filp, raend + ahead) < 0)
+		if (page_cache_read(filp, ra_index) < 0)
 			break;
+
+		ahead++;
 	}
 /*
  * If we tried to read ahead some pages,

	- Chuck Lever
--
corporate:	<cel at netapp dot com>
personal:	<chucklever at bigfoot dot com>

Re: [PATCH] 2.4 read ahead never reads the last page in a file

[Andrea Arcangeli]

On Wed, Dec 03, 2003 at 06:10:31PM -0500, Chuck Lever wrote:
> hi marcelo-
> 
> i posted this a while back on fsdevel for comments, but never heard
> anything.  i'd like this patch to be included in 2.4.24, as it improves
> NFS client read performance by a significant margin, and doesn't appear to
> have any negative impact (see fsdevel archives for benchmarks).  this is
> against 2.4.23.
> 
> generic_file_readahead never reads the last page of a file.  this means
> the last page is always read synchronously by do_generic_file_read.
> normally this is not an issue, as most local disk reads are fast.

sending two sync req is quite slower than 1 sync req even with the disk,
infact in my experience the biggest issue with fast storage isn't the
seeking but the need of sending big requests down to the hardware.
Infact I think it's a _much_ bigger issue for disk than for nfs. Can you
benchmark it on a IDE with 128k long files? They should be read with a
single command, two commands are going to hurt.

> diff -X /home/cel/src/linux/dont-diff -Naurp 00-stock/mm/filemap.c 01-readahead1/mm/filemap.c
> --- 00-stock/mm/filemap.c	2003-11-28 13:26:21.000000000 -0500
> +++ 01-readahead1/mm/filemap.c	2003-12-03 16:46:11.000000000 -0500
> @@ -1299,11 +1299,14 @@ static void generic_file_readahead(int r
>   */
>  	ahead = 0;
>  	while (ahead < max_ahead) {
> -		ahead ++;
> -		if ((raend + ahead) >= end_index)
> +		unsigned long ra_index = raend + ahead + 1;
> +
> +		if (ra_index > end_index)
>  			break;
> -		if (page_cache_read(filp, raend + ahead) < 0)
> +		if (page_cache_read(filp, ra_index) < 0)
>  			break;
> +
> +		ahead++;
>  	}
>  /*
>   * If we tried to read ahead some pages,
> 

looks ok to me. I guess this could be an historic paranoid/(possibly
needed in the past) thing, to avoid reading partial pages from there,
but the pagecache layer is robust enough to handle it these days.

Thanks.

Re: [PATCH] 2.4 read ahead never reads the last page in a file

[Mike Fedyk]

On Wed, Dec 03, 2003 at 06:10:31PM -0500, Chuck Lever wrote:
> generic_file_readahead never reads the last page of a file.  this means
> the last page is always read synchronously by do_generic_file_read.

Has this been fixed in 2.6?  If so, it would improve the speed of mail
servers that use maildir, (I think mbox should be ok, since the data that
was appended has a good chance of still being in memory.  Though, to compute
UIDL for pop3, it has to read the entire mbox file, forcing other mbox users
out of cache, and then evening up performance between the two.  Though this
patch should help maildir more.)

Re: [PATCH] 2.4 read ahead never reads the last page in a file

[Marcelo Tosatti]

On Fri, 5 Dec 2003, Andrea Arcangeli wrote:

> On Wed, Dec 03, 2003 at 06:10:31PM -0500, Chuck Lever wrote:
> > hi marcelo-
> > 
> > i posted this a while back on fsdevel for comments, but never heard
> > anything.  i'd like this patch to be included in 2.4.24, as it improves
> > NFS client read performance by a significant margin, and doesn't appear to
> > have any negative impact (see fsdevel archives for benchmarks).  this is
> > against 2.4.23.
> > 
> > generic_file_readahead never reads the last page of a file.  this means
> > the last page is always read synchronously by do_generic_file_read.
> > normally this is not an issue, as most local disk reads are fast.
> 
> sending two sync req is quite slower than 1 sync req even with the disk,
> infact in my experience the biggest issue with fast storage isn't the
> seeking but the need of sending big requests down to the hardware.
> Infact I think it's a _much_ bigger issue for disk than for nfs. Can you
> benchmark it on a IDE with 128k long files? They should be read with a
> single command, two commands are going to hurt.
> 
> > diff -X /home/cel/src/linux/dont-diff -Naurp 00-stock/mm/filemap.c 01-readahead1/mm/filemap.c
> > --- 00-stock/mm/filemap.c	2003-11-28 13:26:21.000000000 -0500
> > +++ 01-readahead1/mm/filemap.c	2003-12-03 16:46:11.000000000 -0500
> > @@ -1299,11 +1299,14 @@ static void generic_file_readahead(int r
> >   */
> >  	ahead = 0;
> >  	while (ahead < max_ahead) {
> > -		ahead ++;
> > -		if ((raend + ahead) >= end_index)
> > +		unsigned long ra_index = raend + ahead + 1;
> > +
> > +		if (ra_index > end_index)
> >  			break;
> > -		if (page_cache_read(filp, raend + ahead) < 0)
> > +		if (page_cache_read(filp, ra_index) < 0)
> >  			break;
> > +
> > +		ahead++;
> >  	}
> >  /*
> >   * If we tried to read ahead some pages,
> > 
> 
> looks ok to me. I guess this could be an historic paranoid/(possibly
> needed in the past) thing, to avoid reading partial pages from there,
> but the pagecache layer is robust enough to handle it these days.

Patch applied. 

Re: [PATCH] 2.4 read ahead never reads the last page in a file

[Willy Tarreau]

Hi Chuck,

I've run 2.4.23 both vanilla and patched, and I confirm a win on NFS reads.
I tested from 1 to 6 parallel reads consisting of md5sum of kernel includes :
  time find include -type f | xargs md5sum >/dev/null &
Results below :

processes   2.4.23       2.4.23-patched
    1       11.5         11.45
    2       12.82        12.68
    3       14.7         14.4
    4       17.8         17.3
    5       21.5         21.0
    6       25.7         25.0  => server is saturated 100% sys
    7       29.3         29.0  => server is saturated 100% sys

Fileset contain 7346 files totalizing 29952 kB.
So there's a gain of nearly 3% in the best case (6 processes). Not bad.
I suspect that I could also gain slightly more by patching the server too.

Cheers,
Willy