xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfs_io: add the lseek() SEEK_DATA/SEEK_HOLE support
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Wed, 21 Aug 2013 09:14:52 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <5213F6AF.8070107@xxxxxxxxxxx>
References: <20130816205409.976658624@xxxxxxx> <5213F6AF.8070107@xxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0

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

On 08/20/13 18:07, Eric Sandeen wrote:
On 8/16/13 3:54 PM, Mark Tinguely wrote:

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

xfs_io>  lseek -h 609k
Type    Offset
hole    630784

HOLE not hole, I guess.  ;)

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

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

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

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


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

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

Index: b/configure.ac
===================================================================
--- a/configure.ac
+++ b/configure.ac
@@ -110,6 +110,7 @@ AC_HAVE_GETMNTINFO
  AC_HAVE_FALLOCATE
  AC_HAVE_FIEMAP
  AC_HAVE_PREADV
+AC_HAVE_SEEK_DATA
  AC_HAVE_SYNC_FILE_RANGE
  AC_HAVE_BLKID_TOPO($enable_blkid)
  AC_HAVE_READDIR
Index: b/include/builddefs.in
===================================================================
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -102,6 +102,7 @@ HAVE_GETMNTINFO = @have_getmntinfo@
  HAVE_FALLOCATE = @have_fallocate@
  HAVE_FIEMAP = @have_fiemap@
  HAVE_PREADV = @have_preadv@
+HAVE_SEEK_DATA = @have_seek_data@
  HAVE_SYNC_FILE_RANGE = @have_sync_file_range@
  HAVE_READDIR = @have_readdir@

Index: b/io/Makefile
===================================================================
--- a/io/Makefile
+++ b/io/Makefile
@@ -85,6 +85,11 @@ CFILES += readdir.c
  LCFLAGS += -DHAVE_READDIR
  endif

+ifeq ($(HAVE_SEEK_DATA),yes)
+LCFLAGS += -DHAVE_SEEK_DATA
+CFILES += seek.c

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


It was put in to check if SEEK_DATA is supported.

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

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

+endif
+
  default: depend $(LTCOMMAND)

  include $(BUILDRULES)
Index: b/io/init.c
===================================================================
--- a/io/init.c
+++ b/io/init.c
@@ -64,6 +64,7 @@ init_commands(void)
        help_init();
        imap_init();
        inject_init();
+       seek_init();
        madvise_init();
        mincore_init();
        mmap_init();
Index: b/io/io.h
===================================================================
--- a/io/io.h
+++ b/io/io.h
@@ -108,6 +108,12 @@ extern void                quit_init(void);
  extern void           shutdown_init(void);
  extern void           truncate_init(void);

+#ifdef HAVE_SEEK_DATA
+extern void            seek_init(void);
+#else
+#define seek_init()    do { } while (0)
+#endif

this can go when we unconditionally compile it in

+
  #ifdef HAVE_FADVISE
  extern void           fadvise_init(void);
  #else
Index: b/io/seek.c
===================================================================
--- /dev/null
+++ b/io/seek.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright (c) 2013 SGI
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include<libxfs.h>

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

+#include<linux/fs.h>

I think the previous review had a problem with this header which should be removed.
+
+#include<sys/uio.h>
+#include<xfs/xfs.h>
+#include<xfs/command.h>
+#include<xfs/input.h>
+#include<ctype.h>
+#include "init.h"
+#include "io.h"

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

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

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

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

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

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


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

"specified"

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

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


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


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

need to error check that:

xfs_io>  seek -a 8x8
Type    offset
HOLE    EOF


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

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

what if result<  0?

see below - no hole or error

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

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

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

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


not a hole

+
+found_hole:

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

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

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

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


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

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

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

  #
+# Check if we have a working fadvise system call

fadvise...? ;)


my bad, a cut/paste bug.

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

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

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

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

If that is what is desired.


Thanks,
-Eric

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

  .SH MEMORY MAPPED I/O COMMANDS
  .TP


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



<Prev in Thread] Current Thread [Next in Thread>