From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 5/5] vfs: Avoid unnecessary WB_SYNC_NONE writeback during
 sync(1)
Date: Wed, 27 Jul 2011 05:52:48 -0400
Message-ID: <20110727095248.GE11334@infradead.org>
References: <1311719886-1130-1-git-send-email-jack@suse.cz>
 <1311719886-1130-6-git-send-email-jack@suse.cz>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: linux-fsdevel@vger.kernel.org,
	Christoph Hellwig <hch@infradead.org>,
	Curt Wohlgemuth <curtw@google.com>,
	Al Viro <viro@ZenIV.linux.org.uk>
To: Jan Kara <jack@suse.cz>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from 173-166-109-252-newengland.hfc.comcastbusiness.net ([173.166.109.252]:43664
	"EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK)
	by vger.kernel.org with ESMTP id S1754030Ab1G0Jwu (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Wed, 27 Jul 2011 05:52:50 -0400
Content-Disposition: inline
In-Reply-To: <1311719886-1130-6-git-send-email-jack@suse.cz>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.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);