[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