lkml.org 
[lkml]   [1999]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] 2.3.1[34]: filemap_nopage faults on ->readpage() failure
    hi steve-

    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/

    \
     
     \ /
      Last update: 2005-03-22 13:53    [W:0.041 / U:30.724 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site