Re: [PATCH 1/2] xfs: use range primitives for xfs page cache operations

[Dave Chinner <david@fromorbit.com>]

On Tue, Jul 27, 2010 at 02:53:26AM -0400, Christoph Hellwig wrote:
> On Tue, Jul 27, 2010 at 03:55:28PM +1000, Dave Chinner wrote:
> > While XFS passes ranges to operate on from the core code, the
> > functions being called ignore the either the entire range or the end
> > of the range. This is historical because when the function were
> > written linux didn't have the necessary range operations. Update the
> > functions to use the correct operations.
> 
> Assuming you have actually tested this - given that we've ignore
> these parameters so long that I'm really fearing some callers have
> started to rely on that behaviour.

Several xfstests run on different configs haven't shown any
regressions.

All current callers of xfs_tosspages() trim from and offset to EOF,
so no change in behaviour there.

All of the xfs_flush_pages() icallers except one flush the entire
file (0 to -1), except for a single call in xfs_setattr which
between on disk size and the new filesystem when extending the file
size by truncate.  It will now flush just the necessary range
instead of the whole file.

All of xfs_flushinval_pages() callers flush to the end of file,
so once again there should be no change in behaviour there.

So I don't think there really is much risk here - the only one that
I'd be concerned about is the setattr() call and we have tests
covering that specific case (138, 139 and 140)....

> >  	if (mapping->nrpages) {
> 
> I'd drop this check ere as well - no other caller does it.
> 
> >  		xfs_iflags_clear(ip, XFS_ITRUNCATED);
> > -		ret = filemap_write_and_wait(mapping);
> > +		ret = filemap_write_and_wait_range(mapping, first,
> > +					last == -1 ? LLONG_MAX : last);
> >  		if (!ret)
> > -			truncate_inode_pages(mapping, first);
> > +			truncate_inode_pages_range(mapping, first, last);
> >  	}
> >  	return -ret;
> >  }
> > @@ -73,7 +71,8 @@ xfs_flush_pages(
> >  
> >  	if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
> 
> Same for this check.

Ok, killed those checks.

> >  		xfs_iflags_clear(ip, XFS_ITRUNCATED);
> > -		ret = -filemap_fdatawrite(mapping);
> > +		ret = -filemap_fdatawrite_range(mapping, first,
> > +					last == -1 ? LLONG_MAX : last);
> 
> Also for the non-async case we should just use
> filemap_write_and_wait_range, and kill off xfs_wait_on_pages.

That can be done separately, I think. If we don't see any
problems from this patch, then I think we can kill all
the wrappers, not just one of them.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs