[PATCH 0/2] Add FALLOC_FL_ZERO_RANGE to fallocate

[PATCH 0/2] Add FALLOC_FL_ZERO_RANGE to fallocate

[Paolo Bonzini]

This patch adds the FALLOC_FL_ZERO_RANGE operation mode to fallocate,
resembling the similar XFS ioctl.  The new mode can be used with
or without FALLOC_FL_KEEP_SIZE, but of course not together with
FALLOC_FL_PUNCH_HOLE.

Other filesystems can then provide the same functionality with a
standard system call.

Paolo
---
 fs/open.c              |    8 +++++++-
 fs/xfs/xfs_file.c      |   34 +++++++++++++++++++++++-----------
 include/linux/falloc.h |    1 +
 3 files changed, 32 insertions(+), 13 deletions(-)

[PATCH 1/2] vfs: add FALLOC_FL_ZERO_RANGE to fallocate

[Paolo Bonzini]

Add a new operation mode to fallocate, called FALLOC_FL_ZERO_RANGE.
It resembles the similarly named XFS ioctl.  Filesystems should
preallocate blocks for regions that span holes in the file, and convert
the entire range to unwritten extents.   This operation is a fast method
of overwriting any from the range specified with zeros without removing
any blocks or having to write zeros to disk.  Any subsequent read in
the given range will return zeros until new data is written.

This functionality requires filesystems to support unwritten extents.
If xfs_info(8) reports unwritten=1, then the filesystem was made to
flag unwritten extents.  It is okay to report EOPNOTSUPP and let the
application deal with the outcome, but it is not okay to succeed or
report EOPNOTSUPP for the same inode depending on the other arguments.

FALLOC_FL_PUNCH_HOLE|FALLOC_FL_ZERO_RANGE is ruled out here, at the
vfs level, rather than leaving it to the filesystems.  This way, in the
future 0x6 could be used as a third mode.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 fs/open.c              |    8 +++++++-
 include/linux/falloc.h |    1 +
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index d6c79a0..dd812b3 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -222,8 +222,14 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (offset < 0 || len <= 0)
 		return -EINVAL;
 
+	/* Punch hole and write-zeroes are mutually exclusive */
+	if ((mode & FALLOC_FL_PUNCH_HOLE) &&
+	    (mode & FALLOC_FL_ZERO_RANGE))
+		return -EINVAL;
+
 	/* Return error if mode is not supported */
-	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
+	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |
+		     FALLOC_FL_ZERO_RANGE))
 		return -EOPNOTSUPP;
 
 	/* Punch hole must have keep size set */
diff --git a/include/linux/falloc.h b/include/linux/falloc.h
index 73e0b62..9aa9599 100644
--- a/include/linux/falloc.h
+++ b/include/linux/falloc.h
@@ -3,6 +3,7 @@
 
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 #define FALLOC_FL_PUNCH_HOLE	0x02 /* de-allocates range */
+#define FALLOC_FL_ZERO_RANGE	0x04 /* write zeroes */
 
 #ifdef __KERNEL__
 
-- 
1.7.1

[PATCH 2/2] xfs: add FALLOC_FL_ZERO_RANGE to fallocate

[Paolo Bonzini]

This is implemented in the same way as the other fallocate modes.  All of
them map to XFS ioctls that are implemented by xfs_change_file_space.

However, we need to cap the length to the inode size if the user requested
FALLOC_FL_KEEP_SIZE.

Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 fs/xfs/xfs_file.c |   36 ++++++++++++++++++++++++------------
 1 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 9f7ec15..c811cf9 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -818,33 +818,45 @@ xfs_file_fallocate(
 	loff_t		new_size = 0;
 	xfs_flock64_t	bf;
 	xfs_inode_t	*ip = XFS_I(inode);
-	int		cmd = XFS_IOC_RESVSP;
+	int		cmd;
 	int		attr_flags = XFS_ATTR_NOLOCK;
 
-	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
+	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |
+		     FALLOC_FL_ZERO_RANGE))
 		return -EOPNOTSUPP;
 
+	BUG_ON((mode & FALLOC_FL_PUNCH_HOLE) && (mode & FALLOC_FL_ZERO_RANGE));
+
 	bf.l_whence = 0;
 	bf.l_start = offset;
 	bf.l_len = len;
 
 	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;
-		error = inode_newsize_ok(inode, new_size);
-		if (error)
-			goto out_unlock;
+	if (offset + len > i_size_read(inode)) {
+		if (mode & FALLOC_FL_KEEP_SIZE) {
+			if (offset > i_size_read(inode))
+				goto out_unlock;
+			len = i_size_read(inode) - offset;
+		} else {
+			/* check validity of new inode size before allocating */
+			new_size = offset + len;
+			error = inode_newsize_ok(inode, new_size);
+			if (error)
+				goto out_unlock;
+		}
 	}
 
 	if (file->f_flags & O_DSYNC)
 		attr_flags |= XFS_ATTR_SYNC;
 
+	if (mode & FALLOC_FL_PUNCH_HOLE)
+		cmd = XFS_IOC_UNRESVSP;
+	else if (mode & FALLOC_FL_ZERO_RANGE)
+		cmd = XFS_IOC_ZERO_RANGE;
+	else
+		cmd = XFS_IOC_RESVSP;
+
 	error = -xfs_change_file_space(ip, cmd, &bf, 0, attr_flags);
 	if (error)
 		goto out_unlock;
-- 
1.7.1

Re: [PATCH 0/2] Add FALLOC_FL_ZERO_RANGE to fallocate

[Dave Chinner]

On Tue, Jun 12, 2012 at 05:36:02PM +0200, Paolo Bonzini wrote:
> This patch adds the FALLOC_FL_ZERO_RANGE operation mode to fallocate,
> resembling the similar XFS ioctl.  The new mode can be used with
> or without FALLOC_FL_KEEP_SIZE, but of course not together with
> FALLOC_FL_PUNCH_HOLE.

!FALLOC_FL_KEEP_SIZE makes no sense for this operation. It is for
zeroing an existing section of a file while retaining the allocated
space, not for extending or truncating the file. It's the same
reason that FALLOC_FL_PUNCH_HOLE must have FALLOC_FL_KEEP_SIZE set.

Also, a minor nit, but you should credit where this code has
originated from in the commit messages, and describe the use case
for requiring it. i.e. based on:

http://permalink.gmane.org/gmane.linux.file-systems/62449

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] vfs: add FALLOC_FL_ZERO_RANGE to fallocate

[Dave Chinner]

On Tue, Jun 12, 2012 at 05:36:03PM +0200, Paolo Bonzini wrote:
> Add a new operation mode to fallocate, called FALLOC_FL_ZERO_RANGE.
> It resembles the similarly named XFS ioctl.  Filesystems should
> preallocate blocks for regions that span holes in the file, and convert
> the entire range to unwritten extents. 

You've described filesystem implementation details, not a
description of the functionality. The functionality is simply that
after this call is made the range of the file specified will return
zeros when read.  You need to write the addition to the fallocate()
man page, and that will help you describe what the API is supposed
to do, not how a filesystem implements it....

FWIW, It is up to the filesytem to optimise this as best they can.
XFS, as you've described, implements with preallocation and
real->unwritten conversion. Other filesystems might simply implement
it as "hole punch + preallocation", for whatever those filesystems
use for that functionality (for some, preallocation means "write
zeros to disk").

> This operation is a fast method
> of overwriting any from the range specified with zeros without removing
> any blocks or having to write zeros to disk.

Well, that is the method XFS uses, but the idea is to avoid that if
they can. i.e. be fast.  Think about that for a moment - if the
range is fragmented or sparse, it may be faster makes sense to punch
out the existing extents and preallocate a single new extent in this
case that to have to allocate multiple (potentially hundreds or
thousands) small extents to fill holes.

Just because I implemented it the easy way in XFS for the person
that requested it (i.e. zeroing preallocated VM images that were
already perfectly laid out) doesn't mean that is the only way it can
be implemented.

> Any subsequent read in the given range will return zeros until new
> data is written.

That should be the second sentence of the commit message.

> This functionality requires filesystems to support unwritten extents.
> If xfs_info(8) reports unwritten=1, then the filesystem was made to
> flag unwritten extents.  It is okay to report EOPNOTSUPP and let the
> application deal with the outcome, but it is not okay to succeed or
> report EOPNOTSUPP for the same inode depending on the other arguments.

I don't think that is true, nor necessary, for the commit message -
filesystems without unwritten extents can implement this quite
easily just by writing zeros to the range just like some do for
preallocation.

> FALLOC_FL_PUNCH_HOLE|FALLOC_FL_ZERO_RANGE is ruled out here, at the
> vfs level, rather than leaving it to the filesystems.  This way, in the
> future 0x6 could be used as a third mode.

We have more than enough feature bits that we don't need to
contemplate mixing various combinations to provide different
features in future.

Besides, a filesystem could interpret that pair as "punch the range,
then preallocate it" rather than "convert to unwritten and hole fill
with preallocation", so I do not see them as mutually exclusive. If
the filesystem wants to treat them that way, then they are welcome
to, but I can definitely see a use case for allowing them both to be
set....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: add FALLOC_FL_ZERO_RANGE to fallocate

[Dave Chinner]

On Tue, Jun 12, 2012 at 05:36:04PM +0200, Paolo Bonzini wrote:
> This is implemented in the same way as the other fallocate modes.  All of
> them map to XFS ioctls that are implemented by xfs_change_file_space.
> 
> However, we need to cap the length to the inode size if the user requested
> FALLOC_FL_KEEP_SIZE.

That's done on purpose.  fallocate() explicitly allows preallocation
beyond EOF and that's what the FALLOC_FL_KEEP_SIZE flag is for - to
allow both offset and offset+len to lie beyond the current inode
size and have the preallocation succeed without changing the file
size.

This is so that we can prevent fragmentation of slow growing
append-only files like log files - we can preallocate way beyond EOF
without changing EOF so as the file grows over days and weeks it
does not fragment.

Similarly, hole punch needs to be able to punch out such
preallocated extents beyond EOF if requested, and it definitely must
not change EOF. So capping/erroring out when offset/offset+len is
definitely the wrong thing to do when FALLOC_FL_KEEP_SIZE is set.

> Cc: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  fs/xfs/xfs_file.c |   36 ++++++++++++++++++++++++------------
>  1 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 9f7ec15..c811cf9 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -818,33 +818,45 @@ xfs_file_fallocate(
>  	loff_t		new_size = 0;
>  	xfs_flock64_t	bf;
>  	xfs_inode_t	*ip = XFS_I(inode);
> -	int		cmd = XFS_IOC_RESVSP;
> +	int		cmd;
>  	int		attr_flags = XFS_ATTR_NOLOCK;
>  
> -	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
> +	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |
> +		     FALLOC_FL_ZERO_RANGE))
>  		return -EOPNOTSUPP;
>  
> +	BUG_ON((mode & FALLOC_FL_PUNCH_HOLE) && (mode & FALLOC_FL_ZERO_RANGE));

Never put BUG_ON() or BUG() in XFS code that can return an error.
Return EINVAL if we chose not to support it, and if it's really
something we consider bad, emit a warning to syslog (i.e.
xfs_warn()) and potentially add a ASSERT() case so that debug
kernels will trip over it. Nobody should be panicing a production
system just because a user supplied a set of incorrect syscall
paramters....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/2] Add FALLOC_FL_ZERO_RANGE to fallocate

[Dave Chinner]

On Wed, Jun 13, 2012 at 11:35:49AM +1000, Dave Chinner wrote:
> On Tue, Jun 12, 2012 at 05:36:02PM +0200, Paolo Bonzini wrote:
> > This patch adds the FALLOC_FL_ZERO_RANGE operation mode to fallocate,
> > resembling the similar XFS ioctl.  The new mode can be used with
> > or without FALLOC_FL_KEEP_SIZE, but of course not together with
> > FALLOC_FL_PUNCH_HOLE.
> 
> !FALLOC_FL_KEEP_SIZE makes no sense for this operation. It is for
> zeroing an existing section of a file while retaining the allocated
> space, not for extending or truncating the file. It's the same
> reason that FALLOC_FL_PUNCH_HOLE must have FALLOC_FL_KEEP_SIZE set.
> 
> Also, a minor nit, but you should credit where this code has
> originated from in the commit messages, and describe the use case
> for requiring it. i.e. based on:
> 
> http://permalink.gmane.org/gmane.linux.file-systems/62449

Oh, and you'll need to provide a new xfstests test based on 242 that
uses the new fallocate interface, and to do that you'll also need to
add support for the new fallocate function in xfs_io.

Adding generic filesystem functionality is not as simple as writing a
kernel patch anymore... :/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/2] Add FALLOC_FL_ZERO_RANGE to fallocate

[Paolo Bonzini]

Il 13/06/2012 05:30, Dave Chinner ha scritto:
>> > 
>> > Also, a minor nit, but you should credit where this code has
>> > originated from in the commit messages, and describe the use case
>> > for requiring it. i.e. based on:
>> > 
>> > http://permalink.gmane.org/gmane.linux.file-systems/62449

Interesting, I didn't know this---I wrote the code from scratch.

I took the description from the man pages ("This operation is a fast
method of overwriting any from the range specified with zeros without
removing any blocks or having to write zeros to disk"), so perhaps those
will have to be patched as well.

> Oh, and you'll need to provide a new xfstests test based on 242 that
> uses the new fallocate interface, and to do that you'll also need to
> add support for the new fallocate function in xfs_io.

Sure.  Thanks for the hints, I'll submit a v2 (may take a while as I'll
leave for holiday soon).

Paolo

Re: [PATCH 2/2] xfs: add FALLOC_FL_ZERO_RANGE to fallocate

[Paolo Bonzini]

Il 13/06/2012 04:16, Dave Chinner ha scritto:
>> > +	BUG_ON((mode & FALLOC_FL_PUNCH_HOLE) && (mode & FALLOC_FL_ZERO_RANGE));
> Never put BUG_ON() or BUG() in XFS code that can return an error.
> Return EINVAL if we chose not to support it, and if it's really
> something we consider bad, emit a warning to syslog (i.e.
> xfs_warn()) and potentially add a ASSERT() case so that debug
> kernels will trip over it. Nobody should be panicing a production
> system just because a user supplied a set of incorrect syscall
> paramters....

I know, the BUG_ON() is because it is ruled out in VFS code.  Of course
if I remove that code, this will not be a BUG_ON() anymore.

Paolo

Re: [PATCH 0/2] Add FALLOC_FL_ZERO_RANGE to fallocate

[Dave Chinner]

On Wed, Jun 13, 2012 at 08:13:29AM +0200, Paolo Bonzini wrote:
> Il 13/06/2012 05:30, Dave Chinner ha scritto:
> >> > 
> >> > Also, a minor nit, but you should credit where this code has
> >> > originated from in the commit messages, and describe the use case
> >> > for requiring it. i.e. based on:
> >> > 
> >> > http://permalink.gmane.org/gmane.linux.file-systems/62449
> 
> Interesting, I didn't know this---I wrote the code from scratch.
> 
> I took the description from the man pages ("This operation is a fast
> method of overwriting any from the range specified with zeros without
> removing any blocks or having to write zeros to disk"), so perhaps those
> will have to be patched as well.

The first line of the man page description is the important one:

$ man 3 xfsctl
.....
XFS_IOC_ZERO_RANGE
      This  command is used to convert a range of a file to zeros
      without issuing data IO. ....

The rest of the description is details about the implementation in
XFS, so users have some idea what to expect in terms or behaviour.
The line that you quoted:

	This operation is a fast method of overwriting any from the
	range specified with zeros without removing any blocks or
	having to write zeros to disk.

describes how the implementation is different from hole punching
(i.e XFS_IOC_UNRESVSP).  All the stuff about unwritten extents is
copied directly from the about XFS_IOC_RESVSP preallocation
description to provide consistent information in the man page.

The XFS_IOC_ZERO_RANGE man page is not a requirements specification
- it's user-level documentation. Yes, it describes the
implementation, but does not convey any of the reasons that the
functionality was implemented that way. Hence using that as the
requirements spec for equivalent fallocate functionality will miss
the mark...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: add FALLOC_FL_ZERO_RANGE to fallocate

[Dave Chinner]

On Wed, Jun 13, 2012 at 08:24:12AM +0200, Paolo Bonzini wrote:
> Il 13/06/2012 04:16, Dave Chinner ha scritto:
> >> > +	BUG_ON((mode & FALLOC_FL_PUNCH_HOLE) && (mode & FALLOC_FL_ZERO_RANGE));
> > Never put BUG_ON() or BUG() in XFS code that can return an error.
> > Return EINVAL if we chose not to support it, and if it's really
> > something we consider bad, emit a warning to syslog (i.e.
> > xfs_warn()) and potentially add a ASSERT() case so that debug
> > kernels will trip over it. Nobody should be panicing a production
> > system just because a user supplied a set of incorrect syscall
> > paramters....
> 
> I know, the BUG_ON() is because it is ruled out in VFS code.  Of course
> if I remove that code, this will not be a BUG_ON() anymore.

If we put a BUG_ON() for every condition the VFS checked in every
filesystem, we'd have so many BUG_ON checks we wouldn't be able to
find the code. If it's banned at the VFS, there's no need to assert
that inthe filesystem code....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/2] Add FALLOC_FL_ZERO_RANGE to fallocate

[Christoph Hellwig]

On Wed, Jun 13, 2012 at 08:13:29AM +0200, Paolo Bonzini wrote:
> Sure.  Thanks for the hints, I'll submit a v2 (may take a while as I'll
> leave for holiday soon).

Did you ever get back to this?

Re: [PATCH 0/2] Add FALLOC_FL_ZERO_RANGE to fallocate

[Paolo Bonzini]

Il 05/11/2013 11:34, Christoph Hellwig ha scritto:
> On Wed, Jun 13, 2012 at 08:13:29AM +0200, Paolo Bonzini wrote:
>> Sure.  Thanks for the hints, I'll submit a v2 (may take a while as I'll
>> leave for holiday soon).
> 
> Did you ever get back to this?

I have some patches for both kernel and xfstests, but I never got round
to testing them properly.  I don't think I'll have time soon to finish
that, but I might coerce someone into doing it.

Paolo

Re: [PATCH 0/2] Add FALLOC_FL_ZERO_RANGE to fallocate

[Christoph Hellwig]

On Tue, Nov 05, 2013 at 11:36:48AM +0100, Paolo Bonzini wrote:
> Il 05/11/2013 11:34, Christoph Hellwig ha scritto:
> > On Wed, Jun 13, 2012 at 08:13:29AM +0200, Paolo Bonzini wrote:
> >> Sure.  Thanks for the hints, I'll submit a v2 (may take a while as I'll
> >> leave for holiday soon).
> > 
> > Did you ever get back to this?
> 
> I have some patches for both kernel and xfstests, but I never got round
> to testing them properly.  I don't think I'll have time soon to finish
> that, but I might coerce someone into doing it.

Feel free to send the current state my way and I'll handle it.