Re: Re: Re: [PATCH 2/5] mm/readahead: Change the condition for SetPageReadahead

[Fengguang Wu <fengguang.wu@intel.com>]

Hi Raghavendra,

> An implication of 51daa88ebd8e0d437289f589af29d4b39379ea76 is that,
> a redundant check for PageReadahead(page) is avoided since  async is
> piggy-backed into the synchronous readahead itself.

That's right.

> So, in case of
> 
>     page = find_get_page()
>     if(!page)
>         page_cache_sync_readahead()
>     else if (PageReadahead(page))
>         page_cache_async_readahead();
> 
> isnt' there a possibility that PG_readahead won't be set at all if
> page is not in cache (causing page_cache_sync_readahead) but page at
> index  (nr_to_read - lookahead_size) is already in the cache? (due
> to if (page) continue; in the code)?

Yes, and I'm fully aware of that. It's left alone because it's assumed
to be a rare case. The nature of readahead is, there are all kinds of
less common cases that we deliberately ignore in order to keep the
code simple and maintainable.

> Hence, I changed the condition from equality to >= for setting
> SetPageReadahead(page) (and added a variable so that it is done only
> once).

It's excellent that you noticed that case. And sorry that I come to
realize that your change

-               if (page_idx == nr_to_read - lookahead_size)
+               if (page_idx >= nr_to_read - lookahead_size) {
                        SetPageReadahead(page);
+                       lookahead_size = 0;
+               }

won't negatively impact cache hot reads. So I have no strong feelings
about the patch now.

Thanks,
Fengguang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>