Re: [PATCH 10/11] hfsplus: optimize fsync

[Christoph Hellwig <hch@lst.de>]

On Thu, Nov 18, 2010 at 05:40:44PM +1100, Nick Piggin wrote:
> Does this guy ever get cleared in the synchronous path? Or only in writeback?
> I couldn't see where it gets cleared from direct writeout, I wonder if we should
> import the pagecache tagged dirty check from writeback to keep this in sync
> better? (anyway, separate issue)
> 
> Anyway, what if writeback noticed pagecache was cleaned at this point, and then
> clears I_DIRTY bits from inode before you test it above? Won't that leave your
> metadata not on disk?

What do you mean with direct writeout?  For hfsplus we always write
the inode through writeback_single_inode, either from the writeback
threads, or through sync_inode_metadata (or previously write_inode_now)
in fsync.  Both of these clear I_DIRTY.  But one thing I noticed when
reading through the above is that I_DIRTY is overkill here - we only
care about metadata, as the pagecache is handled by vfs_fsync. Just
checking I_DIRTY_SYNC (and eventually I_DIRTY_DATASYNC when adding
support for an optimized fdatasync) would be enough.  The code is
take from generic_file_fsync, which is used or copied by a lot of
filesystems, so it looks like no one really cared about the possibly
superflous I_DIRTY_PAGES checking so far.

> > @@ -590,6 +602,8 @@ int hfsplus_cat_write_inode(struct inode
> > ? ? ? ? ? ? ? ?hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof(struct hfsplus_cat_file));
> > ? ? ? ?}
> > +
> > + ? ? ? set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags);
> > ?out:
> > ? ? ? ?hfs_find_exit(&fd);
> > ? ? ? ?return 0;
> 
> So your I_CAT_DIRTY will be set, but not I_DIRTY, AFAIKS.

This in ->write_inode which is called to clear I_DIRTY (well,
(I_DIRTY_SYNC|I_DIRTY_DATASYNC).  But given the way how hfsplus stores
it's equivalents of inode and dirent in the catalog btree it means
dirtying the btree.  If ->write_inode is called by fsync that gets
picked up by the fsync code checking for I_CAT_DIRTY, if it's for
sync or during umount it's caught by the unconditionaly flush of it
in hfsplus_sync_fs.  The write_inode during nfs exporting is not handled
yet, but adding a commit_metadata operation is on my todo list.

> > @@ -530,4 +532,5 @@ out:
> > ? ? ? ?hip->fs_blocks = (inode->i_size + sb->s_blocksize - 1) >> sb->s_blocksize_bits;
> > ? ? ? ?inode_set_bytes(inode, hip->fs_blocks << sb->s_blocksize_bits);
> > ? ? ? ?mark_inode_dirty(inode);
> > + ? ? ? set_bit(HFSPLUS_I_ALLOC_DIRTY, &HFSPLUS_I(inode)->flags);
> 
> If you mark these sub-parts of the inode dirty after marking the inode
> dirty, and no
> locking, then AFAIKS you get no guarantee they will be noticed at the point that
> I_DIRTY bits are cleared.

Yes, it should be before.  I had fixed that up in the other places but
missed this one.  Thanks for the review!