From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 4/6] vfs: Reorder operations during sys_sync
Date: Thu, 20 Oct 2011 05:53:44 -0400
Message-ID: <20111020095344.GD11291@infradead.org>
References: <1318020055-4450-1-git-send-email-jack@suse.cz>
 <1318020055-4450-5-git-send-email-jack@suse.cz>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Christoph Hellwig <hch@infradead.org>,
	Curt Wohlgemuth <curtw@google.com>,
	linux-fsdevel@vger.kernel.org, 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]:43495
	"EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK)
	by vger.kernel.org with ESMTP id S1754041Ab1JTJxp (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Thu, 20 Oct 2011 05:53:45 -0400
Content-Disposition: inline
In-Reply-To: <1318020055-4450-5-git-send-email-jack@suse.cz>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

Looks good except for some minor comments below:


Reviewed-by: Christoph Hellwig <hch@lst.de>

> diff --git a/fs/sync.c b/fs/sync.c
> index 3367d04..5fbeee6 100644
> --- a/fs/sync.c
> +++ b/fs/sync.c
> @@ -68,18 +68,26 @@ int sync_filesystem(struct super_block *sb)
>  }
>  EXPORT_SYMBOL_GPL(sync_filesystem);
>  
> -static void sync_one_sb(struct super_block *sb, void *arg)
> +static void sync_inodes_one_sb(struct super_block *sb, void *arg)
>  {
> -	if (!(sb->s_flags & MS_RDONLY))
> -		__sync_filesystem(sb, *(int *)arg);
> +	if (!(sb->s_flags & MS_RDONLY)) {
> +		if (!*(int *)arg)
> +			writeback_inodes_sb(sb);
> +		else
> +			sync_inodes_sb(sb);
> +	}

This would be a lot cleaner if you split it into two functions for
the writeback_inodes_sb and sync_inodes_sb cases.

>  }
> -/*
> - * Sync all the data for all the filesystems (called by sys_sync() and
> - * emergency sync)
> - */
> -static void sync_filesystems(int wait)
> +
> +static void sync_fs_one_sb(struct super_block *sb, void *arg)
>  {
> -	iterate_supers(sync_one_sb, &wait);
> +	if (!(sb->s_flags & MS_RDONLY) && sb->s_op->sync_fs)
> +		sb->s_op->sync_fs(sb, *(int *)arg);
> +}
> +
> +static void sync_blkdev_one_sb(struct super_block *sb, void *arg)
> +{
> +	if (!(sb->s_flags & MS_RDONLY))
> +		__sync_blockdev(sb->s_bdev, *(int *)arg);
>  }

Same here, not having these odd wait/nowait arguments whos address
is taken in the caller would make the thing a lot more readable.

It would also allow to kill of that nasty __sync_blockdev interface
eventually.