[PATCH v3 0/3] data integrity: Stabilize pages during writeback for ext4

[PATCH v3 0/3] data integrity: Stabilize pages during writeback for ext4

[Darrick J. Wong]

Hi all,

This is v3 of the stable-page-writes patchset for ext4.  A lot of code has been
cut out since v2 of this patch set.  For v3, the large hairy function to walk
the page tables of every process is gone since Chris Mason pointed out that
page_mkclean does what I need.  The set_memory_* hack is also gone, since (I
think) the only time the kernel maps a file data blocks for writing is in the
buffered IO case.  That leaves us with some surgery to ext4_page_mkwrite to
return locked pages and to be careful about re-checking the writeback status
after dropping and re-grabbing the page lock; and a slight modification to the
mm code to wait for page writeback when grabbing pages for buffered writes.
There are also some cleanups for wait_on_page_writeback use in ext4.

I ran my write-after-checksum ("wac") reproducer program to try to create the
DIF checksum errors by madly rewriting the same memory pages.  In fact, I tried
the following combinations:

a. 64 write() threads + sync_file_range
b. 64 mmap write threads + msync
c. 32 write() threads + sync_file_range + 32 mmap write threads + msync
d. Same as C, but with all threads in directio mode
e. Same as A, but with all threads in directio mode
f. Same as B, but with all threads in directio mode

After some 44 hours of safety testing across 4 machines, I saw zero errors.
Before the patchset, I could run any of A-F for 10 seconds or less and have a
screen full of errors.

To assess the performance impact of stable page writes, I moved to a disk that
doesn't have DIF support so that I could measure just the impact of waiting for
writeback.  I first ran wac with 64 threads madly scribbling on a 64k file and
saw about a 12% performance decrease.  I then reran the wac program with 64
threads and a 64MB file and saw about the same performance numbers.  I will of
course be testing a wider range of hardware now that I have a functioning patch
set, though as I suspected the patchset only seems to impact workloads that
rewrite the same memory page frequently.

As always, questions and comments are welcome; and thank you to all the
previous reviewers of this patchset!

--D

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v3 1/3] ext4: Clean up some wait_on_page_writeback calls

[Darrick J. Wong]

wait_on_page_writeback already checks the writeback bit, so callers of it
needn't do that test.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 fs/ext4/inode.c       |    4 +---
 fs/ext4/move_extent.c |    3 +--
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index f2fa5e8..3db34b2 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2796,9 +2796,7 @@ static int write_cache_pages_da(struct address_space *mapping,
 				continue;
 			}
 
-			if (PageWriteback(page))
-				wait_on_page_writeback(page);
-
+			wait_on_page_writeback(page);
 			BUG_ON(PageWriteback(page));
 
 			if (mpd->next_page != page->index)
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index d5c5783..d1548b1 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -876,8 +876,7 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
 	 * It needs to call wait_on_page_writeback() to wait for the
 	 * writeback of the page.
 	 */
-	if (PageWriteback(page))
-		wait_on_page_writeback(page);
+	wait_on_page_writeback(page);
 
 	/* Release old bh and drop refs */
 	try_to_release_page(page, 0);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v3 2/3] ext4: Wait for writeback to complete while making pages writable

[Darrick J. Wong]

In order to stabilize pages during disk writes, ext4_page_mkwrite must wait for
writeback operations to complete before making a page writable.  Furthermore,
the function must return locked pages, and recheck the writeback status if the
page lock is ever dropped.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 fs/ext4/inode.c |   24 +++++++++++++++++++-----
 1 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3db34b2..1d162a2 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5809,15 +5809,19 @@ int ext4_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 		goto out_unlock;
 	}
 	ret = 0;
-	if (PageMappedToDisk(page))
-		goto out_unlock;
+
+	lock_page(page);
+	wait_on_page_writeback(page);
+	if (PageMappedToDisk(page)) {
+		up_read(&inode->i_alloc_sem);
+		return VM_FAULT_LOCKED;
+	}
 
 	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
@@ -5827,8 +5831,8 @@ int ext4_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 	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;
+			up_read(&inode->i_alloc_sem);
+			return VM_FAULT_LOCKED;
 		}
 	}
 	unlock_page(page);
@@ -5848,6 +5852,16 @@ int ext4_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 	if (ret < 0)
 		goto out_unlock;
 	ret = 0;
+
+	/*
+	 * write_begin/end might have created a dirty page and someone
+	 * could wander in and start the IO.  Make sure that hasn't
+	 * happened.
+	 */
+	lock_page(page);
+	wait_on_page_writeback(page);
+	up_read(&inode->i_alloc_sem);
+	return VM_FAULT_LOCKED;
 out_unlock:
 	if (ret)
 		ret = VM_FAULT_SIGBUS;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v3 3/3] mm: Wait for writeback when grabbing pages to begin a write

[Darrick J. Wong]

When grabbing a page for a buffered IO write, the mm should wait for writeback
on the page to complete so that the page does not become writable during the IO
operation.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 mm/filemap.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index c641edf..c22675f 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2287,8 +2287,10 @@ struct page *grab_cache_page_write_begin(struct address_space *mapping,
 		gfp_notmask = __GFP_FS;
 repeat:
 	page = find_lock_page(mapping, index);
-	if (page)
+	if (page) {
+		wait_on_page_writeback(page);
 		return page;
+	}
 
 	page = __page_cache_alloc(mapping_gfp_mask(mapping) & ~gfp_notmask);
 	if (!page)
@@ -2301,6 +2303,7 @@ repeat:
 			goto repeat;
 		return NULL;
 	}
+	wait_on_page_writeback(page);
 	return page;
 }
 EXPORT_SYMBOL(grab_cache_page_write_begin);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v3 0/3] data integrity: Stabilize pages during writeback for ext4

[Christoph Hellwig]

This seems to miss out on a lot of the generic functionality like
write_cache_pages and block_page_mkwrite and just patch it into
the ext4 copy & paste variants.  Please make sure your patches also
work for filesystem that use more of the generic functionality like
xfs or ext2 (the latter one might be fun for the mmap case).

Also what's the status of btrfs?  I remembered there was one or two
bits missing despite doing the right thing in most areas.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v3 3/3] mm: Wait for writeback when grabbing pages to begin a write

[Christoph Hellwig]

On Wed, May 04, 2011 at 10:42:27AM -0700, Darrick J. Wong wrote:
> When grabbing a page for a buffered IO write, the mm should wait for writeback
> on the page to complete so that the page does not become writable during the IO
> operation.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
> 
>  mm/filemap.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c641edf..c22675f 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2287,8 +2287,10 @@ struct page *grab_cache_page_write_begin(struct address_space *mapping,
>  		gfp_notmask = __GFP_FS;
>  repeat:
>  	page = find_lock_page(mapping, index);
> -	if (page)
> +	if (page) {
> +		wait_on_page_writeback(page);
>  		return page;
> +	}

		goto found;

>  
>  	page = __page_cache_alloc(mapping_gfp_mask(mapping) & ~gfp_notmask);
>  	if (!page)
> @@ -2301,6 +2303,7 @@ repeat:
>  			goto repeat;
>  		return NULL;
>  	}

found:
> +	wait_on_page_writeback(page);
>  	return page;
>  }
>  EXPORT_SYMBOL(grab_cache_page_write_begin);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v3 0/3] data integrity: Stabilize pages during writeback for ext4

[Chris Mason]

Excerpts from Christoph Hellwig's message of 2011-05-04 14:46:44 -0400:
> This seems to miss out on a lot of the generic functionality like
> write_cache_pages and block_page_mkwrite and just patch it into
> the ext4 copy & paste variants.  Please make sure your patches also
> work for filesystem that use more of the generic functionality like
> xfs or ext2 (the latter one might be fun for the mmap case).

Probably after the block_commit_write in block_page_mkwrite()
Another question is, do we want to introduce a wait_on_stable_page_writeback()?

This would allow us to add a check against the bdi requesting stable
pages.

> 
> Also what's the status of btrfs?  I remembered there was one or two
> bits missing despite doing the right thing in most areas.

As far as I know btrfs is getting it right.  The only bit missing is the
one Nick Piggin pointed out where it is possible to change mmap'd O_DIRECT
memory in flight while a DIO is in progress.  Josef has a test case that
demonstrates this.

Nick had a plan to fix it, but it involved redoing the get_user_pages
api.

-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v3 0/3] data integrity: Stabilize pages during writeback for ext4

[Darrick J. Wong]

On Wed, May 04, 2011 at 03:21:55PM -0400, Chris Mason wrote:
> Excerpts from Christoph Hellwig's message of 2011-05-04 14:46:44 -0400:
> > This seems to miss out on a lot of the generic functionality like
> > write_cache_pages and block_page_mkwrite and just patch it into
> > the ext4 copy & paste variants.  Please make sure your patches also
> > work for filesystem that use more of the generic functionality like
> > xfs or ext2 (the latter one might be fun for the mmap case).
> 
> Probably after the block_commit_write in block_page_mkwrite()

Yes, I'm working on providing more generic fixes for ext3 & friends too, but
they're not really working yet, so I was posting the parts that fix ext4, since
they seem usable.

> Another question is, do we want to introduce a wait_on_stable_page_writeback()?
> 
> This would allow us to add a check against the bdi requesting stable
> pages.

Sounds like a good idea.

> > Also what's the status of btrfs?  I remembered there was one or two
> > bits missing despite doing the right thing in most areas.
> 
> As far as I know btrfs is getting it right.  The only bit missing is the
> one Nick Piggin pointed out where it is possible to change mmap'd O_DIRECT
> memory in flight while a DIO is in progress.  Josef has a test case that
> demonstrates this.
> 
> Nick had a plan to fix it, but it involved redoing the get_user_pages
> api.

I ran the same six tests A-F on btrfs and it reported -ENOSPC with 1% of the
space used, though until it did that I didn't see any checksum errors.

--D

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v3 0/3] data integrity: Stabilize pages during writeback for ext4

[Darrick J. Wong]

On Wed, May 04, 2011 at 03:21:55PM -0400, Chris Mason wrote:
> Excerpts from Christoph Hellwig's message of 2011-05-04 14:46:44 -0400:
> > This seems to miss out on a lot of the generic functionality like
> > write_cache_pages and block_page_mkwrite and just patch it into
> > the ext4 copy & paste variants.  Please make sure your patches also
> > work for filesystem that use more of the generic functionality like
> > xfs or ext2 (the latter one might be fun for the mmap case).
> 
> Probably after the block_commit_write in block_page_mkwrite()
> Another question is, do we want to introduce a wait_on_stable_page_writeback()?

Something like this here?  It fixes block_page_mkwrite users and sticks in a
simple page_mkwrite for fses that don't provide one at all.  From a quick wac
run it seems to make xfs work.  ext2 seems to have some issues with modifying a
buffer_head's bh_data without locking the bh during the update, so I guess it
needs some review.

--D

--

fs: Modify/provide generic writepage/page_mkwrite functions to wait for writeback

Modify the generic writepage function, and add an empty page_mkwrite function,
to wait for page writeback to finish before allowing writes.  This is so that
simple filesystems have stable pages during write operations.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 fs/buffer.c  |    1 +
 mm/filemap.c |   10 ++++++++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index a08bb8e..cf9a795 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2361,6 +2361,7 @@ block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 	if (!ret)
 		ret = block_commit_write(page, 0, end);
 
+	wait_on_page_writeback(page);
 	if (unlikely(ret)) {
 		unlock_page(page);
 		if (ret == -ENOMEM)
diff --git a/mm/filemap.c b/mm/filemap.c
index c22675f..9cb4e51 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1713,8 +1713,18 @@ page_not_uptodate:
 }
 EXPORT_SYMBOL(filemap_fault);
 
+static int empty_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	struct page *page = vmf->page;
+
+	lock_page(page);
+	wait_on_page_writeback(page);
+	return VM_FAULT_LOCKED;
+}
+
 const struct vm_operations_struct generic_file_vm_ops = {
 	.fault		= filemap_fault,
+	.page_mkwrite	= empty_page_mkwrite,
 };
 
 /* This is used for a general mmap of a disk file */
--

Here's the beginning of a patch against ext2 also.  It fixes most of the
checksum errors when metadata writes are in progress, though I suspect there
are more spots that I haven't caught yet.

--

ext2: Lock buffer_heads during metadata update

ext2 does not protect buffer head that represent metadata against modifications
during write operations.  This is a problem if the memory buffer needs to be
stable during writes; I think there are a few more spots in ext2 that need this
treatment. :)

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 fs/ext2/inode.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 788e09a..5314b0b 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1426,6 +1426,7 @@ static int __ext2_write_inode(struct inode *inode, int do_sync)
 	if (IS_ERR(raw_inode))
  		return -EIO;
 
+	lock_buffer(bh);
 	/* For fields not not tracking in the in-memory inode,
 	 * initialise them to zero for new inodes. */
 	if (ei->i_state & EXT2_STATE_NEW)
@@ -1502,6 +1503,8 @@ static int __ext2_write_inode(struct inode *inode, int do_sync)
 		}
 	} else for (n = 0; n < EXT2_N_BLOCKS; n++)
 		raw_inode->i_block[n] = ei->i_data[n];
+	unlock_buffer(bh);
+
 	mark_buffer_dirty(bh);
 	if (do_sync) {
 		sync_dirty_buffer(bh);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v3 0/3] data integrity: Stabilize pages during writeback for ext4

[Jan Kara]

  Hello,

On Wed 04-05-11 16:57:06, Darrick J. Wong wrote:
> On Wed, May 04, 2011 at 03:21:55PM -0400, Chris Mason wrote:
> > Excerpts from Christoph Hellwig's message of 2011-05-04 14:46:44 -0400:
> > > This seems to miss out on a lot of the generic functionality like
> > > write_cache_pages and block_page_mkwrite and just patch it into
> > > the ext4 copy & paste variants.  Please make sure your patches also
> > > work for filesystem that use more of the generic functionality like
> > > xfs or ext2 (the latter one might be fun for the mmap case).
> > 
> > Probably after the block_commit_write in block_page_mkwrite()
> > Another question is, do we want to introduce a wait_on_stable_page_writeback()?
> 
> Something like this here?  It fixes block_page_mkwrite users and sticks in a
> simple page_mkwrite for fses that don't provide one at all.  From a quick wac
> run it seems to make xfs work.  ext2 seems to have some issues with modifying a
> buffer_head's bh_data without locking the bh during the update, so I guess it
> needs some review.
  Yes, ext2 is rather difficult because of all the metadata updates to
buffers happening. That would need a serious work I suspect.

> fs: Modify/provide generic writepage/page_mkwrite functions to wait for writeback
> 
> Modify the generic writepage function, and add an empty page_mkwrite function,
> to wait for page writeback to finish before allowing writes.  This is so that
> simple filesystems have stable pages during write operations.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
> 
>  fs/buffer.c  |    1 +
>  mm/filemap.c |   10 ++++++++++
>  2 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index a08bb8e..cf9a795 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2361,6 +2361,7 @@ block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
>  	if (!ret)
>  		ret = block_commit_write(page, 0, end);
>  
> +	wait_on_page_writeback(page);
>  	if (unlikely(ret)) {
>  		unlock_page(page);
>  		if (ret == -ENOMEM)
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c22675f..9cb4e51 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1713,8 +1713,18 @@ page_not_uptodate:
>  }
>  EXPORT_SYMBOL(filemap_fault);
>  
> +static int empty_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
> +{
> +	struct page *page = vmf->page;
> +
> +	lock_page(page);
> +	wait_on_page_writeback(page);
> +	return VM_FAULT_LOCKED;
> +}
> +
  I guess you miss the whether the page has been truncated here (in which
case you should return VM_FAULT_NOPAGE).

>  const struct vm_operations_struct generic_file_vm_ops = {
>  	.fault		= filemap_fault,
> +	.page_mkwrite	= empty_page_mkwrite,
>  };
>  
>  /* This is used for a general mmap of a disk file */

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>