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@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
