[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 11:52:32 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <5214EAAC.80800@xxxxxxxxxxx>
References: <20130816205409.976658624@xxxxxxx> <5213F6AF.8070107@xxxxxxxxxxx> <5214CB5C.4050608@xxxxxxx> <5214EAAC.80800@xxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 08/21/13 11:28, Eric Sandeen wrote:
On 8/21/13 9:14 AM, Mark Tinguely wrote:

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


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

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

only 1?

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

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

xfs_io>   lseek -h 609k
Type    Offset
hole    630784

HOLE not hole, I guess.  ;)

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

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

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

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

Signed-off-by: Mark Tinguely<tinguely@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 
   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
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

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

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

It was put in to check if SEEK_DATA is supported.

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

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

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

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

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

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

yes, needed anyway if removing linux/fs.h

   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)
+    seek_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);

+extern void        seek_init(void);
+#define seek_init()    do { } while (0)

this can go when we unconditionally compile it in

   #ifdef HAVE_FADVISE
   extern void        fadvise_init(void);
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
+ * 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
+ */

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.


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

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

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

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

+#include "init.h"
+#include "io.h"

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

+static cmdinfo_t seek_cmd;
+static void
+    printf(_(
+" returns the next hole and/or data offset at or after the specified offset\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"
+" 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

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

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

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

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

For a 1M 100% nonsparse file we get:

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

or this could be HOLE EOF depends on the version.


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

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

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

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

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

... or we will get "HOLE EOF"

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

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

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

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

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

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

I can drop the table header.

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

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


+" -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"
+#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[] = {

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?

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

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

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

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

okay, thanks.

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

super-nitpick, extra tab before the comment.

not a problem.

+    if (!(flag&   (SEEK_DFLAG | SEEK_HFLAG)) ||
+             optind != argc - 1)
+        return command_usage(&seek_cmd);
+    offset = cvtnum(fsblocksize, fssectsize, argv[optind]);

need to error check that:

xfs_io>   seek -a 8x8
Type    offset

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.




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