Re: [patch 1/2] xfs: xfs_tosspages() bug

[Ben Myers <bpm@sgi.com>]

Hey Dave,

On Fri, Nov 09, 2012 at 10:06:49AM +1100, Dave Chinner wrote:
> On Thu, Nov 08, 2012 at 04:23:16PM -0600, Andrew Dahl wrote:
> > xfs_tosspages() takes a closed interval as an argument, take 
> > this into account when rounding down to the last byte of the
> > last complete page. If the request consists of a single 
> > partial page, there will be nothing to toss. 
> > 
> > Signed-off-by: Andrew Dahl <adahl@sgi.com>
> > 
> > ---

...

> So the change is good.
> 
> However, there's a bigger issue here. We've planned to remove these
> wrappers for a long time, just never got around to doing it. Seeing
> as there is a bug in this wrapper and it needs to be fixed, now
> seems like the right time to remove it.

The removal of the wrappers would not be appropriate for -stable.  This fix
needs to go in separately from any refactoring so that it can be pulled back
within the rules outlined in Documentation/stable_kernel_rules.txt.

> Hence I'd suggest that fixing this particular bug should just
> remove xfs_tosspages() and call truncate_inode_pages_range()
> directly. There are only two calls to this function, so it should be
> a simple conversion.  That can then be followed up with more patches
> to remove the other wrappers in xfs_fs_subr.c and hence remove the
> file completely...

I have no objection to doing so in a followup series, and I don't consider it
to be a high priority either.

> >  int
> > Index: xfs/fs/xfs/xfs_vnodeops.c
> > ===================================================================
> > --- xfs.orig/fs/xfs/xfs_vnodeops.c
> > +++ xfs/fs/xfs/xfs_vnodeops.c
> > @@ -2172,7 +2172,7 @@ xfs_change_file_space(
> >  	switch (cmd) {
> >  	case XFS_IOC_ZERO_RANGE:
> >  		prealloc_type |= XFS_BMAPI_CONVERT;
> > -		xfs_tosspages(ip, startoffset, startoffset + bf->l_len, 0);
> > +		xfs_tosspages(ip, startoffset, bf->l_len ? startoffset + llen : -1, 0);
> >  		/* FALLTHRU */
> >  	case XFS_IOC_RESVSP:
> >  	case XFS_IOC_RESVSP64:
> 
> What's this hunk for? Indeed, one of the first things that the
> xfs_alloc_file_space() checks is this:
> 
>         if (len <= 0)
> 		return XFS_ERROR(EINVAL);
> 
> xfs_free_file_space() does the same check, so it is invalid to pass
> a bf_len <= 0 for any of these specific functions. Hence this change
> is wrong regardless of what the comment on the struct xfs_flock64_t
> says - preallocation and hole punch operations must have a positive
> length associated with them.

Andrew, if you agree that this second change is unnecessary go ahead and remove
it and repost.  Otherwise,

Reviewed-by: Ben Myers <bpm@sgi.com>

Welcome to the XFS community!

-Ben

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