ZFS supports SEEK_HOLE/SEEK_DATA in "lseek()" syscall.

Oracle Solaris man page por "lseek":

"""
[...]
       o  If whence is SEEK_HOLE, the offset of the start of  the
          next  hole greater than or equal to the supplied offset
          is returned. The definition of a hole is provided  near
          the end of the DESCRIPTION.

       o  If whence is SEEK_DATA, the file pointer is set to  the
          start  of the next non-hole file region greater than or
          equal to the supplied offset.


     The  symbolic  constants   SEEK_SET,   SEEK_CUR,   SEEK_END,
     SEEK_HOLE,   and   SEEK_DATA   are  defined  in  the  header
     <unistd.h>.
[...]
     A "hole" is defined as a contiguous  range  of  bytes  in  a
     file,  all  having the value of zero, but not all zeros in a
     file are guaranteed to be represented as holes returned with
     SEEK_HOLE. Filesystems are allowed to expose ranges of zeros
     with SEEK_HOLE, but not required to.  Applications  can  use
     SEEK_HOLE  to  optimise  their behavior for ranges of zeros,
     but must not depend on it to find all such ranges in a file.
     The  existence  of  a  hole  at the end of every data region
     allows for easy programming and implies that a virtual  hole
     exists  at  the  end  of  the  file. Applications should use
     fpathconf(_PC_MIN_HOLE_SIZE) or  pathconf(_PC_MIN_HOLE_SIZE)
     to  determine if a filesystem supports SEEK_HOLE. See fpath-
     conf(2).

     For filesystems that do not supply information about  holes,
     the file will be represented as one entire data region.
"""

Implementation would be trivial. Simply conditionally compile the constant export in the C module. Or adopt the approach in the last phrase.

Any novice?.

Martin, any thoughts on adding a ZFS dependent feature?  ISTM this Solaris feature hasn't taken hold elsewhere and it may be a mistake to immortalize it in Python.

Seems to be adopted too in *bsd: http://manpages.ubuntu.com/manpages/lucid/man2/lseek.2freebsd.html .

The feature has patches available too for Linux, but never integrated in mainline kernel, AFAIK. Googling "SEEK_HOLE" is interesting.

If it's just additional constants then I don't see the problem. We already have a lot of platform-specific constants.

However, it would be a lot better if the io module were made to work properly with these constants, too. There are a lot of places where we hardcode tests such as `whence == SEEK_SET` or even `whence == 0`, and whence values above 2 aren't generally considered.

So the patch is probably a bit less trivial than what you imagine :)

Am 19.10.2010 06:58, schrieb Raymond Hettinger:
> 
> Raymond Hettinger <rhettinger@users.sourceforge.net> added the
> comment:
> 
> Martin, any thoughts on adding a ZFS dependent feature?  ISTM this
> Solaris feature hasn't taken hold elsewhere and it may be a mistake
> to immortalize it in Python.

There isn't any specific patch to review yet. However, Python has
a long tradition of exposing all symbolic constants that users have
asked for, and these are no different (e.g. how many systems support
EX_SOFTWARE, or ENOTACTIVE). As long as it's just new constants, and
as long as they are guarded with an ifdef, no approval is necessary
for adding them.

Jesús, can you attach a patch (with the appropriate #ifdefs)?

I attach patch. I have reviewed the IO module and I think we don't need to do any change there, since values over 2 are not touched.

The patch is trivial. My plan was to leave this patch for a novice :-).

Please, review. But let me do the final commit (I have commit privileges).

[jcea@babylon5 py3k]$ ./python
Python 3.2a3+ (py3k:85834M, Oct 25 2010, 15:37:04)
[GCC 4.5.1] on sunos5
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.SEEK_DATA
3
>>> os.SEEK_HOLE
4
>>>

The patch lacks a documentation change. Otherwise, it looks fine.

I think the docs should also make it clear that these flags are only supported by os.lseek() (unless you have tested the IO lib to work properly, that is, but in this case it would be nice to add unit tests).

I will update documentation.

Antoine, it is difficult to write a testcase when I can only test under a system with ZFS. I don't think we have a buildbot with Solaris/*BSD and ZFS.

Any suggestion?.

> I will update documentation.
> 
> Antoine, it is difficult to write a testcase when I can only test
> under a system with ZFS. I don't think we have a buildbot with
> Solaris/*BSD and ZFS.

If you ascertain yourself that the test works under ZFS then I think it
is enough. Of course, it would be better if a buildbot ran that test,
but we can live without it (IMHO).

> If you ascertain yourself that the test works under ZFS then I think it
> is enough. Of course, it would be better if a buildbot ran that test,
> but we can live without it (IMHO).

I agree. I trust that the patch is correct, and we really don't need to
test whether ZFS works correctly (but trust that Oracle will).

This is far more complicated that expected, because I would like to modify "file.seek()" to be able to support these flags too. Analizing "Modules/_io/bufferedio.c:buffered_seek()", the logic is pretty convoluted and the ramifications are extensible.

Should I limit myself to "os.lseek()"/"os.read()"/"os.write()"?. This flags are obscure, but useful enough to be useful even with this limitation.

Fut I don't feel comfy with this partial support.

Please, review this. I had changed IO to support the new flags too. To do it, I must relax error control in IO, a bit.

So, the new flag are supported both in "os.lseek()" and in standard file objects.

Please, Antoine, could you review?.

I have checked the patch manually, but I can't think a way to automate it. We don't have any ZFS machine in buildbot.

I would love to commit this next week. Thanks for your time.

"""
>>> import os
>>> f=open("XX","w+b")
>>> f.seek(1024*1024)
1048576
>>> f.write(b"hello")
5
>>> f.seek(1024*1024*2)
2097152
>>> f.write(b"bye")
3
>>> f.seek(0,os.SEEK_HOLE)
0
>>> f.seek(0,os.SEEK_DATA)
1048576
>>> f.seek(1048576,os.SEEK_HOLE)
1179648
>>> f.seek(1179648,os.SEEK_DATA)
2097152
>>> f.seek(2097152,os.SEEK_HOLE)
2097155
>>> fd=f.fileno()
>>> os.lseek(fd,0,os.SEEK_HOLE)
0
>>> os.lseek(fd,0,os.SEEK_DATA)
1048576
>>> os.lseek(fd,1048576,os.SEEK_HOLE)
1179648
>>> os.lseek(fd,1179648,os.SEEK_DATA)
2097152
"""

- The patch modifies the _io module but not _pyio, why?
(try   f=_pyio.open("XX","w+b")   at the beginning of the script above)

- One test was *removed*, but nothing was added to test this new feature.
This is the most likely way to lose it in future versions!
An idea for the test is to do like the MockRawIO class in test_io.py, which "implements" a file but fakes all system calls, and can be wrapped in a BufferedReader.

- The feature seems to be not applicable to text files, this should be tested.

Amaury, thanks for your valuable feedback.

1. I forgot about the python implementation. I am not familiar with the new IO framework. Implemented now.

3. These new SEEK modes should not be used in text mode. In my new patch the modes are rejected in text mode, allowed in binary mode.

2. I have spend the last 3 hours studying "test_io.py", and I don't understand the Mock* usage. Your MockRawIO hint is valuable, but I can't think a way to test that C/Python implementation passes the new flags to the OS. If I understand correctly, any method implemented in the Mock will be hit instead of the IO implementation, and anything that reach IO implementation will not go back to the Mock. I don't understand... :-?. Or can you magically insert the Mock between the IO module and the OS?. I know something like this is happening, but I don't understand the mechanism.

How do you choose what Mock* are you using in each test?.

I have read about Mock testing in the past, but I don't understand the use here. Are they actually Mocks, as explained in http://agiletesting.blogspot.com/2009/07/python-mock-testing-techniques-and.html , for example?

Sorry.

Could you provide some hint?. Maybe in python-dev, for more audience?.

I have implemented a test "test_posix", if your OS can report holes in a file (so far modern Solaris/OpenSolaris/OpenIndiana, don't know about *bsd). Other OSs will ignore the test.

I know your time is valuable. Thanking for investing in me.

Jesus, perhaps you can address Amaury's comments by uploading a new patch?

> ZFS supports SEEK_HOLE/SEEK_DATA in "lseek()" syscall.
> The feature has patches available too for Linux, but never integrated in mainline kernel, AFAIK.
> it is difficult to write a testcase when I can only test under a system with ZFS

I don't know if this could help, but recently Linux did get some commits regarding SEEK_HOLE/SEEK_DATA ( e.g. http://git.kernel.org/linus/982d816581eeeacfe5b2b7c6d47d13a157616eff ). I haven't tested whether it works / which filesystems support it. Grepping the Linux sources for SEEK_HOLES gives me several hits, e.g. for btrfs.

+    /* SEEK_SET and SEEK_CUR are special because we could seek inside the
+       buffer. Other whence values must be managed without this optimization.
+       Some Operating Systems can provide additional values, like
+       SEEK_HOLE/SEEK_DATA. */
+    if (((whence == 0) || (whence == 1)) && self->readable) {

Why not using SEEK_SET and SEEK_CUR instead of 0 and 1 here?

+      .. versionadded:: 3.2

This is now outdated, it should be 3.3.

Victor, internally Python uses 0, 1 and 2 as wired values independently of the platform values. This is probably an historic accident.

Please, review.

I am going to integrate next week

Please, review.

Is there a reason to say (several times) 'can support' instead of just 'support'? Do the OSes in question just optionally support rather than always support?

The first version added: change 'depend of' to 'depend on'.

In several functions you delete error checks:
-        if not (0 <= whence <= 2):
-            raise ValueError("invalid whence value")
(or C equivalent). What happens with patch if invalid whence value is passed? Error from deeper in the call stack? Silently pass error, with no message?

Could import of io create set of valid_whences for system? Then check would be "if whence not in valid_whences:" (or C equivalent).

In 3.3, unittest has new mock submodule. Perhaps that would help testing.

Terry, yes, skiping the test in the code will raise an error anyway when doing the real "seek()" OS syscall.

> Is there a reason to say (several times) 'can support' instead of
> just 'support'? Do the OSes in question just optionally support
> rather than always support?

Sometimes depends of the concrete filesystem used, or the concrete OS version.

> The first version added: change 'depend of' to 'depend on'.

Done in my repository. Thanks.

> Could import of io create set of valid_whences for system?
> Then check would be "if whence not in valid_whences:" (or C
> equivalent).

This is far from trivial and I don't see the point if OS "seek()" is going to give an error anyway. The only point would to provide a maybe more useful error message.

> In 3.3, unittest has new mock submodule. Perhaps that would help
> testing.

This is a very thin layer over the OS.

Thanks for the feedback.

> I don't see the point if OS "seek()" is going to give an error anyway.
Please check that Windows won't crash the interpreter with bad 'whence' values, like it already does for closed file descriptors.

New version, addressing Amaury concerns and Neologix review.

Please, do a final review.

- In test_posix.py: it's better to use the "with" statement when opening a file
- In Misc/NEWS: the entries should be kept in reverse chronological order

Another version, after Antoine feedback.

Please, review.

New changeset 86dc014cdd74 by Jesus Cea in branch 'default':
Close #10142: Support for SEEK_HOLE/SEEK_DATA
http://hg.python.org/cpython/rev/86dc014cdd74

You broke test_io

www.python.org/dev/buildbot/all/builders/x86 Gentoo Non-Debug 3.x/builds/2143/steps/test/logs/stdio

Yes, backing out changeset.

Never suppose anything...

New patch proposed, with testsuite fixed.

Please, review. Last chance :-).

New patch, after neologix@free.fr review.

<bikeshed>In some cases you change "invalid" to "unsupported" when encountering an invalid/unsupported `whence' and in others you keep them on "invalid". I find it rather hard to really differentiate these two words in that context; care to shed a light and tell me the thought process behind that?</bikeshed>

New changeset de2a0cb6ba52 by Jesus Cea in branch 'default':
Closes #10142: Support for SEEK_HOLE/SEEK_DATA
http://hg.python.org/cpython/rev/de2a0cb6ba52

Looks like the FreeBSD bot fails in test_posix:

======================================================================
ERROR: test_fs_holes (test.test_posix.PosixTester)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/home/buildbot/buildarea/3.x.krah-freebsd/build/Lib/test/test_posix.py", line 1033, in test_fs_holes
    self.assertEqual(i, os.lseek(fno, i, os.SEEK_DATA))
OSError: [Errno 25] Inappropriate ioctl for device

----------------------------------------------------------------------
Ran 81 tests in 1.878s

New changeset 13f5a329d5ea by Jesus Cea in branch 'default':
Kernel bug in freebsd9 - #10142: Support for SEEK_HOLE/SEEK_DATA
http://hg.python.org/cpython/rev/13f5a329d5ea

This looks like a bug in freebsd:

http://lists.freebsd.org/pipermail/freebsd-amd64/2012-January/014332.html

Since looks like a kernel bug, skipping test in that case.

Committed patch.

Thanks for the head-up.

New changeset 8acaa341df53 by Jesus Cea in branch 'default':
Skip the test only if neccesary - Kernel bug in freebsd9 - #10142: Support for SEEK_HOLE/SEEK_DATA
http://hg.python.org/cpython/rev/8acaa341df53

> This looks like a bug in freebsd:
> 
> http://lists.freebsd.org/pipermail/freebsd-amd64/2012-January/014332.html

I tested that one already yesterday (it was late, so I forgot to mention
it) and the test case attached to the bug report runs fine on the buildbot:

#include <unistd.h>
#include <fcntl.h>
#include <errno.h>

int main(void)
{
    int fd = open("ccc.c", O_RDONLY);
    off_t offset=lseek(fd,0,SEEK_HOLE);
    if (offset==-1) {
        if (errno==ENXIO) {
            // No more data
            printf("no more data\n");
            close(fd);
            exit(-1);
        }
    }
    return 0;
}


The skip looks good to me though, I wouldn't be surprised if there is a kernel
bug. This bug is still present on my machine:

http://www.freebsd.org/cgi/query-pr.cgi?pr=94729

> int main(void)
> {
>     int fd = open("ccc.c", O_RDONLY);
>     off_t offset=lseek(fd,0,SEEK_HOLE);
>     if (offset==-1) {
>         if (errno==ENXIO) {

Darn, the errno in test_posix should be ENOTTY. Indeed, with ENOTTY the
test case for the bug is positive.

The test case is till failing for the freebsd7 buildbot:

http://www.python.org/dev/buildbot/all/builders/x86%20FreeBSD%207.2%203.x/builds/3155/steps/test/logs/stdio

And the Ubuntu ARM buildbot.

New changeset 814557548af5 by Jesus Cea in branch 'default':
Skip test in freebsd entirely - Kernel bug in freebsd7/8/9 - #10142: Support for SEEK_HOLE/SEEK_DATA
http://hg.python.org/cpython/rev/814557548af5

Stefan, I am confused with your comments. The thing is that freebsd defines SEEK_HOLE/SEEK_DATA but reports an error when you use them. I guess they are errors when used on a filesystem that doesn't support them. This is a bug, in my opinion (if a FS doesn't support them, compatible implementation are trivial).

In the meantime, I am skipping the test completely under freebsd7/8/9. patch just committed.

Testing the patch, but freebsd7 buildbot is really SLOW.

Greog, I am confused about ARM ubuntu errors. Do we know what ubuntu version is running there?. What is the filesystem?.

I am seriously thinking about supporting these flags only in "real" OSs like Solaris & derivatives :-)

Jesus, what do you think about removing that test entirely?

IMO it is not our job to verify the OS' proper behavior in the face of SEEK_HOLE/SEEK_DATA; it is enough to provide the constants, and let whoever uses it face the perils of the platform.

Georg, I am fine with that if you are fine with that :-). Please, confirm :)

(sorry for mistyping your name before!)

Jes??s Cea Avi??n <report@bugs.python.org> wrote:
> Stefan, I am confused with your comments.

The FreeBSD bug report you linked to had a test case attached. The test case
uses errno == ENXIO. I could not reproduce the failure, so in my *first* comment
I questioned whether the failures in test_posix were caused by that particular
bug.

Then I noticed that the test_posix traceback shows errno == 25 == ENOTTY.
So I ran the test case with errno == ENOTTY and I *could* reproduce the
bug.

In short, I think you linked to the right bug after all. :)

As long as we have a test confirming that SEEK_HOLE/SEEK_DATA are *accepted* by the io module (which is a big part of the patch), it is fine.

E.g. calling seek() with the constants and check that it only raises OSError or nothing should work, right?

Ping. The ARM buildbot still fails on test_fs_holes:
http://buildbot.python.org/all/builders/ARM%20Ubuntu%203.x/builds/775/steps/test/logs/stdio

New changeset d69f95e57792 by Jesus Cea in branch 'default':
Cope with OSs lying - #10142: Support for SEEK_HOLE/SEEK_DATA
http://hg.python.org/cpython/rev/d69f95e57792

Thanks for the head-up, Antoine.