Re: [PATCH 3/7] writeback: Move I_DIRTY_PAGES handling

[Fengguang Wu <fengguang.wu@intel.com>]

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