From mboxrd@z Thu Jan  1 00:00:00 1970
From: Dave Chinner <david@fromorbit.com>
Subject: Re: [PATCH 6/7] writeback: Refactor writeback_single_inode()
Date: Wed, 21 Mar 2012 10:35:54 +1100
Message-ID: <20120320233554.GS5091@dastard>
References: <1332284191-21076-1-git-send-email-jack@suse.cz>
 <1332284191-21076-7-git-send-email-jack@suse.cz>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Wu Fengguang <fengguang.wu@intel.com>,
	Christoph Hellwig <hch@infradead.org>,
	linux-fsdevel@vger.kernel.org
To: Jan Kara <jack@suse.cz>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from ipmail06.adl2.internode.on.net ([150.101.137.129]:9939 "EHLO
	ipmail06.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK)
	by vger.kernel.org with ESMTP id S1750879Ab2CTXgK (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Tue, 20 Mar 2012 19:36:10 -0400
Content-Disposition: inline
In-Reply-To: <1332284191-21076-7-git-send-email-jack@suse.cz>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Tue, Mar 20, 2012 at 11:56:30PM +0100, Jan Kara wrote:
> The code in writeback_single_inode() is relatively complex. The list requeing
> logic makes sense only for flusher thread but not really for sync_inode() or
> write_inode_now() callers. Also when we want to get rid of inode references
> held by flusher thread, we will need a special I_SYNC handling there.
> 
> So separate part of writeback_single_inode() which does the real writeback work
> into __writeback_single_inode() and make writeback_single_inode() do only stuff
> necessary for callers writing only one inode, moving the special list handling
> into writeback_sb_inodes(). As a sideeffect this fixes a possible race where we
> could skip some inode during sync(2) because other writer refiled it from b_io
> to b_dirty list. Also I_SYNC handling is moved into the callers of
> __writeback_single_inode() to make locking easier.
.....
> +static int
> +writeback_single_inode(struct inode *inode, struct bdi_writeback *wb,
> +		       struct writeback_control *wbc)
> +{
> +	int ret = 0;
> +
> +	spin_lock(&inode->i_lock);
> +	if (!atomic_read(&inode->i_count))
> +		WARN_ON(!(inode->i_state & (I_WILL_FREE|I_FREEING)));
> +	else
> +		WARN_ON(inode->i_state & I_WILL_FREE);
> +
> +	if (inode->i_state & I_SYNC) {
> +		if (wbc->sync_mode != WB_SYNC_ALL)
> +			goto out;
> +		/*
> +		 * It's a data-integrity sync.  We must wait.
> +		 */
> +		inode_wait_for_writeback(inode);
> +	}
> +	BUG_ON(inode->i_state & I_SYNC);

BUG_ON() seems a little harsh to me. I mean, having I_SYNC set is
not really a fatal error. It's an indication of a problem, but
writeback will continue and not fail catastrophically if this
occurs. So perhaps WARN_ON() might be better here.

.....

> @@ -576,23 +612,24 @@ static long writeback_sb_inodes(struct super_block *sb,
>  		spin_unlock(&wb->list_lock);
>  
>  		__iget(inode);
> +		if (inode->i_state & I_SYNC)
> +			inode_wait_for_writeback(inode);

Even for WB_SYNC_NONE writeback?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com