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

[Jan Kara <jack@suse.cz>]

On Wed 21-03-12 10:35:54, Dave Chinner wrote:
> 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.
  Yeah, I just copy-and-pasted this from the old code but I agree. I'll
change this.

> > @@ -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?
  Above, we already requeued the inode if I_SYNC was set and we are doing
WB_SYNC_NONE writeback. So this is really only for WB_SYNC_ALL writeback.
I'll add a short comment about this.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR