Re: [PATCH 6/7] writeback: Refactor writeback_single_inode()

[Dave Chinner <david@fromorbit.com>]

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