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@xxxxxxx>
> >
> > ---
...
> 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@xxxxxxx>
Welcome to the XFS community!
-Ben
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
