From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 6/7] writeback: Refactor writeback_single_inode() Date: Wed, 21 Mar 2012 11:03:20 +0100 Message-ID: <20120321100320.GA22938@quack.suse.cz> References: <1332284191-21076-1-git-send-email-jack@suse.cz> <1332284191-21076-7-git-send-email-jack@suse.cz> <20120320233554.GS5091@dastard> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kara , Wu Fengguang , Christoph Hellwig , linux-fsdevel@vger.kernel.org To: Dave Chinner Return-path: Received: from cantor2.suse.de ([195.135.220.15]:43368 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030243Ab2CUKDX (ORCPT ); Wed, 21 Mar 2012 06:03:23 -0400 Content-Disposition: inline In-Reply-To: <20120320233554.GS5091@dastard> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 SUSE Labs, CR