Re: [PATCH 2/6] XFS: handle hole punching via fallocate properly

Dave Chinner
2010-11-09T01:21:57Z

On Mon, Nov 08, 2010 at 03:32:03PM -0500, Josef Bacik wrote:
> This patch simply allows XFS to handle the hole punching flag in fallocate
> properly. I've tested this with a little program that does a bunch of random
> hole punching with FL_KEEP_SIZE and without it to make sure it does the right
> thing. Thanks,
>
> Signed-off-by: Josef Bacik <josef@redhat.com>
> ---
> fs/xfs/linux-2.6/xfs_iops.c | 12 +++++++++---
> 1 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/linux-2.6/xfs_iops.c b/fs/xfs/linux-2.6/xfs_iops.c
> index 96107ef..99df347 100644
> --- a/fs/xfs/linux-2.6/xfs_iops.c
> +++ b/fs/xfs/linux-2.6/xfs_iops.c
> @@ -516,6 +516,7 @@ xfs_vn_fallocate(
> loff_t new_size = 0;
> xfs_flock64_t bf;
> xfs_inode_t *ip = XFS_I(inode);
> + int cmd = XFS_IOC_RESVSP;
>
> /* preallocation on directories not yet supported */
> error = -ENODEV;
> @@ -528,17 +529,22 @@ xfs_vn_fallocate(
>
> xfs_ilock(ip, XFS_IOLOCK_EXCL);
>
> + if (mode & FALLOC_FL_PUNCH_HOLE)
> + cmd = XFS_IOC_UNRESVSP;
> +
> /* check the new inode size is valid before allocating */
> if (!(mode & FALLOC_FL_KEEP_SIZE) &&
> offset + len > i_size_read(inode)) {
> - new_size = offset + len;
> + if (cmd == XFS_IOC_UNRESVSP)
> + new_size = offset;
> + else
> + new_size = offset + len;

What semantic is FALLOC_FL_KEEP_SIZE supposed to convey during a
hole punch? Doesn't this just turn the hole punch operation into
a truncate? If so, what's the point of punching the hole when you
can just call ftruncate() to get the same result?

I'd suggest that FALLOC_FL_PUNCH_HOLE should, by definition, not
change the file size, and have no option to be able to change it.
This needs to be defined and documented - can you include a man
page update in this series that defines the expected behaviour
of FALLOC_FL_PUNCH_HOLE?

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com