Re: filemap_nopage() tries to copy to page 0, eek.

Chuck Lever (cel@monkey.org)
Thu, 12 Aug 1999 21:12:59 -0400 (EDT)


On Thu, 12 Aug 1999, Steve Dodd wrote:
> Err, yes, 2.3.12 and 2.3.13 (I think). I'm away from my machine at the
> moment so I can't check, but I was using whatever was current when I posted
> the message. The simplest option seemed to be to stick a
>
> if (!new_page)
> new_page = page_cache_alloc();
>
> just before we attempt to copy to new_page, but I was wondering if we could
> overlap the page allocation for new_page and the previous ->readpage()..

here's a cleaner patch i came up with flying back from san jose. it's
against 2.3.10, but should apply to 2.3.1[23] without much fiddling.
essentially it's the same as what i described yesterday, but there's some
additional clean-up and extra comments that go along with the deletion of
redundant code. i've been testing a more complicated variant of this for
a few weeks, and haven't seen any trouble with it.

Steve, can you try this out and let me know if it helps?

--- linux-2.3.10-ref/mm/filemap.c Sat Jul 10 12:27:06 1999
+++ linux/mm/filemap.c Thu Aug 12 16:03:00 1999
@@ -1302,7 +1302,6 @@
if (!page)
goto no_cached_page;

-found_page:
/*
* Ok, found a page in the page cache, now we need to check
* that it's up-to-date. First check whether we'll need an
@@ -1314,12 +1313,8 @@
goto failure;
}

- if (!Page_Uptodate(page)) {
- lock_page(page);
- if (!Page_Uptodate(page))
- goto page_not_uptodate;
- UnlockPage(page);
- }
+ if (!Page_Uptodate(page))
+ goto page_not_uptodate;

success:
/*
@@ -1358,44 +1353,28 @@
for (i = 1 << page_cluster; i > 0; --i, reada += PAGE_CACHE_SIZE)
new_page = try_to_read_ahead(file, reada, new_page);

- if (!new_page)
- new_page = page_cache_alloc();
- if (!new_page)
- goto no_page;
-
/*
- * During getting the above page we might have slept,
- * so we need to re-check the situation with the page
- * cache.. The page we just got may be useful if we
- * can't share, so don't get rid of it here.
+ * The page we want has now been added to the page cache.
+ * In the unlikely event that someone removed it in the
+ * meantime, we'll just come back here and read it again.
*/
- page = __find_get_page(inode, offset, hash);
- if (page)
- goto found_page;
-
- /*
- * Now, create a new page-cache page from the page we got
- */
- page = page_cache_entry(new_page);
- if (add_to_page_cache_unique(page, inode, offset, hash))
- goto retry_find;
-
- /*
- * Now it's ours and locked, we can do initial IO to it:
- */
- new_page = 0;
+ goto retry_find;

page_not_uptodate:
+ lock_page(page);
+ if (Page_Uptodate(page)) {
+ UnlockPage(page);
+ goto success;
+ }
+
error = inode->i_op->readpage(file, page);

if (!error) {
wait_on_page(page);
- if (PageError(page))
- goto page_read_error;
- goto success;
+ if (Page_Uptodate(page))
+ goto success;
}

-page_read_error:
/*
* Umm, take care of errors if the page isn't up-to-date.
* Try to re-read it _once_. We do this synchronously,

- Chuck Lever

--
corporate:	<chuckl@netscape.com>
personal:	<chucklever@netscape.net> or <cel@monkey.org>

The Linux Scalability project: http://www.citi.umich.edu/projects/linux-scalability/

- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/