[PATCH] xfs_io: support a basic extent swap command

[PATCH] xfs_io: support a basic extent swap command

[Brian Foster]

Extent swap is a low level mechanism exported by XFS to facilitate
filesystem defragmentation. It is typically invoked by xfs_fsr under
conditions that will atomically adjust inode extent state without
loss of file data.

xfs_fsr does not provide the necessary controls for low-level
testing of the extent swap command, however. For example, xfs_fsr
implements policies that dictate when to perform an extent swap and
offers no control over the donor file characteristics.

Add a basic swapext command to xfs_io that allows userspace
invocation of the command under more controlled conditions. This
command makes no effort to retain data across the operation. As
such, it is primarily for testing purposes (i.e., in support of
xfstests, etc.).

Signed-off-by: Brian Foster <bfoster@redhat.com>
---

This command is in support of an xfstests test to be posted shortly...

Brian

 io/Makefile  |   3 +-
 io/init.c    |   1 +
 io/io.h      |   1 +
 io/swapext.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+), 1 deletion(-)
 create mode 100644 io/swapext.c

diff --git a/io/Makefile b/io/Makefile
index 6725936d..2f9249e8 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -11,7 +11,8 @@ HFILES = init.h io.h
 CFILES = init.c \
 	attr.c bmap.c cowextsize.c encrypt.c file.c freeze.c fsync.c \
 	getrusage.c imap.c link.c mmap.c open.c parent.c pread.c prealloc.c \
-	pwrite.c reflink.c seek.c shutdown.c stat.c sync.c truncate.c utimes.c
+	pwrite.c reflink.c seek.c shutdown.c stat.c swapext.c sync.c \
+	truncate.c utimes.c
 
 LLDLIBS = $(LIBXCMD) $(LIBHANDLE) $(LIBPTHREAD)
 LTDEPENDENCIES = $(LIBXCMD) $(LIBHANDLE)
diff --git a/io/init.c b/io/init.c
index 20d5f80d..2ade03f4 100644
--- a/io/init.c
+++ b/io/init.c
@@ -88,6 +88,7 @@ init_commands(void)
 	sendfile_init();
 	shutdown_init();
 	stat_init();
+	swapext_init();
 	sync_init();
 	sync_range_init();
 	truncate_init();
diff --git a/io/io.h b/io/io.h
index 3862985f..258311f1 100644
--- a/io/io.h
+++ b/io/io.h
@@ -118,6 +118,7 @@ extern void		quit_init(void);
 extern void		seek_init(void);
 extern void		shutdown_init(void);
 extern void		stat_init(void);
+extern void		swapext_init(void);
 extern void		sync_init(void);
 extern void		truncate_init(void);
 extern void		utimes_init(void);
diff --git a/io/swapext.c b/io/swapext.c
new file mode 100644
index 00000000..5e161d69
--- /dev/null
+++ b/io/swapext.c
@@ -0,0 +1,107 @@
+/*
+ * Copyright (c) 2018 Red Hat, Inc.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include "command.h"
+#include "input.h"
+#include "init.h"
+#include "io.h"
+
+static cmdinfo_t swapext_cmd;
+
+static void
+swapext_help(void)
+{
+	printf(_(
+"\n"
+" Swaps extents between the open file descriptor and the supplied filename.\n"
+"\n"));
+}
+
+static int
+xfs_bulkstat_single(
+	int			fd,
+	xfs_ino_t		*lastip,
+	struct xfs_bstat	*ubuffer)
+{
+	struct xfs_fsop_bulkreq	bulkreq;
+
+	bulkreq.lastip = (__u64 *)lastip;
+	bulkreq.icount = 1;
+	bulkreq.ubuffer = ubuffer;
+	bulkreq.ocount = NULL;
+	return ioctl(fd, XFS_IOC_FSBULKSTAT_SINGLE, &bulkreq);
+}
+
+static int
+swapext_f(
+	int			argc,
+	char			**argv)
+{
+	int			fd;
+	int			error;
+	struct xfs_swapext	sx;
+	struct stat		stat;
+
+	/* open the donor file */
+	fd = openfile(argv[1], NULL, 0, 0, NULL);
+	if (fd < 0)
+		return 0;
+
+	/*
+	 * stat the target file to get the inode number and use the latter to
+	 * get the bulkstat info for the swapext cmd.
+	 */
+	error = fstat(file->fd, &stat);
+	if (error) {
+		perror("fstat");
+		goto out;
+	}
+
+	error = xfs_bulkstat_single(file->fd, &stat.st_ino, &sx.sx_stat);
+	if (error) {
+		perror("bulkstat");
+		goto out;
+	}
+	sx.sx_version = XFS_SX_VERSION;
+	sx.sx_fdtarget = file->fd;
+	sx.sx_fdtmp = fd;
+	sx.sx_offset = 0;
+	sx.sx_length = stat.st_size;
+	error = ioctl(file->fd, XFS_IOC_SWAPEXT, &sx);
+	if (error)
+		perror("swapext");
+
+out:
+	close(fd);
+	return 0;
+}
+
+void
+swapext_init(void)
+{
+	swapext_cmd.name = "swapext";
+	swapext_cmd.cfunc = swapext_f;
+	swapext_cmd.argmin = 1;
+	swapext_cmd.argmax = 1;
+	swapext_cmd.flags = CMD_NOMAP_OK;
+	swapext_cmd.args = _("<donorfile>");
+	swapext_cmd.oneline = _("Swap extents between files.");
+	swapext_cmd.help = swapext_help;
+
+	add_command(&swapext_cmd);
+}
-- 
2.13.6

Re: [PATCH] xfs_io: support a basic extent swap command

[Darrick J. Wong]

On Tue, Feb 06, 2018 at 08:02:47AM -0500, Brian Foster wrote:
> Extent swap is a low level mechanism exported by XFS to facilitate
> filesystem defragmentation. It is typically invoked by xfs_fsr under
> conditions that will atomically adjust inode extent state without
> loss of file data.
> 
> xfs_fsr does not provide the necessary controls for low-level
> testing of the extent swap command, however. For example, xfs_fsr
> implements policies that dictate when to perform an extent swap and
> offers no control over the donor file characteristics.
> 
> Add a basic swapext command to xfs_io that allows userspace
> invocation of the command under more controlled conditions. This
> command makes no effort to retain data across the operation. As
> such, it is primarily for testing purposes (i.e., in support of
> xfstests, etc.).
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> This command is in support of an xfstests test to be posted shortly...
> 
> Brian
> 
>  io/Makefile  |   3 +-
>  io/init.c    |   1 +
>  io/io.h      |   1 +
>  io/swapext.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 111 insertions(+), 1 deletion(-)
>  create mode 100644 io/swapext.c
> 
> diff --git a/io/Makefile b/io/Makefile
> index 6725936d..2f9249e8 100644
> --- a/io/Makefile
> +++ b/io/Makefile
> @@ -11,7 +11,8 @@ HFILES = init.h io.h
>  CFILES = init.c \
>  	attr.c bmap.c cowextsize.c encrypt.c file.c freeze.c fsync.c \
>  	getrusage.c imap.c link.c mmap.c open.c parent.c pread.c prealloc.c \
> -	pwrite.c reflink.c seek.c shutdown.c stat.c sync.c truncate.c utimes.c
> +	pwrite.c reflink.c seek.c shutdown.c stat.c swapext.c sync.c \
> +	truncate.c utimes.c
>  
>  LLDLIBS = $(LIBXCMD) $(LIBHANDLE) $(LIBPTHREAD)
>  LTDEPENDENCIES = $(LIBXCMD) $(LIBHANDLE)
> diff --git a/io/init.c b/io/init.c
> index 20d5f80d..2ade03f4 100644
> --- a/io/init.c
> +++ b/io/init.c
> @@ -88,6 +88,7 @@ init_commands(void)
>  	sendfile_init();
>  	shutdown_init();
>  	stat_init();
> +	swapext_init();
>  	sync_init();
>  	sync_range_init();
>  	truncate_init();
> diff --git a/io/io.h b/io/io.h
> index 3862985f..258311f1 100644
> --- a/io/io.h
> +++ b/io/io.h
> @@ -118,6 +118,7 @@ extern void		quit_init(void);
>  extern void		seek_init(void);
>  extern void		shutdown_init(void);
>  extern void		stat_init(void);
> +extern void		swapext_init(void);
>  extern void		sync_init(void);
>  extern void		truncate_init(void);
>  extern void		utimes_init(void);
> diff --git a/io/swapext.c b/io/swapext.c
> new file mode 100644
> index 00000000..5e161d69
> --- /dev/null
> +++ b/io/swapext.c
> @@ -0,0 +1,107 @@
> +/*
> + * Copyright (c) 2018 Red Hat, Inc.
> + * All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation,
> + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#include "command.h"
> +#include "input.h"
> +#include "init.h"
> +#include "io.h"
> +
> +static cmdinfo_t swapext_cmd;
> +
> +static void
> +swapext_help(void)
> +{
> +	printf(_(
> +"\n"
> +" Swaps extents between the open file descriptor and the supplied filename.\n"

An update to the xfs_io manpage is needed, but this otherwise looks ok to me.

--D

> +"\n"));
> +}
> +
> +static int
> +xfs_bulkstat_single(
> +	int			fd,
> +	xfs_ino_t		*lastip,
> +	struct xfs_bstat	*ubuffer)
> +{
> +	struct xfs_fsop_bulkreq	bulkreq;
> +
> +	bulkreq.lastip = (__u64 *)lastip;
> +	bulkreq.icount = 1;
> +	bulkreq.ubuffer = ubuffer;
> +	bulkreq.ocount = NULL;
> +	return ioctl(fd, XFS_IOC_FSBULKSTAT_SINGLE, &bulkreq);
> +}
> +
> +static int
> +swapext_f(
> +	int			argc,
> +	char			**argv)
> +{
> +	int			fd;
> +	int			error;
> +	struct xfs_swapext	sx;
> +	struct stat		stat;
> +
> +	/* open the donor file */
> +	fd = openfile(argv[1], NULL, 0, 0, NULL);
> +	if (fd < 0)
> +		return 0;
> +
> +	/*
> +	 * stat the target file to get the inode number and use the latter to
> +	 * get the bulkstat info for the swapext cmd.
> +	 */
> +	error = fstat(file->fd, &stat);
> +	if (error) {
> +		perror("fstat");
> +		goto out;
> +	}
> +
> +	error = xfs_bulkstat_single(file->fd, &stat.st_ino, &sx.sx_stat);
> +	if (error) {
> +		perror("bulkstat");
> +		goto out;
> +	}
> +	sx.sx_version = XFS_SX_VERSION;
> +	sx.sx_fdtarget = file->fd;
> +	sx.sx_fdtmp = fd;
> +	sx.sx_offset = 0;
> +	sx.sx_length = stat.st_size;
> +	error = ioctl(file->fd, XFS_IOC_SWAPEXT, &sx);
> +	if (error)
> +		perror("swapext");
> +
> +out:
> +	close(fd);
> +	return 0;
> +}
> +
> +void
> +swapext_init(void)
> +{
> +	swapext_cmd.name = "swapext";
> +	swapext_cmd.cfunc = swapext_f;
> +	swapext_cmd.argmin = 1;
> +	swapext_cmd.argmax = 1;
> +	swapext_cmd.flags = CMD_NOMAP_OK;
> +	swapext_cmd.args = _("<donorfile>");
> +	swapext_cmd.oneline = _("Swap extents between files.");
> +	swapext_cmd.help = swapext_help;
> +
> +	add_command(&swapext_cmd);
> +}
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs_io: support a basic extent swap command

[Brian Foster]

On Tue, Feb 06, 2018 at 09:32:13AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 06, 2018 at 08:02:47AM -0500, Brian Foster wrote:
> > Extent swap is a low level mechanism exported by XFS to facilitate
> > filesystem defragmentation. It is typically invoked by xfs_fsr under
> > conditions that will atomically adjust inode extent state without
> > loss of file data.
> > 
> > xfs_fsr does not provide the necessary controls for low-level
> > testing of the extent swap command, however. For example, xfs_fsr
> > implements policies that dictate when to perform an extent swap and
> > offers no control over the donor file characteristics.
> > 
> > Add a basic swapext command to xfs_io that allows userspace
> > invocation of the command under more controlled conditions. This
> > command makes no effort to retain data across the operation. As
> > such, it is primarily for testing purposes (i.e., in support of
> > xfstests, etc.).
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > 
> > This command is in support of an xfstests test to be posted shortly...
> > 
> > Brian
> > 
> >  io/Makefile  |   3 +-
> >  io/init.c    |   1 +
> >  io/io.h      |   1 +
> >  io/swapext.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 111 insertions(+), 1 deletion(-)
> >  create mode 100644 io/swapext.c
> > 
> > diff --git a/io/Makefile b/io/Makefile
> > index 6725936d..2f9249e8 100644
> > --- a/io/Makefile
> > +++ b/io/Makefile
> > @@ -11,7 +11,8 @@ HFILES = init.h io.h
> >  CFILES = init.c \
> >  	attr.c bmap.c cowextsize.c encrypt.c file.c freeze.c fsync.c \
> >  	getrusage.c imap.c link.c mmap.c open.c parent.c pread.c prealloc.c \
> > -	pwrite.c reflink.c seek.c shutdown.c stat.c sync.c truncate.c utimes.c
> > +	pwrite.c reflink.c seek.c shutdown.c stat.c swapext.c sync.c \
> > +	truncate.c utimes.c
> >  
> >  LLDLIBS = $(LIBXCMD) $(LIBHANDLE) $(LIBPTHREAD)
> >  LTDEPENDENCIES = $(LIBXCMD) $(LIBHANDLE)
> > diff --git a/io/init.c b/io/init.c
> > index 20d5f80d..2ade03f4 100644
> > --- a/io/init.c
> > +++ b/io/init.c
> > @@ -88,6 +88,7 @@ init_commands(void)
> >  	sendfile_init();
> >  	shutdown_init();
> >  	stat_init();
> > +	swapext_init();
> >  	sync_init();
> >  	sync_range_init();
> >  	truncate_init();
> > diff --git a/io/io.h b/io/io.h
> > index 3862985f..258311f1 100644
> > --- a/io/io.h
> > +++ b/io/io.h
> > @@ -118,6 +118,7 @@ extern void		quit_init(void);
> >  extern void		seek_init(void);
> >  extern void		shutdown_init(void);
> >  extern void		stat_init(void);
> > +extern void		swapext_init(void);
> >  extern void		sync_init(void);
> >  extern void		truncate_init(void);
> >  extern void		utimes_init(void);
> > diff --git a/io/swapext.c b/io/swapext.c
> > new file mode 100644
> > index 00000000..5e161d69
> > --- /dev/null
> > +++ b/io/swapext.c
> > @@ -0,0 +1,107 @@
> > +/*
> > + * Copyright (c) 2018 Red Hat, Inc.
> > + * All Rights Reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it would be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write the Free Software Foundation,
> > + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > + */
> > +
> > +#include "command.h"
> > +#include "input.h"
> > +#include "init.h"
> > +#include "io.h"
> > +
> > +static cmdinfo_t swapext_cmd;
> > +
> > +static void
> > +swapext_help(void)
> > +{
> > +	printf(_(
> > +"\n"
> > +" Swaps extents between the open file descriptor and the supplied filename.\n"
> 
> An update to the xfs_io manpage is needed, but this otherwise looks ok to me.
> 

Ok, will fix..

Brian

> --D
> 
> > +"\n"));
> > +}
> > +
> > +static int
> > +xfs_bulkstat_single(
> > +	int			fd,
> > +	xfs_ino_t		*lastip,
> > +	struct xfs_bstat	*ubuffer)
> > +{
> > +	struct xfs_fsop_bulkreq	bulkreq;
> > +
> > +	bulkreq.lastip = (__u64 *)lastip;
> > +	bulkreq.icount = 1;
> > +	bulkreq.ubuffer = ubuffer;
> > +	bulkreq.ocount = NULL;
> > +	return ioctl(fd, XFS_IOC_FSBULKSTAT_SINGLE, &bulkreq);
> > +}
> > +
> > +static int
> > +swapext_f(
> > +	int			argc,
> > +	char			**argv)
> > +{
> > +	int			fd;
> > +	int			error;
> > +	struct xfs_swapext	sx;
> > +	struct stat		stat;
> > +
> > +	/* open the donor file */
> > +	fd = openfile(argv[1], NULL, 0, 0, NULL);
> > +	if (fd < 0)
> > +		return 0;
> > +
> > +	/*
> > +	 * stat the target file to get the inode number and use the latter to
> > +	 * get the bulkstat info for the swapext cmd.
> > +	 */
> > +	error = fstat(file->fd, &stat);
> > +	if (error) {
> > +		perror("fstat");
> > +		goto out;
> > +	}
> > +
> > +	error = xfs_bulkstat_single(file->fd, &stat.st_ino, &sx.sx_stat);
> > +	if (error) {
> > +		perror("bulkstat");
> > +		goto out;
> > +	}
> > +	sx.sx_version = XFS_SX_VERSION;
> > +	sx.sx_fdtarget = file->fd;
> > +	sx.sx_fdtmp = fd;
> > +	sx.sx_offset = 0;
> > +	sx.sx_length = stat.st_size;
> > +	error = ioctl(file->fd, XFS_IOC_SWAPEXT, &sx);
> > +	if (error)
> > +		perror("swapext");
> > +
> > +out:
> > +	close(fd);
> > +	return 0;
> > +}
> > +
> > +void
> > +swapext_init(void)
> > +{
> > +	swapext_cmd.name = "swapext";
> > +	swapext_cmd.cfunc = swapext_f;
> > +	swapext_cmd.argmin = 1;
> > +	swapext_cmd.argmax = 1;
> > +	swapext_cmd.flags = CMD_NOMAP_OK;
> > +	swapext_cmd.args = _("<donorfile>");
> > +	swapext_cmd.oneline = _("Swap extents between files.");
> > +	swapext_cmd.help = swapext_help;
> > +
> > +	add_command(&swapext_cmd);
> > +}
> > -- 
> > 2.13.6
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs_io: support a basic extent swap command

[Dave Chinner]

On Tue, Feb 06, 2018 at 08:02:47AM -0500, Brian Foster wrote:
> Extent swap is a low level mechanism exported by XFS to facilitate
> filesystem defragmentation. It is typically invoked by xfs_fsr under
> conditions that will atomically adjust inode extent state without
> loss of file data.
> 
> xfs_fsr does not provide the necessary controls for low-level
> testing of the extent swap command, however. For example, xfs_fsr
> implements policies that dictate when to perform an extent swap and
> offers no control over the donor file characteristics.

Hmmmm. I'm confused - we already use xfs_fsr for low level testing
of swapext functionality with carefully constructed donor file
characteristics in xfs/227.

What am I missing here?

I don't see any problem with adding a specific command to run
more controlled swapext tests, i just want to make sure I understand
what xfs_fsr is not providing you with.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs_io: support a basic extent swap command

[Brian Foster]

On Wed, Feb 07, 2018 at 12:34:41PM +1100, Dave Chinner wrote:
> On Tue, Feb 06, 2018 at 08:02:47AM -0500, Brian Foster wrote:
> > Extent swap is a low level mechanism exported by XFS to facilitate
> > filesystem defragmentation. It is typically invoked by xfs_fsr under
> > conditions that will atomically adjust inode extent state without
> > loss of file data.
> > 
> > xfs_fsr does not provide the necessary controls for low-level
> > testing of the extent swap command, however. For example, xfs_fsr
> > implements policies that dictate when to perform an extent swap and
> > offers no control over the donor file characteristics.
> 
> Hmmmm. I'm confused - we already use xfs_fsr for low level testing
> of swapext functionality with carefully constructed donor file
> characteristics in xfs/227.
> 
> What am I missing here?
> 

AFAICT, xfs/227 does so by formatting the filesystem in a particular way
and running fsr over it, so it wasn't immediately clear to me what
you're referring to. Staring at it some more, it appears to be using the
-C debug option which looks like it forces an extent count..? If that's
the case, the test[1] that was posted to use this feature could probably
be modified to do a similar thing via xfs_fsr.

Giving it a quick test, it doesn't appear to do exactly what I want with
larger extent counts, however. It seems to work with a few extents or
so, then doesn't ever create more than 4 or 5 if that's what I ask for
with the donor file. Hmm, I wonder if prealloc or something is getting
in the way of whatever it's attempting to do. If I try a similar test
using falloc instead of pwrite, it doesn't do the swap at all. :/

Note that xfs/227 uses counts 5-15, so that also makes me wonder whether
that test is actually doing what is expected, but that's a separate
question (and I haven't dug into any of this deeply enough). I could
just be misunderstanding the option.

> I don't see any problem with adding a specific command to run
> more controlled swapext tests, i just want to make sure I understand
> what xfs_fsr is not providing you with.
> 

I wasn't aware of 'xfs_fsr -C', thanks for pointing it out. I just
looked at the fsr help/manpage to see if there was any "input file"
capability and missed it. Having given it a quick test, I still prefer
the existing test because it is explicit and doesn't have to copy data
or anything like that. IOW, the -C thing to me seems like an fsr unit
test whereas what I really want is a swapext unit test.

FWIW, I do already have a variant of the test that reproduces the
problem in question using xfs_fsr (without -C). The donor file extent
count thing isn't actually a hard requirement to reproduce, just an
advantage I was trying to document in the commit log for this patch.
With respect to the test, the predictable extent count thing is more for
efficiency purposes because it allows the test to run having only done a
one-time set up of two files (the rest is hole punching and extent
swaps). So I had already abandoned using xfs_fsr in favor of the current
test for similar reasons: it was significantly slower due to the
additional work required to set up the fs, unnecessary file data copying
involved, etc.

So unless there's any objections, I can try to reword the commit log for
this patch to make the advantages/differences more clear.

Brian

[1] https://marc.info/?l=fstests&m=151792263511472&w=2

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs_io: support a basic extent swap command

[Dave Chinner]

On Wed, Feb 07, 2018 at 09:47:12AM -0500, Brian Foster wrote:
> On Wed, Feb 07, 2018 at 12:34:41PM +1100, Dave Chinner wrote:
> > On Tue, Feb 06, 2018 at 08:02:47AM -0500, Brian Foster wrote:
> > > Extent swap is a low level mechanism exported by XFS to facilitate
> > > filesystem defragmentation. It is typically invoked by xfs_fsr under
> > > conditions that will atomically adjust inode extent state without
> > > loss of file data.
> > > 
> > > xfs_fsr does not provide the necessary controls for low-level
> > > testing of the extent swap command, however. For example, xfs_fsr
> > > implements policies that dictate when to perform an extent swap and
> > > offers no control over the donor file characteristics.
> > 
> > Hmmmm. I'm confused - we already use xfs_fsr for low level testing
> > of swapext functionality with carefully constructed donor file
> > characteristics in xfs/227.
> > 
> > What am I missing here?
> > 
> 
> AFAICT, xfs/227 does so by formatting the filesystem in a particular way
> and running fsr over it, so it wasn't immediately clear to me what
> you're referring to. Staring at it some more, it appears to be using the
> -C debug option which looks like it forces an extent count..? If that's
> the case, the test[1] that was posted to use this feature could probably
> be modified to do a similar thing via xfs_fsr.
> 
> Giving it a quick test, it doesn't appear to do exactly what I want with
> larger extent counts, however. It seems to work with a few extents or
> so, then doesn't ever create more than 4 or 5 if that's what I ask for
> with the donor file. Hmm, I wonder if prealloc or something is getting
> in the way of whatever it's attempting to do. If I try a similar test
> using falloc instead of pwrite, it doesn't do the swap at all. :/

Yeah, that's because of the way xfs_fsr preserves unwritten extents
across defragmentation. i.e. it assumes unwritten extents are a
result of application level preallocation and so it's there
intentionally and be preserved.

When I wrote this yesterday, I hadn't see the test you'd posted to
the fstests list so I didn't have that context. Now that I've seen
it, and you've explained the problems, I can see why you wrote the
simpler xfs_io test command....

[....]

> So unless there's any objections, I can try to reword the commit log for
> this patch to make the advantages/differences more clear.

Yeah, I think that's all that is necessary - just expand on
"xfs_fsr does not provide the necessary controls" a bit so I don't
have to rely on my brain to remember this conversation. :P

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs_io: support a basic extent swap command

[Darrick J. Wong]

On Thu, Feb 08, 2018 at 10:02:17AM +1100, Dave Chinner wrote:
> On Wed, Feb 07, 2018 at 09:47:12AM -0500, Brian Foster wrote:
> > On Wed, Feb 07, 2018 at 12:34:41PM +1100, Dave Chinner wrote:
> > > On Tue, Feb 06, 2018 at 08:02:47AM -0500, Brian Foster wrote:
> > > > Extent swap is a low level mechanism exported by XFS to facilitate
> > > > filesystem defragmentation. It is typically invoked by xfs_fsr under
> > > > conditions that will atomically adjust inode extent state without
> > > > loss of file data.
> > > > 
> > > > xfs_fsr does not provide the necessary controls for low-level
> > > > testing of the extent swap command, however. For example, xfs_fsr
> > > > implements policies that dictate when to perform an extent swap and
> > > > offers no control over the donor file characteristics.
> > > 
> > > Hmmmm. I'm confused - we already use xfs_fsr for low level testing
> > > of swapext functionality with carefully constructed donor file
> > > characteristics in xfs/227.
> > > 
> > > What am I missing here?
> > > 
> > 
> > AFAICT, xfs/227 does so by formatting the filesystem in a particular way
> > and running fsr over it, so it wasn't immediately clear to me what
> > you're referring to. Staring at it some more, it appears to be using the
> > -C debug option which looks like it forces an extent count..? If that's
> > the case, the test[1] that was posted to use this feature could probably
> > be modified to do a similar thing via xfs_fsr.
> > 
> > Giving it a quick test, it doesn't appear to do exactly what I want with
> > larger extent counts, however. It seems to work with a few extents or
> > so, then doesn't ever create more than 4 or 5 if that's what I ask for
> > with the donor file. Hmm, I wonder if prealloc or something is getting
> > in the way of whatever it's attempting to do. If I try a similar test
> > using falloc instead of pwrite, it doesn't do the swap at all. :/
> 
> Yeah, that's because of the way xfs_fsr preserves unwritten extents
> across defragmentation. i.e. it assumes unwritten extents are a
> result of application level preallocation and so it's there
> intentionally and be preserved.
> 
> When I wrote this yesterday, I hadn't see the test you'd posted to
> the fstests list so I didn't have that context. Now that I've seen
> it, and you've explained the problems, I can see why you wrote the
> simpler xfs_io test command....
> 
> [....]
> 
> > So unless there's any objections, I can try to reword the commit log for
> > this patch to make the advantages/differences more clear.
> 
> Yeah, I think that's all that is necessary - just expand on
> "xfs_fsr does not provide the necessary controls" a bit so I don't
> have to rely on my brain to remember this conversation. :P

<shrug> I had figured xfs_fsr == "polished user program not to be
cluttered up with weird xfstests warts" and xfs_io == "warty program we
use to make raw calls for testing" so except for tests that check the
behavior of xfs_fsr itself, there ought to be a raw interface via xfs_io
that regression tests use directly. :)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs_io: support a basic extent swap command

[Brian Foster]

On Wed, Feb 07, 2018 at 03:23:19PM -0800, Darrick J. Wong wrote:
> On Thu, Feb 08, 2018 at 10:02:17AM +1100, Dave Chinner wrote:
> > On Wed, Feb 07, 2018 at 09:47:12AM -0500, Brian Foster wrote:
> > > On Wed, Feb 07, 2018 at 12:34:41PM +1100, Dave Chinner wrote:
> > > > On Tue, Feb 06, 2018 at 08:02:47AM -0500, Brian Foster wrote:
> > > > > Extent swap is a low level mechanism exported by XFS to facilitate
> > > > > filesystem defragmentation. It is typically invoked by xfs_fsr under
> > > > > conditions that will atomically adjust inode extent state without
> > > > > loss of file data.
> > > > > 
> > > > > xfs_fsr does not provide the necessary controls for low-level
> > > > > testing of the extent swap command, however. For example, xfs_fsr
> > > > > implements policies that dictate when to perform an extent swap and
> > > > > offers no control over the donor file characteristics.
> > > > 
> > > > Hmmmm. I'm confused - we already use xfs_fsr for low level testing
> > > > of swapext functionality with carefully constructed donor file
> > > > characteristics in xfs/227.
> > > > 
> > > > What am I missing here?
> > > > 
> > > 
> > > AFAICT, xfs/227 does so by formatting the filesystem in a particular way
> > > and running fsr over it, so it wasn't immediately clear to me what
> > > you're referring to. Staring at it some more, it appears to be using the
> > > -C debug option which looks like it forces an extent count..? If that's
> > > the case, the test[1] that was posted to use this feature could probably
> > > be modified to do a similar thing via xfs_fsr.
> > > 
> > > Giving it a quick test, it doesn't appear to do exactly what I want with
> > > larger extent counts, however. It seems to work with a few extents or
> > > so, then doesn't ever create more than 4 or 5 if that's what I ask for
> > > with the donor file. Hmm, I wonder if prealloc or something is getting
> > > in the way of whatever it's attempting to do. If I try a similar test
> > > using falloc instead of pwrite, it doesn't do the swap at all. :/
> > 
> > Yeah, that's because of the way xfs_fsr preserves unwritten extents
> > across defragmentation. i.e. it assumes unwritten extents are a
> > result of application level preallocation and so it's there
> > intentionally and be preserved.
> > 
> > When I wrote this yesterday, I hadn't see the test you'd posted to
> > the fstests list so I didn't have that context. Now that I've seen
> > it, and you've explained the problems, I can see why you wrote the
> > simpler xfs_io test command....
> > 
> > [....]
> > 
> > > So unless there's any objections, I can try to reword the commit log for
> > > this patch to make the advantages/differences more clear.
> > 
> > Yeah, I think that's all that is necessary - just expand on
> > "xfs_fsr does not provide the necessary controls" a bit so I don't
> > have to rely on my brain to remember this conversation. :P
> 
> <shrug> I had figured xfs_fsr == "polished user program not to be
> cluttered up with weird xfstests warts" and xfs_io == "warty program we
> use to make raw calls for testing" so except for tests that check the
> behavior of xfs_fsr itself, there ought to be a raw interface via xfs_io
> that regression tests use directly. :)
> 

Yep. I thought about adding an input file param or some such to xfs_fsr
for about 3 seconds or so, then came to the same conclusion. ;P

Brian

> --D
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html