[PATCH linux-2.6.1-rc1-mm1] filemap_fdatawait.patch

[Daniel McNeil]

The other potential race in filemap_fdatawait() was that it
removed the page from the locked list while waiting for a writeback
and if there was a 2nd filemap_fdatawait() running on another cpu,
it would not wait for the page being written since it would never see
it on the list.

I've been running with filemap_fdatawait() that waits for the
locked page and writeback with the page still on the locked list.
Any other filemap_fdatawait() would see the page being written
and then wait.

I've re-diffed this patch against 2.6.1-rc1-mm1.

Thoughts?

Daniel



On Wed, 2003-12-31 at 03:17, Andrew Morton wrote:
> Andrew Morton <akpm@xxxxxxxx> wrote:
> >
> > Let me actually think about this a bit.
> 
> Nasty.  The same race is present in 2.4.x...
> 
> How's about we start new I/O in filemap_fdatawait() if the page is dirty?
> 
> 
> diff -puN mm/filemap.c~a mm/filemap.c
> --- 25/mm/filemap.c~a	2003-12-31 03:10:29.000000000 -0800
> +++ 25-akpm/mm/filemap.c	2003-12-31 03:17:05.000000000 -0800
> @@ -206,7 +206,13 @@ restart:
>  		page_cache_get(page);
>  		spin_unlock(&mapping->page_lock);
>  
> -		wait_on_page_writeback(page);
> +		lock_page(page);
> +		if (PageDirty(page) && mapping->a_ops->writepage) {
> +			write_one_page(page, 1);
> +		} else {
> +			wait_on_page_writeback(page);
> +			unlock_page(page);
> +		}
>  		if (PageError(page))
>  			ret = -EIO;
>  
> 
--- linux-2.6.1-rc1-mm1/mm/filemap.c	2003-12-31 10:52:47.039988554 -0800
+++ linux-2.6.1-rc1-mm1.fdatawait/mm/filemap.c	2003-12-31 14:10:43.114197926 -0800
@@ -188,6 +188,32 @@ restart:
 		struct page *page;
 
 		page = list_entry(mapping->locked_pages.next,struct page,list);
+		/*
+		 * If the page is locked, it might be in process of being 
+		 * setup for writeback but without PG_writeback set 
+		 * and with PG_dirty cleared.
+		 * (PG_dirty is cleared BEFORE PG_writeback is set)
+		 * So, wait for the PG_locked to clear, then start over.
+		 */
+		if (PageLocked(page)) {
+			page_cache_get(page);
+			spin_unlock(&mapping->page_lock);
+			wait_on_page_locked(page);
+			page_cache_release(page);
+			goto restart;
+		}
+		/*
+		 * Wait for page writeback with it still on the locked list.
+		 */
+		if (PageWriteback(page)) {
+			page_cache_get(page);
+			spin_unlock(&mapping->page_lock);
+			wait_on_page_writeback(page);
+			if (PageError(page))
+				ret = -EIO;
+			page_cache_release(page);
+			goto restart;
+		}
 		list_del(&page->list);
 		if (PageDirty(page))
 			list_add(&page->list, &mapping->dirty_pages);