From mboxrd@z Thu Jan 1 00:00:00 1970 From: Fengguang Wu Subject: Re: [PATCH 3/7] writeback: Move I_DIRTY_PAGES handling Date: Wed, 28 Mar 2012 11:11:54 +0800 Message-ID: <20120328031154.GA20342@localhost> References: <1332284191-21076-1-git-send-email-jack@suse.cz> <1332284191-21076-4-git-send-email-jack@suse.cz> <20120322024114.GA20507@localhost> <20120322083540.GA14485@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Christoph Hellwig , linux-fsdevel@vger.kernel.org, David Howells To: Jan Kara Return-path: Received: from mga01.intel.com ([192.55.52.88]:37984 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751365Ab2C1DLz (ORCPT ); Tue, 27 Mar 2012 23:11:55 -0400 Content-Disposition: inline In-Reply-To: <20120322083540.GA14485@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Mar 22, 2012 at 09:35:40AM +0100, Jan Kara wrote: > On Thu 22-03-12 10:41:14, Wu Fengguang wrote: > > On Tue, Mar 20, 2012 at 11:56:27PM +0100, Jan Kara wrote: > > > Instead of clearing I_DIRTY_PAGES and resetting it when we didn't succeed in > > > writing them all, just clear the bit only when we succeeded writing all the > > > pages. > > > > Is this just to reduce one line of code? Well it hardly deserves to > > think about the tricky implications.. > It's not because of the reduction. It's because I don't want to take > i_lock in the beginning of writeback_single_inode() just to clear > I_DIRTY_PAGES when it's not really needed. OK, that makes good sense. > > > We also move the clearing of the bit close to other i_state handling to > > > separate it from writeback list handling. This is desirable because list > > > handling will differ for flusher thread and other writeback_single_inode() > > > callers in future. > > > > > > No filesystem plays any tricks with I_DIRTY_PAGES (like checking it > > > in ->writepages or ->write_inode implementation) so this movement is > > > safe. > > > > afs_writeback_all() calls > > > > __mark_inode_dirty(mapping->host, I_DIRTY_PAGES); > > > > which creates the subtle state (I_DIRTY_PAGES && !PAGECACHE_TAG_DIRTY) > > which will no longer exist after this patch. > > > > That line is introduced in 2007 in commit 31143d5d5 ("AFS: implement > > basic file write support"), so it should not be trying to alter the > > requeue/redirty behavior here. But this patch might still alter the > > behavior from redirty_tail() to list_del_init() since the > > (inode->i_state & I_DIRTY) test may no longer be true. > Hmm, I don't really see a reason for that __mark_inode_dirty() call. > Since AFS uses PAGECACHE_TAG_DIRTY to track dirty pages in it's > writepages() implementation, having I_DIRTY_PAGES set without any dirty > page just doesn't make any sense. David, do you remember why that > __mark_inode_dirty() is there? Cannot speak for AFS, however I don't see the difference, either. Why would afs_fsync/afs_setattr need to keep the inode in dirty list? It seems fine to just remove that line? Thanks, Fengguang