Skip to contentSkip to conversation

xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support

PATCH In review Mark Tinguely 3 participants 11 messages Aug 16, 2013 – Aug 21, 2013

lore
Share & tools
lore
#0 lore raw

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

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

---
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(+)
configure.ac +1 -0
Index: 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
include/builddefs.in +1 -0
Index: 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@
 
io/Makefile +5 -0
Index: 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)
io/init.c +1 -0
Index: 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();
io/io.h +6 -0
Index: 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
io/seek.c new file +187 -0

187 lines not on this page · raw message

m4/package_libcdev.m4 +15 -0
Index: 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],
man/man8/xfs_io.8 +35 -0
Index: 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#2 lore raw
In reply to Mark Tinguely
Quoted 7 lines · Mark Tinguely

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...)

Quoted 46 lines

---
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
===================================================================

configure.ac +1 -0
@@ -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
===================================================================

include/builddefs.in +1 -0
@@ -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
===================================================================

io/Makefile CFILES += readdir.c · line 90

Quoted 6 lines
io/Makefile +3 -0
@@ -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

Quoted 22 lines
io/Makefile +2 -0
+endif
+
 default: depend $(LTCOMMAND)
 
 include $(BUILDRULES)

Index: b/io/init.c
===================================================================

io/init.c +1 -0
@@ -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
===================================================================

io/io.h extern void quit_init(void); · line 115

Quoted 8 lines
io/io.h +5 -0
@@ -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

Quoted 18 lines
io/io.h +1 -0
+
 #ifdef HAVE_FADVISE
 extern void		fadvise_init(void);
 #else

Index: b/io/seek.c
===================================================================

io/seek.c new file +9 -0
@@ -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,

io/seek.c line 19

Quoted 10 lines
io/seek.c new file +10 -0
+ * 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.

io/seek.c line 28

Quoted 9 lines
io/seek.c new file +9 -0
+#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

Quoted 6 lines
io/seek.c new file +6 -0
+
+static cmdinfo_t seek_cmd;
+
+static void
+seek_help(void)
+{

io/seek.c

Quoted 10 lines
io/seek.c new file +10 -0
+	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?

Quoted 2 lines

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

"specified"

Quoted 10 lines
+" -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"));

io/seek.c new file +5 -0
+}
+
+#define	SEEK_DFLAG	(1 << 0)
+#define	SEEK_HFLAG	(1 << 1)
+#define	SEEK_RFLAG	(1 << 2)

io/seek.c line 65

Quoted 10 lines
io/seek.c new file +10 -0
+#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. ;)

Quoted 44 lines
io/seek.c new file +44 -0
+};
+
+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;

io/seek.c line 119

Quoted 10 lines
io/seek.c new file +10 -0
+		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

io/seek.c line 128

Quoted 9 lines
io/seek.c new file +9 -0
+
+	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?

io/seek.c line 133

Quoted 5 lines
io/seek.c new file +5 -0
+	}
+
+	/* 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.

Quoted 34 lines
io/seek.c new file +34 -0
+
+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,

io/seek.c line 177

Quoted 10 lines
io/seek.c new file +10 -0
+					 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.

Quoted 15 lines
io/seek.c new file +10 -0
+	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
===================================================================

m4/package_libcdev.m4 AC_DEFUN([AC_HAVE_PREADV], · line 157

Quoted 4 lines
m4/package_libcdev.m4 +1 -0
@@ -154,6 +154,21 @@ AC_DEFUN([AC_HAVE_PREADV],
   ])
 
 #
+# Check if we have a working fadvise system call

fadvise...? ;)

m4/package_libcdev.m4 AC_DEFUN([AC_HAVE_PREADV], · line 160

Quoted 3 lines
m4/package_libcdev.m4 +3 -0
+#
+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

Quoted 62 lines
m4/package_libcdev.m4 +11 -0
+    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
===================================================================

man/man8/xfs_io.8 +35 -0
@@ -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
Quoted 5 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#3 lore raw

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.

Quoted 10 lines · Eric Sandeen
Quoted 7 lines · Mark Tinguely

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

Quoted 8 lines

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.

Quoted 50 lines
Quoted 50 lines

---
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
===================================================================

configure.ac +1 -0
@@ -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
===================================================================

include/builddefs.in +1 -0
@@ -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
===================================================================

io/Makefile +0 -0
@@ -85,6 +85,11 @@ CFILES += readdir.c
  LCFLAGS += -DHAVE_READDIR
  endif
 
Quoted 7 lines

io/Makefile CFILES += readdir.c · line 90

Quoted 3 lines
io/Makefile +3 -0
+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.

Quoted 57 lines
Quoted 22 lines
io/Makefile +2 -0
+endif
+
  default: depend $(LTCOMMAND)
 
  include $(BUILDRULES)

Index: b/io/init.c
===================================================================

io/init.c +1 -0
@@ -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
===================================================================

io/io.h extern void quit_init(void); · line 115

Quoted 8 lines
io/io.h +5 -0
@@ -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

Quoted 24 lines
io/io.h +1 -0
+
  #ifdef HAVE_FADVISE
  extern void		fadvise_init(void);
  #else

Index: b/io/seek.c
===================================================================

io/seek.c new file +15 -0
@@ -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,
Quoted 10 lines

io/seek.c

Quoted 4 lines
io/seek.c new file +4 -0
+ * 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.

Quoted 1 line
io/seek.c new file +1 -0
+#include<linux/fs.h>

I think the previous review had a problem with this header which should be removed.

Quoted 34 lines

io/seek.c line 28

Quoted 8 lines
io/seek.c new file +8 -0
+
+#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

Quoted 6 lines
io/seek.c new file +6 -0
+
+static cmdinfo_t seek_cmd;
+
+static void
+seek_help(void)
+{

io/seek.c

Quoted 10 lines
io/seek.c new file +10 -0
+	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:

Quoted 7 lines

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.

Quoted 21 lines
Quoted 3 lines

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

"specified"

Quoted 15 lines
+" -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"));

io/seek.c new file +10 -0
+}
+
+#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;
Quoted 9 lines

io/seek.c line 65

Quoted 5 lines
io/seek.c new file +5 -0
+	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 ="

Quoted 57 lines
Quoted 44 lines
io/seek.c new file +44 -0
+};
+
+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;

io/seek.c line 119

Quoted 10 lines
io/seek.c new file +10 -0
+		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:

Quoted 4 lines

xfs_io> seek -a 8x8
Type offset
HOLE EOF

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

Quoted 11 lines

io/seek.c line 128

Quoted 9 lines
io/seek.c new file +9 -0
+
+	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

Quoted 8 lines
# 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?

Quoted 9 lines

io/seek.c line 133

Quoted 5 lines
io/seek.c new file +5 -0
+	}
+
+	/* 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

Quoted 2 lines

io/seek.c

Quoted 2 lines
io/seek.c new file +2 -0
+
+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.

Quoted 62 lines
Quoted 32 lines
io/seek.c new file +32 -0
+	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,

io/seek.c line 177

Quoted 10 lines
io/seek.c new file +10 -0
+					 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.

Quoted 17 lines
io/seek.c new file +10 -0
+	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
===================================================================

m4/package_libcdev.m4 +0 -0
@@ -154,6 +154,21 @@ AC_DEFUN([AC_HAVE_PREADV],
    ])
 
Quoted 5 lines

m4/package_libcdev.m4 AC_DEFUN([AC_HAVE_PREADV], · line 157

Quoted 2 lines
m4/package_libcdev.m4 +1 -0
  #
+# Check if we have a working fadvise system call

fadvise...? ;)

my bad, a cut/paste bug.

Quoted 11 lines

m4/package_libcdev.m4 AC_DEFUN([AC_HAVE_PREADV], · line 160

Quoted 3 lines
m4/package_libcdev.m4 +3 -0
+#
+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.

Quoted 66 lines

Thanks,
-Eric

Quoted 62 lines
m4/package_libcdev.m4 +11 -0
+    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
===================================================================

man/man8/xfs_io.8 +35 -0
@@ -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
Quoted 6 lines
Quoted 5 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#4 lore raw
Quoted 2 lines · 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.

*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...

Quoted 83 lines · Eric Sandeen
Quoted 10 lines · Mark Tinguely
Quoted 8 lines

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

Quoted 8 lines

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.

Quoted 50 lines
Quoted 50 lines

---
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
===================================================================

configure.ac +1 -0
@@ -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
===================================================================

include/builddefs.in +1 -0
@@ -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
===================================================================

io/Makefile +0 -0
@@ -85,6 +85,11 @@ CFILES += readdir.c
  LCFLAGS += -DHAVE_READDIR
  endif
 
Quoted 7 lines

io/Makefile CFILES += readdir.c · line 90

Quoted 3 lines
io/Makefile +3 -0
+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

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 66 lines
Quoted 66 lines
Quoted 22 lines
io/Makefile +2 -0
+endif
+
  default: depend $(LTCOMMAND)
 
  include $(BUILDRULES)

Index: b/io/init.c
===================================================================

io/init.c +1 -0
@@ -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
===================================================================

io/io.h extern void quit_init(void); · line 115

Quoted 8 lines
io/io.h +5 -0
@@ -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

Quoted 18 lines
io/io.h +1 -0
+
  #ifdef HAVE_FADVISE
  extern void        fadvise_init(void);
  #else

Index: b/io/seek.c
===================================================================

io/seek.c new file +9 -0
@@ -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,

io/seek.c line 19

Quoted 10 lines
io/seek.c new file +10 -0
+ * 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.

Quoted 3 lines
Quoted 1 line

io/seek.c line 20

Quoted 1 line
io/seek.c new file +1 -0
+#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.

Quoted 40 lines
Quoted 40 lines

io/seek.c line 28

Quoted 8 lines
io/seek.c new file +8 -0
+
+#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

Quoted 6 lines
io/seek.c new file +6 -0
+
+static cmdinfo_t seek_cmd;
+
+static void
+seek_help(void)
+{

io/seek.c

Quoted 10 lines
io/seek.c new file +10 -0
+    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
Quoted 5 lines
Quoted 1 line

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?

Quoted 31 lines
Quoted 21 lines
Quoted 3 lines

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

"specified"

Quoted 15 lines
+" -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"));

io/seek.c new file +10 -0
+}
+
+#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;
Quoted 9 lines

io/seek.c line 65

Quoted 5 lines
io/seek.c new file +5 -0
+    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. ;)

Quoted 1 line

:) 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.

Quoted 15 lines
Quoted 15 lines
Quoted 5 lines
io/seek.c new file +4 -0
+};
+
+void
+seek_output(

io/seek.c line 79

Quoted 10 lines
io/seek.c new file +10 -0
+    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.

Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
io/seek.c new file +27 -0
+}
+
+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;
Quoted 8 lines
Quoted 8 lines

io/seek.c line 114

Quoted 8 lines
io/seek.c new file +8 -0
+        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.

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
io/seek.c new file +5 -0
+    if (!(flag&  (SEEK_DFLAG | SEEK_HFLAG)) ||
+             optind != argc - 1)
+        return command_usage(&seek_cmd);
+
+    offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
Quoted 8 lines
Quoted 6 lines

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#5 lore raw
Quoted 11 lines · Eric Sandeen
Quoted 2 lines · 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.

*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?

Quoted 94 lines
Quoted 84 lines · Eric Sandeen
Quoted 10 lines · Mark Tinguely
Quoted 8 lines

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

Quoted 8 lines

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.

Quoted 50 lines
Quoted 50 lines

---
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
===================================================================

configure.ac +1 -0
@@ -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
===================================================================

include/builddefs.in +1 -0
@@ -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
===================================================================

io/Makefile +0 -0
@@ -85,6 +85,11 @@ CFILES += readdir.c
   LCFLAGS += -DHAVE_READDIR
   endif
 
Quoted 7 lines

io/Makefile CFILES += readdir.c · line 90

Quoted 3 lines
io/Makefile +3 -0
+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

Quoted 3 lines

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?

Quoted 8 lines

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

Quoted 1 line

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

Quoted 66 lines
Quoted 66 lines
Quoted 66 lines
Quoted 22 lines
io/Makefile +2 -0
+endif
+
   default: depend $(LTCOMMAND)
 
   include $(BUILDRULES)

Index: b/io/init.c
===================================================================

io/init.c +1 -0
@@ -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
===================================================================

io/io.h extern void quit_init(void); · line 115

Quoted 8 lines
io/io.h +5 -0
@@ -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

Quoted 18 lines
io/io.h +1 -0
+
   #ifdef HAVE_FADVISE
   extern void        fadvise_init(void);
   #else

Index: b/io/seek.c
===================================================================

io/seek.c new file +9 -0
@@ -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,

io/seek.c line 19

Quoted 10 lines
io/seek.c new file +10 -0
+ * 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.

Quoted 9 lines
Quoted 3 lines
Quoted 1 line

io/seek.c line 20

Quoted 1 line
io/seek.c new file +1 -0
+#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.

Quoted 49 lines
Quoted 40 lines
Quoted 40 lines

io/seek.c line 28

Quoted 8 lines
io/seek.c new file +8 -0
+
+#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

Quoted 6 lines
io/seek.c new file +6 -0
+
+static cmdinfo_t seek_cmd;
+
+static void
+seek_help(void)
+{

io/seek.c

Quoted 10 lines
io/seek.c new file +10 -0
+    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
Quoted 5 lines
Quoted 1 line

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...

Quoted 8 lines

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.

Quoted 27 lines

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
Quoted 4 lines

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 ...

Quoted 14 lines

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
Quoted 3 lines

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.

Quoted 31 lines
Quoted 31 lines
Quoted 30 lines
Quoted 3 lines

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

"specified"

Quoted 10 lines
+" -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"));

io/seek.c new file +5 -0
+}
+
+#define    SEEK_DFLAG    (1<<   0)
+#define    SEEK_HFLAG    (1<<   1)
+#define    SEEK_RFLAG    (1<<   2)

io/seek.c line 65

Quoted 10 lines
io/seek.c new file +10 -0
+#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. ;)

Quoted 5 lines
Quoted 1 line

:) 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?

Quoted 18 lines
Quoted 15 lines
Quoted 15 lines
Quoted 5 lines
io/seek.c new file +4 -0
+};
+
+void
+seek_output(

io/seek.c line 79

Quoted 10 lines
io/seek.c new file +10 -0
+    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

Quoted 4 lines

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

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

okay, thanks.

Quoted 29 lines
Quoted 29 lines
Quoted 29 lines
Quoted 29 lines
io/seek.c new file +29 -0
+}
+
+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;
Quoted 9 lines
Quoted 6 lines
Quoted 6 lines

io/seek.c line 114

Quoted 6 lines
io/seek.c new file +6 -0
+            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.

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines

io/seek.c line 119

Quoted 5 lines
io/seek.c new file +5 -0
+    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

Quoted 5 lines
Quoted 1 line

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.

Quoted 2 lines

Thanks,
-Eric

Thanks.

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#6 lore raw

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

Quoted 6 lines
Quoted 4 lines

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.

Quoted 10 lines
Quoted 8 lines

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?

Quoted 32 lines
Quoted 27 lines

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
Quoted 4 lines

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?

Quoted 6 lines

... 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.

Quoted 4 lines

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

Quoted 15 lines
Quoted 15 lines

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
Quoted 6 lines
Quoted 3 lines

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)

Quoted 11 lines
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#7 lore raw
Quoted 26 lines · Eric Sandeen

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

Quoted 6 lines
Quoted 4 lines

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.

Quoted 4 lines
Quoted 4 lines

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

For a 1M 100% nonsparse file we get:

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
# 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.

Quoted 35 lines
Quoted 35 lines
Quoted 28 lines

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
Quoted 4 lines

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"

Quoted 7 lines
Quoted 4 lines

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.

Quoted 8 lines
Quoted 5 lines

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. :)

Quoted 30 lines
Quoted 15 lines
Quoted 15 lines

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
Quoted 6 lines
Quoted 3 lines

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?

Quoted 5 lines

(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.

Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

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.

Quoted 6 lines
Quoted 1 line

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#8 lore raw
Quoted 1 line · Mark Tinguely

On 08/21/13 13:31, Eric Sandeen wrote:

...

Quoted 14 lines
Quoted 7 lines
Quoted 4 lines

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
Quoted 5 lines

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:

Quoted 4 lines

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.

Quoted 6 lines

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#9 lore raw
Quoted 41 lines · Eric Sandeen
Quoted 1 line · Mark Tinguely

On 08/21/13 13:31, Eric Sandeen wrote:

...

Quoted 14 lines
Quoted 7 lines
Quoted 4 lines

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
Quoted 5 lines

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:

Quoted 4 lines

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.

Quoted 6 lines

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).

Quoted 3 lines

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

Quoted 7 lines

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#10 lore raw
In reply to Eric Sandeen
Quoted 25 lines · Eric Sandeen
Quoted 1 line · Mark Tinguely

On 08/21/13 13:31, Eric Sandeen wrote:

...

Quoted 14 lines
Quoted 7 lines
Quoted 4 lines

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
Quoted 5 lines

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.

Quoted 8 lines

Well, at least the man page says:

Quoted 4 lines

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.

Quoted 9 lines
Quoted 7 lines

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.

Quoted 4 lines

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs