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

[Nick Piggin <npiggin@suse.de>]

On Fri, Oct 31, 2008 at 10:10:19AM -0400, Chris Mason wrote:
> On Fri, 2008-10-31 at 10:16 +0100, Nick Piggin wrote:
> > On Thu, Oct 30, 2008 at 04:13:44PM -0700, Andrew Morton wrote:
> > > 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.
> > > > 
> 
> [ use WB_SYNC_ALL instead of WB_SYNC_NONE ]
> 
> > >  If the caller
> > > is using sync_file_range() for integrity then the caller has done a
> > > SYNC_FILE_RANGE_WAIT_BEFORE.
> > 
> > No disputes about whether the API works "by design". But I think the
> > implementation has a bug. I'll explain:
> 
> I'll definitely agree the current usage is clumsy, and there is a bug in
> fs/buffer.c.  A grep through the rest of the filesystems doesn't turn up
> many assumptions that WB_SYNC_NONE means it's ok to skip dirty pages.
> 
> Greps for WB_SYNC_ALL and WB_SYNC_NONE in the fs code reveal:
> 
> fs/buffer.c:__block_write_full_page()
>                if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) {
>                         lock_buffer(bh);
>                 } else if (!trylock_buffer(bh)) {
>                         redirty_page_for_writepage(wbc, page);
>                         continue;
>                 }
> 
> Easily fixed s/||/&&/, which is what XFS does.  reiser3 has the same bug
> in fs/reiserfs/inode.c
> 
> ntfs and gfs2 each have a check that assumes WB_SYNC_NONE means optional
> writeback, both seem fixable with one liners.

Well, we went over this before :) Semantics... Whether or not it can be
considered a bug, WB_SYNC_NONE is very much considered not to be a data
integrity sync in code and comments (eg. buffer.c fs-writeback.c and
file systems) for a long time.

So that's not something I can really change (especially not for stable
releases).

It's _probably_ a good idea not to fragment bios, and it's probably not
going to introduce bugs if we convert everything over. But presently,
all the bugs/misunderstandings/whatever mean that WB_SYNC_NONE is not
usable for data integrity. Obviously this has been the case for long
before sync_mapping_range was added.


> Everywhere we wait on page writeback while we're trying to build nice
> big bios hurts performance.  I'd rather see us switch to something
> closer to the do_sync_mapping_range expectation of WB_SYNC_NONE than
> sprinkle WB_SYNC_ALLs everywhere.

They already are I guess. At least, this is the only place I have seen
that needed changing. After that series, I think we get a lot closer to
being correct in a lot of the writeback paths, which is a good starting
point to make improvements. I'll be looking at the "livelock" avoidance,
but you or other fs developers probably in a better position to look at
things like waiting for locked or writeback pages...