From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [patch 9/9] mm: do_sync_mapping_range integrity fix
Date: Thu, 30 Oct 2008 16:13:44 -0700
Message-ID: <20081030161344.0ed5ca52.akpm@linux-foundation.org>
References: <20081028144715.683011000@suse.de>
	<20081028145734.706927000@nick.local0.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: linux-fsdevel@vger.kernel.org, david@fromorbit.com,
	chris.mason@oracle.com
To: npiggin@suse.de
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from smtp1.linux-foundation.org ([140.211.169.13]:50930 "EHLO
	smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK)
	by vger.kernel.org with ESMTP id S1754350AbYJ3XPM (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Thu, 30 Oct 2008 19:15:12 -0400
In-Reply-To: <20081028145734.706927000@nick.local0.net>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Wed, 29 Oct 2008 01:47:24 +1100
npiggin@suse.de wrote:

> Chris Mason notices do_sync_mapping_range didn't actually ask for data
> integrity writeout. Unfortunately, it is advertised as being usable for
> data integrity operations.
> 
> This is a data interity bug.
> 
> Signed-off-by: Nick Piggin <npiggin@suse.de>
> ---
> Index: linux-2.6/fs/sync.c
> ===================================================================
> --- linux-2.6.orig/fs/sync.c
> +++ linux-2.6/fs/sync.c
> @@ -269,7 +269,7 @@ int do_sync_mapping_range(struct address
>  
>  	if (flags & SYNC_FILE_RANGE_WRITE) {
>  		ret = __filemap_fdatawrite_range(mapping, offset, endbyte,
> -						WB_SYNC_NONE);
> +						WB_SYNC_ALL);
>  		if (ret < 0)
>  			goto out;
>  	}
> 

Really?

Some thought did go into the code which you're "fixing".  If the caller
is using sync_file_range() for integrity then the caller has done a
SYNC_FILE_RANGE_WAIT_BEFORE.

So all we need to guarantee here is that
__filemap_fdatawrite_range(WB_SYNC_NONE) will start writeout on all
dirty pages in the range.  Probably that gets broken lower down as part
of various hacks^woptimisations have gone in, but which ones, and
where?  Perhaps _this_ (if it's there) is what should be fixed.

And if we _do_ make the above change, we don't need to run the
wait_on_page_writeback_range() if userspace asked for
SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE, yes?



IOW, I don't think enough thought (or at least description of that
thought) has gone into this one.