From: Christoph Hellwig Subject: Re: [rfc][patch 2/3] fs: sync_sb_inodes fix Date: Mon, 10 Nov 2008 08:41:33 -0500 Message-ID: <20081110134133.GA7425@infradead.org> References: <20081110132205.GA7183@wotan.suse.de> <20081110132308.GC7183@wotan.suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, Andrew Morton To: Nick Piggin Return-path: Received: from bombadil.infradead.org ([18.85.46.34]:56392 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754899AbYKJNlf (ORCPT ); Mon, 10 Nov 2008 08:41:35 -0500 Content-Disposition: inline In-Reply-To: <20081110132308.GC7183@wotan.suse.de> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Nov 10, 2008 at 02:23:08PM +0100, Nick Piggin wrote: > > Fix data integrity semantics required by sys_sync, by iterating over all > inodes and waiting for any writeback pages after the initial writeout. > Comments explain the exact problem. > > --- > Index: linux-2.6/fs/fs-writeback.c > =================================================================== > --- linux-2.6.orig/fs/fs-writeback.c > +++ linux-2.6/fs/fs-writeback.c > @@ -440,6 +440,7 @@ void generic_sync_sb_inodes(struct super > struct writeback_control *wbc) > { > const unsigned long start = jiffies; /* livelock avoidance */ > + int sync = wbc->sync_mode == WB_SYNC_ALL; > > spin_lock(&inode_lock); > if (!wbc->for_kupdate || list_empty(&sb->s_io)) > @@ -516,7 +517,42 @@ void generic_sync_sb_inodes(struct super > if (!list_empty(&sb->s_more_io)) > wbc->more_io = 1; > } > - spin_unlock(&inode_lock); > + > + if (unlikely(sync)) { Please don't mark things like this unlikely. Static branch prediction is really nasty on many cpus when the prediction was wrong, and dynamic branch brediction can do thing much better. In general never use unlikely for something like this which can happen with a normal workload. > + * we still have to wait for that writeout. > + */ > + list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > + struct address_space *mapping; > + > + if (inode->i_state & (I_FREEING|I_WILL_FREE)) > + continue; > + mapping = inode->i_mapping; > + if (mapping->nrpages == 0) > + continue; > + __iget(inode); > + spin_unlock(&inode_lock); > + /* see dquot.c comment */ Not an exactly useful comment :)