The attached patch fixes a few memory leak bugs in filemap.c, one of
which may be significant to NFS users. It corrects an error exit that
would lead to stuck pages, causing a progressive loss of available
memory.
I'd like to hear from anyone who has had a problem of this sort with NFS
whether the patch helps out. The specific error condition could be
triggered, for example, by an attempt to overwrite an existing file to
which the user doesn't have read permission.
-Bill Hawes
--------------60EC83366462BCA3429109AC
Content-Type: text/plain; charset=us-ascii; name="filemap-patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="filemap-patch"
--- mm/filemap.c.old Tue May 20 13:16:23 1997
+++ mm/filemap.c Thu Jun 5 14:25:53 1997
@@ -43,6 +43,7 @@
* Simple routines for both non-shared and shared mappings.
*/
+#ifdef 0
/*
* This is a special fast page-free routine that _only_ works
* on page-cache pages that we are currently using. We can
@@ -55,6 +56,17 @@
{
atomic_dec(&page->count);
}
+#endif
+/*
+ * WSH 06/04/97:
+ * The above function may leave "lost" pages (page->count==0 but not freed)
+ * under certain conditions. If we're waiting on an inode cache page, once
+ * the PG_locked flag clears it's possible for another task to be scheduled.
+ * That task might do something (e.g. truncate_inode_pages) that kicks our
+ * page out of the inode page cache. Thus page->count might drop to 1 before
+ * this call, and calling the code above would lose the page.
+ */
+#define release_page(page) __free_page((page))
/*
* Invalidate the pages of an inode, removing all pages that aren't
@@ -770,6 +782,9 @@
* The goto's are kind of ugly, but this streamlines the normal case of having
* it in the page cache, and handles the special cases reasonably without
* having a lot of duplicated code.
+ *
+ * WSH 06/04/97: fixed a memory leak and moved the allocation of new_page
+ * ahead of the wait if we're sure to need it.
*/
static unsigned long filemap_nopage(struct vm_area_struct * area, unsigned long address, int no_share)
{
@@ -794,8 +809,15 @@
found_page:
/*
* Ok, found a page in the page cache, now we need to check
- * that it's up-to-date
+ * that it's up-to-date. First check whether we'll need an
+ * extra page -- better to overlap the allocation with the I/O.
*/
+ if (no_share && !new_page) {
+ new_page = __get_free_page(GFP_KERNEL);
+ if (!new_page)
+ goto failure;
+ }
+
if (PageLocked(page))
goto page_locked_wait;
if (!PageUptodate(page))
@@ -820,13 +842,8 @@
}
/*
- * Check that we have another page to copy it over to..
+ * No sharing ... copy to the new page.
*/
- if (!new_page) {
- new_page = __get_free_page(GFP_KERNEL);
- if (!new_page)
- goto failure;
- }
copy_page(new_page, old_page);
flush_page_to_ram(new_page);
release_page(page);
@@ -890,6 +907,8 @@
*/
failure:
release_page(page);
+ if (new_page)
+ free_page(new_page);
no_page:
return 0;
}
@@ -1300,7 +1319,7 @@
unsigned long ppos, offset;
unsigned int bytes, written;
unsigned long pos;
- int status, sync, didread = 0;
+ int status, sync, didread;
if (!inode->i_op || !inode->i_op->updatepage)
return -EIO;
@@ -1339,9 +1358,14 @@
page_cache = 0;
}
-lockit:
- while (test_and_set_bit(PG_locked, &page->flags))
- wait_on_page(page);
+ /*
+ * WSH 06/05/97: restructured slightly to avoid locking the
+ * page if we can't update it, and to make sure we release
+ * the page on an error exit.
+ */
+ didread = 0;
+page_wait:
+ wait_on_page(page);
/*
* If the page is not uptodate, and we're writing less
@@ -1350,28 +1374,25 @@
* after the current end of file.
*/
if (!PageUptodate(page)) {
- /* Already tried to read it twice... too bad */
- if (didread > 1) {
- status = -EIO;
- break;
- }
if (bytes < PAGE_SIZE && ppos < inode->i_size) {
- /* readpage implicitly unlocks the page */
- status = inode->i_op->readpage(inode, page);
+ if (didread < 2)
+ status = inode->i_op->readpage(inode, page);
+ else
+ status = -EIO; /* two tries ... error out */
if (status < 0)
- break;
+ goto done_with_page;
didread++;
- goto lockit;
+ goto page_wait;
}
set_bit(PG_uptodate, &page->flags);
}
- didread = 0;
- /* Alright, the page is there, and we've locked it. Now
- * update it. */
+ /* Alright, the page is there. Now lock it and update it. */
+ set_bit(PG_locked, &page->flags);
status = inode->i_op->updatepage(inode, page, buf,
offset, bytes, sync);
- free_page(page_address(page));
+done_with_page:
+ __free_page(page);
if (status < 0)
break;
--------------60EC83366462BCA3429109AC--