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! -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
