On Thu 23-04-09 12:57:36, Christoph Hellwig wrote:
> > +int __sync_blockdev(struct block_device *bdev, int wait)
> > +{
> > + if (!bdev)
> > + return 0;
> > + if (!wait)
> > + return filemap_flush(bdev->bd_inode->i_mapping);
> > + return filemap_write_and_wait(bdev->bd_inode->i_mapping);
> > +}
>
> I wonder if the filemap_flush for the async case really buys us much,
> all the async and then later sync writeback activities of the FS will
> redirty lots of bits of the blockdev mapping that we then have to write
> twice.
Well, I think it does. Because if you call write_inode() with wait==1,
then filesystems usually do sync_dirty_buffer() for the buffer with inode.
But with wait==0 filesystems (e.g. ext2 and other "simple" filesystems)
just mark the buffer with inode dirty. So sync_inodes_sb(sb, 0) just
dirties lots of buffers and then filemap_flush() submits all the IO more
effectively than doing sync_dirty_buffer() for each inode... But I guess it
deserves a comment.
Also I'd think that async case submits most of the IO and later sync just
gathers last bits we might have skipped.
> > @@ -284,7 +277,12 @@ static int __fsync_super(struct super_block *sb)
> > */
> > int fsync_super(struct super_block *sb)
> > {
> > - return __fsync_super(sb);
> > + int ret;
> > +
> > + ret = __fsync_super(sb, 0);
> > + if (ret < 0)
> > + return ret;
> > + return __fsync_super(sb, 1);
>
> This async first then wait approach does have some benefits when syncing
> multiple filesystems, but I wonder if it isn't actually conta-productive
> when syncing a single one.
>
> Maybe this should be a separate patch ontop to allow for more
> fine-grained benchmarking.
But __fsync_super() previously did:
sync_inodes_sb(sb, 0);
...
sync_inodes_sb(sb, 1);
So the change is only in calling write_super(), vfs_dq_sync() and
sync_fs() twice. I can certainly change the function to call vfs_dq_sync()
and write_super() only if wait == 1, so only sync_fs() would be called
twice. But then if someone uses __fsync_super() in future, he might get
surprised...
> > /*
> > - * Call the ->sync_fs super_op against all filesystems which are r/w and
> > - * which implement it.
> > + * Sync all the data for all the filesystems (called by do_sync())
>
> Your patch removes do_sync :)
Yup, thanks.
> > static void do_sync_work(struct work_struct *work)
> > {
> > - do_sync(0);
> > + /*
> > + * Sync twice to reduce the possibility we skipped some inodes / pages
> > + * because they were temporarily locked
> > + */
> > + sync_filesystems(0);
> > + sync_filesystems(0);
> > + printk("Emergency Sync complete\n");
> > kfree(work);
>
> Ah, nice. Good to have this out of the sys_sync path.
>
> The patch looks generally good but I'll need some heavy testing. I'll
> do some XFS testing with it ASAP.
Great. I think the first (maybe first two) patch can go in quickly to fix
the bug and the others can go in after more serious testing.
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
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
