[RFT PATCH 1/2] Revert "fb_defio: fix for non-dirty ptes"

[RFT PATCH 1/2] Revert "fb_defio: fix for non-dirty ptes"

[Albert Herranz]

This reverts commit 49bbd815fd8ba26d0354900b783b767c7f47c816.
Although the fix provided is correct, it's been suggested to avoid the
underlying race in the same way as it is currently done in filesystems
like NFS, for maintainability.

A following patch "fb_defio: redo fix for non-dirty ptes" will provide
such an alternate fix.

LKML-Reference: <20100525160149.GE20853@laptop>
Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
---
 drivers/video/fb_defio.c |   40 ++++++++--------------------------------
 1 files changed, 8 insertions(+), 32 deletions(-)

diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
index 6e10a3a..0e4dcdb 100644
--- a/drivers/video/fb_defio.c
+++ b/drivers/video/fb_defio.c
@@ -155,41 +155,25 @@ static void fb_deferred_io_work(struct work_struct *work)
 {
 	struct fb_info *info = container_of(work, struct fb_info,
 						deferred_work.work);
+	struct list_head *node, *next;
+	struct page *cur;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
-	struct page *page, *tmp_page;
-	struct list_head *node, *tmp_node;
-	struct list_head non_dirty;
-
-	INIT_LIST_HEAD(&non_dirty);
 
 	/* here we mkclean the pages, then do all deferred IO */
 	mutex_lock(&fbdefio->lock);
-	list_for_each_entry_safe(page, tmp_page, &fbdefio->pagelist, lru) {
-		lock_page(page);
-		/*
-		 * The workqueue callback can be triggered after a
-		 * ->page_mkwrite() call but before the PTE has been marked
-		 * dirty. In this case page_mkclean() won't "rearm" the page.
-		 *
-		 * To avoid this, remove those "non-dirty" pages from the
-		 * pagelist before calling the driver's callback, then add
-		 * them back to get processed on the next work iteration.
-		 * At that time, their PTEs will hopefully be dirty for real.
-		 */
-		if (!page_mkclean(page))
-			list_move_tail(&page->lru, &non_dirty);
-		unlock_page(page);
+	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+		lock_page(cur);
+		page_mkclean(cur);
+		unlock_page(cur);
 	}
 
 	/* driver's callback with pagelist */
 	fbdefio->deferred_io(info, &fbdefio->pagelist);
 
-	/* clear the list... */
-	list_for_each_safe(node, tmp_node, &fbdefio->pagelist) {
+	/* clear the list */
+	list_for_each_safe(node, next, &fbdefio->pagelist) {
 		list_del(node);
 	}
-	/* ... and add back the "non-dirty" pages to the list */
-	list_splice_tail(&non_dirty, &fbdefio->pagelist);
 	mutex_unlock(&fbdefio->lock);
 }
 
@@ -218,20 +202,12 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_open);
 void fb_deferred_io_cleanup(struct fb_info *info)
 {
 	struct fb_deferred_io *fbdefio = info->fbdefio;
-	struct list_head *node, *tmp_node;
 	struct page *page;
 	int i;
 
 	BUG_ON(!fbdefio);
 	flush_delayed_work(&info->deferred_work);
 
-	/*  the list may have still some non-dirty pages at this point */
-	mutex_lock(&fbdefio->lock);
-	list_for_each_safe(node, tmp_node, &fbdefio->pagelist) {
-		list_del(node);
-	}
-	mutex_unlock(&fbdefio->lock);
-
 	/* clear out the mapping that we setup */
 	for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
 		page = fb_deferred_io_page(info, i);
-- 
1.7.0.4

[RFT PATCH 2/2] fb_defio: redo fix for non-dirty ptes

[Albert Herranz]

As pointed by Nick Piggin, ->page_mkwrite provides a way to keep a page
locked until the associated PTE is marked dirty.

Re-implement the fix by using this mechanism.

LKML-Reference: <20100525160149.GE20853@laptop>
Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
---
 drivers/video/fb_defio.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
index 0e4dcdb..a3e8cc7 100644
--- a/drivers/video/fb_defio.c
+++ b/drivers/video/fb_defio.c
@@ -100,6 +100,16 @@ static int fb_deferred_io_mkwrite(struct vm_area_struct *vma,
 	/* protect against the workqueue changing the page list */
 	mutex_lock(&fbdefio->lock);
 
+	/*
+	 * We want the page to remain locked from ->page_mkwrite until
+	 * the PTE is marked dirty to avoid page_mkclean() being called
+	 * before the PTE is updated, which would leave the page ignored
+	 * by defio.
+	 * Do this by locking the page here and informing the caller
+	 * about it with VM_FAULT_LOCKED.
+	 */
+	lock_page(page);
+
 	/* we loop through the pagelist before adding in order
 	to keep the pagelist sorted */
 	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
@@ -121,7 +131,7 @@ page_already_added:
 
 	/* come back after delay to process the deferred IO */
 	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
-	return 0;
+	return VM_FAULT_LOCKED;
 }
 
 static const struct vm_operations_struct fb_deferred_io_vm_ops = {
-- 
1.7.0.4

Re: [RFT PATCH 2/2] fb_defio: redo fix for non-dirty ptes

[Jaya Kumar]

On Wed, May 26, 2010 at 6:17 AM, Albert Herranz <albert_herranz@yahoo.es> wrote:
> As pointed by Nick Piggin, ->page_mkwrite provides a way to keep a page
> locked until the associated PTE is marked dirty.
>
> Re-implement the fix by using this mechanism.
>
> LKML-Reference: <20100525160149.GE20853@laptop>
> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>

Looks good to me. Thanks.

Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>

Re: [RFT PATCH 2/2] fb_defio: redo fix for non-dirty ptes

[Nick Piggin]

On Wed, May 26, 2010 at 12:17:00AM +0200, Albert Herranz wrote:
> As pointed by Nick Piggin, ->page_mkwrite provides a way to keep a page
> locked until the associated PTE is marked dirty.
> 
> Re-implement the fix by using this mechanism.
> 
> LKML-Reference: <20100525160149.GE20853@laptop>
> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>

Thanks for taking a look at this,

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