[PATCH 2/4] page_cache_readahead: remove duplicated code

[PATCH 2/4] page_cache_readahead: remove duplicated code

[Oleg Nesterov]

Cases "no ahead window" and "crossed into ahead window"
can be unified.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 2.6.11-rc2/mm/readahead.c~	2005-01-25 14:28:22.000000000 +0300
+++ 2.6.11-rc2/mm/readahead.c	2005-01-25 15:17:13.000000000 +0300
@@ -483,43 +483,24 @@ page_cache_readahead(struct address_spac
 	 * occurence (ie we have an existing window)
 	 */
 
-	if (ra->ahead_start == 0) {	 /* no ahead window yet */
-		ra->ahead_size = get_next_ra_size(ra->size, max, min,
-						  &ra->flags);
-		ra->ahead_start = ra->start + ra->size;
-		block = ((offset + newsize -1) >= ra->ahead_start);
-		if (!blockable_page_cache_readahead(mapping, filp,
-		    ra->ahead_start, ra->ahead_size, ra, block)) {
-			/* A read failure in blocking mode, implies pages are
-			 * all cached. So we can safely assume we have taken
-			 * care of all the pages requested in this call. A read
-			 * failure in non-blocking mode, implies we are reading
-			 * more pages than requested in this call.  So we safely
-			 * assume we have taken care of all the pages requested
-			 * in this call.
-			 *
-			 * Just reset the ahead window in case we failed due to
-			 * congestion.  The ahead window will any way be closed
-			 * in case we failed due to exessive page cache hits.
-			 */
-			ra->ahead_start = 0;
-			ra->ahead_size = 0;
-			goto out;
-		}
-	}
 	/*
-	 * Already have an ahead window, check if we crossed into it.
+	 * Check if we crossed into ahead window.
 	 * If so, shift windows and issue a new ahead window.
 	 * Only return the #pages that are in the current window, so that
 	 * we get called back on the first page of the ahead window which
 	 * will allow us to submit more IO.
 	 */
 	if ((offset + newsize - 1) >= ra->ahead_start) {
-		ra->start = ra->ahead_start;
-		ra->size = ra->ahead_size;
-		ra->ahead_start = ra->ahead_start + ra->ahead_size;
-		ra->ahead_size = get_next_ra_size(ra->ahead_size,
-						  max, min, &ra->flags);
+		/* Check if we already have an ahead window */
+		if (ra->ahead_start != 0) {
+			ra->start = ra->ahead_start;
+			ra->size = ra->ahead_size;
+		}
+
+		ra->ahead_size = get_next_ra_size(ra->size, max, min,
+							&ra->flags);
+		ra->ahead_start = ra->start + ra->size;
+
 		block = ((offset + newsize - 1) >= ra->ahead_start);
 		if (!blockable_page_cache_readahead(mapping, filp,
 			ra->ahead_start, ra->ahead_size, ra, block)) {

Re: [PATCH 2/4] page_cache_readahead: remove duplicated code

[Steven Pratt]

I like this one, especially getting rid of the large duplicate comment.  
No functional difference.

Signed-off-by: Steven Pratt <slpratt@austin.bm.com>

Oleg Nesterov wrote:

>Cases "no ahead window" and "crossed into ahead window"
>can be unified.
>
>Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
>
>--- 2.6.11-rc2/mm/readahead.c~	2005-01-25 14:28:22.000000000 +0300
>+++ 2.6.11-rc2/mm/readahead.c	2005-01-25 15:17:13.000000000 +0300
>@@ -483,43 +483,24 @@ page_cache_readahead(struct address_spac
> 	 * occurence (ie we have an existing window)
> 	 */
> 
>-	if (ra->ahead_start == 0) {	 /* no ahead window yet */
>-		ra->ahead_size = get_next_ra_size(ra->size, max, min,
>-						  &ra->flags);
>-		ra->ahead_start = ra->start + ra->size;
>-		block = ((offset + newsize -1) >= ra->ahead_start);
>-		if (!blockable_page_cache_readahead(mapping, filp,
>-		    ra->ahead_start, ra->ahead_size, ra, block)) {
>-			/* A read failure in blocking mode, implies pages are
>-			 * all cached. So we can safely assume we have taken
>-			 * care of all the pages requested in this call. A read
>-			 * failure in non-blocking mode, implies we are reading
>-			 * more pages than requested in this call.  So we safely
>-			 * assume we have taken care of all the pages requested
>-			 * in this call.
>-			 *
>-			 * Just reset the ahead window in case we failed due to
>-			 * congestion.  The ahead window will any way be closed
>-			 * in case we failed due to exessive page cache hits.
>-			 */
>-			ra->ahead_start = 0;
>-			ra->ahead_size = 0;
>-			goto out;
>-		}
>-	}
> 	/*
>-	 * Already have an ahead window, check if we crossed into it.
>+	 * Check if we crossed into ahead window.
> 	 * If so, shift windows and issue a new ahead window.
> 	 * Only return the #pages that are in the current window, so that
> 	 * we get called back on the first page of the ahead window which
> 	 * will allow us to submit more IO.
> 	 */
> 	if ((offset + newsize - 1) >= ra->ahead_start) {
>-		ra->start = ra->ahead_start;
>-		ra->size = ra->ahead_size;
>-		ra->ahead_start = ra->ahead_start + ra->ahead_size;
>-		ra->ahead_size = get_next_ra_size(ra->ahead_size,
>-						  max, min, &ra->flags);
>+		/* Check if we already have an ahead window */
>+		if (ra->ahead_start != 0) {
>+			ra->start = ra->ahead_start;
>+			ra->size = ra->ahead_size;
>+		}
>+
>+		ra->ahead_size = get_next_ra_size(ra->size, max, min,
>+							&ra->flags);
>+		ra->ahead_start = ra->start + ra->size;
>+
> 		block = ((offset + newsize - 1) >= ra->ahead_start);
> 		if (!blockable_page_cache_readahead(mapping, filp,
> 			ra->ahead_start, ra->ahead_size, ra, block)) {
>  
>

Re: [PATCH 2/4] page_cache_readahead: remove duplicated code

[Ram]

On Tue, 2005-01-25 at 03:59, Oleg Nesterov wrote:
> Cases "no ahead window" and "crossed into ahead window"
> can be unified.


No. There is a reason why we had some duplication. With your patch,
we will end up reading-on-demand instead of reading ahead.

When we notice a sequential reads have resumed, we first read in the
data that is requested. 
However if the read request is for more pages than what are being held
in the current window, we make the ahead window as the current window
and read in more pages in the ahead window. Doing that gives the
opportunity of always having pages in the ahead window when the next
sequential read request comes in.  If we apply this patch, we will
always have to read the pages that are being requested instead of
satisfying them from the ahead window.

Ok, if this does not make it clear, here is another way of proving that
your patch does not exactly behave the way it did earlier.

With your patch you will have only one call to
block_page_cache_readahead(), when earlier there could be cases where
block_page_cache_readahead() could be called twice.

Am I am making sense?
RP
 

Re: [PATCH 2/4] page_cache_readahead: remove duplicated code

[Steven Pratt]

Ram wrote:

>On Tue, 2005-01-25 at 03:59, Oleg Nesterov wrote:
>  
>
>>Cases "no ahead window" and "crossed into ahead window"
>>can be unified.
>>    
>>
>
>
>No. There is a reason why we had some duplication. With your patch,
>we will end up reading-on-demand instead of reading ahead.
>
>When we notice a sequential reads have resumed, we first read in the
>data that is requested. 
>However if the read request is for more pages than what are being held
>in the current window, we make the ahead window as the current window
>and read in more pages in the ahead window. Doing that gives the
>opportunity of always having pages in the ahead window when the next
>sequential read request comes in.  If we apply this patch, we will
>always have to read the pages that are being requested instead of
>satisfying them from the ahead window.
>  
>
Ah, you are right! 

>Ok, if this does not make it clear, here is another way of proving that
>your patch does not exactly behave the way it did earlier.
>
>With your patch you will have only one call to
>block_page_cache_readahead(), when earlier there could be cases where
>block_page_cache_readahead() could be called twice.
>
>Am I am making sense?
>

Completely, this patch should not be applied.  Good catch.

Steve

>RP
> 
>  
>

Re: [PATCH 2/4] page_cache_readahead: remove duplicated code

[Oleg Nesterov]

Ram wrote:
>
> No. There is a reason why we had some duplication. With your patch,
> we will end up reading-on-demand instead of reading ahead.
>
> When we notice a sequential reads have resumed, we first read in the
> data that is requested.
> However if the read request is for more pages than what are being held
> in the current window, we make the ahead window as the current window
> and read in more pages in the ahead window. Doing that gives the
> opportunity of always having pages in the ahead window when the next
> sequential read request comes in.

Yes, sorry. I have not noticed that this 'goto out' is conditional in
the 'no ahead window' case.

Thank you for explanation.

However, I still think it makes sense to factor out the common code in
these two cases, just for readability.

I'll redo these patches.

Oleg.

Re: [PATCH 2/4] page_cache_readahead: remove duplicated code

[Ram]

On Wed, 2005-01-26 at 04:02, Oleg Nesterov wrote:
> Ram wrote:
> >
> > No. There is a reason why we had some duplication. With your patch,
> > we will end up reading-on-demand instead of reading ahead.
> >
> > When we notice a sequential reads have resumed, we first read in the
> > data that is requested.
> > However if the read request is for more pages than what are being held
> > in the current window, we make the ahead window as the current window
> > and read in more pages in the ahead window. Doing that gives the
> > opportunity of always having pages in the ahead window when the next
> > sequential read request comes in.
> 
> Yes, sorry. I have not noticed that this 'goto out' is conditional in
> the 'no ahead window' case.
> 
> Thank you for explanation.
> 
> However, I still think it makes sense to factor out the common code in
> these two cases, just for readability.

We did consider putting a while loop, which looped twice. That looked
ugly too. So we left it as is. You might have better ideas.

> 
> I'll redo these patches.
> 
Your 1st patch was fine. I have not looked deeply through your 3rd and
4th patch. However I will wait till you redo your patches.

RP
> Oleg.