[PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Mark Tinguely]

[-- Attachment #1: xfs_io-support-SEEK_DATA-SEEK_HOLE.patch --]
[-- Type: text/plain, Size: 6620 bytes --]

Add the lseek() SEEK_DATA/SEEK_HOLE support into xfs_io.
The result from the lseek() call is printed to the output:
  xfs_io> lseek -h 609k
  lseek for hole starting at offset 623616 result offset 630784

Configure this feature only on Linux distributions that support
SEEK_DATA/SEEK_HOLE.

Signed-off-by: Mark Tinguely <tinguely@sgi.com> 
---
 configure.in          |    1 
 include/builddefs.in  |    1 
 io/Makefile           |    5 ++
 io/init.c             |    1 
 io/io.h               |    6 ++
 io/lseek.c            |  101 ++++++++++++++++++++++++++++++++++++++++++++++++++
 m4/package_libcdev.m4 |   15 +++++++
 man/man8/xfs_io.8     |    7 +++
 8 files changed, 137 insertions(+)

Index: b/configure.in
===================================================================
--- a/configure.in
+++ b/configure.in
@@ -109,6 +109,7 @@ AC_HAVE_GETMNTINFO
 AC_HAVE_FALLOCATE
 AC_HAVE_FIEMAP
 AC_HAVE_PREADV
+AC_HAVE_LSEEK_DATA
 AC_HAVE_SYNC_FILE_RANGE
 AC_HAVE_BLKID_TOPO($enable_blkid)
 
Index: b/include/builddefs.in
===================================================================
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -102,6 +102,7 @@ HAVE_GETMNTINFO = @have_getmntinfo@
 HAVE_FALLOCATE = @have_fallocate@
 HAVE_FIEMAP = @have_fiemap@
 HAVE_PREADV = @have_preadv@
+HAVE_LSEEK_DATA = @have_lseek_data@
 HAVE_SYNC_FILE_RANGE = @have_sync_file_range@
 
 GCCFLAGS = -funsigned-char -fno-strict-aliasing -Wall 
Index: b/io/Makefile
===================================================================
--- a/io/Makefile
+++ b/io/Makefile
@@ -80,6 +80,11 @@ ifeq ($(HAVE_PREADV),yes)
 LCFLAGS += -DHAVE_PREADV -DHAVE_PWRITEV
 endif
 
+ifeq ($(HAVE_LSEEK_DATA),yes)
+LCFLAGS += -DHAVE_LSEEK_DATA
+CFILES += lseek.c
+endif
+
 default: depend $(LTCOMMAND)
 
 include $(BUILDRULES)
Index: b/io/init.c
===================================================================
--- a/io/init.c
+++ b/io/init.c
@@ -64,6 +64,7 @@ init_commands(void)
 	help_init();
 	imap_init();
 	inject_init();
+	lseek_init();
 	madvise_init();
 	mincore_init();
 	mmap_init();
Index: b/io/io.h
===================================================================
--- a/io/io.h
+++ b/io/io.h
@@ -108,6 +108,12 @@ extern void		quit_init(void);
 extern void		shutdown_init(void);
 extern void		truncate_init(void);
 
+#ifdef HAVE_LSEEK_DATA
+extern void		lseek_init(void);
+#else
+#define lseek_init()	do { } while (0)
+#endif
+
 #ifdef HAVE_FADVISE
 extern void		fadvise_init(void);
 #else
Index: b/io/lseek.c
===================================================================
--- /dev/null
+++ b/io/lseek.c
@@ -0,0 +1,101 @@
+/*
+ * Copyright (c) 2012 SGI
+ * 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
+ */
+
+#define _LARGEFILE64_SOURCE     /* See feature_test_macros(7) */
+#include <sys/types.h>
+#include <unistd.h>
+#include <linux/fs.h>
+
+#include <sys/uio.h>
+#include <xfs/xfs.h>
+#include <xfs/command.h>
+#include <xfs/input.h>
+#include <ctype.h>
+#include "init.h"
+#include "io.h"
+
+static cmdinfo_t lseek_cmd;
+
+static void
+lseek_help(void)
+{
+	printf(_(
+"\n"
+" returns the next hole or data offset at or after the specified offset\n"
+"\n"
+" Example:\n"
+" 'lseek -d 512'  - offset of data at or following offset 512\n"
+"\n"
+" Repositions and returns the offset of either the next data or hole.\n"
+" There is an implied hole at the end of file. If the specified offset is\n"
+" past end of file, or there is no data past the specied offset, the offset\n"
+" -1 is returned.\n"
+" -d   -- search for data starting at the specified offset.\n"
+" -h   -- search for a hole starting at the specified offset.\n"
+"\n"));
+}
+
+static int
+lseek_f(
+	int		argc,
+	char		**argv)
+{
+	off64_t		offset, res_off;
+	size_t          fsblocksize, fssectsize;
+	int		flag;
+	int		c;
+
+	flag = 0;
+	init_cvtnum(&fsblocksize, &fssectsize);
+
+	while ((c = getopt(argc, argv, "dh")) != EOF) {
+		switch (c) {
+		case 'd':
+			flag = SEEK_DATA;
+			break;
+		case 'h':
+			flag = SEEK_HOLE;
+			break;
+		default:
+			return command_usage(&lseek_cmd);
+		}
+	}
+	if (!flag  || optind > 2)
+		return command_usage(&lseek_cmd);
+	offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
+	res_off = lseek64(file->fd, offset, flag);
+	printf("lseek for %s starting at offset %lld result offset %lld\n",
+		(flag == SEEK_DATA) ? "data" : "hole",	offset, res_off);
+	return 0;
+}
+
+void
+lseek_init(void)
+{
+	lseek_cmd.name = "lseek";
+	lseek_cmd.altname = "seek";
+	lseek_cmd.cfunc = lseek_f;
+	lseek_cmd.argmin = 2;
+	lseek_cmd.argmax = 2;
+	lseek_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	lseek_cmd.args = _("[-d | -h] off");
+	lseek_cmd.oneline = _("locate and postition to next data or hole");
+	lseek_cmd.help = lseek_help;
+
+	add_command(&lseek_cmd);
+}
Index: b/m4/package_libcdev.m4
===================================================================
--- a/m4/package_libcdev.m4
+++ b/m4/package_libcdev.m4
@@ -153,6 +153,21 @@ AC_DEFUN([AC_HAVE_PREADV],
     AC_SUBST(have_preadv)
   ])
 
+# 
+# Check if we have a working fadvise system call
+# 
+AC_DEFUN([AC_HAVE_LSEEK_DATA],
+  [ AC_MSG_CHECKING([for lseek SEEK_DATA])
+    AC_TRY_COMPILE([
+#include <linux/fs.h>
+    ], [
+	lseek(0, 0, SEEK_DATA);
+    ],	have_lseek_data=yes
+	AC_MSG_RESULT(yes),
+	AC_MSG_RESULT(no))
+    AC_SUBST(have_lseek_data)
+  ])
+
 #
 # Check if we have a sync_file_range libc call (Linux)
 #
Index: b/man/man8/xfs_io.8
===================================================================
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -377,6 +377,13 @@ must be specified as another open file
 .RB ( \-f )
 or by path
 .RB ( \-i ).
+.TP
+.BI "lseek [ \-b " offset " | \-h " offset " ]
+Repositions and prints the file pointer to the next offset containing data
+.RB ( \-d )
+or next offset containing a hole 
+.RB ( \-h )
+.TP
 
 .SH MEMORY MAPPED I/O COMMANDS
 .TP


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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Dave Chinner]

On Mon, Oct 22, 2012 at 04:38:00PM -0500, Mark Tinguely wrote:
> Add the lseek() SEEK_DATA/SEEK_HOLE support into xfs_io.
> The result from the lseek() call is printed to the output:
>   xfs_io> lseek -h 609k
>   lseek for hole starting at offset 623616 result offset 630784

Nice, though I think the output is too verbose. We really want to
make it easy to parse, and we don't need to know what offset the
seek started at as that is in the command. i.e. something like:

Type		Offset
HOLE		630784

or:

DATA found at 524288

If probably all that is necessary. Indeed, one thing that might be
useful is adding a "-r" option to dump out the result of repeated
seeks of the same type, so if you were to do something like:

xfs_io> lseek -r -d 0
Type		Offset
DATA		0
DATA		65536
DATA		524288
xfs_io> lseek -r -h 0
Type		Offset
HOLE		16384
HOLE		131072
HOLE		1049576

If we then add a "-a" option to dump "all", it could alternate
data/hole after determining if the first offset is a hole or data:

xfs_io> lseek -r -a 0
DATA		0
HOLE		16384
DATA		65536
HOLE		131072
DATA		524288
HOLE		1049576

That gives us a method of verifying the basic layout of the file and
that the basic data/hole search operation (i.e. what cp will do)
works correctly via a single command in a test.

> +#define _LARGEFILE64_SOURCE     /* See feature_test_macros(7) */

That's defined by _GNU_SOURCE, which is set in the makefiles, so not
necessary here.

> +static void
> +lseek_help(void)
> +{
> +	printf(_(
> +"\n"
> +" returns the next hole or data offset at or after the specified offset\n"
> +"\n"
> +" Example:\n"
> +" 'lseek -d 512'  - offset of data at or following offset 512\n"
> +"\n"
> +" Repositions and returns the offset of either the next data or hole.\n"
> +" There is an implied hole at the end of file. If the specified offset is\n"
> +" past end of file, or there is no data past the specied offset, the offset\n"
> +" -1 is returned.\n"

I'd prefer that "EOF" rather than "-1" is printed in this case.

> +" -d   -- search for data starting at the specified offset.\n"
> +" -h   -- search for a hole starting at the specified offset.\n"
> +"\n"));
> +}
> +
> +static int
> +lseek_f(
> +	int		argc,
> +	char		**argv)
> +{
> +	off64_t		offset, res_off;
> +	size_t          fsblocksize, fssectsize;
> +	int		flag;
> +	int		c;
> +
> +	flag = 0;
> +	init_cvtnum(&fsblocksize, &fssectsize);
> +
> +	while ((c = getopt(argc, argv, "dh")) != EOF) {
> +		switch (c) {
> +		case 'd':
> +			flag = SEEK_DATA;
> +			break;
> +		case 'h':
> +			flag = SEEK_HOLE;
> +			break;
> +		default:
> +			return command_usage(&lseek_cmd);
> +		}
> +	}
> +	if (!flag  || optind > 2)
> +		return command_usage(&lseek_cmd);
> +	offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
> +	res_off = lseek64(file->fd, offset, flag);
> +	printf("lseek for %s starting at offset %lld result offset %lld\n",
> +		(flag == SEEK_DATA) ? "data" : "hole",	offset, res_off);

I think that needs the _("....") around the format string.

Also, you need to check for ENXIO for a seek beyond EOF rather than
some other execution error encountered during the lseek. i.e. ENXIO
is a valid result, while something like EINVAL or EBADF indicates an
actual error. I think that needs to be differentiated in the output.

>  or by path
>  .RB ( \-i ).
> +.TP
> +.BI "lseek [ \-b " offset " | \-h " offset " ]
                 ^^ -d

The parameters aren't optional - that's what the "[ ... ]" brackets
indicate. i.e. this will render like:

	lseek [ -b _offset_ | -h _offset_ ]

indicating lseek can be executed without parameters successfully.
If should probably render like:

	lseek -d | -h _offset_

Indicating that you must select either -d or -h, and you must supply
an offset parameter....

> +Repositions and prints the file pointer to the next offset containing data
> +.RB ( \-d )
> +or next offset containing a hole 
> +.RB ( \-h )

Given that we only support pread and pwrite operations, the
repositioning of the file pointer is irrelevant so probably should
not be mentioned. If it was relevant, then we'd also need to support
the other seek modes to reposition the file pointer. So jsut
mentioning that it returns the offset of the next ... is probably
sufficient here.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Christoph Hellwig]

In addition to Dave's comments: can we get a testcase for this support
into xfstests?

On Mon, Oct 22, 2012 at 04:38:00PM -0500, Mark Tinguely wrote:
> Add the lseek() SEEK_DATA/SEEK_HOLE support into xfs_io.
> The result from the lseek() call is printed to the output:
>   xfs_io> lseek -h 609k
>   lseek for hole starting at offset 623616 result offset 630784
> 
> Configure this feature only on Linux distributions that support
> SEEK_DATA/SEEK_HOLE.
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com> 
> ---
>  configure.in          |    1 
>  include/builddefs.in  |    1 
>  io/Makefile           |    5 ++
>  io/init.c             |    1 
>  io/io.h               |    6 ++
>  io/lseek.c            |  101 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  m4/package_libcdev.m4 |   15 +++++++
>  man/man8/xfs_io.8     |    7 +++
>  8 files changed, 137 insertions(+)
> 
> Index: b/configure.in
> ===================================================================
> --- a/configure.in
> +++ b/configure.in
> @@ -109,6 +109,7 @@ AC_HAVE_GETMNTINFO
>  AC_HAVE_FALLOCATE
>  AC_HAVE_FIEMAP
>  AC_HAVE_PREADV
> +AC_HAVE_LSEEK_DATA
>  AC_HAVE_SYNC_FILE_RANGE
>  AC_HAVE_BLKID_TOPO($enable_blkid)
>  
> Index: b/include/builddefs.in
> ===================================================================
> --- a/include/builddefs.in
> +++ b/include/builddefs.in
> @@ -102,6 +102,7 @@ HAVE_GETMNTINFO = @have_getmntinfo@
>  HAVE_FALLOCATE = @have_fallocate@
>  HAVE_FIEMAP = @have_fiemap@
>  HAVE_PREADV = @have_preadv@
> +HAVE_LSEEK_DATA = @have_lseek_data@
>  HAVE_SYNC_FILE_RANGE = @have_sync_file_range@
>  
>  GCCFLAGS = -funsigned-char -fno-strict-aliasing -Wall 
> Index: b/io/Makefile
> ===================================================================
> --- a/io/Makefile
> +++ b/io/Makefile
> @@ -80,6 +80,11 @@ ifeq ($(HAVE_PREADV),yes)
>  LCFLAGS += -DHAVE_PREADV -DHAVE_PWRITEV
>  endif
>  
> +ifeq ($(HAVE_LSEEK_DATA),yes)
> +LCFLAGS += -DHAVE_LSEEK_DATA
> +CFILES += lseek.c
> +endif
> +
>  default: depend $(LTCOMMAND)
>  
>  include $(BUILDRULES)
> Index: b/io/init.c
> ===================================================================
> --- a/io/init.c
> +++ b/io/init.c
> @@ -64,6 +64,7 @@ init_commands(void)
>  	help_init();
>  	imap_init();
>  	inject_init();
> +	lseek_init();
>  	madvise_init();
>  	mincore_init();
>  	mmap_init();
> Index: b/io/io.h
> ===================================================================
> --- a/io/io.h
> +++ b/io/io.h
> @@ -108,6 +108,12 @@ extern void		quit_init(void);
>  extern void		shutdown_init(void);
>  extern void		truncate_init(void);
>  
> +#ifdef HAVE_LSEEK_DATA
> +extern void		lseek_init(void);
> +#else
> +#define lseek_init()	do { } while (0)
> +#endif
> +
>  #ifdef HAVE_FADVISE
>  extern void		fadvise_init(void);
>  #else
> Index: b/io/lseek.c
> ===================================================================
> --- /dev/null
> +++ b/io/lseek.c
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright (c) 2012 SGI
> + * 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
> + */
> +
> +#define _LARGEFILE64_SOURCE     /* See feature_test_macros(7) */
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <linux/fs.h>
> +
> +#include <sys/uio.h>
> +#include <xfs/xfs.h>
> +#include <xfs/command.h>
> +#include <xfs/input.h>
> +#include <ctype.h>
> +#include "init.h"
> +#include "io.h"
> +
> +static cmdinfo_t lseek_cmd;
> +
> +static void
> +lseek_help(void)
> +{
> +	printf(_(
> +"\n"
> +" returns the next hole or data offset at or after the specified offset\n"
> +"\n"
> +" Example:\n"
> +" 'lseek -d 512'  - offset of data at or following offset 512\n"
> +"\n"
> +" Repositions and returns the offset of either the next data or hole.\n"
> +" There is an implied hole at the end of file. If the specified offset is\n"
> +" past end of file, or there is no data past the specied offset, the offset\n"
> +" -1 is returned.\n"
> +" -d   -- search for data starting at the specified offset.\n"
> +" -h   -- search for a hole starting at the specified offset.\n"
> +"\n"));
> +}
> +
> +static int
> +lseek_f(
> +	int		argc,
> +	char		**argv)
> +{
> +	off64_t		offset, res_off;
> +	size_t          fsblocksize, fssectsize;
> +	int		flag;
> +	int		c;
> +
> +	flag = 0;
> +	init_cvtnum(&fsblocksize, &fssectsize);
> +
> +	while ((c = getopt(argc, argv, "dh")) != EOF) {
> +		switch (c) {
> +		case 'd':
> +			flag = SEEK_DATA;
> +			break;
> +		case 'h':
> +			flag = SEEK_HOLE;
> +			break;
> +		default:
> +			return command_usage(&lseek_cmd);
> +		}
> +	}
> +	if (!flag  || optind > 2)
> +		return command_usage(&lseek_cmd);
> +	offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
> +	res_off = lseek64(file->fd, offset, flag);
> +	printf("lseek for %s starting at offset %lld result offset %lld\n",
> +		(flag == SEEK_DATA) ? "data" : "hole",	offset, res_off);
> +	return 0;
> +}
> +
> +void
> +lseek_init(void)
> +{
> +	lseek_cmd.name = "lseek";
> +	lseek_cmd.altname = "seek";
> +	lseek_cmd.cfunc = lseek_f;
> +	lseek_cmd.argmin = 2;
> +	lseek_cmd.argmax = 2;
> +	lseek_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	lseek_cmd.args = _("[-d | -h] off");
> +	lseek_cmd.oneline = _("locate and postition to next data or hole");
> +	lseek_cmd.help = lseek_help;
> +
> +	add_command(&lseek_cmd);
> +}
> Index: b/m4/package_libcdev.m4
> ===================================================================
> --- a/m4/package_libcdev.m4
> +++ b/m4/package_libcdev.m4
> @@ -153,6 +153,21 @@ AC_DEFUN([AC_HAVE_PREADV],
>      AC_SUBST(have_preadv)
>    ])
>  
> +# 
> +# Check if we have a working fadvise system call
> +# 
> +AC_DEFUN([AC_HAVE_LSEEK_DATA],
> +  [ AC_MSG_CHECKING([for lseek SEEK_DATA])
> +    AC_TRY_COMPILE([
> +#include <linux/fs.h>
> +    ], [
> +	lseek(0, 0, SEEK_DATA);
> +    ],	have_lseek_data=yes
> +	AC_MSG_RESULT(yes),
> +	AC_MSG_RESULT(no))
> +    AC_SUBST(have_lseek_data)
> +  ])
> +
>  #
>  # Check if we have a sync_file_range libc call (Linux)
>  #
> Index: b/man/man8/xfs_io.8
> ===================================================================
> --- a/man/man8/xfs_io.8
> +++ b/man/man8/xfs_io.8
> @@ -377,6 +377,13 @@ must be specified as another open file
>  .RB ( \-f )
>  or by path
>  .RB ( \-i ).
> +.TP
> +.BI "lseek [ \-b " offset " | \-h " offset " ]
> +Repositions and prints the file pointer to the next offset containing data
> +.RB ( \-d )
> +or next offset containing a hole 
> +.RB ( \-h )
> +.TP
>  
>  .SH MEMORY MAPPED I/O COMMANDS
>  .TP
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Mark Tinguely]

On 10/22/12 18:29, Dave Chinner wrote:
> On Mon, Oct 22, 2012 at 04:38:00PM -0500, Mark Tinguely wrote:

> Type		Offset
> HOLE		630784
>

> xfs_io>  lseek -r -d 0
> Type		Offset
> DATA		0
> DATA		65536
> DATA		524288
> xfs_io>  lseek -r -h 0
> Type		Offset
> HOLE		16384
> HOLE		131072
> HOLE		1049576
>

> xfs_io>  lseek -r -a 0
> DATA		0
> HOLE		16384
> DATA		65536
> HOLE		131072
> DATA		524288
> HOLE		1049576

Good idea.

>> +#define _LARGEFILE64_SOURCE     /* See feature_test_macros(7) */
>
> That's defined by _GNU_SOURCE, which is set in the makefiles, so not
> necessary here.

Okay, I think a couple of the header files are redundantly redundant too.

>
>> +static void
>> +lseek_help(void)
>> +{
>> +	printf(_(
>> +"\n"
>> +" returns the next hole or data offset at or after the specified offset\n"
>> +"\n"
>> +" Example:\n"
>> +" 'lseek -d 512'  - offset of data at or following offset 512\n"
>> +"\n"
>> +" Repositions and returns the offset of either the next data or hole.\n"
>> +" There is an implied hole at the end of file. If the specified offset is\n"
>> +" past end of file, or there is no data past the specied offset, the offset\n"
>> +" -1 is returned.\n"
>
> I'd prefer that "EOF" rather than "-1" is printed in this case.

sounds good.

<deleted mess of things to clean up>

>
> Given that we only support pread and pwrite operations, the
> repositioning of the file pointer is irrelevant so probably should
> not be mentioned. If it was relevant, then we'd also need to support
> the other seek modes to reposition the file pointer. So jsut
> mentioning that it returns the offset of the next ... is probably
> sufficient here.

agreed. I did not the other lseek() whence options for that very reason.

>
> Cheers,
>
> Dave.

Thanks for the feedback.

PS. To Christoph: Yes, a test will be added.

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

[PATCH v2] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Mark Tinguely]

[-- Attachment #1: v2-xfs_io-support-SEEK_DATA-SEEK_HOLE.patch --]
[-- Type: text/plain, Size: 8532 bytes --]

Add the lseek SEEK_DATA/SEEK_HOLE support into xfs_io.
The result from the lseek() call will be printed to the output.
For example:

xfs_io> lseek -h 609k
Type	Offset
hole	630784

Signed-off-by: Mark Tinguely <tinguely@sgi.com> 
---
 configure.in          |    1 
 include/builddefs.in  |    1 
 io/Makefile           |    5 +
 io/init.c             |    1 
 io/io.h               |    6 +
 io/lseek.c            |  165 ++++++++++++++++++++++++++++++++++++++++++++++++++
 m4/package_libcdev.m4 |   15 ++++
 man/man8/xfs_io.8     |   35 ++++++++++
 8 files changed, 229 insertions(+)

Index: b/configure.in
===================================================================
--- a/configure.in
+++ b/configure.in
@@ -109,6 +109,7 @@ AC_HAVE_GETMNTINFO
 AC_HAVE_FALLOCATE
 AC_HAVE_FIEMAP
 AC_HAVE_PREADV
+AC_HAVE_LSEEK_DATA
 AC_HAVE_SYNC_FILE_RANGE
 AC_HAVE_BLKID_TOPO($enable_blkid)
 
Index: b/include/builddefs.in
===================================================================
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -102,6 +102,7 @@ HAVE_GETMNTINFO = @have_getmntinfo@
 HAVE_FALLOCATE = @have_fallocate@
 HAVE_FIEMAP = @have_fiemap@
 HAVE_PREADV = @have_preadv@
+HAVE_LSEEK_DATA = @have_lseek_data@
 HAVE_SYNC_FILE_RANGE = @have_sync_file_range@
 
 GCCFLAGS = -funsigned-char -fno-strict-aliasing -Wall 
Index: b/io/Makefile
===================================================================
--- a/io/Makefile
+++ b/io/Makefile
@@ -80,6 +80,11 @@ ifeq ($(HAVE_PREADV),yes)
 LCFLAGS += -DHAVE_PREADV -DHAVE_PWRITEV
 endif
 
+ifeq ($(HAVE_LSEEK_DATA),yes)
+LCFLAGS += -DHAVE_LSEEK_DATA
+CFILES += lseek.c
+endif
+
 default: depend $(LTCOMMAND)
 
 include $(BUILDRULES)
Index: b/io/init.c
===================================================================
--- a/io/init.c
+++ b/io/init.c
@@ -64,6 +64,7 @@ init_commands(void)
 	help_init();
 	imap_init();
 	inject_init();
+	lseek_init();
 	madvise_init();
 	mincore_init();
 	mmap_init();
Index: b/io/io.h
===================================================================
--- a/io/io.h
+++ b/io/io.h
@@ -108,6 +108,12 @@ extern void		quit_init(void);
 extern void		shutdown_init(void);
 extern void		truncate_init(void);
 
+#ifdef HAVE_LSEEK_DATA
+extern void		lseek_init(void);
+#else
+#define lseek_init()	do { } while (0)
+#endif
+
 #ifdef HAVE_FADVISE
 extern void		fadvise_init(void);
 #else
Index: b/io/lseek.c
===================================================================
--- /dev/null
+++ b/io/lseek.c
@@ -0,0 +1,165 @@
+/*
+ * Copyright (c) 2012 SGI
+ * 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 <linux/fs.h>
+
+#include <sys/uio.h>
+#include <xfs/xfs.h>
+#include <xfs/command.h>
+#include <xfs/input.h>
+#include <ctype.h>
+#include "init.h"
+#include "io.h"
+
+static cmdinfo_t lseek_cmd;
+
+static void
+lseek_help(void)
+{
+	printf(_(
+"\n"
+" returns the next hole and/or data offset at or after the specified offset\n"
+"\n"
+" Example:\n"
+" 'lseek -d 512'   - offset of data at or following offset 512\n"
+" 'lseek -a -r 0'  - offsets of all data and hole in entire file\n"
+"\n"
+" Returns the offset of the next data and/or hole. There is an implied hole\n"
+" at the end of file. If the specified offset is past end of file, or there\n"
+" is no data past the specied offset, EOF is returned.\n"
+" -a   -- return the next data and hole starting at the specified offset.\n"
+" -d   -- return the next data starting at the specified offset.\n"
+" -h   -- return the next hole starting at the specified offset.\n"
+" -r   -- return all remaining type(s) starting at the specified offset.\n"
+"\n"));
+}
+
+#define	LSEEK_DFLAG	1 << 0
+#define	LSEEK_HFLAG	1 << 1
+#define	LSEEK_RFLAG	1 << 2
+
+static int
+lseek_f(
+	int		argc,
+	char		**argv)
+{
+	off64_t		offset, off, roff;
+	size_t          fsblocksize, fssectsize;
+	int		cseg;
+	int		flag;
+	int		i, c;
+
+	flag = 0;
+	init_cvtnum(&fsblocksize, &fssectsize);
+
+	while ((c = getopt(argc, argv, "adhr")) != EOF) {
+		switch (c) {
+		case 'a':
+			flag |= LSEEK_HFLAG;
+			/* fall through to pick up the DFLAG */
+		case 'd':
+			flag |= LSEEK_DFLAG;
+			break;
+		case 'h':
+			flag |= LSEEK_HFLAG;
+			break;
+		case 'r':
+			flag |= LSEEK_RFLAG;
+			break;
+		default:
+			return command_usage(&lseek_cmd);
+		}
+	}
+		/* must have hole or data specified and an offset */
+	if (!(flag & (LSEEK_DFLAG | LSEEK_HFLAG)) ||
+             optind != argc - 1)
+		return command_usage(&lseek_cmd);
+
+	offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
+
+	/* recursively display all information, decide where to start. */
+	off = lseek64(file->fd, offset, SEEK_DATA);
+	if (off == offset)
+		cseg = LSEEK_DFLAG;
+	else
+		cseg = LSEEK_HFLAG;
+	
+	printf("Type	offset\n");
+
+	/* loop to handle the data / hole entries in assending order.
+	 * Only display the EOF when the initial offset was greater
+	 * the end of file.
+	 */
+	for (i = 0; flag & LSEEK_RFLAG || i < 2; i++) {
+		if (cseg == LSEEK_DFLAG) {
+			if (flag & LSEEK_DFLAG) {
+				roff = lseek64(file->fd, offset, SEEK_DATA);
+				if (roff == -1) {
+					if (i < 2) {
+						if (errno == ENXIO)
+							printf("data	EOF\n");
+						else
+							printf("data	%d\n",
+								-errno);
+					}
+					break;
+				} else
+					printf("data	%lld\n", roff);
+			}
+			/* set the offset and type for the next iteration */
+			cseg = LSEEK_HFLAG;
+			offset = lseek64(file->fd, offset, SEEK_HOLE);
+			continue;
+		}
+
+		/* cseg == LSEEK_HFLAG */
+		if (flag & LSEEK_HFLAG) {
+			roff = lseek64(file->fd, offset, SEEK_HOLE);
+			if (roff == -1) {
+				if (i < 2) {
+					if (errno == ENXIO)
+						printf("hole	EOF\n");
+					else
+						printf("hole	%d\n", -errno);
+				}
+				break;
+			} else
+				printf("hole	%lld\n", roff);
+		}
+		/* set the offset and type for the next iteration */
+		cseg = LSEEK_DFLAG;
+		offset = lseek64(file->fd, offset, SEEK_DATA);
+	}
+	return 0;
+}
+
+void
+lseek_init(void)
+{
+	lseek_cmd.name = "lseek";
+	lseek_cmd.altname = "seek";
+	lseek_cmd.cfunc = lseek_f;
+	lseek_cmd.argmin = 2;
+	lseek_cmd.argmax = 5;
+	lseek_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	lseek_cmd.args = _("-a | -d | -h [-r] off");
+	lseek_cmd.oneline = _("locate the next data and/or hole");
+	lseek_cmd.help = lseek_help;
+
+	add_command(&lseek_cmd);
+}
Index: b/m4/package_libcdev.m4
===================================================================
--- a/m4/package_libcdev.m4
+++ b/m4/package_libcdev.m4
@@ -153,6 +153,21 @@ AC_DEFUN([AC_HAVE_PREADV],
     AC_SUBST(have_preadv)
   ])
 
+# 
+# Check if we have a working fadvise system call
+# 
+AC_DEFUN([AC_HAVE_LSEEK_DATA],
+  [ AC_MSG_CHECKING([for lseek SEEK_DATA])
+    AC_TRY_COMPILE([
+#include <linux/fs.h>
+    ], [
+	lseek(0, 0, SEEK_DATA);
+    ],	have_lseek_data=yes
+	AC_MSG_RESULT(yes),
+	AC_MSG_RESULT(no))
+    AC_SUBST(have_lseek_data)
+  ])
+
 #
 # Check if we have a sync_file_range libc call (Linux)
 #
Index: b/man/man8/xfs_io.8
===================================================================
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -377,6 +377,41 @@ must be specified as another open file
 .RB ( \-f )
 or by path
 .RB ( \-i ).
+.TP
+.BI "lseek  \-a | \-d | \-h [ \-r ] offset"
+On platforms that support the
+.BR lseek (2)
+.B SEEK_DATA
+and
+.B SEEK_HOLE
+options, display the offsets of the specified segments.
+.RS 1.0i
+.PD 0
+.TP 0.4i
+.B \-a
+Display both
+.B data
+and
+.B hole
+segments starting at the specified
+.B offset.
+.TP
+.B \-d
+Display the 
+.B data
+segment starting at the specified
+.B offset.
+.TP
+.B \-h
+Display the 
+.B hole
+segment starting at the specified
+.B offset.
+.TP
+.B \-r
+Recursively display all the specified segments starting at the specified
+.B offset.
+.TP
 
 .SH MEMORY MAPPED I/O COMMANDS
 .TP


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

[PATCH v3] xfs_io: [v3] add the lseek() SEEK_DATA/SEEK_HOLE support

[Mark Tinguely]

[-- Attachment #1: v3-xfs_io-support-SEEK_DATA-SEEK_HOLE.patch --]
[-- Type: text/plain, Size: 8833 bytes --]

Add the lseek SEEK_DATA/SEEK_HOLE support into xfs_io.
The result from the lseek() call will be printed to the output.
For example:

xfs_io> lseek -h 609k
Type	Offset
hole	630784

v1 -> v2 Add "-a" and "-r" options.
	 Simplify the output.
v2 -> v3 Refactor for configure.in -> configure.ac change.
	 SEEK_DATA with -1 offset behaves badly on older Linux.
	 Display error message as "ERR <errno>".
      
Signed-off-by: Mark Tinguely <tinguely@sgi.com> 
---
 configure.ac          |    1 
 include/builddefs.in  |    1 
 io/Makefile           |    5 +
 io/init.c             |    1 
 io/io.h               |    6 +
 io/lseek.c            |  169 ++++++++++++++++++++++++++++++++++++++++++++++++++
 m4/package_libcdev.m4 |   15 ++++
 man/man8/xfs_io.8     |   35 ++++++++++
 8 files changed, 233 insertions(+)

Index: b/configure.ac
===================================================================
--- a/configure.ac
+++ b/configure.ac
@@ -109,6 +109,7 @@ AC_HAVE_GETMNTINFO
 AC_HAVE_FALLOCATE
 AC_HAVE_FIEMAP
 AC_HAVE_PREADV
+AC_HAVE_LSEEK_DATA
 AC_HAVE_SYNC_FILE_RANGE
 AC_HAVE_BLKID_TOPO($enable_blkid)
 
Index: b/include/builddefs.in
===================================================================
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -102,6 +102,7 @@ HAVE_GETMNTINFO = @have_getmntinfo@
 HAVE_FALLOCATE = @have_fallocate@
 HAVE_FIEMAP = @have_fiemap@
 HAVE_PREADV = @have_preadv@
+HAVE_LSEEK_DATA = @have_lseek_data@
 HAVE_SYNC_FILE_RANGE = @have_sync_file_range@
 
 GCCFLAGS = -funsigned-char -fno-strict-aliasing -Wall 
Index: b/io/Makefile
===================================================================
--- a/io/Makefile
+++ b/io/Makefile
@@ -80,6 +80,11 @@ ifeq ($(HAVE_PREADV),yes)
 LCFLAGS += -DHAVE_PREADV -DHAVE_PWRITEV
 endif
 
+ifeq ($(HAVE_LSEEK_DATA),yes)
+LCFLAGS += -DHAVE_LSEEK_DATA
+CFILES += lseek.c
+endif
+
 default: depend $(LTCOMMAND)
 
 include $(BUILDRULES)
Index: b/io/init.c
===================================================================
--- a/io/init.c
+++ b/io/init.c
@@ -64,6 +64,7 @@ init_commands(void)
 	help_init();
 	imap_init();
 	inject_init();
+	lseek_init();
 	madvise_init();
 	mincore_init();
 	mmap_init();
Index: b/io/io.h
===================================================================
--- a/io/io.h
+++ b/io/io.h
@@ -108,6 +108,12 @@ extern void		quit_init(void);
 extern void		shutdown_init(void);
 extern void		truncate_init(void);
 
+#ifdef HAVE_LSEEK_DATA
+extern void		lseek_init(void);
+#else
+#define lseek_init()	do { } while (0)
+#endif
+
 #ifdef HAVE_FADVISE
 extern void		fadvise_init(void);
 #else
Index: b/io/lseek.c
===================================================================
--- /dev/null
+++ b/io/lseek.c
@@ -0,0 +1,169 @@
+/*
+ * Copyright (c) 2012 SGI
+ * 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 <linux/fs.h>
+
+#include <sys/uio.h>
+#include <xfs/xfs.h>
+#include <xfs/command.h>
+#include <xfs/input.h>
+#include <ctype.h>
+#include "init.h"
+#include "io.h"
+
+static cmdinfo_t lseek_cmd;
+
+static void
+lseek_help(void)
+{
+	printf(_(
+"\n"
+" returns the next hole and/or data offset at or after the specified offset\n"
+"\n"
+" Example:\n"
+" 'lseek -d 512'   - offset of data at or following offset 512\n"
+" 'lseek -a -r 0'  - offsets of all data and hole in entire file\n"
+"\n"
+" Returns the offset of the next data and/or hole. There is an implied hole\n"
+" at the end of file. If the specified offset is past end of file, or there\n"
+" is no data past the specied offset, EOF is returned.\n"
+" -a   -- return the next data and hole starting at the specified offset.\n"
+" -d   -- return the next data starting at the specified offset.\n"
+" -h   -- return the next hole starting at the specified offset.\n"
+" -r   -- return all remaining type(s) starting at the specified offset.\n"
+"\n"));
+}
+
+#define	LSEEK_DFLAG	1 << 0
+#define	LSEEK_HFLAG	1 << 1
+#define	LSEEK_RFLAG	1 << 2
+
+static int
+lseek_f(
+	int		argc,
+	char		**argv)
+{
+	off64_t		offset, roff;
+	size_t          fsblocksize, fssectsize;
+	int		cseg;
+	int		flag;
+	int		i, c;
+
+	flag = 0;
+	init_cvtnum(&fsblocksize, &fssectsize);
+
+	while ((c = getopt(argc, argv, "adhr")) != EOF) {
+		switch (c) {
+		case 'a':
+			flag |= LSEEK_HFLAG;
+			/* fall through to pick up the DFLAG */
+		case 'd':
+			flag |= LSEEK_DFLAG;
+			break;
+		case 'h':
+			flag |= LSEEK_HFLAG;
+			break;
+		case 'r':
+			flag |= LSEEK_RFLAG;
+			break;
+		default:
+			return command_usage(&lseek_cmd);
+		}
+	}
+		/* must have hole or data specified and an offset */
+	if (!(flag & (LSEEK_DFLAG | LSEEK_HFLAG)) ||
+             optind != argc - 1)
+		return command_usage(&lseek_cmd);
+
+	offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
+
+	/* recursively display all information, decide where to start. */
+	roff = lseek64(file->fd, offset, SEEK_DATA);
+	if (roff == offset)
+		cseg = LSEEK_DFLAG;
+	else
+		cseg = LSEEK_HFLAG;
+	
+	printf("Type	offset\n");
+
+	/* loop to handle the data / hole entries in assending order.
+	 * Only display the EOF when the initial offset was greater
+	 * the end of file.
+	 */
+	for (i = 0; flag & LSEEK_RFLAG || i < 2; i++) {
+		if (cseg == LSEEK_DFLAG) {
+			if (flag & LSEEK_DFLAG) {
+				roff = lseek64(file->fd, offset, SEEK_DATA);
+				if (roff == -1) {
+					if (i < 2) {
+						if (errno == ENXIO)
+							printf("data	EOF\n");
+						else
+							printf("data	ERR %d\n",
+								errno);
+					}
+					break;
+				} else
+					printf("data	%lld\n", roff);
+			}
+			/* set the offset and type for the next iteration */
+			cseg = LSEEK_HFLAG;
+			roff = lseek64(file->fd, offset, SEEK_HOLE);
+			if (roff != -1)
+				offset = roff;
+			continue;
+		}
+
+		/* cseg == LSEEK_HFLAG */
+		if (flag & LSEEK_HFLAG) {
+			roff = lseek64(file->fd, offset, SEEK_HOLE);
+			if (roff == -1) {
+				if (i < 2) {
+					if (errno == ENXIO)
+						printf("hole	EOF\n");
+					else
+						printf("hole	ERR %d\n", errno);
+				}
+				break;
+			} else
+				printf("hole	%lld\n", roff);
+		}
+		/* set the offset and type for the next iteration */
+		cseg = LSEEK_DFLAG;
+		roff = lseek64(file->fd, offset, SEEK_DATA);
+		if (roff != -1)
+			offset = roff;
+	}
+	return 0;
+}
+
+void
+lseek_init(void)
+{
+	lseek_cmd.name = "lseek";
+	lseek_cmd.altname = "seek";
+	lseek_cmd.cfunc = lseek_f;
+	lseek_cmd.argmin = 2;
+	lseek_cmd.argmax = 5;
+	lseek_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	lseek_cmd.args = _("-a | -d | -h [-r] off");
+	lseek_cmd.oneline = _("locate the next data and/or hole");
+	lseek_cmd.help = lseek_help;
+
+	add_command(&lseek_cmd);
+}
Index: b/m4/package_libcdev.m4
===================================================================
--- a/m4/package_libcdev.m4
+++ b/m4/package_libcdev.m4
@@ -153,6 +153,21 @@ AC_DEFUN([AC_HAVE_PREADV],
     AC_SUBST(have_preadv)
   ])
 
+# 
+# Check if we have a working fadvise system call
+# 
+AC_DEFUN([AC_HAVE_LSEEK_DATA],
+  [ AC_MSG_CHECKING([for lseek SEEK_DATA])
+    AC_TRY_COMPILE([
+#include <linux/fs.h>
+    ], [
+	lseek(0, 0, SEEK_DATA);
+    ],	have_lseek_data=yes
+	AC_MSG_RESULT(yes),
+	AC_MSG_RESULT(no))
+    AC_SUBST(have_lseek_data)
+  ])
+
 #
 # Check if we have a sync_file_range libc call (Linux)
 #
Index: b/man/man8/xfs_io.8
===================================================================
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -377,6 +377,41 @@ must be specified as another open file
 .RB ( \-f )
 or by path
 .RB ( \-i ).
+.TP
+.BI "lseek  \-a | \-d | \-h [ \-r ] offset"
+On platforms that support the
+.BR lseek (2)
+.B SEEK_DATA
+and
+.B SEEK_HOLE
+options, display the offsets of the specified segments.
+.RS 1.0i
+.PD 0
+.TP 0.4i
+.B \-a
+Display both
+.B data
+and
+.B hole
+segments starting at the specified
+.B offset.
+.TP
+.B \-d
+Display the 
+.B data
+segment starting at the specified
+.B offset.
+.TP
+.B \-h
+Display the 
+.B hole
+segment starting at the specified
+.B offset.
+.TP
+.B \-r
+Recursively display all the specified segments starting at the specified
+.B offset.
+.TP
 
 .SH MEMORY MAPPED I/O COMMANDS
 .TP


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

Re: [PATCH v3] xfs_io: [v3] add the lseek() SEEK_DATA/SEEK_HOLE support

[Dave Chinner]

On Thu, Oct 25, 2012 at 09:14:44AM -0500, Mark Tinguely wrote:
> Add the lseek SEEK_DATA/SEEK_HOLE support into xfs_io.
> The result from the lseek() call will be printed to the output.
> For example:
> 
> xfs_io> lseek -h 609k
> Type	Offset
> hole	630784
> 
> v1 -> v2 Add "-a" and "-r" options.
> 	 Simplify the output.
> v2 -> v3 Refactor for configure.in -> configure.ac change.
> 	 SEEK_DATA with -1 offset behaves badly on older Linux.
> 	 Display error message as "ERR <errno>".
....
> +
> +#include <linux/fs.h>

I missed this first time around - why is this include necessary?

> +#define	LSEEK_DFLAG	1 << 0
> +#define	LSEEK_HFLAG	1 << 1
> +#define	LSEEK_RFLAG	1 << 2

Need parenthesis there, i.e. "(1 << 0)", so we don't get weird bugs
due to operator precendence causing unintended behaviour, but....

> +static int
> +lseek_f(
> +	int		argc,
> +	char		**argv)
> +{
> +	off64_t		offset, roff;
> +	size_t          fsblocksize, fssectsize;
> +	int		cseg;
> +	int		flag;
> +	int		i, c;
> +
> +	flag = 0;
> +	init_cvtnum(&fsblocksize, &fssectsize);
> +
> +	while ((c = getopt(argc, argv, "adhr")) != EOF) {
> +		switch (c) {
> +		case 'a':
> +			flag |= LSEEK_HFLAG;
> +			/* fall through to pick up the DFLAG */

I'd just set the flags directly - much more obvious, less likely to
go wrong in the future...

> +	offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
> +
> +	/* recursively display all information, decide where to start. */
> +	roff = lseek64(file->fd, offset, SEEK_DATA);
> +	if (roff == offset)
> +		cseg = LSEEK_DFLAG;
> +	else
> +		cseg = LSEEK_HFLAG;

I'm not really sure what these variables mean. "roff" is returned
offset, "cseg" is current segment? Perhaps a comment or a better
names is in order because right now I'm guessing at what a segment
is as it is not referenced in any of the comments...

> +	
> +	printf("Type	offset\n");
> +
> +	/* loop to handle the data / hole entries in assending order.
> +	 * Only display the EOF when the initial offset was greater
> +	 * the end of file.
> +	 */
> +	for (i = 0; flag & LSEEK_RFLAG || i < 2; i++) {

I'm very surprised gcc doesn't warn here about lack of parenthesis.

As it is, I think the code would be much clearer if you separated
the non-recursive case from the recursive case given it took me 20
minutes to work out whether this loop worked correctly for both
cases (e.g. why does it need 2 loops instead of 1 for the non
recursive case, and does the double loop behaviour give the right
results?). Something like:

	if (!(flags & RECURSIVE))
		return seek_single(offset, flags);

	return seek_recursive(offset, flags);

makes the simple case is easy to verify correct (it's just a
single seek), and the recursive case doesn't get complicated by the
requirements of the single seek case. That results in code that is
much easier to maintain and modify in future....

> +		if (cseg == LSEEK_DFLAG) {
> +			if (flag & LSEEK_DFLAG) {
> +				roff = lseek64(file->fd, offset, SEEK_DATA);
> +				if (roff == -1) {
> +					if (i < 2) {
> +						if (errno == ENXIO)
> +							printf("data	EOF\n");
> +						else
> +							printf("data	ERR %d\n",
> +								errno);
> +					}

Why would you only print a seek error for the first seek? I
would have thought that the recursive case also needs differentiate
between an error and EOF detection.

> +				} else
> +					printf("data	%lld\n", roff);
> +			}
> +			/* set the offset and type for the next iteration */
> +			cseg = LSEEK_HFLAG;
> +			roff = lseek64(file->fd, offset, SEEK_HOLE);
> +			if (roff != -1)
> +				offset = roff;
> +			continue;

This extra seek only needs to be done if LSEEK_HFLAG is not set
to move the offset to the point where the next SEEK_DATA will find
the next data extent....

> +		}
> +
> +		/* cseg == LSEEK_HFLAG */
> +		if (flag & LSEEK_HFLAG) {
> +			roff = lseek64(file->fd, offset, SEEK_HOLE);

But if LSEEK_HFLAG is set, we do the same seek anyway on the next
loop iteration. The logic currently is:

	loop {
		if (in data) {
			if (seek data) {
				move to next data
				print
			}
			move to next hole
			update offset
			continue
		}
		/* in hole */
		if (seek hole) {
			move to next hole
			print
		}
		move to next data
		update offset
	}

Effectively, the loop is only dealing with a data extent or a hole
extent in each loop. But given that holes and data always alternate,
the same canbe acheived in a single loop:

	loop {
		move to next data
		if (data)
			print
		update offset
		move to next hole
		if (hole)
			print
		update offset
	}

And the initial loop start (i.e. offset starts in a hole or data)
can be handled with a simple "skip data" variable that is cleared on
the first pass through the loop. This gets rid of needing to track
the current segment type and gets rid of the redundant seeks to the
same offset when dumping both data and holes....

And the printing can become a simple functions that takes a "data"
or "hole" string and can be common for all callers (single and
recursive)...

> +lseek_init(void)
> +{
> +	lseek_cmd.name = "lseek";
> +	lseek_cmd.altname = "seek";

I'd just call them both "seek" - the "l" part of the lseek() call is
an implementation detail.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH v3] xfs_io: [v3] add the lseek() SEEK_DATA/SEEK_HOLE support

[Mark Tinguely]

On 10/25/12 17:29, Dave Chinner wrote:
> On Thu, Oct 25, 2012 at 09:14:44AM -0500, Mark Tinguely wrote:
>> Add the lseek SEEK_DATA/SEEK_HOLE support into xfs_io.
>> The result from the lseek() call will be printed to the output.
>> For example:
>>
>> xfs_io>  lseek -h 609k
>> Type	Offset
>> hole	630784
>>
>> v1 ->  v2 Add "-a" and "-r" options.
>> 	 Simplify the output.
>> v2 ->  v3 Refactor for configure.in ->  configure.ac change.
>> 	 SEEK_DATA with -1 offset behaves badly on older Linux.
>> 	 Display error message as "ERR<errno>".
> ....
>> +
>> +#include<linux/fs.h>
>
> I missed this first time around - why is this include necessary?

Take it out and you will find that it contains the
defines for SEEK_DATA/SEEK_HOLE.

--Mark.

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

Re: [PATCH v3] xfs_io: [v3] add the lseek() SEEK_DATA/SEEK_HOLE support

[Dave Chinner]

On Fri, Oct 26, 2012 at 08:31:11AM -0500, Mark Tinguely wrote:
> On 10/25/12 17:29, Dave Chinner wrote:
> >On Thu, Oct 25, 2012 at 09:14:44AM -0500, Mark Tinguely wrote:
> >>Add the lseek SEEK_DATA/SEEK_HOLE support into xfs_io.
> >>The result from the lseek() call will be printed to the output.
> >>For example:
> >>
> >>xfs_io>  lseek -h 609k
> >>Type	Offset
> >>hole	630784
> >>
> >>v1 ->  v2 Add "-a" and "-r" options.
> >>	 Simplify the output.
> >>v2 ->  v3 Refactor for configure.in ->  configure.ac change.
> >>	 SEEK_DATA with -1 offset behaves badly on older Linux.
> >>	 Display error message as "ERR<errno>".
> >....
> >>+
> >>+#include<linux/fs.h>
> >
> >I missed this first time around - why is this include necessary?
> 
> Take it out and you will find that it contains the
> defines for SEEK_DATA/SEEK_HOLE.

It was added in glibc 2.14, IIRC. All this means is that your
userspace libraries are not current, while your kernel headers are.

So, you shouldn't be including linux/fs.h directly, I think,
especially as SEEK_DATA/SEEK_HOLE is functionality that is not linux
specific. I suspect that you should do something more like:

#ifndef SEEK_DATA
#define SEEK_DATA 3
#define SEEK_HOLE 4
#endif

Because the autoconf test passed, but the parameters are not defined
correctly by userspace. That way it will still work on other
platforms if they support this functionality....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Mark Tinguely]

On 08/21/13 14:37, Eric Sandeen wrote:
> On 8/21/13 2:20 PM, Mark Tinguely wrote:
>> On 08/21/13 13:31, Eric Sandeen wrote:
>
> ...
>
>>>> There are different versions of XFS seek_data and they will
>>>> detect/report the start of data and holes differently so output
>>>> parsing will be a bear. The existing C code sends the 2 different
>>>> value numbers that could be reported.
>>>
>>> are they ... both correct?  If one is a bug, it can just be a bug, right?
>>> I'm sorry I'm not up on the history.
>>
>> Lets say we have a file
>> hole    0-4K
>> data    4K-8K
>> hole    8-12K
>> data    12-16K
>>
>> for data/hole check starting at offset 0, valid response are
>> 0K or 4K for data
>> 0K or 16K or -1 for holes
>>
>> This feature and test was for Jeff fine-tuned seek_data/seek_hole support. The tests would be more specific to that feature and output is specific.
>
> Well, at least the man page says:
>
>> SEEK_DATA
>> Adjust the file offset to the next location in the file greater than
>> or equal to offset containing data. If offset points to data, then
>> the file offset is set to offset.
>
> So above, if we say "SEEK_DATA at offset 0" it seems like 0k is clearly wrong, and 4k is clearly right.

If the implementation can't really detect a hole, then everything is data.

>
>> SEEK_HOLE
>> Adjust the file offset to the next hole in the file greater than or
>> equal to offset. If offset points into the middle of a hole, then the
>> file offset is set to offset. If there is no hole past offset, then
>> the file offset is adjusted to the end of the file (i.e., there is an
>> implicit hole at the end of any file).
>
> and "SEEK_HOLE at offset 0" should pretty clearly return 0, and 16k would be wrong.

probably my bad notation - the data started at 12K ended at 16K-1. Some 
report the hole at the end of the file in bytes (16k), some cases as -1. 
Real fun stuff.

The current seek_data/seek_hole xfstest correct for this in the C code 
by say what values it will accept as being good.

>
> It's not POSIX yet, so I guess there's no gospel, but that's what the man page says.
>
> -Eric

--Mark.

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Eric Sandeen]

On 8/21/13 2:37 PM, Eric Sandeen wrote:
> On 8/21/13 2:20 PM, Mark Tinguely wrote:
>> On 08/21/13 13:31, Eric Sandeen wrote:
> 
> ...
> 
>>>> There are different versions of XFS seek_data and they will
>>>> detect/report the start of data and holes differently so output
>>>> parsing will be a bear. The existing C code sends the 2 different
>>>> value numbers that could be reported.
>>>
>>> are they ... both correct?  If one is a bug, it can just be a bug, right?
>>> I'm sorry I'm not up on the history.
>>
>> Lets say we have a file
>> hole    0-4K
>> data    4K-8K
>> hole    8-12K
>> data    12-16K
>>
>> for data/hole check starting at offset 0, valid response are
>> 0K or 4K for data
>> 0K or 16K or -1 for holes
>>
>> This feature and test was for Jeff fine-tuned seek_data/seek_hole support. The tests would be more specific to that feature and output is specific.
> 
> Well, at least the man page says:
> 
>> SEEK_DATA
>> Adjust the file offset to the next location in the file greater than
>> or equal to offset containing data. If offset points to data, then
>> the file offset is set to offset.
> 
> So above, if we say "SEEK_DATA at offset 0" it seems like 0k is clearly wrong, and 4k is clearly right.
>  
>> SEEK_HOLE
>> Adjust the file offset to the next hole in the file greater than or
>> equal to offset. If offset points into the middle of a hole, then the
>> file offset is set to offset. If there is no hole past offset, then
>> the file offset is adjusted to the end of the file (i.e., there is an
>> implicit hole at the end of any file).
> 
> and "SEEK_HOLE at offset 0" should pretty clearly return 0, and 16k would be wrong.
> 
> It's not POSIX yet, so I guess there's no gospel, but that's what the man page says.

though I see the seek sanity test has all kinds of exceptions.  Yuck.

So back to: Just report what lseek says, and leave interpretation to the caller. ;)

-Eric

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

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Eric Sandeen]

On 8/21/13 2:20 PM, Mark Tinguely wrote:
> On 08/21/13 13:31, Eric Sandeen wrote:

...

>>> There are different versions of XFS seek_data and they will
>>> detect/report the start of data and holes differently so output
>>> parsing will be a bear. The existing C code sends the 2 different
>>> value numbers that could be reported.
>>
>> are they ... both correct?  If one is a bug, it can just be a bug, right?
>> I'm sorry I'm not up on the history.
> 
> Lets say we have a file
> hole    0-4K
> data    4K-8K
> hole    8-12K
> data    12-16K
> 
> for data/hole check starting at offset 0, valid response are
> 0K or 4K for data
> 0K or 16K or -1 for holes
> 
> This feature and test was for Jeff fine-tuned seek_data/seek_hole support. The tests would be more specific to that feature and output is specific.

Well, at least the man page says:

> SEEK_DATA
> Adjust the file offset to the next location in the file greater than
> or equal to offset containing data. If offset points to data, then
> the file offset is set to offset.

So above, if we say "SEEK_DATA at offset 0" it seems like 0k is clearly wrong, and 4k is clearly right.
 
> SEEK_HOLE
> Adjust the file offset to the next hole in the file greater than or
> equal to offset. If offset points into the middle of a hole, then the
> file offset is set to offset. If there is no hole past offset, then
> the file offset is adjusted to the end of the file (i.e., there is an
> implicit hole at the end of any file).

and "SEEK_HOLE at offset 0" should pretty clearly return 0, and 16k would be wrong.

It's not POSIX yet, so I guess there's no gospel, but that's what the man page says.

-Eric

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Mark Tinguely]

On 08/21/13 13:31, Eric Sandeen wrote:
> On 8/21/13 11:52 AM, Mark Tinguely wrote:
> ...
>
>>> I think it makes sense to build it&   locally define if necessary.
>>> On my RHEL6 root w/ an upstream devel kernel seek.c wouldn't have
>>> built, even though it'd have worked perfectly w/ a local define.
>>>
>>
>> yes, needed anyway if removing linux/fs.h
>
> lseek only should need:
>
>         #include<sys/types.h>
>         #include<unistd.h>
>
> right; those may internally get to linux/fs.h but it shouldn't be
> directly required, I'd expect.  Oh!  it needs
>
> #define _GNU_SOURCE
>
> first, to get it - but xfsprogs build does that already.
>
>>> So let me just think out loud here w/ examples.
>>>
>>> For a 1M 100% nonsparse file we get:
>>>
>>> # io/xfs_io -c "seek -ar 0" alldata
>>> Type    offset
>>> DATA    0
>>> HOLE    1048576
>>
>> or this could be HOLE EOF depends on the version.
>
> xfs version?  Just for my own education, which version does that?

yeah.

can't remember. I will eventually have to rebuild them all starting with 
Linux 3.0 (where seek_data was not supported), 3.1-3.3 used the vfs 
defaults. Linux 3.4 is where seek_data was introduced to XFS. There are 
3-4 incremental changes to the seek_data since then and they all change 
some output.

>
>>> DATA    EOF
>>>
>>> For a 1M 100% sparse file (i_size and no blocks at all) we get:
>>>
>>> # io/xfs_io -c "seek -ar 0" allsparse
>>> Type    offset
>>> HOLE    0
>>> DATA    EOF
>>>
>>> For a 1M file w/ only the first 512k w/ data, then hole,
>>> we get:
>>>
>>> # io/xfs_io -c "seek -ar 0" endhole
>>> Type    offset
>>> DATA    0
>>> HOLE    524288
>>> DATA    EOF
>>>
>>> For a 1M file w/ 512k of hole and then 512k w/ data, we get:
>>>
>>> # io/xfs_io -c "seek -ar 0" starthole
>>> Type    offset
>>> HOLE    0
>>> DATA    524288
>>> HOLE    1048576
>>> DATA    EOF
>>>
>>> So in each case, the "DATA  EOF" at the end seems odd to me.
>>>
>>> And in each case above, the output is unique w/o the EOF flag
>>> anwyway, right?
>>
>> ... or we will get "HOLE EOF"
>>
>> There are different versions of XFS seek_data and they will
>> detect/report the start of data and holes differently so output
>> parsing will be a bear. The existing C code sends the 2 different
>> value numbers that could be reported.
>
> are they ... both correct?  If one is a bug, it can just be a bug, right?
> I'm sorry I'm not up on the history.

Lets say we have a file
hole    0-4K
data    4K-8K
hole    8-12K
data    12-16K

for data/hole check starting at offset 0, valid response are
0K or 4K for data
0K or 16K or -1 for holes

This feature and test was for Jeff fine-tuned seek_data/seek_hole 
support. The tests would be more specific to that feature and output is 
specific.

>
>> The later, advance dirty page detection is more fine tuned. Jeff and
>> I had C tests for this feature that I turned into a xfstest; it was
>> suggested that the C test become xfs_io call, and then 5 versions
>> later, we have this ...
>
> 6 versions.  :D
>

auuuuugggh, even I lost count. :)

>>
>>>
>>> I'm probably missing it; in what cases is the EOF record
>>> useful?  It just seems beyond the scope of SEEK_HOLE/SEEK_DATA.
>>> (i.e. EOF is SEEK_END)
>>>
>>> If the EOF is really helpful, maybe it is possible instead to do something like:
>>>
>>> # io/xfs_io -c "seek -ar 0" starthole
>>> Type    offset
>>> HOLE    0
>>> DATA    524288
>>> EOF    1048575
>>> HOLE    1048576
>>>
>>> That makes more intuitive sense to me if you really need the EOF
>>> record, but I dunno, what do you think?
>>>
>> I can drop the table header.
>>
>> We can leave off the implied eof - there will be cases where there is no entries.
>
> Well, whatever you think, I guess.  Given that the interface is _defined_ as having
> an implicit hole past EOF, saying "DATA EOF" just hurts my brain.
>
> Maybe the guiding principle should be: this is a tool to exercise lseek for
> SEEK_DATA / SEEK_HOLE.  It should report the results of those ops, and no
> more; just present what the requested call(s) said.  If you really want to know
> where EOF is, use stat?
>
> (Since the command is just called "seek" maybe some day it will grow
> -e -s -c options for SEEK_END, SEEK_SET, and SEEK_CUR as well, to be
> able to exercise every "whence" - and then if you want to know EOF,
> just send it SEEK_END and see what it returns)
>

In one of my many versions, I made sure there was at least one entry - 
if there was no entry I put the EOF.

I can live with no output.


>>>>> I guess "DATA" doesn't get replaced by "0" ?  Sorry, I failed cpp 101.
>>>>> It prints the right thing so I guess not.  ;)
>>>>>
>>>>
>>>> :) no the defines are subscripts = see "current ="
>>>
>>> I did see that, I just wasn't sure if it'd replace it in literal
>>> strings, but apparently not.
>>>
>>
>> nope, strings are safe - did Coverity complain?
>
> No, just my dumb brain.
>
>
> -Eric

Igor fetched Abbie Normal's brain for me.

--Mark.

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Eric Sandeen]

On 8/21/13 11:52 AM, Mark Tinguely wrote:
...

>> I think it makes sense to build it&  locally define if necessary.
>> On my RHEL6 root w/ an upstream devel kernel seek.c wouldn't have
>> built, even though it'd have worked perfectly w/ a local define.
>>
> 
> yes, needed anyway if removing linux/fs.h

lseek only should need:

       #include <sys/types.h>
       #include <unistd.h>

right; those may internally get to linux/fs.h but it shouldn't be
directly required, I'd expect.  Oh!  it needs

#define _GNU_SOURCE

first, to get it - but xfsprogs build does that already.

>> So let me just think out loud here w/ examples.
>>
>> For a 1M 100% nonsparse file we get:
>>
>> # io/xfs_io -c "seek -ar 0" alldata
>> Type    offset
>> DATA    0
>> HOLE    1048576
> 
> or this could be HOLE EOF depends on the version.

xfs version?  Just for my own education, which version does that?

>> DATA    EOF
>>
>> For a 1M 100% sparse file (i_size and no blocks at all) we get:
>>
>> # io/xfs_io -c "seek -ar 0" allsparse
>> Type    offset
>> HOLE    0
>> DATA    EOF
>>
>> For a 1M file w/ only the first 512k w/ data, then hole,
>> we get:
>>
>> # io/xfs_io -c "seek -ar 0" endhole
>> Type    offset
>> DATA    0
>> HOLE    524288
>> DATA    EOF
>>
>> For a 1M file w/ 512k of hole and then 512k w/ data, we get:
>>
>> # io/xfs_io -c "seek -ar 0" starthole
>> Type    offset
>> HOLE    0
>> DATA    524288
>> HOLE    1048576
>> DATA    EOF
>>
>> So in each case, the "DATA  EOF" at the end seems odd to me.
>>
>> And in each case above, the output is unique w/o the EOF flag
>> anwyway, right?
> 
> ... or we will get "HOLE EOF"
> 
> There are different versions of XFS seek_data and they will
> detect/report the start of data and holes differently so output
> parsing will be a bear. The existing C code sends the 2 different
> value numbers that could be reported.

are they ... both correct?  If one is a bug, it can just be a bug, right?
I'm sorry I'm not up on the history.

> The later, advance dirty page detection is more fine tuned. Jeff and
> I had C tests for this feature that I turned into a xfstest; it was
> suggested that the C test become xfs_io call, and then 5 versions
> later, we have this ...

6 versions.  :D

> 
>>
>> I'm probably missing it; in what cases is the EOF record
>> useful?  It just seems beyond the scope of SEEK_HOLE/SEEK_DATA.
>> (i.e. EOF is SEEK_END)
>>
>> If the EOF is really helpful, maybe it is possible instead to do something like:
>>
>> # io/xfs_io -c "seek -ar 0" starthole
>> Type    offset
>> HOLE    0
>> DATA    524288
>> EOF    1048575
>> HOLE    1048576
>>
>> That makes more intuitive sense to me if you really need the EOF
>> record, but I dunno, what do you think?
>>
> I can drop the table header.
> 
> We can leave off the implied eof - there will be cases where there is no entries.

Well, whatever you think, I guess.  Given that the interface is _defined_ as having
an implicit hole past EOF, saying "DATA EOF" just hurts my brain.

Maybe the guiding principle should be: this is a tool to exercise lseek for
SEEK_DATA / SEEK_HOLE.  It should report the results of those ops, and no
more; just present what the requested call(s) said.  If you really want to know
where EOF is, use stat?

(Since the command is just called "seek" maybe some day it will grow
-e -s -c options for SEEK_END, SEEK_SET, and SEEK_CUR as well, to be
able to exercise every "whence" - and then if you want to know EOF,
just send it SEEK_END and see what it returns)

>>>> I guess "DATA" doesn't get replaced by "0" ?  Sorry, I failed cpp 101.
>>>> It prints the right thing so I guess not.  ;)
>>>>
>>>
>>> :) no the defines are subscripts = see "current ="
>>
>> I did see that, I just wasn't sure if it'd replace it in literal
>> strings, but apparently not.
>>
> 
> nope, strings are safe - did Coverity complain?

No, just my dumb brain.


-Eric

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Mark Tinguely]

On 08/21/13 11:28, Eric Sandeen wrote:
> On 8/21/13 9:14 AM, Mark Tinguely wrote:
>>
>> This patch started as an xfstest to test Jeff's advanced seek_data features. The C code we had for that feature was deemed as an xfs_io feature.
>
> *nod*
>
> Forgive me for looking more carefully now than then, sorry.
>
> Argh, and for missing that you're already on V5, I missed
> the previous reviews.  Well, I did find at least one speling eror,
> so there's that.  But more below...

only 1?


>
>> On 08/20/13 18:07, Eric Sandeen wrote:
>>> On 8/16/13 3:54 PM, Mark Tinguely wrote:
>>>
>>>> Add the lseek SEEK_DATA/SEEK_HOLE support into xfs_io.
>>>> The result from the lseek() call will be printed to the output.
>>>> For example:
>>>>
>>>> xfs_io>   lseek -h 609k
>>>> Type    Offset
>>>> hole    630784
>>>
>>> HOLE not hole, I guess.  ;)
>>>
>>> I was going to say that's a lot of verbosity for a single output,
>>> but I guess the other options might have many lines, so I suppose
>>> it makes sense.
>>>
>>> (I was just thinking about what xfstests might need to do to filter
>>> &   parse output...)
>>
>> parsing is a bear because there are multiple correct answers.
>> There is always a legal hole at EOF and that if SEEK_HOLE is not implemented that is the answer they give.
>>
>> Different versions of XFS seek_data code will give different answer to the same test depending on what is supported in that version.
>>
>>>
>>>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>>>> ---
>>>>    Not trying to be difficult. Dave wanted the single hole/data/hole and data
>>>>    seperated from the recursive loop, but doing it that way is basically unrolling
>>>>    the loop into a if-then-else and is really terrible. If this is still not
>>>>    acceptable, then we can throw this feature into /dev/null.
>>>>
>>>>    configure.ac          |    1
>>>>    include/builddefs.in  |    1
>>>>    io/Makefile           |    5 +
>>>>    io/init.c             |    1
>>>>    io/io.h               |    6 +
>>>>    io/seek.c             |  187 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    m4/package_libcdev.m4 |   15 ++++
>>>>    man/man8/xfs_io.8     |   35 +++++++++
>>>>    8 files changed, 251 insertions(+)
>>>>
>>>> Index: b/configure.ac
>>>> ===================================================================
>>>> --- a/configure.ac
>>>> +++ b/configure.ac
>>>> @@ -110,6 +110,7 @@ AC_HAVE_GETMNTINFO
>>>>    AC_HAVE_FALLOCATE
>>>>    AC_HAVE_FIEMAP
>>>>    AC_HAVE_PREADV
>>>> +AC_HAVE_SEEK_DATA
>>>>    AC_HAVE_SYNC_FILE_RANGE
>>>>    AC_HAVE_BLKID_TOPO($enable_blkid)
>>>>    AC_HAVE_READDIR
>>>> Index: b/include/builddefs.in
>>>> ===================================================================
>>>> --- a/include/builddefs.in
>>>> +++ b/include/builddefs.in
>>>> @@ -102,6 +102,7 @@ HAVE_GETMNTINFO = @have_getmntinfo@
>>>>    HAVE_FALLOCATE = @have_fallocate@
>>>>    HAVE_FIEMAP = @have_fiemap@
>>>>    HAVE_PREADV = @have_preadv@
>>>> +HAVE_SEEK_DATA = @have_seek_data@
>>>>    HAVE_SYNC_FILE_RANGE = @have_sync_file_range@
>>>>    HAVE_READDIR = @have_readdir@
>>>>
>>>> Index: b/io/Makefile
>>>> ===================================================================
>>>> --- a/io/Makefile
>>>> +++ b/io/Makefile
>>>> @@ -85,6 +85,11 @@ CFILES += readdir.c
>>>>    LCFLAGS += -DHAVE_READDIR
>>>>    endif
>>>>
>>>> +ifeq ($(HAVE_SEEK_DATA),yes)
>>>> +LCFLAGS += -DHAVE_SEEK_DATA
>>>> +CFILES += seek.c
>>>
>>> see below; we should unconditionally compile, but conditionally
>>> locally define SEEK_DATA / SEEK_HOLE
>>>
>>
>> It was put in to check if SEEK_DATA is supported.
>>
>> Yes, it expects the user headers to reflect what the kernel is capable of doing.
>
> well, especially on a development system, the installed headers may not
> reflect or match the running kernel.
>
> So even if system headers don't have SEEK_DATA it, the running kernel may
> still be capable of it, right?
>
> We've done similar things for i.e. fallocate PUNCH_HOLE.
>
>> If you don't want it, then it will be removed.
>
> I think it makes sense to build it&  locally define if necessary.
> On my RHEL6 root w/ an upstream devel kernel seek.c wouldn't have
> built, even though it'd have worked perfectly w/ a local define.
>

yes, needed anyway if removing linux/fs.h

>>>> +endif
>>>> +
>>>>    default: depend $(LTCOMMAND)
>>>>
>>>>    include $(BUILDRULES)
>>>> Index: b/io/init.c
>>>> ===================================================================
>>>> --- a/io/init.c
>>>> +++ b/io/init.c
>>>> @@ -64,6 +64,7 @@ init_commands(void)
>>>>        help_init();
>>>>        imap_init();
>>>>        inject_init();
>>>> +    seek_init();
>>>>        madvise_init();
>>>>        mincore_init();
>>>>        mmap_init();
>>>> Index: b/io/io.h
>>>> ===================================================================
>>>> --- a/io/io.h
>>>> +++ b/io/io.h
>>>> @@ -108,6 +108,12 @@ extern void        quit_init(void);
>>>>    extern void        shutdown_init(void);
>>>>    extern void        truncate_init(void);
>>>>
>>>> +#ifdef HAVE_SEEK_DATA
>>>> +extern void        seek_init(void);
>>>> +#else
>>>> +#define seek_init()    do { } while (0)
>>>> +#endif
>>>
>>> this can go when we unconditionally compile it in
>>>
>>>> +
>>>>    #ifdef HAVE_FADVISE
>>>>    extern void        fadvise_init(void);
>>>>    #else
>>>> Index: b/io/seek.c
>>>> ===================================================================
>>>> --- /dev/null
>>>> +++ b/io/seek.c
>>>> @@ -0,0 +1,187 @@
>>>> +/*
>>>> + * Copyright (c) 2013 SGI
>>>> + * 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<libxfs.h>
>>>
>>> hm, including this clashes w/ the min() define in io/init.h,
>>> which is maybe a separate problem down the line, but libxfs.h
>>> isn't required anyway for this file, so I'd just drop this include.
>>>
>>>> +#include<linux/fs.h>
>>
>> I think the previous review had a problem with this header which should be removed.
>
> oh yeah, Dave did ask (now that I'm caught up with the last 4 reviews) :(
>
> And yeah it builds fine w/o either libxfs.h or linux/fs.h, so I'd just yank
> 'em both.
>

yes, I missed them from Dave's review - my mistake.

>>>> +
>>>> +#include<sys/uio.h>
>>>> +#include<xfs/xfs.h>
>>>> +#include<xfs/command.h>
>>>> +#include<xfs/input.h>
>>>> +#include<ctype.h>
>>>> +#include "init.h"
>>>> +#include "io.h"
>>>
>>> #ifndef HAVE_SEEK_DATA
>>> #define SEEK_DATA       3       /* seek to the next data */
>>> #define SEEK_HOLE       4       /* seek to the next hole */
>>> #endif
>>>
>>>> +
>>>> +static cmdinfo_t seek_cmd;
>>>> +
>>>> +static void
>>>> +seek_help(void)
>>>> +{
>>>> +    printf(_(
>>>> +"\n"
>>>> +" returns the next hole and/or data offset at or after the specified offset\n"
>>>> +"\n"
>>>> +" Example:\n"
>>>> +" 'seek -d 512'   - offset of data at or following offset 512\n"
>>>> +" 'seek -a -r 0'  - offsets of all data and hole in entire file\n"
>>>> +"\n"
>>>> +" Returns the offset of the next data and/or hole. There is an implied hole\n"
>>>> +" at the end of file.
>>>
>>> is this expected, given the hole at the end of the file?  This is for a single
>>> non-sparse file:
>>>
>>> xfs_io>   seek -ar 0
>>> Type    offset
>>> DATA    0
>>> HOLE    3022
>>> DATA    EOF
>>>
>>> That last line doesn't make sense, does it?
>>
>> Parsing is the reason the entry is there so the output always has
>> consistent ending entry - some queries that is the only answer (or
>> now no message) no biggy one way or the other.
>
> Hm, ok, clearly you've thought about this more than I have.
> It just surprised me...
>
> So let me just think out loud here w/ examples.
>
> For a 1M 100% nonsparse file we get:
>
> # io/xfs_io -c "seek -ar 0" alldata
> Type	offset
> DATA	0
> HOLE	1048576

or this could be HOLE EOF depends on the version.

> DATA	EOF
>
> For a 1M 100% sparse file (i_size and no blocks at all) we get:
>
> # io/xfs_io -c "seek -ar 0" allsparse
> Type	offset
> HOLE	0
> DATA	EOF
>
> For a 1M file w/ only the first 512k w/ data, then hole,
> we get:
>
> # io/xfs_io -c "seek -ar 0" endhole
> Type	offset
> DATA	0
> HOLE	524288
> DATA	EOF
>
> For a 1M file w/ 512k of hole and then 512k w/ data, we get:
>
> # io/xfs_io -c "seek -ar 0" starthole
> Type	offset
> HOLE	0
> DATA	524288
> HOLE	1048576
> DATA	EOF
>
> So in each case, the "DATA  EOF" at the end seems odd to me.
>
> And in each case above, the output is unique w/o the EOF flag
> anwyway, right?

... or we will get "HOLE EOF"

There are different versions of XFS seek_data and they will 
detect/report the start of data and holes differently so output parsing 
will be a bear. The existing C code sends the 2 different value numbers
that could be reported.

The later, advance dirty page detection is more fine tuned. Jeff and I 
had C tests for this feature that I turned into a xfstest; it was 
suggested that the C test become xfs_io call, and then 5 versions later, 
we have this ...

>
> I'm probably missing it; in what cases is the EOF record
> useful?  It just seems beyond the scope of SEEK_HOLE/SEEK_DATA.
> (i.e. EOF is SEEK_END)
>
> If the EOF is really helpful, maybe it is possible instead to do something like:
>
> # io/xfs_io -c "seek -ar 0" starthole
> Type	offset
> HOLE	0
> DATA	524288
> EOF	1048575
> HOLE	1048576
>
> That makes more intuitive sense to me if you really need the EOF
> record, but I dunno, what do you think?
>
I can drop the table header.

We can leave off the implied eof - there will be cases where there is no 
entries.

>>>
>>>> If the specified offset is past end of file, or there\n"
>>>> +" is no data past the specied offset, EOF is returned.\n"
>>>
>>> "specified"
>>>
>>>> +" -a   -- return the next data and hole starting at the specified offset.\n"
>>>> +" -d   -- return the next data starting at the specified offset.\n"
>>>> +" -h   -- return the next hole starting at the specified offset.\n"
>>>> +" -r   -- return all remaining type(s) starting at the specified offset.\n"
>>>> +"\n"));
>>>> +}
>>>> +
>>>> +#define    SEEK_DFLAG    (1<<   0)
>>>> +#define    SEEK_HFLAG    (1<<   1)
>>>> +#define    SEEK_RFLAG    (1<<   2)
>>>> +#define    DATA        0
>>>> +#define    HOLE        1
>>>> +
>>>> +struct seekinfo {
>>>> +    char        *name;
>>>> +    int        seektype;
>>>> +    int        mask;
>>>> +} seekinfo[] = {
>>>> +        {"DATA", SEEK_DATA, SEEK_DFLAG},
>>>> +        {"HOLE", SEEK_HOLE, SEEK_HFLAG}
>>>
>>> I guess "DATA" doesn't get replaced by "0" ?  Sorry, I failed cpp 101.
>>> It prints the right thing so I guess not.  ;)
>>>
>>
>> :) no the defines are subscripts = see "current ="
>
> I did see that, I just wasn't sure if it'd replace it in literal
> strings, but apparently not.
>

nope, strings are safe - did Coverity complain?

>>
>>>> +};
>>>> +
>>>> +void
>>>> +seek_output(
>>>> +    char    *type,
>>>> +    off64_t    offset)
>>>> +{
>>>> +    if (offset == -1) {
>>>> +        if (errno == ENXIO)
>>>> +            printf("%s    EOF\n", type);
>>>> +        else
>>>> +            printf("%s    ERR %d\n", type, errno);
>>>> +    } else
>>>> +        printf("%s    %ld\n", type, offset);
>
> one more; for 32-bit systems I think this should be
>
> printf("%s    %lld\n", type, (long long)offset);
>
> to avoid a warning; that's what i.e. the pwrite printf's do.
>

okay, thanks.

>>>> +}
>>>> +
>>>> +static int
>>>> +seek_f(
>>>> +    int    argc,
>>>> +    char    **argv)
>>>> +{
>>>> +    off64_t        offset, result;
>>>> +    size_t          fsblocksize, fssectsize;
>>>> +    int        flag;
>>>> +    int        current;    /* specify data or hole */
>>>> +    int        c;
>>>> +
>>>> +    flag = 0;
>>>> +    init_cvtnum(&fsblocksize,&fssectsize);
>>>> +
>>>> +    while ((c = getopt(argc, argv, "adhr")) != EOF) {
>>>> +        switch (c) {
>>>> +        case 'a':
>>>> +            flag |= (SEEK_HFLAG | SEEK_DFLAG);
>>>> +            break;
>>>> +        case 'd':
>>>> +            flag |= SEEK_DFLAG;
>>>> +            break;
>>>> +        case 'h':
>>>> +            flag |= SEEK_HFLAG;
>>>> +            break;
>>>> +        case 'r':
>>>> +            flag |= SEEK_RFLAG;
>>>> +            break;
>>>> +        default:
>>>> +            return command_usage(&seek_cmd);
>>>> +        }
>>>> +    }
>>>> +        /* must have hole or data specified and an offset */
>
> super-nitpick, extra tab before the comment.
>

not a problem.

>>>> +    if (!(flag&   (SEEK_DFLAG | SEEK_HFLAG)) ||
>>>> +             optind != argc - 1)
>>>> +        return command_usage(&seek_cmd);
>>>> +
>>>> +    offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
>>>
>>> need to error check that:
>>>
>>> xfs_io>   seek -a 8x8
>>> Type    offset
>>> HOLE    EOF
>>>
>>
>> Some version of XFS seek_data will treat it as a 0, but okay.
>
> but I mean the error from cvtnum, if you don't give it a valid string;
> nothing to do w/ seek_data ...
>

duh me. okay.

> Thanks,
> -Eric

Thanks.

--Mark.

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Eric Sandeen]

On 8/21/13 9:14 AM, Mark Tinguely wrote:
> 
> This patch started as an xfstest to test Jeff's advanced seek_data features. The C code we had for that feature was deemed as an xfs_io feature.

*nod*

Forgive me for looking more carefully now than then, sorry.

Argh, and for missing that you're already on V5, I missed
the previous reviews.  Well, I did find at least one speling eror,
so there's that.  But more below...

> On 08/20/13 18:07, Eric Sandeen wrote:
>> On 8/16/13 3:54 PM, Mark Tinguely wrote:
>>
>>> Add the lseek SEEK_DATA/SEEK_HOLE support into xfs_io.
>>> The result from the lseek() call will be printed to the output.
>>> For example:
>>>
>>> xfs_io>  lseek -h 609k
>>> Type    Offset
>>> hole    630784
>>
>> HOLE not hole, I guess.  ;)
>>
>> I was going to say that's a lot of verbosity for a single output,
>> but I guess the other options might have many lines, so I suppose
>> it makes sense.
>>
>> (I was just thinking about what xfstests might need to do to filter
>> &  parse output...)
> 
> parsing is a bear because there are multiple correct answers.
> There is always a legal hole at EOF and that if SEEK_HOLE is not implemented that is the answer they give.
> 
> Different versions of XFS seek_data code will give different answer to the same test depending on what is supported in that version.
> 
>>
>>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>>> ---
>>>   Not trying to be difficult. Dave wanted the single hole/data/hole and data
>>>   seperated from the recursive loop, but doing it that way is basically unrolling
>>>   the loop into a if-then-else and is really terrible. If this is still not
>>>   acceptable, then we can throw this feature into /dev/null.
>>>
>>>   configure.ac          |    1
>>>   include/builddefs.in  |    1
>>>   io/Makefile           |    5 +
>>>   io/init.c             |    1
>>>   io/io.h               |    6 +
>>>   io/seek.c             |  187 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   m4/package_libcdev.m4 |   15 ++++
>>>   man/man8/xfs_io.8     |   35 +++++++++
>>>   8 files changed, 251 insertions(+)
>>>
>>> Index: b/configure.ac
>>> ===================================================================
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -110,6 +110,7 @@ AC_HAVE_GETMNTINFO
>>>   AC_HAVE_FALLOCATE
>>>   AC_HAVE_FIEMAP
>>>   AC_HAVE_PREADV
>>> +AC_HAVE_SEEK_DATA
>>>   AC_HAVE_SYNC_FILE_RANGE
>>>   AC_HAVE_BLKID_TOPO($enable_blkid)
>>>   AC_HAVE_READDIR
>>> Index: b/include/builddefs.in
>>> ===================================================================
>>> --- a/include/builddefs.in
>>> +++ b/include/builddefs.in
>>> @@ -102,6 +102,7 @@ HAVE_GETMNTINFO = @have_getmntinfo@
>>>   HAVE_FALLOCATE = @have_fallocate@
>>>   HAVE_FIEMAP = @have_fiemap@
>>>   HAVE_PREADV = @have_preadv@
>>> +HAVE_SEEK_DATA = @have_seek_data@
>>>   HAVE_SYNC_FILE_RANGE = @have_sync_file_range@
>>>   HAVE_READDIR = @have_readdir@
>>>
>>> Index: b/io/Makefile
>>> ===================================================================
>>> --- a/io/Makefile
>>> +++ b/io/Makefile
>>> @@ -85,6 +85,11 @@ CFILES += readdir.c
>>>   LCFLAGS += -DHAVE_READDIR
>>>   endif
>>>
>>> +ifeq ($(HAVE_SEEK_DATA),yes)
>>> +LCFLAGS += -DHAVE_SEEK_DATA
>>> +CFILES += seek.c
>>
>> see below; we should unconditionally compile, but conditionally
>> locally define SEEK_DATA / SEEK_HOLE
>>
> 
> It was put in to check if SEEK_DATA is supported.
> 
> Yes, it expects the user headers to reflect what the kernel is capable of doing.

well, especially on a development system, the installed headers may not
reflect or match the running kernel.

So even if system headers don't have SEEK_DATA it, the running kernel may
still be capable of it, right?

We've done similar things for i.e. fallocate PUNCH_HOLE.

> If you don't want it, then it will be removed.

I think it makes sense to build it & locally define if necessary.
On my RHEL6 root w/ an upstream devel kernel seek.c wouldn't have
built, even though it'd have worked perfectly w/ a local define.

>>> +endif
>>> +
>>>   default: depend $(LTCOMMAND)
>>>
>>>   include $(BUILDRULES)
>>> Index: b/io/init.c
>>> ===================================================================
>>> --- a/io/init.c
>>> +++ b/io/init.c
>>> @@ -64,6 +64,7 @@ init_commands(void)
>>>       help_init();
>>>       imap_init();
>>>       inject_init();
>>> +    seek_init();
>>>       madvise_init();
>>>       mincore_init();
>>>       mmap_init();
>>> Index: b/io/io.h
>>> ===================================================================
>>> --- a/io/io.h
>>> +++ b/io/io.h
>>> @@ -108,6 +108,12 @@ extern void        quit_init(void);
>>>   extern void        shutdown_init(void);
>>>   extern void        truncate_init(void);
>>>
>>> +#ifdef HAVE_SEEK_DATA
>>> +extern void        seek_init(void);
>>> +#else
>>> +#define seek_init()    do { } while (0)
>>> +#endif
>>
>> this can go when we unconditionally compile it in
>>
>>> +
>>>   #ifdef HAVE_FADVISE
>>>   extern void        fadvise_init(void);
>>>   #else
>>> Index: b/io/seek.c
>>> ===================================================================
>>> --- /dev/null
>>> +++ b/io/seek.c
>>> @@ -0,0 +1,187 @@
>>> +/*
>>> + * Copyright (c) 2013 SGI
>>> + * 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<libxfs.h>
>>
>> hm, including this clashes w/ the min() define in io/init.h,
>> which is maybe a separate problem down the line, but libxfs.h
>> isn't required anyway for this file, so I'd just drop this include.
>>
>>> +#include<linux/fs.h>
> 
> I think the previous review had a problem with this header which should be removed.

oh yeah, Dave did ask (now that I'm caught up with the last 4 reviews) :(

And yeah it builds fine w/o either libxfs.h or linux/fs.h, so I'd just yank
'em both.

>>> +
>>> +#include<sys/uio.h>
>>> +#include<xfs/xfs.h>
>>> +#include<xfs/command.h>
>>> +#include<xfs/input.h>
>>> +#include<ctype.h>
>>> +#include "init.h"
>>> +#include "io.h"
>>
>> #ifndef HAVE_SEEK_DATA
>> #define SEEK_DATA       3       /* seek to the next data */
>> #define SEEK_HOLE       4       /* seek to the next hole */
>> #endif
>>
>>> +
>>> +static cmdinfo_t seek_cmd;
>>> +
>>> +static void
>>> +seek_help(void)
>>> +{
>>> +    printf(_(
>>> +"\n"
>>> +" returns the next hole and/or data offset at or after the specified offset\n"
>>> +"\n"
>>> +" Example:\n"
>>> +" 'seek -d 512'   - offset of data at or following offset 512\n"
>>> +" 'seek -a -r 0'  - offsets of all data and hole in entire file\n"
>>> +"\n"
>>> +" Returns the offset of the next data and/or hole. There is an implied hole\n"
>>> +" at the end of file.
>>
>> is this expected, given the hole at the end of the file?  This is for a single
>> non-sparse file:
>>
>> xfs_io>  seek -ar 0
>> Type    offset
>> DATA    0
>> HOLE    3022
>> DATA    EOF
>>
>> That last line doesn't make sense, does it?
> 
> Parsing is the reason the entry is there so the output always has
> consistent ending entry - some queries that is the only answer (or
> now no message) no biggy one way or the other.

Hm, ok, clearly you've thought about this more than I have.
It just surprised me...

So let me just think out loud here w/ examples.

For a 1M 100% nonsparse file we get:

# io/xfs_io -c "seek -ar 0" alldata
Type	offset
DATA	0
HOLE	1048576
DATA	EOF

For a 1M 100% sparse file (i_size and no blocks at all) we get:

# io/xfs_io -c "seek -ar 0" allsparse 
Type	offset
HOLE	0
DATA	EOF

For a 1M file w/ only the first 512k w/ data, then hole,
we get:

# io/xfs_io -c "seek -ar 0" endhole 
Type	offset
DATA	0
HOLE	524288
DATA	EOF

For a 1M file w/ 512k of hole and then 512k w/ data, we get:

# io/xfs_io -c "seek -ar 0" starthole 
Type	offset
HOLE	0
DATA	524288
HOLE	1048576
DATA	EOF

So in each case, the "DATA  EOF" at the end seems odd to me.

And in each case above, the output is unique w/o the EOF flag
anwyway, right?

I'm probably missing it; in what cases is the EOF record
useful?  It just seems beyond the scope of SEEK_HOLE/SEEK_DATA.
(i.e. EOF is SEEK_END)

If the EOF is really helpful, maybe it is possible instead to do something like:

# io/xfs_io -c "seek -ar 0" starthole 
Type	offset
HOLE	0
DATA	524288
EOF	1048575
HOLE	1048576

That makes more intuitive sense to me if you really need the EOF
record, but I dunno, what do you think?

>>
>>> If the specified offset is past end of file, or there\n"
>>> +" is no data past the specied offset, EOF is returned.\n"
>>
>> "specified"
>>
>>> +" -a   -- return the next data and hole starting at the specified offset.\n"
>>> +" -d   -- return the next data starting at the specified offset.\n"
>>> +" -h   -- return the next hole starting at the specified offset.\n"
>>> +" -r   -- return all remaining type(s) starting at the specified offset.\n"
>>> +"\n"));
>>> +}
>>> +
>>> +#define    SEEK_DFLAG    (1<<  0)
>>> +#define    SEEK_HFLAG    (1<<  1)
>>> +#define    SEEK_RFLAG    (1<<  2)
>>> +#define    DATA        0
>>> +#define    HOLE        1
>>> +
>>> +struct seekinfo {
>>> +    char        *name;
>>> +    int        seektype;
>>> +    int        mask;
>>> +} seekinfo[] = {
>>> +        {"DATA", SEEK_DATA, SEEK_DFLAG},
>>> +        {"HOLE", SEEK_HOLE, SEEK_HFLAG}
>>
>> I guess "DATA" doesn't get replaced by "0" ?  Sorry, I failed cpp 101.
>> It prints the right thing so I guess not.  ;)
>>
> 
> :) no the defines are subscripts = see "current ="

I did see that, I just wasn't sure if it'd replace it in literal
strings, but apparently not.

> 
>>> +};
>>> +
>>> +void
>>> +seek_output(
>>> +    char    *type,
>>> +    off64_t    offset)
>>> +{
>>> +    if (offset == -1) {
>>> +        if (errno == ENXIO)
>>> +            printf("%s    EOF\n", type);
>>> +        else
>>> +            printf("%s    ERR %d\n", type, errno);
>>> +    } else
>>> +        printf("%s    %ld\n", type, offset);

one more; for 32-bit systems I think this should be

printf("%s    %lld\n", type, (long long)offset);

to avoid a warning; that's what i.e. the pwrite printf's do.

>>> +}
>>> +
>>> +static int
>>> +seek_f(
>>> +    int    argc,
>>> +    char    **argv)
>>> +{
>>> +    off64_t        offset, result;
>>> +    size_t          fsblocksize, fssectsize;
>>> +    int        flag;
>>> +    int        current;    /* specify data or hole */
>>> +    int        c;
>>> +
>>> +    flag = 0;
>>> +    init_cvtnum(&fsblocksize,&fssectsize);
>>> +
>>> +    while ((c = getopt(argc, argv, "adhr")) != EOF) {
>>> +        switch (c) {
>>> +        case 'a':
>>> +            flag |= (SEEK_HFLAG | SEEK_DFLAG);
>>> +            break;
>>> +        case 'd':
>>> +            flag |= SEEK_DFLAG;
>>> +            break;
>>> +        case 'h':
>>> +            flag |= SEEK_HFLAG;
>>> +            break;
>>> +        case 'r':
>>> +            flag |= SEEK_RFLAG;
>>> +            break;
>>> +        default:
>>> +            return command_usage(&seek_cmd);
>>> +        }
>>> +    }
>>> +        /* must have hole or data specified and an offset */

super-nitpick, extra tab before the comment.

>>> +    if (!(flag&  (SEEK_DFLAG | SEEK_HFLAG)) ||
>>> +             optind != argc - 1)
>>> +        return command_usage(&seek_cmd);
>>> +
>>> +    offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
>>
>> need to error check that:
>>
>> xfs_io>  seek -a 8x8
>> Type    offset
>> HOLE    EOF
>>
> 
> Some version of XFS seek_data will treat it as a 0, but okay.

but I mean the error from cvtnum, if you don't give it a valid string;
nothing to do w/ seek_data ...

Thanks,
-Eric

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Mark Tinguely]

This patch started as an xfstest to test Jeff's advanced seek_data 
features. The C code we had for that feature was deemed as an xfs_io 
feature.

On 08/20/13 18:07, Eric Sandeen wrote:
> On 8/16/13 3:54 PM, Mark Tinguely wrote:
>
>> Add the lseek SEEK_DATA/SEEK_HOLE support into xfs_io.
>> The result from the lseek() call will be printed to the output.
>> For example:
>>
>> xfs_io>  lseek -h 609k
>> Type	Offset
>> hole	630784
>
> HOLE not hole, I guess.  ;)
>
> I was going to say that's a lot of verbosity for a single output,
> but I guess the other options might have many lines, so I suppose
> it makes sense.
>
> (I was just thinking about what xfstests might need to do to filter
> &  parse output...)

parsing is a bear because there are multiple correct answers.
There is always a legal hole at EOF and that if SEEK_HOLE is not 
implemented that is the answer they give.

Different versions of XFS seek_data code will give different answer to 
the same test depending on what is supported in that version.

>
>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>> ---
>>   Not trying to be difficult. Dave wanted the single hole/data/hole and data
>>   seperated from the recursive loop, but doing it that way is basically unrolling
>>   the loop into a if-then-else and is really terrible. If this is still not
>>   acceptable, then we can throw this feature into /dev/null.
>>
>>   configure.ac          |    1
>>   include/builddefs.in  |    1
>>   io/Makefile           |    5 +
>>   io/init.c             |    1
>>   io/io.h               |    6 +
>>   io/seek.c             |  187 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   m4/package_libcdev.m4 |   15 ++++
>>   man/man8/xfs_io.8     |   35 +++++++++
>>   8 files changed, 251 insertions(+)
>>
>> Index: b/configure.ac
>> ===================================================================
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -110,6 +110,7 @@ AC_HAVE_GETMNTINFO
>>   AC_HAVE_FALLOCATE
>>   AC_HAVE_FIEMAP
>>   AC_HAVE_PREADV
>> +AC_HAVE_SEEK_DATA
>>   AC_HAVE_SYNC_FILE_RANGE
>>   AC_HAVE_BLKID_TOPO($enable_blkid)
>>   AC_HAVE_READDIR
>> Index: b/include/builddefs.in
>> ===================================================================
>> --- a/include/builddefs.in
>> +++ b/include/builddefs.in
>> @@ -102,6 +102,7 @@ HAVE_GETMNTINFO = @have_getmntinfo@
>>   HAVE_FALLOCATE = @have_fallocate@
>>   HAVE_FIEMAP = @have_fiemap@
>>   HAVE_PREADV = @have_preadv@
>> +HAVE_SEEK_DATA = @have_seek_data@
>>   HAVE_SYNC_FILE_RANGE = @have_sync_file_range@
>>   HAVE_READDIR = @have_readdir@
>>
>> Index: b/io/Makefile
>> ===================================================================
>> --- a/io/Makefile
>> +++ b/io/Makefile
>> @@ -85,6 +85,11 @@ CFILES += readdir.c
>>   LCFLAGS += -DHAVE_READDIR
>>   endif
>>
>> +ifeq ($(HAVE_SEEK_DATA),yes)
>> +LCFLAGS += -DHAVE_SEEK_DATA
>> +CFILES += seek.c
>
> see below; we should unconditionally compile, but conditionally
> locally define SEEK_DATA / SEEK_HOLE
>

It was put in to check if SEEK_DATA is supported.

Yes, it expects the user headers to reflect what the kernel is capable 
of doing.

If you don't want it, then it will be removed.

>> +endif
>> +
>>   default: depend $(LTCOMMAND)
>>
>>   include $(BUILDRULES)
>> Index: b/io/init.c
>> ===================================================================
>> --- a/io/init.c
>> +++ b/io/init.c
>> @@ -64,6 +64,7 @@ init_commands(void)
>>   	help_init();
>>   	imap_init();
>>   	inject_init();
>> +	seek_init();
>>   	madvise_init();
>>   	mincore_init();
>>   	mmap_init();
>> Index: b/io/io.h
>> ===================================================================
>> --- a/io/io.h
>> +++ b/io/io.h
>> @@ -108,6 +108,12 @@ extern void		quit_init(void);
>>   extern void		shutdown_init(void);
>>   extern void		truncate_init(void);
>>
>> +#ifdef HAVE_SEEK_DATA
>> +extern void		seek_init(void);
>> +#else
>> +#define seek_init()	do { } while (0)
>> +#endif
>
> this can go when we unconditionally compile it in
>
>> +
>>   #ifdef HAVE_FADVISE
>>   extern void		fadvise_init(void);
>>   #else
>> Index: b/io/seek.c
>> ===================================================================
>> --- /dev/null
>> +++ b/io/seek.c
>> @@ -0,0 +1,187 @@
>> +/*
>> + * Copyright (c) 2013 SGI
>> + * 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<libxfs.h>
>
> hm, including this clashes w/ the min() define in io/init.h,
> which is maybe a separate problem down the line, but libxfs.h
> isn't required anyway for this file, so I'd just drop this include.
>
>> +#include<linux/fs.h>

I think the previous review had a problem with this header which should 
be removed.
>> +
>> +#include<sys/uio.h>
>> +#include<xfs/xfs.h>
>> +#include<xfs/command.h>
>> +#include<xfs/input.h>
>> +#include<ctype.h>
>> +#include "init.h"
>> +#include "io.h"
>
> #ifndef HAVE_SEEK_DATA
> #define SEEK_DATA       3       /* seek to the next data */
> #define SEEK_HOLE       4       /* seek to the next hole */
> #endif
>
>> +
>> +static cmdinfo_t seek_cmd;
>> +
>> +static void
>> +seek_help(void)
>> +{
>> +	printf(_(
>> +"\n"
>> +" returns the next hole and/or data offset at or after the specified offset\n"
>> +"\n"
>> +" Example:\n"
>> +" 'seek -d 512'   - offset of data at or following offset 512\n"
>> +" 'seek -a -r 0'  - offsets of all data and hole in entire file\n"
>> +"\n"
>> +" Returns the offset of the next data and/or hole. There is an implied hole\n"
>> +" at the end of file.
>
> is this expected, given the hole at the end of the file?  This is for a single
> non-sparse file:
>
> xfs_io>  seek -ar 0
> Type	offset
> DATA	0
> HOLE	3022
> DATA	EOF
>
> That last line doesn't make sense, does it?

Parsing is the reason the entry is there so the output always has 
consistent ending entry - some queries that is the only answer (or now 
no message) no biggy one way or the other.

>
>> If the specified offset is past end of file, or there\n"
>> +" is no data past the specied offset, EOF is returned.\n"
>
> "specified"
>
>> +" -a   -- return the next data and hole starting at the specified offset.\n"
>> +" -d   -- return the next data starting at the specified offset.\n"
>> +" -h   -- return the next hole starting at the specified offset.\n"
>> +" -r   -- return all remaining type(s) starting at the specified offset.\n"
>> +"\n"));
>> +}
>> +
>> +#define	SEEK_DFLAG	(1<<  0)
>> +#define	SEEK_HFLAG	(1<<  1)
>> +#define	SEEK_RFLAG	(1<<  2)
>> +#define	DATA		0
>> +#define	HOLE		1
>> +
>> +struct seekinfo {
>> +	char		*name;
>> +	int		seektype;
>> +	int		mask;
>> +} seekinfo[] = {
>> +		{"DATA", SEEK_DATA, SEEK_DFLAG},
>> +		{"HOLE", SEEK_HOLE, SEEK_HFLAG}
>
> I guess "DATA" doesn't get replaced by "0" ?  Sorry, I failed cpp 101.
> It prints the right thing so I guess not.  ;)
>

:) no the defines are subscripts = see "current ="


>> +};
>> +
>> +void
>> +seek_output(
>> +	char	*type,
>> +	off64_t	offset)
>> +{
>> +	if (offset == -1) {
>> +		if (errno == ENXIO)
>> +			printf("%s	EOF\n", type);
>> +		else
>> +			printf("%s	ERR %d\n", type, errno);
>> +	} else
>> +		printf("%s	%ld\n", type, offset);
>> +}
>> +
>> +static int
>> +seek_f(
>> +	int	argc,
>> +	char	**argv)
>> +{
>> +	off64_t		offset, result;
>> +	size_t          fsblocksize, fssectsize;
>> +	int		flag;
>> +	int		current;	/* specify data or hole */
>> +	int		c;
>> +
>> +	flag = 0;
>> +	init_cvtnum(&fsblocksize,&fssectsize);
>> +
>> +	while ((c = getopt(argc, argv, "adhr")) != EOF) {
>> +		switch (c) {
>> +		case 'a':
>> +			flag |= (SEEK_HFLAG | SEEK_DFLAG);
>> +			break;
>> +		case 'd':
>> +			flag |= SEEK_DFLAG;
>> +			break;
>> +		case 'h':
>> +			flag |= SEEK_HFLAG;
>> +			break;
>> +		case 'r':
>> +			flag |= SEEK_RFLAG;
>> +			break;
>> +		default:
>> +			return command_usage(&seek_cmd);
>> +		}
>> +	}
>> +		/* must have hole or data specified and an offset */
>> +	if (!(flag&  (SEEK_DFLAG | SEEK_HFLAG)) ||
>> +             optind != argc - 1)
>> +		return command_usage(&seek_cmd);
>> +
>> +	offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
>
> need to error check that:
>
> xfs_io>  seek -a 8x8
> Type	offset
> HOLE	EOF
>

Some version of XFS seek_data will treat it as a 0, but okay.

>> +
>> +	if (flag&  SEEK_HFLAG) {
>> +		result = lseek64(file->fd, offset, SEEK_HOLE);
>> +		if ((result == offset) ||
>> +		    !(flag&  SEEK_DFLAG)) {
>> +			offset = result;	/* in case it was EOF */
>> +			current = HOLE;
>> +			goto found_hole;
>> +		}
>
> what if result<  0?

see below - no hole or error
>
> # io/xfs_io /tmp/fifo
> xfs_io>  seek -a 0
> Type	offset
> DATA	ERR 29
>
> hum I guess seek_output handles it?  perror would be nice, maybe?
>
>> +	}
>> +
>> +	/* found data or -1 when "-a" option was requested */
>> +	current = DATA;
>> +	offset = lseek64(file->fd, offset, SEEK_DATA);
>
> Ok, I guess I have to think harder about how the error handling
> from lseek is supposed to work.
>

not a hole

>> +
>> +found_hole:

At this point we figure out what come first, a hole or data.

we have to alternate between request to find the next hole and/or data

we print the item(s) that we want along the way.

if we do not find what we are looking for or get an error it is time to 
stop.


>> +	printf("Type	offset\n");
>> +
>> +	while (flag) {
>> +		/*
>> +		 * handle the data / hole entries in assending order from
>> +		 * the specified offset.
>> +		 *
>> +		 * flag determines if there are more items to be displayed.
>> +		 * SEEK_RFLAG is an infinite loop that is terminated with
>> +		 * a offset at EOF.
>> +		 *
>> +		 * current determines next type to process/display where
>> +		 * 0 is data and 1 is data.
>> +		 */
>> +
>> +		if (flag&  seekinfo[current].mask) {
>> +			seek_output(seekinfo[current].name, offset);
>> +			if (offset == -1)
>> +				break;		/* stop on error or EOF */
>> +		}
>> +
>> +		/*
>> +		 * When displaying only a single data and/or hole item, mask
>> +		 * off the item as it is displayed. The loop will end when all
>> +		 * requested items have been displayed.
>> +		 */
>> +		if (!(flag&  SEEK_RFLAG))
>> +			flag&= ~seekinfo[current].mask;
>> +
>> +		current ^= 1;		/* alternate between data and hole */
>> +		if (offset != -1)
>> +			offset = lseek64(file->fd, offset,
>> +					 seekinfo[current].seektype);
>> +	}
>> +	return 0;
>> +}
>> +
>> +void
>> +seek_init(void)
>> +{
>> +	seek_cmd.name = "seek";
>> +	seek_cmd.altname = "seek";
>
> altname isn't required, but I don't think there's a reason to re-specify the same name.
>
>> +	seek_cmd.cfunc = seek_f;
>> +	seek_cmd.argmin = 2;
>> +	seek_cmd.argmax = 5;
>> +	seek_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
>> +	seek_cmd.args = _("-a | -d | -h [-r] off");
>> +	seek_cmd.oneline = _("locate the next data and/or hole");
>> +	seek_cmd.help = seek_help;
>> +
>> +	add_command(&seek_cmd);
>> +}
>> Index: b/m4/package_libcdev.m4
>> ===================================================================
>> --- a/m4/package_libcdev.m4
>> +++ b/m4/package_libcdev.m4
>> @@ -154,6 +154,21 @@ AC_DEFUN([AC_HAVE_PREADV],
>>     ])
>>
>>   #
>> +# Check if we have a working fadvise system call
>
> fadvise...? ;)
>

my bad, a cut/paste bug.

>> +#
>> +AC_DEFUN([AC_HAVE_SEEK_DATA],
>> +  [ AC_MSG_CHECKING([for seek_data ])
>
> So really, we're checking for the SEEK_DATA&  SEEK_HOLE defines here.
>
> If they aren't present, we can locally define them, and we'll still get
> the functionality (though io has to cope w/ EINVAL or whatnot if the
> system xfs_io runs on doesn't understand the flags)
>
> Also, coverity didn't find any errors in seek.c \o/ :)

If that is what is desired.

>
> Thanks,
> -Eric
>
>> +    AC_TRY_COMPILE([
>> +#include<linux/fs.h>
>> +    ], [
>> +         lseek(0, 0, SEEK_DATA);
>> +    ], have_seek_data=yes
>> +       AC_MSG_RESULT(yes),
>> +       AC_MSG_RESULT(no))
>> +    AC_SUBST(have_seek_data)
>> +  ])
>> +
>> +#
>>   # Check if we have a sync_file_range libc call (Linux)
>>   #
>>   AC_DEFUN([AC_HAVE_SYNC_FILE_RANGE],
>> Index: b/man/man8/xfs_io.8
>> ===================================================================
>> --- a/man/man8/xfs_io.8
>> +++ b/man/man8/xfs_io.8
>> @@ -418,6 +418,41 @@ to read (in bytes)
>>   .RE
>>   .PD
>>   .TP
>> +.TP
>> +.BI "seek  \-a | \-d | \-h [ \-r ] offset"
>> +On platforms that support the
>> +.BR lseek (2)
>> +.B SEEK_DATA
>> +and
>> +.B SEEK_HOLE
>> +options, display the offsets of the specified segments.
>> +.RS 1.0i
>> +.PD 0
>> +.TP 0.4i
>> +.B \-a
>> +Display both
>> +.B data
>> +and
>> +.B hole
>> +segments starting at the specified
>> +.B offset.
>> +.TP
>> +.B \-d
>> +Display the
>> +.B data
>> +segment starting at the specified
>> +.B offset.
>> +.TP
>> +.B \-h
>> +Display the
>> +.B hole
>> +segment starting at the specified
>> +.B offset.
>> +.TP
>> +.B \-r
>> +Recursively display all the specified segments starting at the specified
>> +.B offset.
>> +.TP
>>
>>   .SH MEMORY MAPPED I/O COMMANDS
>>   .TP
>>
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
>>
>

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Eric Sandeen]

On 8/16/13 3:54 PM, Mark Tinguely wrote:

> Add the lseek SEEK_DATA/SEEK_HOLE support into xfs_io.
> The result from the lseek() call will be printed to the output.
> For example:
> 
> xfs_io> lseek -h 609k
> Type	Offset
> hole	630784

HOLE not hole, I guess.  ;)

I was going to say that's a lot of verbosity for a single output,
but I guess the other options might have many lines, so I suppose
it makes sense.

(I was just thinking about what xfstests might need to do to filter
& parse output...)

> Signed-off-by: Mark Tinguely <tinguely@sgi.com> 
> ---
>  Not trying to be difficult. Dave wanted the single hole/data/hole and data
>  seperated from the recursive loop, but doing it that way is basically unrolling
>  the loop into a if-then-else and is really terrible. If this is still not
>  acceptable, then we can throw this feature into /dev/null.
> 
>  configure.ac          |    1 
>  include/builddefs.in  |    1 
>  io/Makefile           |    5 +
>  io/init.c             |    1 
>  io/io.h               |    6 +
>  io/seek.c             |  187 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  m4/package_libcdev.m4 |   15 ++++
>  man/man8/xfs_io.8     |   35 +++++++++
>  8 files changed, 251 insertions(+)
> 
> Index: b/configure.ac
> ===================================================================
> --- a/configure.ac
> +++ b/configure.ac
> @@ -110,6 +110,7 @@ AC_HAVE_GETMNTINFO
>  AC_HAVE_FALLOCATE
>  AC_HAVE_FIEMAP
>  AC_HAVE_PREADV
> +AC_HAVE_SEEK_DATA
>  AC_HAVE_SYNC_FILE_RANGE
>  AC_HAVE_BLKID_TOPO($enable_blkid)
>  AC_HAVE_READDIR
> Index: b/include/builddefs.in
> ===================================================================
> --- a/include/builddefs.in
> +++ b/include/builddefs.in
> @@ -102,6 +102,7 @@ HAVE_GETMNTINFO = @have_getmntinfo@
>  HAVE_FALLOCATE = @have_fallocate@
>  HAVE_FIEMAP = @have_fiemap@
>  HAVE_PREADV = @have_preadv@
> +HAVE_SEEK_DATA = @have_seek_data@
>  HAVE_SYNC_FILE_RANGE = @have_sync_file_range@
>  HAVE_READDIR = @have_readdir@
>  
> Index: b/io/Makefile
> ===================================================================
> --- a/io/Makefile
> +++ b/io/Makefile
> @@ -85,6 +85,11 @@ CFILES += readdir.c
>  LCFLAGS += -DHAVE_READDIR
>  endif
>  
> +ifeq ($(HAVE_SEEK_DATA),yes)
> +LCFLAGS += -DHAVE_SEEK_DATA
> +CFILES += seek.c

see below; we should unconditionally compile, but conditionally
locally define SEEK_DATA / SEEK_HOLE

> +endif
> +
>  default: depend $(LTCOMMAND)
>  
>  include $(BUILDRULES)
> Index: b/io/init.c
> ===================================================================
> --- a/io/init.c
> +++ b/io/init.c
> @@ -64,6 +64,7 @@ init_commands(void)
>  	help_init();
>  	imap_init();
>  	inject_init();
> +	seek_init();
>  	madvise_init();
>  	mincore_init();
>  	mmap_init();
> Index: b/io/io.h
> ===================================================================
> --- a/io/io.h
> +++ b/io/io.h
> @@ -108,6 +108,12 @@ extern void		quit_init(void);
>  extern void		shutdown_init(void);
>  extern void		truncate_init(void);
>  
> +#ifdef HAVE_SEEK_DATA
> +extern void		seek_init(void);
> +#else
> +#define seek_init()	do { } while (0)
> +#endif

this can go when we unconditionally compile it in

> +
>  #ifdef HAVE_FADVISE
>  extern void		fadvise_init(void);
>  #else
> Index: b/io/seek.c
> ===================================================================
> --- /dev/null
> +++ b/io/seek.c
> @@ -0,0 +1,187 @@
> +/*
> + * Copyright (c) 2013 SGI
> + * 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 <libxfs.h>

hm, including this clashes w/ the min() define in io/init.h,
which is maybe a separate problem down the line, but libxfs.h
isn't required anyway for this file, so I'd just drop this include.

> +#include <linux/fs.h>
> +
> +#include <sys/uio.h>
> +#include <xfs/xfs.h>
> +#include <xfs/command.h>
> +#include <xfs/input.h>
> +#include <ctype.h>
> +#include "init.h"
> +#include "io.h"

#ifndef HAVE_SEEK_DATA
#define SEEK_DATA       3       /* seek to the next data */
#define SEEK_HOLE       4       /* seek to the next hole */
#endif

> +
> +static cmdinfo_t seek_cmd;
> +
> +static void
> +seek_help(void)
> +{
> +	printf(_(
> +"\n"
> +" returns the next hole and/or data offset at or after the specified offset\n"
> +"\n"
> +" Example:\n"
> +" 'seek -d 512'   - offset of data at or following offset 512\n"
> +" 'seek -a -r 0'  - offsets of all data and hole in entire file\n"
> +"\n"
> +" Returns the offset of the next data and/or hole. There is an implied hole\n"
> +" at the end of file.

is this expected, given the hole at the end of the file?  This is for a single
non-sparse file:

xfs_io> seek -ar 0
Type	offset
DATA	0
HOLE	3022
DATA	EOF

That last line doesn't make sense, does it?

> If the specified offset is past end of file, or there\n"
> +" is no data past the specied offset, EOF is returned.\n"

"specified"

> +" -a   -- return the next data and hole starting at the specified offset.\n"
> +" -d   -- return the next data starting at the specified offset.\n"
> +" -h   -- return the next hole starting at the specified offset.\n"
> +" -r   -- return all remaining type(s) starting at the specified offset.\n"
> +"\n"));
> +}
> +
> +#define	SEEK_DFLAG	(1 << 0)
> +#define	SEEK_HFLAG	(1 << 1)
> +#define	SEEK_RFLAG	(1 << 2)
> +#define	DATA		0
> +#define	HOLE		1
> +
> +struct seekinfo {
> +	char		*name;
> +	int		seektype;
> +	int		mask;
> +} seekinfo[] = {
> +		{"DATA", SEEK_DATA, SEEK_DFLAG},
> +		{"HOLE", SEEK_HOLE, SEEK_HFLAG}

I guess "DATA" doesn't get replaced by "0" ?  Sorry, I failed cpp 101.
It prints the right thing so I guess not.  ;)

> +};
> +
> +void
> +seek_output(
> +	char	*type,
> +	off64_t	offset)
> +{
> +	if (offset == -1) {
> +		if (errno == ENXIO)
> +			printf("%s	EOF\n", type);
> +		else
> +			printf("%s	ERR %d\n", type, errno);
> +	} else
> +		printf("%s	%ld\n", type, offset);
> +}
> +
> +static int
> +seek_f(
> +	int	argc,
> +	char	**argv)
> +{
> +	off64_t		offset, result;
> +	size_t          fsblocksize, fssectsize;
> +	int		flag;
> +	int		current;	/* specify data or hole */
> +	int		c;
> +
> +	flag = 0;
> +	init_cvtnum(&fsblocksize, &fssectsize);
> +
> +	while ((c = getopt(argc, argv, "adhr")) != EOF) {
> +		switch (c) {
> +		case 'a':
> +			flag |= (SEEK_HFLAG | SEEK_DFLAG);
> +			break;
> +		case 'd':
> +			flag |= SEEK_DFLAG;
> +			break;
> +		case 'h':
> +			flag |= SEEK_HFLAG;
> +			break;
> +		case 'r':
> +			flag |= SEEK_RFLAG;
> +			break;
> +		default:
> +			return command_usage(&seek_cmd);
> +		}
> +	}
> +		/* must have hole or data specified and an offset */
> +	if (!(flag & (SEEK_DFLAG | SEEK_HFLAG)) ||
> +             optind != argc - 1)
> +		return command_usage(&seek_cmd);
> +
> +	offset = cvtnum(fsblocksize, fssectsize, argv[optind]);

need to error check that:

xfs_io> seek -a 8x8
Type	offset
HOLE	EOF

> +
> +	if (flag & SEEK_HFLAG) {
> +		result = lseek64(file->fd, offset, SEEK_HOLE);
> +		if ((result == offset) ||
> +		    !(flag & SEEK_DFLAG)) {
> +			offset = result;	/* in case it was EOF */
> +			current = HOLE;
> +			goto found_hole;
> +		}

what if result < 0?

# io/xfs_io /tmp/fifo
xfs_io> seek -a 0
Type	offset
DATA	ERR 29

hum I guess seek_output handles it?  perror would be nice, maybe?

> +	}
> +
> +	/* found data or -1 when "-a" option was requested */
> +	current = DATA;
> +	offset = lseek64(file->fd, offset, SEEK_DATA);

Ok, I guess I have to think harder about how the error handling
from lseek is supposed to work.

> +
> +found_hole:
> +	printf("Type	offset\n");
> +
> +	while (flag) {
> +		/*
> +		 * handle the data / hole entries in assending order from
> +		 * the specified offset.
> +		 *
> +		 * flag determines if there are more items to be displayed.
> +		 * SEEK_RFLAG is an infinite loop that is terminated with
> +		 * a offset at EOF.
> +		 *
> +		 * current determines next type to process/display where
> +		 * 0 is data and 1 is data.
> +		 */
> +
> +		if (flag & seekinfo[current].mask) {
> +			seek_output(seekinfo[current].name, offset);
> +			if (offset == -1)
> +				break;		/* stop on error or EOF */
> +		}
> +
> +		/*
> +		 * When displaying only a single data and/or hole item, mask
> +		 * off the item as it is displayed. The loop will end when all
> +		 * requested items have been displayed.
> +		 */
> +		if (!(flag & SEEK_RFLAG))
> +			flag &= ~seekinfo[current].mask;
> +
> +		current ^= 1;		/* alternate between data and hole */
> +		if (offset != -1)
> +			offset = lseek64(file->fd, offset,
> +					 seekinfo[current].seektype);
> +	}
> +	return 0;
> +}
> +
> +void
> +seek_init(void)
> +{
> +	seek_cmd.name = "seek";
> +	seek_cmd.altname = "seek";

altname isn't required, but I don't think there's a reason to re-specify the same name.

> +	seek_cmd.cfunc = seek_f;
> +	seek_cmd.argmin = 2;
> +	seek_cmd.argmax = 5;
> +	seek_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	seek_cmd.args = _("-a | -d | -h [-r] off");
> +	seek_cmd.oneline = _("locate the next data and/or hole");
> +	seek_cmd.help = seek_help;
> +
> +	add_command(&seek_cmd);
> +}
> Index: b/m4/package_libcdev.m4
> ===================================================================
> --- a/m4/package_libcdev.m4
> +++ b/m4/package_libcdev.m4
> @@ -154,6 +154,21 @@ AC_DEFUN([AC_HAVE_PREADV],
>    ])
>  
>  #
> +# Check if we have a working fadvise system call

fadvise...? ;)

> +#
> +AC_DEFUN([AC_HAVE_SEEK_DATA],
> +  [ AC_MSG_CHECKING([for seek_data ])

So really, we're checking for the SEEK_DATA & SEEK_HOLE defines here.

If they aren't present, we can locally define them, and we'll still get
the functionality (though io has to cope w/ EINVAL or whatnot if the
system xfs_io runs on doesn't understand the flags)

Also, coverity didn't find any errors in seek.c \o/ :)

Thanks,
-Eric

> +    AC_TRY_COMPILE([
> +#include <linux/fs.h>
> +    ], [
> +         lseek(0, 0, SEEK_DATA);
> +    ], have_seek_data=yes
> +       AC_MSG_RESULT(yes),
> +       AC_MSG_RESULT(no))
> +    AC_SUBST(have_seek_data)
> +  ])
> +
> +#
>  # Check if we have a sync_file_range libc call (Linux)
>  #
>  AC_DEFUN([AC_HAVE_SYNC_FILE_RANGE],
> Index: b/man/man8/xfs_io.8
> ===================================================================
> --- a/man/man8/xfs_io.8
> +++ b/man/man8/xfs_io.8
> @@ -418,6 +418,41 @@ to read (in bytes)
>  .RE
>  .PD
>  .TP
> +.TP
> +.BI "seek  \-a | \-d | \-h [ \-r ] offset"
> +On platforms that support the
> +.BR lseek (2)
> +.B SEEK_DATA
> +and
> +.B SEEK_HOLE
> +options, display the offsets of the specified segments.
> +.RS 1.0i
> +.PD 0
> +.TP 0.4i
> +.B \-a
> +Display both
> +.B data
> +and
> +.B hole
> +segments starting at the specified
> +.B offset.
> +.TP
> +.B \-d
> +Display the
> +.B data
> +segment starting at the specified
> +.B offset.
> +.TP
> +.B \-h
> +Display the
> +.B hole
> +segment starting at the specified
> +.B offset.
> +.TP
> +.B \-r
> +Recursively display all the specified segments starting at the specified
> +.B offset.
> +.TP
>  
>  .SH MEMORY MAPPED I/O COMMANDS
>  .TP
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Rich Johnston]

Looks good there than the one typo below.

Reviewed-by: Rich Johnston <rjohnston@sgi.com>

Add the lseek SEEK_DATA/SEEK_HOLE support into xfs_io.
The result from the lseek() call will be printed to the output.
For example:

xfs_io> lseek -h 609k
         ^

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

[PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

[Mark Tinguely]

[-- Attachment #1: xfs_io-support-SEEK_DATA-SEEK_HOLE.patch --]
[-- Type: text/plain, Size: 9150 bytes --]

Add the lseek SEEK_DATA/SEEK_HOLE support into xfs_io.
The result from the lseek() call will be printed to the output.
For example:

xfs_io> lseek -h 609k
Type	Offset
hole	630784

Signed-off-by: Mark Tinguely <tinguely@sgi.com> 
---
 Not trying to be difficult. Dave wanted the single hole/data/hole and data
 seperated from the recursive loop, but doing it that way is basically unrolling
 the loop into a if-then-else and is really terrible. If this is still not
 acceptable, then we can throw this feature into /dev/null.

 configure.ac          |    1 
 include/builddefs.in  |    1 
 io/Makefile           |    5 +
 io/init.c             |    1 
 io/io.h               |    6 +
 io/seek.c             |  187 ++++++++++++++++++++++++++++++++++++++++++++++++++
 m4/package_libcdev.m4 |   15 ++++
 man/man8/xfs_io.8     |   35 +++++++++
 8 files changed, 251 insertions(+)

Index: b/configure.ac
===================================================================
--- a/configure.ac
+++ b/configure.ac
@@ -110,6 +110,7 @@ AC_HAVE_GETMNTINFO
 AC_HAVE_FALLOCATE
 AC_HAVE_FIEMAP
 AC_HAVE_PREADV
+AC_HAVE_SEEK_DATA
 AC_HAVE_SYNC_FILE_RANGE
 AC_HAVE_BLKID_TOPO($enable_blkid)
 AC_HAVE_READDIR
Index: b/include/builddefs.in
===================================================================
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -102,6 +102,7 @@ HAVE_GETMNTINFO = @have_getmntinfo@
 HAVE_FALLOCATE = @have_fallocate@
 HAVE_FIEMAP = @have_fiemap@
 HAVE_PREADV = @have_preadv@
+HAVE_SEEK_DATA = @have_seek_data@
 HAVE_SYNC_FILE_RANGE = @have_sync_file_range@
 HAVE_READDIR = @have_readdir@
 
Index: b/io/Makefile
===================================================================
--- a/io/Makefile
+++ b/io/Makefile
@@ -85,6 +85,11 @@ CFILES += readdir.c
 LCFLAGS += -DHAVE_READDIR
 endif
 
+ifeq ($(HAVE_SEEK_DATA),yes)
+LCFLAGS += -DHAVE_SEEK_DATA
+CFILES += seek.c
+endif
+
 default: depend $(LTCOMMAND)
 
 include $(BUILDRULES)
Index: b/io/init.c
===================================================================
--- a/io/init.c
+++ b/io/init.c
@@ -64,6 +64,7 @@ init_commands(void)
 	help_init();
 	imap_init();
 	inject_init();
+	seek_init();
 	madvise_init();
 	mincore_init();
 	mmap_init();
Index: b/io/io.h
===================================================================
--- a/io/io.h
+++ b/io/io.h
@@ -108,6 +108,12 @@ extern void		quit_init(void);
 extern void		shutdown_init(void);
 extern void		truncate_init(void);
 
+#ifdef HAVE_SEEK_DATA
+extern void		seek_init(void);
+#else
+#define seek_init()	do { } while (0)
+#endif
+
 #ifdef HAVE_FADVISE
 extern void		fadvise_init(void);
 #else
Index: b/io/seek.c
===================================================================
--- /dev/null
+++ b/io/seek.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright (c) 2013 SGI
+ * 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 <libxfs.h>
+#include <linux/fs.h>
+
+#include <sys/uio.h>
+#include <xfs/xfs.h>
+#include <xfs/command.h>
+#include <xfs/input.h>
+#include <ctype.h>
+#include "init.h"
+#include "io.h"
+
+static cmdinfo_t seek_cmd;
+
+static void
+seek_help(void)
+{
+	printf(_(
+"\n"
+" returns the next hole and/or data offset at or after the specified offset\n"
+"\n"
+" Example:\n"
+" 'seek -d 512'   - offset of data at or following offset 512\n"
+" 'seek -a -r 0'  - offsets of all data and hole in entire file\n"
+"\n"
+" Returns the offset of the next data and/or hole. There is an implied hole\n"
+" at the end of file. If the specified offset is past end of file, or there\n"
+" is no data past the specied offset, EOF is returned.\n"
+" -a   -- return the next data and hole starting at the specified offset.\n"
+" -d   -- return the next data starting at the specified offset.\n"
+" -h   -- return the next hole starting at the specified offset.\n"
+" -r   -- return all remaining type(s) starting at the specified offset.\n"
+"\n"));
+}
+
+#define	SEEK_DFLAG	(1 << 0)
+#define	SEEK_HFLAG	(1 << 1)
+#define	SEEK_RFLAG	(1 << 2)
+#define	DATA		0
+#define	HOLE		1
+
+struct seekinfo {
+	char		*name;
+	int		seektype;
+	int		mask;
+} seekinfo[] = {
+		{"DATA", SEEK_DATA, SEEK_DFLAG},
+		{"HOLE", SEEK_HOLE, SEEK_HFLAG}
+};
+
+void
+seek_output(
+	char	*type,
+	off64_t	offset)
+{
+	if (offset == -1) {
+		if (errno == ENXIO)
+			printf("%s	EOF\n", type);
+		else
+			printf("%s	ERR %d\n", type, errno);
+	} else
+		printf("%s	%ld\n", type, offset);
+}
+
+static int
+seek_f(
+	int	argc,
+	char	**argv)
+{
+	off64_t		offset, result;
+	size_t          fsblocksize, fssectsize;
+	int		flag;
+	int		current;	/* specify data or hole */
+	int		c;
+
+	flag = 0;
+	init_cvtnum(&fsblocksize, &fssectsize);
+
+	while ((c = getopt(argc, argv, "adhr")) != EOF) {
+		switch (c) {
+		case 'a':
+			flag |= (SEEK_HFLAG | SEEK_DFLAG);
+			break;
+		case 'd':
+			flag |= SEEK_DFLAG;
+			break;
+		case 'h':
+			flag |= SEEK_HFLAG;
+			break;
+		case 'r':
+			flag |= SEEK_RFLAG;
+			break;
+		default:
+			return command_usage(&seek_cmd);
+		}
+	}
+		/* must have hole or data specified and an offset */
+	if (!(flag & (SEEK_DFLAG | SEEK_HFLAG)) ||
+             optind != argc - 1)
+		return command_usage(&seek_cmd);
+
+	offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
+
+	if (flag & SEEK_HFLAG) {
+		result = lseek64(file->fd, offset, SEEK_HOLE);
+		if ((result == offset) ||
+		    !(flag & SEEK_DFLAG)) {
+			offset = result;	/* in case it was EOF */
+			current = HOLE;
+			goto found_hole;
+		}
+	}
+
+	/* found data or -1 when "-a" option was requested */
+	current = DATA;
+	offset = lseek64(file->fd, offset, SEEK_DATA);
+
+found_hole:
+	printf("Type	offset\n");
+
+	while (flag) {
+		/*
+		 * handle the data / hole entries in assending order from
+		 * the specified offset.
+		 *
+		 * flag determines if there are more items to be displayed.
+		 * SEEK_RFLAG is an infinite loop that is terminated with
+		 * a offset at EOF.
+		 *
+		 * current determines next type to process/display where
+		 * 0 is data and 1 is data.
+		 */
+
+		if (flag & seekinfo[current].mask) {
+			seek_output(seekinfo[current].name, offset);
+			if (offset == -1)
+				break;		/* stop on error or EOF */
+		}
+
+		/*
+		 * When displaying only a single data and/or hole item, mask
+		 * off the item as it is displayed. The loop will end when all
+		 * requested items have been displayed.
+		 */
+		if (!(flag & SEEK_RFLAG))
+			flag &= ~seekinfo[current].mask;
+
+		current ^= 1;		/* alternate between data and hole */
+		if (offset != -1)
+			offset = lseek64(file->fd, offset,
+					 seekinfo[current].seektype);
+	}
+	return 0;
+}
+
+void
+seek_init(void)
+{
+	seek_cmd.name = "seek";
+	seek_cmd.altname = "seek";
+	seek_cmd.cfunc = seek_f;
+	seek_cmd.argmin = 2;
+	seek_cmd.argmax = 5;
+	seek_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	seek_cmd.args = _("-a | -d | -h [-r] off");
+	seek_cmd.oneline = _("locate the next data and/or hole");
+	seek_cmd.help = seek_help;
+
+	add_command(&seek_cmd);
+}
Index: b/m4/package_libcdev.m4
===================================================================
--- a/m4/package_libcdev.m4
+++ b/m4/package_libcdev.m4
@@ -154,6 +154,21 @@ AC_DEFUN([AC_HAVE_PREADV],
   ])
 
 #
+# Check if we have a working fadvise system call
+#
+AC_DEFUN([AC_HAVE_SEEK_DATA],
+  [ AC_MSG_CHECKING([for seek_data ])
+    AC_TRY_COMPILE([
+#include <linux/fs.h>
+    ], [
+         lseek(0, 0, SEEK_DATA);
+    ], have_seek_data=yes
+       AC_MSG_RESULT(yes),
+       AC_MSG_RESULT(no))
+    AC_SUBST(have_seek_data)
+  ])
+
+#
 # Check if we have a sync_file_range libc call (Linux)
 #
 AC_DEFUN([AC_HAVE_SYNC_FILE_RANGE],
Index: b/man/man8/xfs_io.8
===================================================================
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -418,6 +418,41 @@ to read (in bytes)
 .RE
 .PD
 .TP
+.TP
+.BI "seek  \-a | \-d | \-h [ \-r ] offset"
+On platforms that support the
+.BR lseek (2)
+.B SEEK_DATA
+and
+.B SEEK_HOLE
+options, display the offsets of the specified segments.
+.RS 1.0i
+.PD 0
+.TP 0.4i
+.B \-a
+Display both
+.B data
+and
+.B hole
+segments starting at the specified
+.B offset.
+.TP
+.B \-d
+Display the
+.B data
+segment starting at the specified
+.B offset.
+.TP
+.B \-h
+Display the
+.B hole
+segment starting at the specified
+.B offset.
+.TP
+.B \-r
+Recursively display all the specified segments starting at the specified
+.B offset.
+.TP
 
 .SH MEMORY MAPPED I/O COMMANDS
 .TP


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