[PATCH 0/3] Make block_page_mkwrite() handle frozen fs (V2)

[PATCH 0/3] Make block_page_mkwrite() handle frozen fs (V2)

[Jan Kara]

  Hi,

  this is a second version of my patches which rewrite ext4_page_mkwrite() to
use generic helper. When we are already changing this code, I also added proper
handling of frozen filesystem there in patch 2/3. I've incorporated suggestions
from Christoph in the patches which resulted in some patch reorg. I'd like to
get these patches reviewed and then get them merged possibly via ext4-tree
(although the first two patches might go via Al but that would complicate
merging of ext4 part where I expect more conflicts with ongoing work).

                                                                        Honza

[PATCH 1/3] fs: Create __block_page_mkwrite() helper passing error values back

[Jan Kara]

Create __block_page_mkwrite() helper which does all what block_page_mkwrite()
does except that it passes back errors from __block_write_begin /
block_commit_write calls.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c                 |   28 +++++++++++++++-------------
 include/linux/buffer_head.h |   14 ++++++++++++++
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index a08bb8e..9c5dd88 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2332,21 +2332,21 @@ EXPORT_SYMBOL(block_commit_write);
  * beyond EOF, then the page is guaranteed safe against truncation until we
  * unlock the page.
  */
-int
-block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
-		   get_block_t get_block)
+int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+			 get_block_t get_block)
 {
 	struct page *page = vmf->page;
 	struct inode *inode = vma->vm_file->f_path.dentry->d_inode;
 	unsigned long end;
 	loff_t size;
-	int ret = VM_FAULT_NOPAGE; /* make the VM retry the fault */
+	int ret = 0;
 
 	lock_page(page);
 	size = i_size_read(inode);
 	if ((page->mapping != inode->i_mapping) ||
 	    (page_offset(page) > size)) {
-		/* page got truncated out from underneath us */
+		/* We overload EFAULT to mean page got truncated */
+		ret = -EFAULT;
 		unlock_page(page);
 		goto out;
 	}
@@ -2361,18 +2361,20 @@ block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 	if (!ret)
 		ret = block_commit_write(page, 0, end);
 
-	if (unlikely(ret)) {
+	if (unlikely(ret < 0))
 		unlock_page(page);
-		if (ret == -ENOMEM)
-			ret = VM_FAULT_OOM;
-		else /* -ENOSPC, -EIO, etc */
-			ret = VM_FAULT_SIGBUS;
-	} else
-		ret = VM_FAULT_LOCKED;
-
 out:
 	return ret;
 }
+EXPORT_SYMBOL(__block_page_mkwrite);
+
+int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+		   get_block_t get_block)
+{
+	int ret = __block_page_mkwrite(vma, vmf, get_block);
+
+	return block_page_mkwrite_return(ret);
+}
 EXPORT_SYMBOL(block_page_mkwrite);
 
 /*
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index f5df235..2bf6a91 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -217,8 +217,22 @@ int cont_write_begin(struct file *, struct address_space *, loff_t,
 			get_block_t *, loff_t *);
 int generic_cont_expand_simple(struct inode *inode, loff_t size);
 int block_commit_write(struct page *page, unsigned from, unsigned to);
+int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+				get_block_t get_block);
 int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 				get_block_t get_block);
+/* Convert errno to return value from ->page_mkwrite() call */
+static inline int block_page_mkwrite_return(int err)
+{
+	if (err == 0)
+		return VM_FAULT_LOCKED;
+	if (err == -EFAULT)
+		return VM_FAULT_NOPAGE;
+	if (err == -ENOMEM)
+		return VM_FAULT_OOM;
+	/* -ENOSPC, -EDQUOT, -EIO ... */
+	return VM_FAULT_SIGBUS;
+}
 sector_t generic_block_bmap(struct address_space *, sector_t, get_block_t *);
 int block_truncate_page(struct address_space *, loff_t, get_block_t *);
 int nobh_write_begin(struct address_space *, loff_t, unsigned, unsigned,
-- 
1.7.1

Re: [PATCH 1/3] fs: Create __block_page_mkwrite() helper passing error values back

[Christoph Hellwig]

On Wed, May 18, 2011 at 05:18:00PM +0200, Jan Kara wrote:
> Create __block_page_mkwrite() helper which does all what block_page_mkwrite()
> does except that it passes back errors from __block_write_begin /
> block_commit_write calls.

Looks good to me.  Long term I wonder wether we should force
->page_mkwrite to always return the page locked and just use normal
errnos all the way up to do_wp_page, but that can be left for a later
iteration as it would be quite invasive.

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/3] vfs: Block mmapped writes while the fs is frozen

[Jan Kara]

We should not allow file modification via mmap while the filesystem is
frozen. So block in block_page_mkwrite() while the filesystem is frozen.
We cannot do the blocking wait in __block_page_mkwrite() since e.g. ext4
will want to call that function with transaction started in some cases
and that would deadlock. But we can at least do the non-blocking reliable
check in __block_page_mkwrite() which is the hardest part anyway.

We have to check for frozen filesystem with the page marked dirty and under
page lock with which we then return from ->page_mkwrite(). Only that way we
cannot race with writeback done by freezing code - either we mark the page
dirty after the writeback has started, see freezing in progress and block, or
writeback will wait for our page lock which is released only when the fault is
done and then writeback will writeout and writeprotect the page again.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c                 |   28 +++++++++++++++++++++++++++-
 include/linux/buffer_head.h |    2 ++
 2 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 9c5dd88..030f808 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2331,6 +2331,9 @@ EXPORT_SYMBOL(block_commit_write);
  * page lock we can determine safely if the page is beyond EOF. If it is not
  * beyond EOF, then the page is guaranteed safe against truncation until we
  * unlock the page.
+ *
+ * Direct callers of this function should call vfs_check_frozen() so that page
+ * fault does not busyloop until the fs is thawed.
  */
 int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 			 get_block_t get_block)
@@ -2363,6 +2366,22 @@ int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 
 	if (unlikely(ret < 0))
 		unlock_page(page);
+	else {
+		/*
+		 * Freezing in progress? We check after the page is marked
+		 * dirty and with page lock held so if the test here fails, we
+		 * are sure freezing code will wait during syncing until the
+		 * page fault is done - at that point page will be dirty and
+		 * unlocked so freezing code will write it and writeprotect it
+		 * again.
+		 */
+		set_page_dirty(page);
+		if (inode->i_sb->s_frozen != SB_UNFROZEN) {
+			unlock_page(page);
+			ret = -EAGAIN;
+			goto out;
+		}
+	}
 out:
 	return ret;
 }
@@ -2371,8 +2390,15 @@ EXPORT_SYMBOL(__block_page_mkwrite);
 int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 		   get_block_t get_block)
 {
-	int ret = __block_page_mkwrite(vma, vmf, get_block);
+	int ret;
+	struct super_block *sb = vma->vm_file->f_path.dentry->d_inode->i_sb;
 
+	/*
+	 * This check is racy but catches the common case. The check in
+	 * __block_page_mkwrite() is reliable.
+	 */
+	vfs_check_frozen(sb, SB_FREEZE_WRITE);
+	ret = __block_page_mkwrite(vma, vmf, get_block);
 	return block_page_mkwrite_return(ret);
 }
 EXPORT_SYMBOL(block_page_mkwrite);
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 2bf6a91..503c8a6 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -230,6 +230,8 @@ static inline int block_page_mkwrite_return(int err)
 		return VM_FAULT_NOPAGE;
 	if (err == -ENOMEM)
 		return VM_FAULT_OOM;
+	if (err == -EAGAIN)
+		return VM_FAULT_RETRY;
 	/* -ENOSPC, -EDQUOT, -EIO ... */
 	return VM_FAULT_SIGBUS;
 }
-- 
1.7.1

Re: [PATCH 2/3] vfs: Block mmapped writes while the fs is frozen

[Christoph Hellwig]

>  
>  	if (unlikely(ret < 0))
>  		unlock_page(page);
> +	else {
> +		/*
> +		 * Freezing in progress? We check after the page is marked
> +		 * dirty and with page lock held so if the test here fails, we
> +		 * are sure freezing code will wait during syncing until the
> +		 * page fault is done - at that point page will be dirty and
> +		 * unlocked so freezing code will write it and writeprotect it
> +		 * again.
> +		 */
> +		set_page_dirty(page);
> +		if (inode->i_sb->s_frozen != SB_UNFROZEN) {
> +			unlock_page(page);
> +			ret = -EAGAIN;
> +			goto out;
> +		}
> +	}
>  out:
>  	return ret;

The code structure looks a bit odd, why not:

	if (ret < 0)
		goto out_unlock;

	set_page_dirty(page);
	if (inode->i_sb->s_frozen != SB_UNFROZEN) {
		ret = -EAGAIN;
		goto out_unlock;
	}

	return 0;

out_unlock:
	unlock_page(page);
	return ret;
}

Otherwise looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] vfs: Block mmapped writes while the fs is frozen

[Jan Kara]

On Wed 18-05-11 14:12:06, Christoph Hellwig wrote:
> >  
> >  	if (unlikely(ret < 0))
> >  		unlock_page(page);
> > +	else {
> > +		/*
> > +		 * Freezing in progress? We check after the page is marked
> > +		 * dirty and with page lock held so if the test here fails, we
> > +		 * are sure freezing code will wait during syncing until the
> > +		 * page fault is done - at that point page will be dirty and
> > +		 * unlocked so freezing code will write it and writeprotect it
> > +		 * again.
> > +		 */
> > +		set_page_dirty(page);
> > +		if (inode->i_sb->s_frozen != SB_UNFROZEN) {
> > +			unlock_page(page);
> > +			ret = -EAGAIN;
> > +			goto out;
> > +		}
> > +	}
> >  out:
> >  	return ret;
> 
> The code structure looks a bit odd, why not:
> 
> 	if (ret < 0)
> 		goto out_unlock;
> 
> 	set_page_dirty(page);
> 	if (inode->i_sb->s_frozen != SB_UNFROZEN) {
> 		ret = -EAGAIN;
> 		goto out_unlock;
> 	}
> 
> 	return 0;
> 
> out_unlock:
> 	unlock_page(page);
> 	return ret;
> }
> 
> Otherwise looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
  Thanks, I've changed the flow as you suggested.

								Honza

[PATCH 3/3] ext4: Rewrite ext4_page_mkwrite() to return locked page

[Jan Kara]

ext4_page_mkwrite() does not return page locked. This makes it hard
to avoid races with filesystem freezing code (so that we don't leave
writeable page on a frozen fs) or writeback code (so that we allow page
to be stable during writeback).

Also the current code uses i_alloc_sem to avoid races with truncate but that
seems to be the wrong locking order according to lock ordering documented in
mm/rmap.c.

Also add a check for frozen filesystem so that we don't busyloop in page fault
when the filesystem is frozen.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c |   94 ++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 55 insertions(+), 39 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index f2fa5e8..3e7b6b6 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5793,66 +5793,82 @@ int ext4_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 	struct page *page = vmf->page;
 	loff_t size;
 	unsigned long len;
-	int ret = -EINVAL;
-	void *fsdata;
+	int ret;
 	struct file *file = vma->vm_file;
 	struct inode *inode = file->f_path.dentry->d_inode;
 	struct address_space *mapping = inode->i_mapping;
+	handle_t *handle;
+	get_block_t *get_block;
+	int retries = 0;
 
 	/*
-	 * Get i_alloc_sem to stop truncates messing with the inode. We cannot
-	 * get i_mutex because we are already holding mmap_sem.
+	 * This check is racy but catches the common case. We rely on
+	 * __block_page_mkwrite() to do a reliable check.
 	 */
-	down_read(&inode->i_alloc_sem);
+	vfs_check_frozen(inode->i_sb, SB_FREEZE_WRITE);
+	/* Delalloc case is easy... */
+	if (test_opt(inode->i_sb, DELALLOC) &&
+	    !ext4_should_journal_data(inode) &&
+	    !ext4_nonda_switch(inode->i_sb)) {
+		do {
+			ret = __block_page_mkwrite(vma, vmf,
+						   ext4_da_get_block_prep);
+		} while (ret == -ENOSPC &&
+		       ext4_should_retry_alloc(inode->i_sb, &retries));
+		goto out_ret;
+	}
+
+	lock_page(page);
 	size = i_size_read(inode);
-	if (page->mapping != mapping || size <= page_offset(page)
-	    || !PageUptodate(page)) {
-		/* page got truncated from under us? */
-		goto out_unlock;
+	/* Page got truncated from under us? */
+	if (page->mapping != mapping || page_offset(page) > size) {
+		unlock_page(page);
+		ret = VM_FAULT_NOPAGE;
+		goto out;
 	}
-	ret = 0;
-	if (PageMappedToDisk(page))
-		goto out_unlock;
 
 	if (page->index == size >> PAGE_CACHE_SHIFT)
 		len = size & ~PAGE_CACHE_MASK;
 	else
 		len = PAGE_CACHE_SIZE;
-
-	lock_page(page);
 	/*
-	 * return if we have all the buffers mapped. This avoid
-	 * the need to call write_begin/write_end which does a
-	 * journal_start/journal_stop which can block and take
-	 * long time
+	 * Return if we have all the buffers mapped. This avoids the need to do
+	 * journal_start/journal_stop which can block and take a long time
 	 */
 	if (page_has_buffers(page)) {
 		if (!walk_page_buffers(NULL, page_buffers(page), 0, len, NULL,
 					ext4_bh_unmapped)) {
-			unlock_page(page);
-			goto out_unlock;
+			ret = VM_FAULT_LOCKED;
+			goto out;
 		}
 	}
 	unlock_page(page);
-	/*
-	 * OK, we need to fill the hole... Do write_begin write_end
-	 * to do block allocation/reservation.We are not holding
-	 * inode.i__mutex here. That allow * parallel write_begin,
-	 * write_end call. lock_page prevent this from happening
-	 * on the same page though
-	 */
-	ret = mapping->a_ops->write_begin(file, mapping, page_offset(page),
-			len, AOP_FLAG_UNINTERRUPTIBLE, &page, &fsdata);
-	if (ret < 0)
-		goto out_unlock;
-	ret = mapping->a_ops->write_end(file, mapping, page_offset(page),
-			len, len, page, fsdata);
-	if (ret < 0)
-		goto out_unlock;
-	ret = 0;
-out_unlock:
-	if (ret)
+	/* OK, we need to fill the hole... */
+	if (ext4_should_dioread_nolock(inode))
+		get_block = ext4_get_block_write;
+	else
+		get_block = ext4_get_block;
+retry_alloc:
+	handle = ext4_journal_start(inode, ext4_writepage_trans_blocks(inode));
+	if (IS_ERR(handle)) {
 		ret = VM_FAULT_SIGBUS;
-	up_read(&inode->i_alloc_sem);
+		goto out;
+	}
+	ret = __block_page_mkwrite(vma, vmf, get_block);
+	if (!ret && ext4_should_journal_data(inode)) {
+		if (walk_page_buffers(handle, page_buffers(page), 0,
+			  PAGE_CACHE_SIZE, NULL, do_journal_get_write_access)) {
+			unlock_page(page);
+			ret = VM_FAULT_SIGBUS;
+			goto out;
+		}
+		ext4_set_inode_state(inode, EXT4_STATE_JDATA);
+	}
+	ext4_journal_stop(handle);
+	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
+		goto retry_alloc;
+out_ret:
+	ret = block_page_mkwrite_return(ret);
+out:
 	return ret;
 }
-- 
1.7.1

Re: [PATCH 0/3] Make block_page_mkwrite() handle frozen fs (V2)

[Darrick J. Wong]

On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
> 
>   Hi,
> 
>   this is a second version of my patches which rewrite ext4_page_mkwrite() to
> use generic helper. When we are already changing this code, I also added proper
> handling of frozen filesystem there in patch 2/3. I've incorporated suggestions
> from Christoph in the patches which resulted in some patch reorg. I'd like to
> get these patches reviewed and then get them merged possibly via ext4-tree
> (although the first two patches might go via Al but that would complicate
> merging of ext4 part where I expect more conflicts with ongoing work).

Do you want me to generate a patch to add wait_on_page_writeback to the
appropriate place(s) in your new *_page_mkwrite functions, or do you plan to
add them yourself?

(iow: is there an order for pushing these ext4_page_mkwrite changes?)

--D

Re: [PATCH 0/3] Make block_page_mkwrite() handle frozen fs (V2)

[Jan Kara]

On Wed 18-05-11 11:00:22, Darrick J. Wong wrote:
> On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
> > 
> >   Hi,
> > 
> >   this is a second version of my patches which rewrite ext4_page_mkwrite() to
> > use generic helper. When we are already changing this code, I also added proper
> > handling of frozen filesystem there in patch 2/3. I've incorporated suggestions
> > from Christoph in the patches which resulted in some patch reorg. I'd like to
> > get these patches reviewed and then get them merged possibly via ext4-tree
> > (although the first two patches might go via Al but that would complicate
> > merging of ext4 part where I expect more conflicts with ongoing work).
> 
> Do you want me to generate a patch to add wait_on_page_writeback to the
> appropriate place(s) in your new *_page_mkwrite functions, or do you plan to
> add them yourself?
  Since Ted has merged your stable pages patches touching ext4, I'll rebase
my ext4 patch on top of that. Regarding block_page_mkwrite() I will add
that one-liner to the series (because the ext4_page_mkwrite() now depends
on that).

									Honza

Re: [PATCH 0/3] Make block_page_mkwrite() handle frozen fs (V2)

[Ted Ts'o]

On Thu, May 19, 2011 at 12:57:08PM +0200, Jan Kara wrote:
>   Since Ted has merged your stable pages patches touching ext4, I'll rebase
> my ext4 patch on top of that. Regarding block_page_mkwrite() I will add
> that one-liner to the series (because the ext4_page_mkwrite() now depends
> on that).

Hi Jan,

Does your 3/3 ext4 patch have dependencies on your 1/3 and 2/3
VFS-layer patches?

Thanks,

					- Ted

Re: [PATCH 0/3] Make block_page_mkwrite() handle frozen fs (V2)

[Jan Kara]

On Fri 20-05-11 13:59:53, Ted Tso wrote:
> On Thu, May 19, 2011 at 12:57:08PM +0200, Jan Kara wrote:
> >   Since Ted has merged your stable pages patches touching ext4, I'll rebase
> > my ext4 patch on top of that. Regarding block_page_mkwrite() I will add
> > that one-liner to the series (because the ext4_page_mkwrite() now depends
> > on that).
  Hi Ted,

> Does your 3/3 ext4 patch have dependencies on your 1/3 and 2/3
> VFS-layer patches?
  Yes, it does depend on the new helper introduced in 1/3. Should I push
3/3 via Al as well or just wait with it until Al merges the first two
patches and then push it to you?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 0/3] Make block_page_mkwrite() handle frozen fs (V2)

[Ted Ts'o]

On Mon, May 23, 2011 at 03:21:58PM +0200, Jan Kara wrote:
> > Does your 3/3 ext4 patch have dependencies on your 1/3 and 2/3
> > VFS-layer patches?
>   Yes, it does depend on the new helper introduced in 1/3. Should I push
> 3/3 via Al as well or just wait with it until Al merges the first two
> patches and then push it to you?

Hmm...  I guess it depends on how quickly Al is going to be pushing
VFS changes to Linus (I assume you're hoping this is going to get
pushed during this merge window, yes?).  I still have at least a day
or two worth of final merging and testing to do, and your 3/3 ext4
patch has a dependency on Darrick's T10 DIF/DIX patch as I recall.

      	    	       	  	    	- Ted

Re: [PATCH 0/3] Make block_page_mkwrite() handle frozen fs (V2)

[Jan Kara]

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

On Wed 18-05-11 11:00:22, Darrick J. Wong wrote:
> On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
> > 
> >   Hi,
> > 
> >   this is a second version of my patches which rewrite ext4_page_mkwrite() to
> > use generic helper. When we are already changing this code, I also added proper
> > handling of frozen filesystem there in patch 2/3. I've incorporated suggestions
> > from Christoph in the patches which resulted in some patch reorg. I'd like to
> > get these patches reviewed and then get them merged possibly via ext4-tree
> > (although the first two patches might go via Al but that would complicate
> > merging of ext4 part where I expect more conflicts with ongoing work).
> 
> Do you want me to generate a patch to add wait_on_page_writeback to the
> appropriate place(s) in your new *_page_mkwrite functions, or do you plan to
> add them yourself?
> 
> (iow: is there an order for pushing these ext4_page_mkwrite changes?)
  Attached is a rebase of your block_page_mkwrite change on top of my
patches. I'm not sure what is the actual status your series - have you
pushed VFS changes to Al?

									Honza

[-- Attachment #2: 0001-vfs-Wait-in-__block_page_mkwrite-for-IO-to-finish.patch --]
[-- Type: text/x-patch, Size: 1124 bytes --]

>From 408d0a667ebdc7edd24d05c8de4b42cc1629e84f Mon Sep 17 00:00:00 2001
From: Darrick J. Wong <djwong@us.ibm.com>
Date: Thu, 19 May 2011 14:14:11 +0200
Subject: [PATCH] vfs: Wait in __block_page_mkwrite for IO to finish

For filesystems such as nilfs2 and xfs that use block_page_mkwrite, modify that
function to wait for pending writeback before allowing the page to become
writable. This is needed to stabilize pages during writeback for those two
filesystems.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index b0675bf..161685d 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2365,6 +2365,8 @@ int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 
 	if (unlikely(ret < 0))
 		goto out_unlock;
+	/* Wait so that we don't change page under IO */
+	wait_on_page_writeback(page);
 	/*
 	 * Freezing in progress? We check after the page is marked dirty and
 	 * with page lock held so if the test here fails, we are sure freezing
-- 
1.7.1

Re: [PATCH 0/3] Make block_page_mkwrite() handle frozen fs (V2)

[Darrick J. Wong]

On Thu, May 19, 2011 at 02:30:29PM +0200, Jan Kara wrote:
> On Wed 18-05-11 11:00:22, Darrick J. Wong wrote:
> > On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
> > > 
> > >   Hi,
> > > 
> > >   this is a second version of my patches which rewrite ext4_page_mkwrite() to
> > > use generic helper. When we are already changing this code, I also added proper
> > > handling of frozen filesystem there in patch 2/3. I've incorporated suggestions
> > > from Christoph in the patches which resulted in some patch reorg. I'd like to
> > > get these patches reviewed and then get them merged possibly via ext4-tree
> > > (although the first two patches might go via Al but that would complicate
> > > merging of ext4 part where I expect more conflicts with ongoing work).
> > 
> > Do you want me to generate a patch to add wait_on_page_writeback to the
> > appropriate place(s) in your new *_page_mkwrite functions, or do you plan to
> > add them yourself?
> > 
> > (iow: is there an order for pushing these ext4_page_mkwrite changes?)
>   Attached is a rebase of your block_page_mkwrite change on top of my
> patches. I'm not sure what is the actual status your series - have you
> pushed VFS changes to Al?

Yes, the previous set of patches was sent to Al.  I guess I should resend
without the parts that have gone upstream or aren't really needed.

--D
> 
> 									Honza

> From 408d0a667ebdc7edd24d05c8de4b42cc1629e84f Mon Sep 17 00:00:00 2001
> From: Darrick J. Wong <djwong@us.ibm.com>
> Date: Thu, 19 May 2011 14:14:11 +0200
> Subject: [PATCH] vfs: Wait in __block_page_mkwrite for IO to finish
> 
> For filesystems such as nilfs2 and xfs that use block_page_mkwrite, modify that
> function to wait for pending writeback before allowing the page to become
> writable. This is needed to stabilize pages during writeback for those two
> filesystems.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/buffer.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index b0675bf..161685d 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2365,6 +2365,8 @@ int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
> 
>  	if (unlikely(ret < 0))
>  		goto out_unlock;
> +	/* Wait so that we don't change page under IO */
> +	wait_on_page_writeback(page);
>  	/*
>  	 * Freezing in progress? We check after the page is marked dirty and
>  	 * with page lock held so if the test here fails, we are sure freezing
> -- 
> 1.7.1
> 

Re: [PATCH 0/3] Make block_page_mkwrite() handle frozen fs (V2)

[Christoph Hellwig]

On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
> from Christoph in the patches which resulted in some patch reorg. I'd like to
> get these patches reviewed and then get them merged possibly via ext4-tree
> (although the first two patches might go via Al but that would complicate
> merging of ext4 part where I expect more conflicts with ongoing work).

The core canges are clear VFS tree material, and given that they are
needed for the stable pages patches as well it really doesn't make sense
to funnel this through a fs tree.  It might make sense to include the
ext4 patch in the VFS pile if that makes merging easier.

Re: [PATCH 0/3] Make block_page_mkwrite() handle frozen fs (V2)

[Jan Kara]

On Wed 18-05-11 14:13:21, Christoph Hellwig wrote:
> On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
> > from Christoph in the patches which resulted in some patch reorg. I'd like to
> > get these patches reviewed and then get them merged possibly via ext4-tree
> > (although the first two patches might go via Al but that would complicate
> > merging of ext4 part where I expect more conflicts with ongoing work).
> 
> The core canges are clear VFS tree material, and given that they are
> needed for the stable pages patches as well it really doesn't make sense
> to funnel this through a fs tree.  It might make sense to include the
> ext4 patch in the VFS pile if that makes merging easier.
  OK, I'll merge the first two patches via Al.

								Honza

[PATCH 0/3] Rewrite ext4_page_mkwrite, fix fs freezing

[Jan Kara]

  Hi,

  following three patches reimplement ext4_page_mkwrite() so that it returns
locked pages (which is necessary for stable pages work and also for fixing of
freezing code). As a bonus we also avoid taking i_alloc_sem as it's not
necessary and use generic block_page_mkwrite() helper. The common delalloc
path should be more straightforward now.

The last patch in the series blocks mmaped writes on frozen filesystem which
is simple to do now.

I've tested these patches by xfstests and also running fsx-linux for all
modes - delalloc, nodelalloc (data=writeback), nodelalloc (data=ordered),
nodelalloc (data=journal).

								Honza

[PATCH 1/3] fs: Create __block_page_mkwrite() helper passing error values back

[Jan Kara]

Create __block_page_mkwrite() helper which does all what block_page_mkwrite()
does except that it passes back errors from __block_write_begin /
block_commit_write calls. This is needed for some filesystems so that they
can detect errors such as ENOSPC and try harder.

CC: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c                 |   26 ++++++++++++++++++--------
 include/linux/buffer_head.h |    2 ++
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index a08bb8e..469c832 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2333,7 +2333,7 @@ EXPORT_SYMBOL(block_commit_write);
  * unlock the page.
  */
 int
-block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+__block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 		   get_block_t get_block)
 {
 	struct page *page = vmf->page;
@@ -2361,18 +2361,28 @@ block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 	if (!ret)
 		ret = block_commit_write(page, 0, end);
 
-	if (unlikely(ret)) {
+	if (unlikely(ret < 0))
 		unlock_page(page);
-		if (ret == -ENOMEM)
-			ret = VM_FAULT_OOM;
-		else /* -ENOSPC, -EIO, etc */
-			ret = VM_FAULT_SIGBUS;
-	} else
+	else
 		ret = VM_FAULT_LOCKED;
-
 out:
 	return ret;
 }
+EXPORT_SYMBOL(__block_page_mkwrite);
+
+int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+		   get_block_t get_block)
+{
+	int ret = __block_page_mkwrite(vma, vmf, get_block);
+
+	if (unlikely(ret < 0)) {
+		if (ret == -ENOMEM)
+			return VM_FAULT_OOM;
+		/* -ENOSPC, -EIO, etc */
+		return VM_FAULT_SIGBUS;
+	}
+	return ret;
+}
 EXPORT_SYMBOL(block_page_mkwrite);
 
 /*
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index f5df235..0b719b0 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -217,6 +217,8 @@ int cont_write_begin(struct file *, struct address_space *, loff_t,
 			get_block_t *, loff_t *);
 int generic_cont_expand_simple(struct inode *inode, loff_t size);
 int block_commit_write(struct page *page, unsigned from, unsigned to);
+int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+				get_block_t get_block);
 int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 				get_block_t get_block);
 sector_t generic_block_bmap(struct address_space *, sector_t, get_block_t *);
-- 
1.7.1

Re: [PATCH 1/3] fs: Create __block_page_mkwrite() helper passing error values back

[Christoph Hellwig]

> +	if (unlikely(ret < 0))
>  		unlock_page(page);
> +	else
>  		ret = VM_FAULT_LOCKED;
>  out:
>  	return ret;

Using two different types of return values here seems rather unclean.
I'd rather use 0 instead of VM_FAULT_LOCKED here, and maybe overload
-EAGAIN for VM_FAULT_NOPAGE.

> +int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
> +		   get_block_t get_block)
> +{
> +	int ret = __block_page_mkwrite(vma, vmf, get_block);
> +
> +	if (unlikely(ret < 0)) {
> +		if (ret == -ENOMEM)
> +			return VM_FAULT_OOM;
> +		/* -ENOSPC, -EIO, etc */
> +		return VM_FAULT_SIGBUS;
> +	}
> +	return ret;

and maybe also add a small inlined block_page_mkwrite_error helper
to translate the values.

Alternatively it might make sense to add a VM_FAULT_ENOSPC return value
so that you could re-use block_page_mkwrite unmodified, and we could
also have better error reporting for that case for the core VM.

Re: [PATCH 1/3] fs: Create __block_page_mkwrite() helper passing error values back

[Jan Kara]

On Tue 17-05-11 11:09:03, Christoph Hellwig wrote:
> > +	if (unlikely(ret < 0))
> >  		unlock_page(page);
> > +	else
> >  		ret = VM_FAULT_LOCKED;
> >  out:
> >  	return ret;
> 
> Using two different types of return values here seems rather unclean.
> I'd rather use 0 instead of VM_FAULT_LOCKED here, and maybe overload
> -EAGAIN for VM_FAULT_NOPAGE.
  OK, makes sense. I'll do that.

> > +int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
> > +		   get_block_t get_block)
> > +{
> > +	int ret = __block_page_mkwrite(vma, vmf, get_block);
> > +
> > +	if (unlikely(ret < 0)) {
> > +		if (ret == -ENOMEM)
> > +			return VM_FAULT_OOM;
> > +		/* -ENOSPC, -EIO, etc */
> > +		return VM_FAULT_SIGBUS;
> > +	}
> > +	return ret;
> 
> and maybe also add a small inlined block_page_mkwrite_error helper
> to translate the values.
  Good idea.

> Alternatively it might make sense to add a VM_FAULT_ENOSPC return value
> so that you could re-use block_page_mkwrite unmodified, and we could
> also have better error reporting for that case for the core VM.
  Umm, I think the first solution is better. For example ext[34] needs to
differentiate ENOSPC and EDQUOT (for the second one it does not make sense
to force transaction commit because quota accounting is always precise) and
other filesystems might possibly have different requirements. So returning
the error value and letting fs sort it out looks like the most versatile
solution.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR