good spotting... i looked at this a while back with the same suspicion.
locking the page again, as in your patch, will cause a deadlock. the only
way for a page to be not up-to-date after the wait_on_page is if an I/O
error occurs, reported by the end_io exits in buffer.c. with the current
logic, that will trigger a BUG instead of doing error recovery. for file
systems that don't use block_read_full_page, sometimes readpage will
return an error value (see romfs for example). i think the readpage
implementations are careful enough to leave the page locked if they return
an error, but it might be good to review each.
i had originally fixed this by removing the I/O retry logic (since the
page will be read once by read-ahead, and then retried once by the
page_not_uptodate logic). but you can probably just get away with
re-locking like this:
error = inode->i_op->readpage(file, page);
if (!error) {
wait_on_page(page);
if (Page_Uptodate(page))
goto success;
lock_page(page);
^^^^^^^^^^^^^^^^
}
do you have a test case?
On Thu, 19 Aug 1999, Steve Dodd wrote:
> The current code in filemap_nopage() seems to expect the page to still be
> locked after an error, but as we've normally just waited on it, it won't be:
>
> ...
> 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 (Page_Uptodate(page))
> goto success;
> }
>
> /*
> * Umm, take care of errors if the page isn't up-to-date.
> * Try to re-read it _once_. We do this synchronously,
> * because there really aren't any performance issues here
> * and we need to check for errors.
> */
> if (!PageLocked(page))
> PAGE_BUG(page);
> ClearPageError(page);
> ...
>
> block_read_full_page() never fails, but I don't know what other
> implementations of ->readpage() might be expected to do in an error situation.
>
> The 'obvious' fix seems to be:
>
> --- mm/filemap.c.orig Thu Aug 19 08:37:26 1999
> +++ mm/filemap.c Thu Aug 19 08:43:22 1999
> @@ -1386,8 +1386,12 @@
> * because there really aren't any performance issues here
> * and we need to check for errors.
> */
> - if (!PageLocked(page))
> - PAGE_BUG(page);
> + lock_page(page);
> + if (Page_Uptodate(page)) {
> + UnlockPage(page);
> + goto success;
> + }
> +
> ClearPageError(page);
> error = inode->i_op->readpage(file, page);
> if (error)
>
> Thoughts?
- 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/