Re: pre2.0.9 (was Re: CD-ROM Access Crashes)

really kuznet@ms2.inr.ac.ru (inr-linux-kernel@ms2.inr.ac.ru)
30 May 1996 19:13:25 +0400


Linus Torvalds (torvalds@cs.helsinki.FI) wrote:

: Note that you might want to try out #9 of the pre-2.0 series. There were
: still some problems in the generic read handling when errors occurred, and I
: hope those are finally fixed (knock wood - the code is certainly not trivial,
: and I haven't been able to test).

It works!

Only one note on filemap_nopage:

if (inode->i_op->readpage(inode, page) != 0)
goto failure; _
if (PageError(page)) |
goto failure; |
if (PageUptodate(page)) |
goto success; |
|
If it was block device, page is still locked at this moment and
flags can change at any moment (race condition!).
It is pretty silly to check them here,
they will be !error && !uptodate (provided we have not
super-fast device 8))
If it was NFS, it is correct but ugly.
More clean solution is to use PG_error bit as bit triggering
synchronous readpage returning error code on failure,
rather than result of page operation and check it before readpage.

/* Page is not locked and not uptodate */
if (!PageError(page))
goto failure;
if (inode->i_op->readpage(inode, page) == 0)
goto success;
/* fall through failure */

Seems, it is more elegant.
It will be even more elegant, if we rename PG_error to PG_synchronous.

Analogously, in generic_file_read chunk:

page_read_error:
/*
* We found the page, but it wasn't up-to-date.
* Try to re-read it _once_. We do this synchronously,
* because this happens only if there were errors.
*/
error = inode->i_op->readpage(inode, page);
if (!error) {
wait_on_page(page);
if (PageUptodate(page) && !PageError(page))
goto success;
error = -EIO; /* Some unspecified error occurred.. */
}
release_page(page);
break;

is replaced by:

page_read_error:
error = -EIO;
if (PageError(page)) {
error = inode->i_op->readpage(inode, page);
if (!error) {
wait_on_page(page);
if (PageUptodate(page))
goto success;
error = -EIO; /* Some unspecified error occurred.. */
}
}
release_page(page);
break;

Alexey Kuznetsov.