Re: [patch 0/9] writeback data integrity and other fixes (take 3)

[Nick Piggin <npiggin@suse.de>]

On Wed, Oct 29, 2008 at 02:26:01PM +1100, Dave Chinner wrote:
> On Wed, Oct 29, 2008 at 02:16:45PM +1100, Dave Chinner wrote:
> > On Wed, Oct 29, 2008 at 01:16:53AM +0100, Nick Piggin wrote:
> > > XFS: fix fsync errors not being propogated back to userspace.
> > > ---
> > > Index: linux-2.6/fs/xfs/xfs_vnodeops.c
> > > ===================================================================
> > > --- linux-2.6.orig/fs/xfs/xfs_vnodeops.c
> > > +++ linux-2.6/fs/xfs/xfs_vnodeops.c
> > > @@ -715,7 +715,7 @@ xfs_fsync(
> > >  	/* capture size updates in I/O completion before writing the inode. */
> > >  	error = filemap_fdatawait(VFS_I(ip)->i_mapping);
> > >  	if (error)
> > > -		return XFS_ERROR(error);
> > > +		return XFS_ERROR(-error);
> > 
> > <groan>
> > 
> > Yeah, that'd do it. Good catch. I can't believe I recently fixed a
> > bug that touched these lines of code without noticing the inversion.
> > Sometimes I wonder if we should just conver the entire of XFS to
> > return negative errors - mistakes in handling negative error numbers
> > in the core XFS code happen all the time.
> 
> Ok, I was right - these problems happen all the time. The above call
> should really call xfs_flush_pages() to do the flush and wait. I
> note that xfs_flush_pages() returns negative errors, and all the
> callers expect positive errors. I bet the same occurs for
> xfs_flushinval_pages() and xfs_tosspages() which are the wrappers
> that core XFS code is supposed to be using for flushing and
> invalidating file ranges....

Just be careful -- in your xfs_flush_pages, I think after the first
filemap_fdatawrite, the mapping may no longer be tagged with
PAGECACHE_TAG_DIRTY, so you may not pick up those writeback ones
you need to wait on.

Might need a different variant, or we could just bite the bullet and
push through the ->fsync conversion so you get full control of the
writeout.

BTW. the Linux pagecache APIs should support range operations quite
nicely for these. Any reason not to use them (it looks like the
wrappers can take ranges)?