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

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

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..

> 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.

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  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).

>  	inode->i_state |= I_SYNC;
> -	inode->i_state &= ~I_DIRTY_PAGES;
>  	spin_unlock(&inode->i_lock);
>  	spin_unlock(&wb->list_lock);
>  
> @@ -407,6 +406,9 @@ writeback_single_inode(struct inode *inode, struct bdi_writeback *wb,
>  	 * write_inode()
>  	 */
>  	spin_lock(&inode->i_lock);
> +	/* Clear I_DIRTY_PAGES if we've written out all dirty pages */
> +	if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY))
> +		inode->i_state &= ~I_DIRTY_PAGES;
>  	dirty = inode->i_state & I_DIRTY;
>  	inode->i_state &= ~(I_DIRTY_SYNC | I_DIRTY_DATASYNC);
>  	spin_unlock(&inode->i_lock);
> @@ -434,7 +436,6 @@ writeback_single_inode(struct inode *inode, struct bdi_writeback *wb,
>  			 * We didn't write back all the pages.  nfs_writepages()
>  			 * sometimes bales out without doing anything.
>  			 */
> -			inode->i_state |= I_DIRTY_PAGES;
>  			if (wbc->nr_to_write <= 0) {
>  				/*
>  				 * slice used up: queue for next turn
> -- 
> 1.7.1