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

[Nick Piggin <npiggin@suse.de>]

On Fri, Oct 31, 2008 at 03:04:32AM -0700, Andrew Morton wrote:
> On Fri, 31 Oct 2008 10:16:16 +0100 Nick Piggin <npiggin@suse.de> wrote:
> 
> >  
> > > 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.
> > 
> > WB_SYNC_NONE has never (until this was introduced) been used for data
> > integrity AFAIKS. There is code littered throughout fs/ which assumes
> > WB_SYNC_NONE ~= "efficiency / background writeback". At least
> > definitely the buffer.c "trylock" will cause dirty pages to be
> > skipped.
> 
> 
> The change seriously wrecks sync_file_range(SYNC_FILE_RANGE_WRITE), the
> whole point of which is to as-nonblockingly-as-possible shove some data
> into the queues.  This is useful.

Right, but what's possible to do nonblockingly according to the API
isn't very much. Because you can't distinguish data integrity writes
from hints (as you note below).

IMO we should implement SYNC_FILE_RANGE_ASYNC to do the real "hint"
thing. (and aside, but further along this topic, a SYNC_FILE_RANGE_SYNC
to complement as well, and allow trivial migration for apps from fsync
to an fsync_range equivalent... also a flag or two for metadata might
be nice). 

 
> Perhaps we could use WB_SYNC_ALL if SYNC_FILE_RANGE_WAIT_BEFORE was
> specified, or WB_SYNC_NONE if it was not (need to think about
> SYNC_FILE_RANGE_WAIT_AFTER, too).
> 
> That's a bit grubby, because userspace could do
> 
> 	sync_file_range(SYNC_FILE_RANGE_WAIT_BEFORE);
> 	sync_file_range(SYNC_FILE_RANGE_WRITE);
> 
> expecting it to have the same behaviour as
> 
> 	sync_file_range(SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE);
> 
> 
> 
> How the hell did that stupid sync_mode thing get into
> writeback_control?  :( We should get rid of WB_SYNC_FOO and migrate to
> better-defined writeback_control fields.

No arguments. It's crusty. But this is the best I think I can do for
stable and 2.6.28 kernels.

As for how it got there... :)