From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 3/4] writeback: pay attention to wbc->nr_to_write in write_cache_pages Date: Thu, 22 Apr 2010 21:07:18 +0200 Message-ID: <20100422190718.GA19286@quack.suse.cz> References: <1271731314-5893-1-git-send-email-david@fromorbit.com> <1271731314-5893-4-git-send-email-david@fromorbit.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, xfs@oss.sgi.com To: Dave Chinner Return-path: Content-Disposition: inline In-Reply-To: <1271731314-5893-4-git-send-email-david@fromorbit.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Tue 20-04-10 12:41:53, Dave Chinner wrote: > From: Dave Chinner >=20 > If a filesystem writes more than one page in ->writepage, write_cache= _pages > fails to notice this and continues to attempt writeback when wbc->nr_= to_write > has gone negative - this trace was captured from XFS: >=20 >=20 > wbc_writeback_start: towrt=3D1024 > wbc_writepage: towrt=3D1024 > wbc_writepage: towrt=3D0 > wbc_writepage: towrt=3D-1 > wbc_writepage: towrt=3D-5 > wbc_writepage: towrt=3D-21 > wbc_writepage: towrt=3D-85 >=20 > This has adverse effects on filesystem writeback behaviour. write_cac= he_pages() > needs to terminate after a certain number of pages are written, not a= fter a > certain number of calls to ->writepage are made. Make it observe the = current > value of wbc->nr_to_write and treat a value of <=3D 0 as though it is= a either a > termination condition or a trigger to reset to MAX_WRITE=E1=B8=86ACK_= PAGES for data > integrity syncs. >=20 > Signed-off-by: Dave Chinner > --- > fs/fs-writeback.c | 9 --------- > include/linux/writeback.h | 9 +++++++++ > include/trace/events/writeback.h | 1 + > mm/page-writeback.c | 20 +++++++++++++++++++- > 4 files changed, 29 insertions(+), 10 deletions(-) >=20 > diff --git a/mm/page-writeback.c b/mm/page-writeback.c > index d45f59e..e22af84 100644 > --- a/mm/page-writeback.c > +++ b/mm/page-writeback.c > @@ -917,6 +917,7 @@ continue_unlock: > if (!clear_page_dirty_for_io(page)) > goto continue_unlock; > =20 > + trace_wbc_writepage(wbc); > ret =3D (*writepage)(page, wbc, data); > if (unlikely(ret)) { > if (ret =3D=3D AOP_WRITEPAGE_ACTIVATE) { > @@ -935,7 +936,7 @@ continue_unlock: > done =3D 1; > break; > } > - } > + } > =20 > if (nr_to_write > 0) { > nr_to_write--; > @@ -955,6 +956,23 @@ continue_unlock: > break; > } > } > + > + /* > + * Some filesystems will write multiple pages in > + * ->writepage, so wbc->nr_to_write can change much, > + * much faster than nr_to_write. Check this as an exit > + * condition, or if we are doing a data integrity sync, > + * reset the wbc to MAX_WRITEBACK_PAGES so that such > + * filesystems can do optimal writeout here. > + */ > + if (wbc->nr_to_write <=3D 0) { > + if (wbc->sync_mode =3D=3D WB_SYNC_NONE) { > + done =3D 1; > + nr_to_write =3D 0; > + break; > + } > + wbc->nr_to_write =3D MAX_WRITEBACK_PAGES; > + } Honestly, this is an ugly hack. I'd rather work towards ignoring nr_to_write completely in WB_SYNC_ALL mode since it doesn't really make any sence to say "write me *safely* 5 pages". Honza --=20 Jan Kara SUSE Labs, CR