[patch] pagecache locking bugfix

Andrew Morton (akpm@zip.com.au)
Thu, 18 Apr 2002 02:17:40 -0700


The bug which Anton found. On the
find_or_create_page->__find_lock_page path we're performing
a read_unlock of an rwlock which is held for writing.

The patch converts that to using a write_lock throughout.

Which penalises find_lock_page() a bit. If it shows up
on profiles then we can clone __find_lock_page() and
use read_lock()s, but for now I'd opt for saving the
cache footprint.

--- 2.5.8/mm/filemap.c~pagecache-screwup Thu Apr 18 00:49:02 2002
+++ 2.5.8-akpm/mm/filemap.c Thu Apr 18 01:02:20 2002
@@ -797,9 +797,9 @@ struct page *find_trylock_page(struct ad
}

/*
- * Must be called with the pagecache lock held,
- * will return with it held (but it may be dropped
- * during blocking operations..
+ * Must be called with the mapping lock held for writing.
+ * Will return with it held for writing, but it may be dropped
+ * while locking the page.
*/
static struct page *__find_lock_page(struct address_space *mapping,
unsigned long offset)
@@ -815,11 +815,11 @@ repeat:
if (page) {
page_cache_get(page);
if (TryLockPage(page)) {
- read_unlock(&mapping->page_lock);
+ write_unlock(&mapping->page_lock);
lock_page(page);
- read_lock(&mapping->page_lock);
+ write_lock(&mapping->page_lock);

- /* Has the page been re-allocated while we slept? */
+ /* Has the page been truncated while we slept? */
if (page->mapping != mapping || page->index != offset) {
UnlockPage(page);
page_cache_release(page);
@@ -830,25 +830,53 @@ repeat:
return page;
}

+/**
+ * find_lock_page - locate, pin and lock a pagecache page
+ *
+ * @mapping - the address_space to search
+ * @offset - the page index
+ *
+ * Locates the desired pagecache page, locks it, increments its reference
+ * count and returns its address.
+ *
+ * Returns zero if the page was not present. find_lock_page() may sleep.
+ */
+
/*
- * Same as the above, but lock the page too, verifying that
- * it's still valid once we own it.
+ * The write_lock is unfortunate, but __find_lock_page() requires that on
+ * behalf of find_or_create_page(). We could just clone __find_lock_page() -
+ * one for find_lock_page(), one for find_or_create_page()...
*/
-struct page * find_lock_page(struct address_space *mapping, unsigned long offset)
+struct page *find_lock_page(struct address_space *mapping,
+ unsigned long offset)
{
struct page *page;

- read_lock(&mapping->page_lock);
+ write_lock(&mapping->page_lock);
page = __find_lock_page(mapping, offset);
- read_unlock(&mapping->page_lock);
-
+ write_unlock(&mapping->page_lock);
return page;
}

-/*
- * Same as above, but create the page if required..
+/**
+ * find_or_create_page - locate or add a pagecache page
+ *
+ * @mapping - the page's address_space
+ * @index - the page's index into the mapping
+ * @gfp_mask - page allocation mode
+ *
+ * Locates a page in the pagecache. If the page is not present, a new page
+ * is allocated using @gfp_mask and is added to the pagecache and to the VM's
+ * LRU list. The returned page is locked and has its reference count
+ * incremented.
+ *
+ * find_or_create_page() may sleep, even if @gfp_flags specifies an atomic
+ * allocation!
+ *
+ * find_or_create_page() returns the desired page's address, or zero on
+ * memory exhaustion.
*/
-struct page * find_or_create_page(struct address_space *mapping,
+struct page *find_or_create_page(struct address_space *mapping,
unsigned long index, unsigned int gfp_mask)
{
struct page *page;

-
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/