Hi,

On Thu, 2013-08-22 at 18:16 +0300, Kirill A. Shutemov wrote:

Steven Whitehouse wrote:

quoted

Hi,

On Thu, 2013-08-22 at 17:30 +0300, Kirill A. Shutemov wrote:
[snip]

quoted

quoted

Andrew's proposed solution makes sense to me, and is probably the
easiest way to solve this.

Move check to no_cached_page?

Yes

quoted

I don't see how it makes any difference for
page cache miss case: we anyway exclude ->readpage() if it's beyond local
i_size.
And for cache hit local i_size will be most likely cover locally cached
pages.

The difference is that as the function is currently written, you cannot
get to no_cached_page without first calling page_cache_sync_readahead(),
i.e. ->readpages() so that i_size will have been updated, even if
->readpages() doesn't return any read-ahead pages.

I guess that it is not very obvious that a call to ->readpages is hidden
in page_cache_sync_readahead() but that is the path that should in the
common case provide the pages from the fs, rather than the ->readpage
call thats further down do_generic_file_read()

I've checked the codepath before and to me it looks like ->readpages()
will not be called beyond i_size. Codepath is:

page_cache_sync_readahead()
 ondemand_readahead()
  __do_page_cache_readahead()
   read_pages()
    mapping->a_ops->readpages()

But if you check __do_page_cache_readahead():

152 static int
153 __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
154                         pgoff_t offset, unsigned long nr_to_read,
155                         unsigned long lookahead_size)
156 {
...
163         loff_t isize = i_size_read(inode);
164
165         if (isize == 0)
166                 goto out;
167
168         end_index = ((isize - 1) >> PAGE_CACHE_SHIFT);
...
173         for (page_idx = 0; page_idx < nr_to_read; page_idx++) {
174                 pgoff_t page_offset = offset + page_idx;
175
176                 if (page_offset > end_index)
177                         break;
...
193         }
...
200         if (ret)
201                 read_pages(mapping, filp, &page_pool, ret);
202         BUG_ON(!list_empty(&page_pool));
203 out:
204         return ret;
205 }

Do I miss something?

Hrm. It looks like that Andrew's proposal is not going to work then :(

The trouble is that readahead has broken silently in this one case,
since the net result of the readahead not going ahead at this point, is
that ->readpage will be called later on. So in the cases where it
matters (i.e. i_size updated remotely), we'd probably not have noticed
it before as the read will still return successfully.

That is real problem though... so maybe this isn't as easy as I'd first
thought,

Steve.


--
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>