- 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/
---------- Forwarded message ---------- Date: Mon, 23 Aug 1999 16:51:22 -0400 (EDT) From: Chuck Lever <cel@monkey.org> To: linux-kernel@vger.rutgers.edu Subject: review of i_op->readpage error handling
i've done a simple survey of the i_op->readpage method to see how each file system handles errors in terms of page locking and properly setting the PG_Error and PG_Uptodate flags. all comments welcome!
there are six implementations i could find:
block_read_full_page coda_readpage nfs_readpage qnx4_readpage romfs_readpage smb_readpage
the upshot of this is: 1. PG_Error is not set reliably. either it should be fixed in every file system, or no-one should use it.
2. synchronous readpage's always unlock the page on return, whether successful or an error occurs.
3. asynch readpage's can unlock on error, but don't always.
4. mainline (that is, non-error) logic generally seems to handle page locking correctly.
the following recommendations are based on the behavior in 2.3.15-pre2, where filemap_nopage assumes that a page is always unlocked if an error occurs. my own opinion is that it would be easier everywhere if readpage leaves a page locked (essentially in the same state as when readpage was called) on error.
block_read_full_page review: never returns an error, so it doesn't touch PG_Error. if all buffers for a page are up to date, it will set PG_Uptdate, unlock the page, and return. otherwise, it schedules I/O. so, on return, either: the page is locked, not uptodate, and not error, or the page is unlocked, uptodate, and not error return code is always zero. recommendation: perhaps it should clear PG_Error before starting.
coda_readpage review: can return ENXIO -- doesn't unlock the page or set PG_Error can return 0, after invoking block_read_full_page recommendation: perhaps it should return code from block_read_full_page instead of zero. if ENXIO is returned, it should set PG_Error, and unlock the page.
nfs_readpage review: oy. if a page write-back error occurs, it unlocks the page, but doesn't set PG_Error. if it decides to try async read, it leaves the page locked, and the callback will set PG_Error/PG_Uptodate, and unlock the page. if an RPC error occurs, the page is left locked. if it decides to try sync read, it unlocks the page, but PG_Error is not set if an error occurs. recommendation: page write-back error handling should set PG_Error. nfs_readpage uses PG_Error to determine whether to use sync or async read -- if filemap_nopage clears PG_Error during retry, that causes the retry to be async, which is not what we want. sync read should set PG_Error properly. async read should unlock the page if an RPC error occurs. otherwise, this leaks locked pages when network connectivity is poor.
qnx4_readpage review: doesn't use lock_page or get_page, uses old bread interface. synchronous read only; it sets PG_Error and PG_Uptodate correctly, and unlocks the page when it's done. recommendation: this file system hasn't been upgraded to use the page cache yet. should probably check value of bits before simply clearing them.
romfs_readpage review: synchronous operation, always unlocks page when done. sets PG_Error and PG_Uptodate correctly. recommendation: no change.
smb_readpage review: synchronous operation, always unlocks page when done. clears PG_Error, but never sets it. sets PG_Uptodate when successful. recommendation: should probably set PG_Error if an error occurs.
- 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/