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> > > --- > > Index: xfs/fs/xfs/xfs_fs_subr.c > =================================================================== > --- xfs.orig/fs/xfs/xfs_fs_subr.c > +++ xfs/fs/xfs/xfs_fs_subr.c > @@ -32,9 +32,17 @@ xfs_tosspages( > xfs_off_t last, > int fiopt) > { > - /* can't toss partial tail pages, so mask them out */ > - last &= ~(PAGE_SIZE - 1); > - truncate_inode_pages_range(VFS_I(ip)->i_mapping, first, last - 1); > + /* > + * Can't toss partial tail pages, so mask them out. If the only > + * page to toss was a partial tail, there will be nothing left > + * to do. > + */ > + if (last != -1) { > + last = ((last + 1) & PAGE_MASK) - 1; > + if (last < first) > + return; > + } > + truncate_inode_pages_range(VFS_I(ip)->i_mapping, first, last); Ok, lets look at critical ranges: passed to truncate_inode_pages_range first,last current patched 0,4095 0,0xffffffff 0,4095 0,4096 0,4095 0,4095 0,4097 0,4095 0,4095 Yup, that's needed. 0,1 0,0xffffffff aborts (0,0xffffffff) Big assumption: xfs_off_t is signed. 0xfffffffe 0xffffefff 0xfffeffff 0xffffffff 0xffffefff 0xffffffff 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. 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... > 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. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs