Patchwork [2/5] ext4: Use redirty_page_for_writepage() in ext4_bio_write_page()

login
register
mail settings
Submitter Jan Kara
Date Jan. 2, 2013, 5:45 p.m.
Message ID <1357148744-4895-3-git-send-email-jack@suse.cz>
Download mbox | patch
Permalink /patch/209122/
State Superseded
Headers show

Comments

Jan Kara - Jan. 2, 2013, 5:45 p.m.
When we cannot write a page we should use redirty_page_for_writepage()
instead of plain set_page_dirty(). That tells writeback code we have
problems, redirties only the page (redirtying buffers is not needed),
and updates mm accounting of failed page writes.

Also move clearing of buffer dirty flag after io_submit_add_bh(). At that
moment we are sure buffer will be going to disk.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/page-io.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
Zheng Liu - Jan. 4, 2013, 7:20 a.m.
On Wed, Jan 02, 2013 at 06:45:41PM +0100, Jan Kara wrote:
> When we cannot write a page we should use redirty_page_for_writepage()
> instead of plain set_page_dirty(). That tells writeback code we have
> problems, redirties only the page (redirtying buffers is not needed),
> and updates mm accounting of failed page writes.
> 
> Also move clearing of buffer dirty flag after io_submit_add_bh(). At that
> moment we are sure buffer will be going to disk.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
Regards,
                                                - Zheng

> ---
>  fs/ext4/page-io.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 0016fbc..8e5722a 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -23,6 +23,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
> +#include <linux/mm.h>
>  
>  #include "ext4_jbd2.h"
>  #include "xattr.h"
> @@ -436,7 +437,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  
>  	io_page = kmem_cache_alloc(io_page_cachep, GFP_NOFS);
>  	if (!io_page) {
> -		set_page_dirty(page);
> +		redirty_page_for_writepage(wbc, page);
>  		unlock_page(page);
>  		return -ENOMEM;
>  	}
> @@ -468,7 +469,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  			set_buffer_uptodate(bh);
>  			continue;
>  		}
> -		clear_buffer_dirty(bh);
>  		ret = io_submit_add_bh(io, io_page, inode, wbc, bh);
>  		if (ret) {
>  			/*
> @@ -476,9 +476,10 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  			 * we can do but mark the page as dirty, and
>  			 * better luck next time.
>  			 */
> -			set_page_dirty(page);
> +			redirty_page_for_writepage(wbc, page);
>  			break;
>  		}
> +		clear_buffer_dirty(bh);
>  	}
>  	unlock_page(page);
>  	/*
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - Jan. 17, 2013, 6:35 p.m.
On Wed, Jan 02, 2013 at 06:45:41PM +0100, Jan Kara wrote:
> When we cannot write a page we should use redirty_page_for_writepage()
> instead of plain set_page_dirty(). That tells writeback code we have
> problems, redirties only the page (redirtying buffers is not needed),
> and updates mm accounting of failed page writes.
> 
> Also move clearing of buffer dirty flag after io_submit_add_bh(). At that
> moment we are sure buffer will be going to disk.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

This sounds like a bug fix, not just a clean up.  Do you think the
impact of not using redirty_page_for_writeback() is bad enough that we
should add a cc: stable@vger.kernel.org tag?  As I recall, not doing
this would end up leaving the page flags and radix tree to be
inconsistent, right?

	    	       	   	      - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara - Jan. 17, 2013, 9:24 p.m.
On Thu 17-01-13 13:35:01, Ted Tso wrote:
> On Wed, Jan 02, 2013 at 06:45:41PM +0100, Jan Kara wrote:
> > When we cannot write a page we should use redirty_page_for_writepage()
> > instead of plain set_page_dirty(). That tells writeback code we have
> > problems, redirties only the page (redirtying buffers is not needed),
> > and updates mm accounting of failed page writes.
> > 
> > Also move clearing of buffer dirty flag after io_submit_add_bh(). At that
> > moment we are sure buffer will be going to disk.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> This sounds like a bug fix, not just a clean up.  Do you think the
  Yes, it's a bug fix.

> impact of not using redirty_page_for_writeback() is bad enough that we
> should add a cc: stable@vger.kernel.org tag?  As I recall, not doing
> this would end up leaving the page flags and radix tree to be
> inconsistent, right?
  No, it shouldn't cause any such serious harm. Just writeback may retry
writing the inode immediately (it backs off if it sees skipped pages) which
could result in needless spinning. Also some mm/bdi statistics will get
slightly off. But all in all I don't think it's a stable material.

								Honza

Patch

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 0016fbc..8e5722a 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -23,6 +23,7 @@ 
 #include <linux/workqueue.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
+#include <linux/mm.h>
 
 #include "ext4_jbd2.h"
 #include "xattr.h"
@@ -436,7 +437,7 @@  int ext4_bio_write_page(struct ext4_io_submit *io,
 
 	io_page = kmem_cache_alloc(io_page_cachep, GFP_NOFS);
 	if (!io_page) {
-		set_page_dirty(page);
+		redirty_page_for_writepage(wbc, page);
 		unlock_page(page);
 		return -ENOMEM;
 	}
@@ -468,7 +469,6 @@  int ext4_bio_write_page(struct ext4_io_submit *io,
 			set_buffer_uptodate(bh);
 			continue;
 		}
-		clear_buffer_dirty(bh);
 		ret = io_submit_add_bh(io, io_page, inode, wbc, bh);
 		if (ret) {
 			/*
@@ -476,9 +476,10 @@  int ext4_bio_write_page(struct ext4_io_submit *io,
 			 * we can do but mark the page as dirty, and
 			 * better luck next time.
 			 */
-			set_page_dirty(page);
+			redirty_page_for_writepage(wbc, page);
 			break;
 		}
+		clear_buffer_dirty(bh);
 	}
 	unlock_page(page);
 	/*