[PATCH] writeback: Update dirty flags in two steps

[PATCH] writeback: Update dirty flags in two steps

[Dmitry Monakhov]

Filesystems with delalloc support may dirty inode during writepages.
As result inode will have dirty metadata flags even after write_inode.
In fact we have two dedicated functions for proper data and metadata
writeback. It is reasonable to separate flags updates in two stages.

https://bugzilla.kernel.org/show_bug.cgi?id=15906

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/fs-writeback.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index fdc07e3..e73d8ff 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -459,11 +459,9 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 
 	BUG_ON(inode->i_state & I_SYNC);
 
-	/* Set I_SYNC, reset I_DIRTY */
-	dirty = inode->i_state & I_DIRTY;
+	/* Set I_SYNC, reset I_DIRTY_PAGES */
 	inode->i_state |= I_SYNC;
-	inode->i_state &= ~I_DIRTY;
-
+	inode->i_state &= ~I_DIRTY_PAGES;
 	spin_unlock(&inode_lock);
 
 	ret = do_writepages(mapping, wbc);
@@ -479,6 +477,15 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 			ret = err;
 	}
 
+	/*
+	 * Some filesystems may redirty the inode during the writeback
+	 * due to delalloc, clear dirty metadata flags right before
+	 * write_inode()
+	 */
+	spin_lock(&inode_lock);
+	dirty = inode->i_state & I_DIRTY;
+	inode->i_state &= ~(I_DIRTY_SYNC | I_DIRTY_DATASYNC);
+	spin_unlock(&inode_lock);
 	/* Don't write the inode if only I_DIRTY_PAGES was set */
 	if (dirty & (I_DIRTY_SYNC | I_DIRTY_DATASYNC)) {
 		int err = write_inode(inode, wbc);
-- 
1.6.6.1

Re: [PATCH] writeback: Update dirty flags in two steps

[Christoph Hellwig]

On Fri, May 07, 2010 at 01:35:44PM +0400, Dmitry Monakhov wrote:
> 
> Filesystems with delalloc support may dirty inode during writepages.
> As result inode will have dirty metadata flags even after write_inode.
> In fact we have two dedicated functions for proper data and metadata
> writeback. It is reasonable to separate flags updates in two stages.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=15906
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>

Looks good and passes XFSQA for me.


Reviewed-by: Christoph Hellwig <hch@lst.de>

> +	spin_lock(&inode_lock);
> +	dirty = inode->i_state & I_DIRTY;
> +	inode->i_state &= ~(I_DIRTY_SYNC | I_DIRTY_DATASYNC);
> +	spin_unlock(&inode_lock);
>  	/* Don't write the inode if only I_DIRTY_PAGES was set */
>  	if (dirty & (I_DIRTY_SYNC | I_DIRTY_DATASYNC)) {

No need for the double masking, the first line could be just

	dirty = inode->i_state;

or you could do:

	/* Don't write the inode if only I_DIRTY_PAGES was set */
	dirty = inode->i_state & (I_DIRTY_SYNC | I_DIRTY_DATASYNC);

and then just

	if (dirty) {