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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>