> On Wed, Jan 14, 2009 at 10:18:34AM -0800, Andrew Morton wrote:> > On Wed, 14 Jan 2009 16:12:28 +0100 Jan Kara <jack@suse.cz> wrote:> > > > > To be really safe that the data hit the platter, we should also flush drive's> > > writeback caches on fsync and for O_SYNC files or O_DIRSYNC inodes.> > > > > > > It's not good to randomly sprinkle blkdev_issue_flush() calls all over> > the filesystem like this. How do we know that you didn't miss a site? > > How do we ensure that people who modify the fs in the future don't> > forget to add the blkdev_issue_flush() call, if needed?> > > > IOW, it is fragile. Is there anything we can do to make this more> > robust? Do the flush calls from some higher-level callsite? Perhaps> > even the VFS?> > The problem with doing it in the VFS is that we might end up calling> flush twice; it may very well be that for some filesystems, we'll be> calling the flush operation as part of writing out the commit block,> for example.

It depends how it's done. One way would be via a newaddress_space_operations entry.

> I'm not sure it's fair to say that we need to "randomly sprinkle> blkdev_issue_flush() calls all over the place". The number of places> where we need to do O_SYNC or fsync() handling is in fact quite small.> We were simply ignoring this problem before.

Partly as a cleanup, yes. But such a function then becomes a sitewhere we can add commentary explaining the reason for its existence,along with commentary which describes the semantics in details,exceptions, caveats, etc.

It also becomes a single site where the feature can be disabled forthose who wish to evaluate its performance cost.

The single site can also be used for convenient gathering of timinginformation.

One thing we may well choose to do is to stop issuing the flush ifblkdev_issue_flush() is returning -EOPNOTSUPP, which some devices willpresumably do. Reissuing a pointless and quite expensive command overand over again doesn't seem clever.

And I can probably think of more reasons :)

> I> personally find extra levels of inline functions

It shouldn't be inlined.

> to be harder to deal> with long term, since I then I have to track down the definition of> ext3_blkdev_issue_flush in the header files to make sure there isn't> any other magic hiding in there other than the i->i_sb->s_bdev> derefencing.> > - Ted