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

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On Wed, 2010-12-29 at 10:01 -0500, Christoph Hellwig wrote:
> As mentioned last round I think the exporting of inode_lock and pusing
> of the I_DIRTY* complexities into the filesystems can be avoided.  See
> the patch below, which compiles and passes xfstests for xfs, but
> otherwise isn't quite done yet.  The only code change vs the opencoded
> variant in the patch is that we do a useless inode_lock roundtrip
> for a non-dirty inode on gfs2, which is I think is acceptable,
> especially once we have the lock split anyway.
> 
> The other thing I don't like yet is passing the datasync flag - the
> callback shouldn't care about what we were called with, but rather
> about which bits it needs to sync out - which the dirty flag already
> tells us about.
> 
> IMHO the behaviour in ocfs2 and gfs2 that relies on it is plain wrong:
> 
>  - ocfs2 really should always force the journal if any bit we care about
>    in the inode is dirty, and only do the pure cache flush is nothing
>    we care about is dirty (similar to the more complex code in XFS)
>  - gfs2 seems really weird.  Doesn't it need to do any log force
>    if an inode has a pending transaction?  Currently it only does for
>    stuffed inodes, and if datasync was set, which seems weird.  Also
>    I can't see why we'd never want to call into ->write_inode to write
>    out the inode for the datasync case - except for not catching
>    timestamp updates fdatasync really isn't any different from fsync.
> 
The algorithm was intended to be:

 - With "journaled data" files
   - Do a log flush conditional upon the inode's glock
   - The core code then writes back any dirty pages

 - With regular files/directories
  - If datasync is not set, we need to write back the metadata including
timestamp updates, so that is done via ->write_inode. Note that an extra
complication here is that we need to get the glock on the inode if we
don't already have it in order to check and conditionally update the
atime.The call to ->write_inode includes an implicit (conditional) log
flush.
 - If datasync is set, we assume that only the data pages need to be
written out. My understanding of datasync was that it was only supposed
to write out data and never any of the metadata. The reason for the call
to flush the log for "stuffed" files is that the data shares a disk
block with the inode metadata, so we cannot avoid the log flush in this
case, since we must unpin the block to write it back.

There is something strange going on here though since there should be a
metadata sync included as well I think - I'm just working back through
the changes to see where that was lost at the moment,

Steve.