ocfs2: fix ocfs2_page_mkwrite path

This patch fixes two problems in ocfs2_page_mkwrite path:
1) it makes ocfs2_page_mkwrite returns VM_FAULT_*
2) it fixes a bug of returning -EINVAL.
The -EINVAL is returned when the page is no longger belong to the inode mapping (pagecache gets truncated). Make it return VM_FAULT_NOPAGE in that case.

---
 fs/ocfs2/aops.c |   47 +++++++++++++++++++++++++++++++++++++++--------
 fs/ocfs2/mmap.c |   53 ++++++++++++++++++++++++-----------------------------
 2 files changed, 63 insertions(+), 37 deletions(-)

index 0d7c554..619a38f 100644
@@ -876,6 +876,12 @@ struct ocfs2_write_ctxt {
 	struct page			*w_target_page;
 
 	/*
+	 * w_target_locked is used for page_mkwrite path indicating no unlocking
+	 * against w_target_page in ocfs2_write_end_nolock.
+	 */
+	unsigned int			w_target_locked:1;
+
+	/*
 	 * ocfs2_write_end() uses this to know what the real range to
 	 * write in the target should be.
 	 */

@@ -908,8 +914,29 @@ void ocfs2_unlock_and_free_pages(struct page **pages, int num_pages)
 
 static void ocfs2_free_write_ctxt(struct ocfs2_write_ctxt *wc)
 {
+	/*
+	 * don't unlock on the target_page for page_mkwrite pach.
+	 * page_mkwrite() wants a return with the page locked.
+	 * re-acquiring after ocfs2_unlock_and_free_pages is not acceptable.
+	 * During the time when it unlocked, the page can changed to
+	 * not belong to the inode mapping any longer.
+	 *
+	 * caller must hold a ref on w_target_page.
+	 */
+	if (wc->w_target_locked) {
+		int i;
+
+		BUG_ON(!wc->w_target_page);
+		for (i = 0; i < wc->w_num_pages; i++) {
+			if (wc->w_target_page == wc->w_pages[i]) {
+				wc->w_pages[i] = NULL;
+				break;
+			}
+		}
+		mark_page_accessed(wc->w_target_page);
+		page_cache_release(wc->w_target_page);
+	}
 	ocfs2_unlock_and_free_pages(wc->w_pages, wc->w_num_pages);
-
 	brelse(wc->w_di_bh);
 	kfree(wc);
 }

@@ -1139,20 +1166,19 @@ static int ocfs2_grab_pages_for_write(struct address_space *mapping,
 			 */
 			lock_page(mmap_page);
 
+			/*
+			 * pagecache gets truncated. Let VM retry it.
+			 */
 			if (mmap_page->mapping != mapping) {
+				WARN_ON(mmap_page->mapping);
 				unlock_page(mmap_page);
-				/*
-				 * Sanity check - the locking in
-				 * ocfs2_pagemkwrite() should ensure
-				 * that this code doesn't trigger.
-				 */
-				ret = -EINVAL;
-				mlog_errno(ret);
+				ret = 0;
 				goto out;
 			}
 
 			page_cache_get(mmap_page);
 			wc->w_pages[i] = mmap_page;
+			wc->w_target_locked = true;
 		} else {
 			wc->w_pages[i] = find_or_create_page(mapping, index,
 							     GFP_NOFS);

@@ -1167,6 +1193,8 @@ static int ocfs2_grab_pages_for_write(struct address_space *mapping,
 			wc->w_target_page = wc->w_pages[i];
 	}
 out:
+	if (ret)
+		wc->w_target_locked = false;
 	return ret;
 }
 

@@ -1786,6 +1814,9 @@ int ocfs2_write_begin_nolock(struct file *filp,
 		mlog_errno(ret);
 		goto out_quota;
 	}
+	/* Did the pagecache lose the page?  Retry. */
+	if (!wc->w_target_page)
+		goto out_quota;
 
 	ret = ocfs2_write_cluster_by_desc(mapping, data_ac, meta_ac, wc, pos,
 					  len);

1.7.2.3

Hi Wengang, thanks again for this patch. It looks pretty good, I had only one comment which is below.

	--Mark

I told him to return 0. I couldn't come up with a good signal.
We want the caller (__ocfs2_page_mkwrite()) to not error; this is a valid case of retry. The check for the locked_page is good for that.
If we do come up with a logical !0 ret for
ocfs2_write_begin_nolock(), we have to make sure that the error paths do all of their processing correctly but otherwise treat it as a good state. Obviously then __ocfs2_page_mkwrite() can trigger on that error code (and BUG_ON(locked_page)).

Joel

"When choosing between two evils, I always like to try the one
 I've never tried before."
        - Mae West

			http://www.jlbec.org/
			jlbec at evilplan.org

This is the return 0 bit in ocfs2_grab_page_for_write, which is only called from ocfs2_write_begin_nolock(). I think we're talking about two different paths. Specifically, I don't like seeing this:

	ret = ocfs2_grab_pages_for_write(mapping, wc, wc->w_cpos, pos, len,
					 cluster_of_pages, mmap_page);
	if (ret) {
		mlog_errno(ret);
		goto out_quota;
	}

	/* Did the pagecache lose the page?  Retry. */                                                                                                                                                                                                       
	if (!wc->w_target_page)                                                                                                                                                                                                                              
		goto out_quota;                                                                                                                                                                                                                              

And instead I'd rather see:

        ret = ocfs2_grab_pages_for_write(mapping, wc, wc->w_cpos, pos, len,
                                         cluster_of_pages, mmap_page);
        if (ret) {
		if (ret == -EWHATEVER) {
			/*
			 * Here is a comment explaining why we're turning
			 * this into zero for ocfs2_page_mkwrite().
			 */
			ret = 0;
		}
		goto out_quota;
	}

Which doesn't wind up changing any API between page_mkwrite and write_begin but results in a more readable and natural calling convention internally.

	--Mark

--
Mark Fasheh

Ok, so what is EWHATEVER?

Joel

	Pitchers and catchers report.

			http://www.jlbec.org/
			jlbec at evilplan.org

-EINVAL? That's what we used to return from ocfs2_grab_pages_for_write(). The only other possible return value right now is ENOMEM so we could pick anything really.

errno values aren't very descriptive, which sucks (but that's what we have to deal with).

Then again, if we want to be super explicit we could also just do:

#define EMAPPINGCHANGED 1
int ocfs2_grab_pages_for_write( ... )
{

}

and return our own nonzero value.

	--Mark

--
Mark Fasheh