[PATCH] xfsprogs: add fpunch command for hole punching via fallocate

[PATCH] xfsprogs: add fpunch command for hole punching via fallocate

[Josef Bacik]

This adds a fpunch command that Dave Chinner recommended.  It simply uses
fallocate to punch a hole for the given offset and length.  It is necessary to
run the xfstest I have for hole punching.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 io/prealloc.c |   45 +++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/io/prealloc.c b/io/prealloc.c
index c8b7df6..9dcfc1c 100644
--- a/io/prealloc.c
+++ b/io/prealloc.c
@@ -32,6 +32,9 @@ static cmdinfo_t unresvsp_cmd;
 static cmdinfo_t zero_cmd;
 #if defined(HAVE_FALLOCATE)
 static cmdinfo_t falloc_cmd;
+#if defined (FALLOC_FL_PUNCH_HOLE)
+static cmdinfo_t fpunch_cmd;
+#endif
 #endif
 
 static int
@@ -153,8 +156,10 @@ fallocate_f(
 	xfs_flock64_t	segment;
 	int		mode = 0;
 	int		c;
+	const char	*opts;
 
-	while ((c = getopt(argc, argv, "k")) != EOF) {
+	opts = "k";
+	while ((c = getopt(argc, argv, opts)) != EOF) {
 		switch (c) {
 		case 'k':
 			mode = FALLOC_FL_KEEP_SIZE;
@@ -176,7 +181,28 @@ fallocate_f(
 	}
 	return 0;
 }
-#endif
+
+#if defined (FALLOC_FL_PUNCH_HOLE)
+static int
+fpunch_f(
+	 int		argc,
+	 char		**argv)
+{
+	xfs_flock64_t	segment;
+	int		mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
+
+	if (!offset_length(argv[1], argv[2], &segment))
+		return 0;
+
+	if (fallocate(file->fd, mode,
+			segment.l_start, segment.l_len)) {
+		perror("fallocate");
+		return 0;
+	}
+	return 0;
+}
+#endif	/* FALLOC_FL_PUNCH_HOLE */
+#endif	/* HAVE_FALLOCATE */
 
 void
 prealloc_init(void)
@@ -239,7 +265,18 @@ prealloc_init(void)
 	falloc_cmd.args = _("[-k] off len");
 	falloc_cmd.oneline =
 		_("allocates space associated with part of a file via fallocate");
-
 	add_command(&falloc_cmd);
-#endif
+
+#if defined (FALLOC_FL_PUNCH_HOLE)
+	fpunch_cmd.name = _("fpunch");
+	fpunch_cmd.cfunc = fpunch_f;
+	fpunch_cmd.argmin = 2;
+	fpunch_cmd.argmax = -1;
+	fpunch_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	fpunch_cmd.args = _("off len");
+	fpunch_cmd.oneline =
+		_("de-allocates space assocated with part of a file via fallocate");
+	add_command(&fpunch_cmd);
+#endif	/* FALLOC_FL_PUNCH_HOLE */
+#endif	/* HAVE_FALLOCATE */
 }
-- 
1.6.6.1

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

Re: [PATCH] xfsprogs: add fpunch command for hole punching via fallocate

[Christoph Hellwig]

Looks mostly good, but I wonder what the point of a new command for this
is.  It's just one new flag to fallocate, so I'd also implement it as
a flag to the fallocate command.

> @@ -153,8 +156,10 @@ fallocate_f(
>  	xfs_flock64_t	segment;
>  	int		mode = 0;
>  	int		c;
> +	const char	*opts;
>  
> -	while ((c = getopt(argc, argv, "k")) != EOF) {
> +	opts = "k";
> +	while ((c = getopt(argc, argv, opts)) != EOF) {
>  		switch (c) {
>  		case 'k':
>  			mode = FALLOC_FL_KEEP_SIZE;

Why do you change unrelated code?

> +#if defined (FALLOC_FL_PUNCH_HOLE)

I'd rather have a

#ifndef FALLOC_FL_PUNCH_HOLE
#define FALLOC_FL_PUNCH_HOLE	0x02
#endif

to avoid requiring newest kernel headers.

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

Re: [PATCH] xfsprogs: add fpunch command for hole punching via fallocate

[Josef Bacik]

On Tue, Jan 18, 2011 at 07:51:12AM -0500, Christoph Hellwig wrote:
> Looks mostly good, but I wonder what the point of a new command for this
> is.  It's just one new flag to fallocate, so I'd also implement it as
> a flag to the fallocate command.
>

:( thats what I did to begin with, and Dave said he'd rather have a seperate
command.
 
> > @@ -153,8 +156,10 @@ fallocate_f(
> >  	xfs_flock64_t	segment;
> >  	int		mode = 0;
> >  	int		c;
> > +	const char	*opts;
> >  
> > -	while ((c = getopt(argc, argv, "k")) != EOF) {
> > +	opts = "k";
> > +	while ((c = getopt(argc, argv, opts)) != EOF) {
> >  		switch (c) {
> >  		case 'k':
> >  			mode = FALLOC_FL_KEEP_SIZE;
> 
> Why do you change unrelated code?
> 

Crap sorry this is left over from my original patch.

> > +#if defined (FALLOC_FL_PUNCH_HOLE)
> 
> I'd rather have a
> 
> #ifndef FALLOC_FL_PUNCH_HOLE
> #define FALLOC_FL_PUNCH_HOLE	0x02
> #endif
> 
> to avoid requiring newest kernel headers.
>

Sounds good.  So which do we want, a new command or a new flag?  Thanks,

Josef 

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

Re: [PATCH] xfsprogs: add fpunch command for hole punching via fallocate

[Christoph Hellwig]

On Tue, Jan 18, 2011 at 08:06:03AM -0500, Josef Bacik wrote:
> Sounds good.  So which do we want, a new command or a new flag?  Thanks,

I'll wait for dave to chime in.  I think we should absolutely expose
it as a fallocate flag, but if there's a good reason we can also expose
it as a separate command.

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

Re: [PATCH] xfsprogs: add fpunch command for hole punching via fallocate

[Dave Chinner]

On Tue, Jan 18, 2011 at 08:12:03AM -0500, Christoph Hellwig wrote:
> On Tue, Jan 18, 2011 at 08:06:03AM -0500, Josef Bacik wrote:
> > Sounds good.  So which do we want, a new command or a new flag?  Thanks,
> 
> I'll wait for dave to chime in.  I think we should absolutely expose
> it as a fallocate flag, but if there's a good reason we can also expose
> it as a separate command.

My reasoning was that:

	a) it is consistent with other xfs_io allocation manipulation
	   command structures such as resvsp/unresvsp
	b) "punch" is less to type than "fallocate -p"
	c) self documenting in scripts e.g. -c "punch 4k 4k" is much
	   more obvious than -c "fallocate -p 4k 4k" and saves a man
	   page lookup when reading the script.
	d) punch as a top level command will show up in the "xfs_io
	   -c help", not require you to know it is a suboption of the
	   "falloc" command to find out how to use it.
	e) the xfs_io command does not have to have the same name
	   and structure as the underlying API that implements the
	   functionality the commands execute.


Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfsprogs: add fpunch command for hole punching via fallocate

[Christoph Hellwig]

On Wed, Jan 19, 2011 at 08:23:03AM +1100, Dave Chinner wrote:
> 	a) it is consistent with other xfs_io allocation manipulation
> 	   command structures such as resvsp/unresvsp

These are all different ioctls.

> 	b) "punch" is less to type than "fallocate -p"
> 	c) self documenting in scripts e.g. -c "punch 4k 4k" is much
> 	   more obvious than -c "fallocate -p 4k 4k" and saves a man
> 	   page lookup when reading the script.
> 	d) punch as a top level command will show up in the "xfs_io
> 	   -c help", not require you to know it is a suboption of the
> 	   "falloc" command to find out how to use it.
> 	e) the xfs_io command does not have to have the same name
> 	   and structure as the underlying API that implements the
> 	   functionality the commands execute.

I still don't like this as a reason to duplicate the code, and not
having the different arguments for fallocate exposed similar to the
syscall level.

What do you think about introducing a concept of aliases in xfs_io
so that we can have a toplevel punch command that just gets aliased
to fallocate -p without having to reimplement it?

I'd take Josef's older falocate -p implementation and will add the alias
support myself.

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