| Message ID | 1357148744-4895-3-git-send-email-jack@suse.cz |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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
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
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); /*
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(-)