lkml.org 
[lkml]   [1999]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] 2.3.14: filemap_nopage hangs when offset >= inode->i_size
i suppose it's true, try_to_read_ahead doesn't do anything past the end of
the file, and in my read-ahead patch (from which i pulled the first fix) i
do check for EOF and do something special. i'll post a better fix for
this later today.

i've also got a fairly complete clean-up that addresses all the issues
you've found so far.

i'm not convinced you need the extra check to see if the page is uptodate
when re-reading a page, though.

On Fri, 20 Aug 1999, Steve Dodd wrote:

> Date: Fri, 20 Aug 1999 05:35:34 +0100
> From: Steve Dodd <dirk@loth.demon.co.uk>
> To: Chuck Lever <cel@monkey.org>, Linus Torvalds <torvalds@transmeta.com>
> Cc: x-linux-kernel@vger.rutgers.edu
> Subject: [PATCH] 2.3.14: filemap_nopage hangs when offset >= inode->i_size
>
> Ok, you know all the stuff that got ripped out of filemap_nopage for 2.3.14?
> It seems we need it :- try_to_read_ahead won't operate beyond of the end of
> the file (obviously), but that means when a non-shared mapping does extend
> past the end, filemap_nopage just loops, calling try_to_read_ahead forever
> to get the page it wants.
>
> The patch below includes the last one, and more or less reverts the 2.3.14
> changes (without reintroducing the bug it was meant to fix, I hope). Would
> it actually make sense to look at the whole function again? AFAICS the comments
> about overlapping allocation and I/O are wrong - surely we have to start the
> I/O, *then* allocate the page, if we want to overlap them? Would that be worth
> doing?
>
> Does lmbench time this sort of stuff?
>
> (Oh, I fixed a rather misleading comment in memory.c as well):
>
> --- mm/filemap.c.orig Fri Aug 20 05:33:00 1999
> +++ mm/filemap.c Fri Aug 20 05:43:59 1999
> @@ -1315,8 +1315,10 @@
> goto failure;
> }
>
> - if (!Page_Uptodate(page))
> + if (!Page_Uptodate(page)) {
> + lock_page(page);
> goto page_not_uptodate;
> + }
>
> success:
> /*
> @@ -1337,8 +1339,13 @@
> }
>
> /*
> - * No sharing ... copy to the new page.
> + * No sharing ... get a new page if necessary and copy to it.
> */
> + if (!new_page)
> + new_page = page_cache_alloc();
> + if (!new_page)
> + goto no_page;
> +
> copy_page(new_page, old_page);
> flush_page_to_ram(new_page);
> page_cache_release(page);
> @@ -1346,6 +1353,13 @@
>
> no_cached_page:
> /*
> + * read ahead won't read past end of file, so we will have
> + * to add the page ourselves.
> + */
> + if (offset >= inode->i_size)
> + goto add_new_page;
> +
> + /*
> * Try to read in an entire cluster at once.
> */
> reada = offset;
> @@ -1362,8 +1376,22 @@
> */
> goto retry_find;
>
> +add_new_page:
> + if (!new_page)
> + new_page = page_cache_alloc();
> + if (!new_page)
> + goto no_page;
> +
> + /*
> + * Add the new page to the page cache. If someone beat
> + * us to it, go and look for the page again.
> + */
> + page = page_cache_entry(new_page);
> + if (add_to_page_cache_unique(page, inode, offset, hash))
> + goto retry_find;
> + new_page = 0;
> +
> page_not_uptodate:
> - lock_page(page);
> if (Page_Uptodate(page)) {
> UnlockPage(page);
> goto success;
> @@ -1375,6 +1403,7 @@
> wait_on_page(page);
> if (Page_Uptodate(page))
> goto success;
> + lock_page(page);
> }
>
> /*
> @@ -1383,8 +1412,11 @@
> * because there really aren't any performance issues here
> * and we need to check for errors.
> */
> - if (!PageLocked(page))
> - PAGE_BUG(page);
> + if (Page_Uptodate(page)) {
> + UnlockPage(page);
> + goto success;
> + }
> +
> ClearPageError(page);
> error = inode->i_op->readpage(file, page);
> if (error)
> --- mm/memory.c.orig Fri Aug 20 04:51:16 1999
> +++ mm/memory.c Fri Aug 20 04:51:29 1999
> @@ -1034,8 +1034,7 @@
> * As this is called only for pages that do not currently exist, we
> * do not need to flush old virtual caches or the TLB.
> *
> - * This is called with the MM semaphore and the kernel lock held.
> - * We need to release the kernel lock as soon as possible..
> + * This is called with the MM semaphore held.
> */
> static int do_no_page(struct task_struct * tsk, struct vm_area_struct * vma,
> unsigned long address, int write_access, pte_t *page_table)
>
> --
> When in doubt, use brute force.
> -- Ken Thompson
>

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

\
 
 \ /
  Last update: 2005-03-22 13:53    [W:0.095 / U:0.624 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site