Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: Christoph Hellwig
Date: Monday, January 3, 2011 - 9:58 am

On Mon, Jan 03, 2011 at 03:03:29PM +0000, Steven Whitehouse wrote:

Any data writeback is done before calling ->fsync.


What happens to indirect blocks, inode size updates, etc?  In general
the only correct form to use the datasync argument is along the lines
of:

	if ((inode->i_state & I_DIRTY_DATASYNC) ||
	    ((inode->i_state & I_DIRTY_SYNC) && !datasync)) {
		/* write out the inode */
	} else {
		/*
		 * VFS inode not dirty, no need to write it out.
		 *
		 * If the filesystem support asynchronous inode writes,
		 * we may have to wait for them here.
		 */
	}

or rather mostly correct, as pointed out by Nick in this series, that's
why the above gets replaced with an equivalent check that also
participates in the writeback locking protocol in this series.

For gfs2 on current mainline an fsync respecting that would look like:

static int gfs2_fsync(struct file *file, int datasync)
{
	struct inode *inode = file->f_mapping->host;
	struct gfs2_inode *ip = GFS2_I(inode);
	int ret = 0;

	if (gfs2_is_jdata(ip) {
		gfs2_log_flush(GFS2_SB(inode), ip);
		return 0;
	}

	if ((inode->i_state & I_DIRTY_DATASYNC) ||
	    ((inode->i_state & I_DIRTY_SYNC) && !datasync))
		sync_inode_metadata(inode, 1);
	else if (gfs2_is_stuffed(ip))
		gfs2_log_flush(GFS2_SB(inode), ip->i_gl);
}

Note that the asynchronous write_inode_now is replaced with a
sync_inode_metadata, which doesn't incorrectly write data again, and
makes sure we do a synchronous write.

I'm still not quite sure how the gfs2_log_flush are supposed to work.
What's the reason we don't need the ->write_inode call for journaled
data mode?  Also is it guaranteed that we might not have an asynchronous
transaction that update the inode in the log, e.g. why doesn't gfs2
need some sort of log flush even if the VFS inode is not dirty, unlike
most other journaled filesystems.

--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
[patch 0/8] Inode data integrity patches, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 1/8] fs: mark_inode_dirty barrier fix, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 2/8] fs: simple fsync race fix, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 3/8] fs: introduce inode writeback helpers, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 5/8] fs: ext2 inode sync fix, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 6/8] fs: fsync optimisations, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 8/8] fs: add i_op->sync_inode, Nick Piggin, (Fri Dec 17, 6:46 pm)
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in f ..., Christoph Hellwig, (Wed Dec 29, 8:01 am)
Re: [patch 8/8] fs: add i_op->sync_inode, Christoph Hellwig, (Wed Dec 29, 8:12 am)
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in f ..., Christoph Hellwig, (Mon Jan 3, 9:58 am)
Re: [patch 8/8] fs: add i_op->sync_inode, Nick Piggin, (Mon Jan 3, 11:27 pm)
Re: [patch 8/8] fs: add i_op->sync_inode, Christoph Hellwig, (Mon Jan 3, 11:57 pm)
Re: [patch 8/8] fs: add i_op->sync_inode, Nick Piggin, (Tue Jan 4, 1:03 am)
Re: [patch 8/8] fs: add i_op->sync_inode, Nick Piggin, (Tue Jan 4, 2:49 am)