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.
> > 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?
> > Signed-off-by: Jan Kara <jack@xxxxxxx>
> > ---
> > fs/fs-writeback.c | 5 +++--
> > 1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 5f17a16..0ef272e 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -384,7 +384,6 @@ writeback_single_inode(struct inode *inode, struct bdi_writeback *wb,
> >
> > /* Set I_SYNC, reset I_DIRTY_PAGES */
>
> That comment is obsoleted (and not really useful).
Yes. I've removed it, just in some later patch.
Honza
--
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
