diff mbox

[VFS] readahead: fix block ordering in __do_page_cache_readahead

Message ID 287059480.9694215.1452805133579.JavaMail.zimbra@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bob Peterson Jan. 14, 2016, 8:58 p.m. UTC
Hi,

I just ran into this today. I had temporarily disabled readpages in GFS2 in order to
test something and discovered, to my surprise, that the block IOs were being issued
in reverse order. In my case, the block_map function was called in reverse block
order. In other words: lblock started at 8, then proceeded to 7, 6, 5, 4, 3, 2, 1, 0.
So I surmised the order must be wrong. After a little digging, I whipped up this
little patch. Can anyone out there corroborate this or tell me if I've I lost my mind?

Viro?

I'll add my Signed-off-by but I really haven't tested it or anything. It's kind of
moot for file systems with readpages, but it might be worth doing.

Regards,

Bob Peterson
Red Hat File Systems

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jan Kara Jan. 21, 2016, 2:09 p.m. UTC | #1
On Thu 14-01-16 15:58:53, Bob Peterson wrote:
> Hi,
> 
> I just ran into this today. I had temporarily disabled readpages in GFS2 in order to
> test something and discovered, to my surprise, that the block IOs were being issued
> in reverse order. In my case, the block_map function was called in reverse block
> order. In other words: lblock started at 8, then proceeded to 7, 6, 5, 4, 3, 2, 1, 0.
> So I surmised the order must be wrong. After a little digging, I whipped up this
> little patch. Can anyone out there corroborate this or tell me if I've I lost my mind?
> 
> Viro?
> 
> I'll add my Signed-off-by but I really haven't tested it or anything. It's kind of
> moot for file systems with readpages, but it might be worth doing.
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>

The patch looks good to me. But I've added CC's to some relevant people.

								Honza

> ---
> diff --git a/mm/readahead.c b/mm/readahead.c
> index ba22d7f..b76fb34 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -185,7 +185,7 @@ int __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>  		if (!page)
>  			break;
>  		page->index = page_offset;
> -		list_add(&page->lru, &page_pool);
> +		list_add_tail(&page->lru, &page_pool);
>  		if (page_idx == nr_to_read - lookahead_size)
>  			SetPageReadahead(page);
>  		ret++;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Morton Jan. 21, 2016, 9:03 p.m. UTC | #2
On Thu, 21 Jan 2016 15:09:16 +0100 Jan Kara <jack@suse.cz> wrote:

> On Thu 14-01-16 15:58:53, Bob Peterson wrote:
> > Hi,
> > 
> > I just ran into this today. I had temporarily disabled readpages in GFS2 in order to
> > test something and discovered, to my surprise, that the block IOs were being issued
> > in reverse order. In my case, the block_map function was called in reverse block
> > order. In other words: lblock started at 8, then proceeded to 7, 6, 5, 4, 3, 2, 1, 0.
> > So I surmised the order must be wrong. After a little digging, I whipped up this
> > little patch. Can anyone out there corroborate this or tell me if I've I lost my mind?
> > 
> > Viro?
> > 
> > I'll add my Signed-off-by but I really haven't tested it or anything. It's kind of
> > moot for file systems with readpages, but it might be worth doing.
> > 
> > Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> 
> The patch looks good to me. But I've added CC's to some relevant people.
> 

The patch looks good to me.  How curious.  It's hard to believe that
this has persisted for so long without being noticed.

However "kernel test robot" is reporting a 13.5% performance regression
from this patch.  http://comments.gmane.org/gmane.linux.kernel/2128714

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/mm/readahead.c b/mm/readahead.c
index ba22d7f..b76fb34 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -185,7 +185,7 @@  int __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
 		if (!page)
 			break;
 		page->index = page_offset;
-		list_add(&page->lru, &page_pool);
+		list_add_tail(&page->lru, &page_pool);
 		if (page_idx == nr_to_read - lookahead_size)
 			SetPageReadahead(page);
 		ret++;