Re: [PATCH 5/5] vfs: Avoid unnecessary WB_SYNC_NONE writeback during sync(1)

[Christoph Hellwig <hch@infradead.org>]

> +static void __sync_filesystem(struct super_block *sb, int emergency)
>  {
> +	/* In case of emergency sync we don't want to wait for locks and IO */
> +	if (unlikely(emergency))
>  		writeback_inodes_sb(sb);
> +	else
> +		sync_inodes_sb(sb);
>  
>  	if (sb->s_op->sync_fs)
> +		sb->s_op->sync_fs(sb, 0);
>  }

This function doesn't make sense to me in the current form.  Why would
be do a blocking sync_inodes_sb, but then a non-blocking ->sync_fs?

> -static void sync_filesystems(int wait)
> +static void sync_filesystems(int emergency)
>  {
> -	iterate_supers(sync_one_sb, &wait);
> +	iterate_supers(sync_one_sb, &emergency);
>  }


I'd just drop the sync_filesystems wrapper, and also use individual
callbacks for the two cases.

>  SYSCALL_DEFINE0(sync)
>  {
> +	/* Start flushing on all devices */
>  	wakeup_flusher_threads(0);
> +	/*
> +	 * Above call queued work doing complete writeout on each filesystem.
> +	 * Now we queue work which guarantees data integrity of all inodes
> +	 * - not much should be left for it to write. The WB_SYNC_ALL inode
> +	 * writeback also guarantees that sync_fs() is called after inodes
> +	 * are written out and thus it can do meaningful work.
> +	 */
>  	sync_filesystems(0);

This really should be an iteration over sb_sync_fs with wait == 0

>  	sync_all_bdevs(0);
> +	/* Call blocking ->sync_fs() for each filesystem */
> +	iterate_supers(sb_sync_fs, NULL);