Re: [rfc][patch] remove racy sync_page?

Nick Piggin
2006-05-30T05:36:20Z

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

Nick Piggin wrote:

> Anyway, clearly a plug friendly bugfix needs to be implemented
> regardless.

I actually think it would be cleaner to introduce a new
mapping_lock_page(mapping, page) or something that does the
unplug on the _mapping_, and have lock_page be callable with
no ref on the inode. Should be a mechanical conversion for
most of the safe callers.

But for 2.6.17, how's this?

--
SUSE Labs, Novell Inc.

[-- Attachment #2: mm-non-syncing-lock_page.patch --]
[-- Type: text/plain, Size: 2455 bytes --]

lock_page needs the caller to have a reference on the page->mapping inode
due to sync_page. So set_page_dirty_lock is obviously buggy according to
its comments.

Solve it by introducing a new lock_page_nosync which does not do a sync_page.

Signed-off-by: Nick Piggin <npiggin@suse.de>

Index: linux-2.6/include/linux/pagemap.h
===================================================================
--- linux-2.6.orig/include/linux/pagemap.h 2006-05-19 12:49:06.000000000 +1000
+++ linux-2.6/include/linux/pagemap.h 2006-05-30 15:12:54.000000000 +1000
@@ -168,6 +168,7 @@ static inline pgoff_t linear_page_index(
}

extern void FASTCALL(__lock_page(struct page *page));
+extern void FASTCALL(__lock_page_nosync(struct page *page));
extern void FASTCALL(unlock_page(struct page *page));

static inline void lock_page(struct page *page)
@@ -176,6 +177,13 @@ static inline void lock_page(struct page
if (TestSetPageLocked(page))
__lock_page(page);
}
+
+static inline void lock_page_nosync(struct page *page)
+{
+ might_sleep();
+ if (TestSetPageLocked(page))
+ __lock_page_nosync(page);
+}

/*
* This is exported only for wait_on_page_locked/wait_on_page_writeback.
Index: linux-2.6/mm/filemap.c
===================================================================
--- linux-2.6.orig/mm/filemap.c 2006-05-30 15:07:58.000000000 +1000
+++ linux-2.6/mm/filemap.c 2006-05-30 15:31:46.000000000 +1000
@@ -544,6 +544,23 @@ void fastcall __lock_page(struct page *p
}
EXPORT_SYMBOL(__lock_page);

+static int __sleep_on_page_lock(void *word)
+{
+ io_schedule();
+ return 0;
+}
+
+/*
+ * Variant of lock_page that does not require the caller to hold a reference
+ * on the page's mapping.
+ */
+void fastcall __lock_page_nosync(struct page *page)
+{
+ DEFINE_WAIT_BIT(wait, &page->flags, PG_locked);
+ __wait_on_bit_lock(page_waitqueue(page), &wait, __sleep_on_page_lock,
+ TASK_UNINTERRUPTIBLE);
+}
+
/*
* a rather lightweight function, finding and getting a reference to a
* hashed page atomically.
Index: linux-2.6/mm/page-writeback.c
===================================================================
--- linux-2.6.orig/mm/page-writeback.c 2006-05-19 12:48:01.000000000 +1000
+++ linux-2.6/mm/page-writeback.c 2006-05-30 15:13:59.000000000 +1000
@@ -702,7 +702,7 @@ int set_page_dirty_lock(struct page *pag
{
int ret;

- lock_page(page);
+ lock_page_nosync(page);
ret = set_page_dirty(page);
unlock_page(page);
return ret;