Messages in this thread | | | Date | Fri, 20 Aug 1999 15:12:21 -0400 (EDT) | From | Chuck Lever <> | Subject | Re: [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/
| |