Re: [PATCH 4/4] vfs: Make sys_sync() use fsync_super() (version 2)

[Christoph Hellwig <hch@infradead.org>]

> +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.

> @@ -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.

>  /*
> - * 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 :)

>  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.