Re: [PATCH 02/17] writeback: update dirtied_when for synced inode to prevent livelock

[Jan Kara <jack@suse.cz>]

On Tue 10-05-11 10:14:21, Wu Fengguang wrote:
> On Sat, May 07, 2011 at 12:33:12AM +0800, Jan Kara wrote:
> > On Fri 06-05-11 11:08:23, Wu Fengguang wrote:
> > > With the more aggressive "keep writeback as long as we wrote something"
> > > logic in wb_writeback(), the "use LONG_MAX .nr_to_write" trick in commit
> > > b9543dac5bbc ("writeback: avoid livelocking WB_SYNC_ALL writeback") is
> > > no longer enough to stop sync livelock.
> > > 
> > > The fix is to explicitly update .dirtied_when on synced inodes, so that
> > > they are no longer considered for writeback in the next round.
> >   The changelog doesn't make sense now when the patch is in the second
> > place in the series... Also as we discussed in the previous iteration of
> > the patches, I thought you'll move the condition before mapping_tagged()
> > test. I.e. the code would be (comment updated):
> > 	if (!(inode->i_state & I_FREEING)) {
> > 		/*
> > 		 * Sync livelock prevention: Each inode is tagged and
> > 		 * synced in one shot. So we can unconditionally update its
> > 		 * dirty time to prevent syncing it again. Note that time
> > 		 * ordering of b_dirty list will be kept because the
> > 		 * following code either removes the inode from b_dirty
> > 		 * or calls redirty_tail().
> > 		 */
> > 		if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_sync)
> > 			inode->dirtied_when = jiffies;
> > 		if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
> 
> It looks good, thanks!  Here is the updated patch with your
> reviewed-by. Note that I retains a modified paragraph in the changelog
> to clarify its relationship to the other two patches.
  Thanks. It's fine now.

								Honza

> Thanks,
> Fengguang
> ---
> Subject: writeback: update dirtied_when for synced inode to prevent livelock
> Date: Wed Apr 27 19:05:21 CST 2011
> 
> Explicitly update .dirtied_when on synced inodes, so that they are no
> longer considered for writeback in the next round.
> 
> We'll do more aggressive "keep writeback as long as we wrote something"
> logic in wb_writeback(). Then the "use LONG_MAX .nr_to_write" trick in
> commit b9543dac5b ("writeback: avoid livelocking WB_SYNC_ALL writeback")
> will no longer be enough to stop sync livelock.
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> ---
>  fs/fs-writeback.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> --- linux-next.orig/fs/fs-writeback.c	2011-05-10 09:50:07.000000000 +0800
> +++ linux-next/fs/fs-writeback.c	2011-05-10 10:03:00.000000000 +0800
> @@ -419,6 +419,15 @@ writeback_single_inode(struct inode *ino
>  	spin_lock(&inode->i_lock);
>  	inode->i_state &= ~I_SYNC;
>  	if (!(inode->i_state & I_FREEING)) {
> +		/*
> +		 * Sync livelock prevention. Each inode is tagged and synced in
> +		 * one shot, so we can unconditionally update its dirty time to
> +		 * prevent syncing it again. Note that time ordering of b_dirty
> +		 * list will be kept because the following code either removes
> +		 * the inode from b_dirty or calls redirty_tail().
> +		 */
> +		if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_sync)
> +			inode->dirtied_when = jiffies;
>  		if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
>  			/*
>  			 * We didn't write back all the pages.  nfs_writepages()
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR