Re: [patch 9/9] mm: do_sync_mapping_range integrity fix

[Andrew Morton <akpm@linux-foundation.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.