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

Mark Tinguely
2013-08-21T14:14:56Z

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

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

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

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

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

It was put in to check if SEEK_DATA is supported.

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

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

>> +endif
>> +
>> default: depend $(LTCOMMAND)
>>
>> include $(BUILDRULES)
>> Index: b/io/init.c
>> ===================================================================
>> --- a/io/init.c
>> +++ b/io/init.c
>> @@ -64,6 +64,7 @@ init_commands(void)
>> help_init();
>> imap_init();
>> inject_init();
>> + seek_init();
>> madvise_init();
>> mincore_init();
>> mmap_init();
>> Index: b/io/io.h
>> ===================================================================
>> --- a/io/io.h
>> +++ b/io/io.h
>> @@ -108,6 +108,12 @@ extern void quit_init(void);
>> extern void shutdown_init(void);
>> extern void truncate_init(void);
>>
>> +#ifdef HAVE_SEEK_DATA
>> +extern void seek_init(void);
>> +#else
>> +#define seek_init() do { } while (0)
>> +#endif
>
> this can go when we unconditionally compile it in
>
>> +
>> #ifdef HAVE_FADVISE
>> extern void fadvise_init(void);
>> #else
>> Index: b/io/seek.c
>> ===================================================================
>> --- /dev/null
>> +++ b/io/seek.c
>> @@ -0,0 +1,187 @@
>> +/*
>> + * Copyright (c) 2013 SGI
>> + * All Rights Reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it would be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write the Free Software Foundation,
>> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +#include<libxfs.h>
>
> hm, including this clashes w/ the min() define in io/init.h,
> which is maybe a separate problem down the line, but libxfs.h
> isn't required anyway for this file, so I'd just drop this include.
>
>> +#include<linux/fs.h>

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

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

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

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

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

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

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

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

not a hole

>> +
>> +found_hole:

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

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

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

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

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

my bad, a cut/paste bug.

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

If that is what is desired.

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