LUCENE-9774: TestDirectIODirectory fails with EINVAL on some filesystems

[rmuir]

TestDirectIODirectory will currently fail if run on an unsupported filesystem (e.g. tmpfs). Add an "assume" that probes if the filesystem supports Direct I/O.

Also tweak javadocs to indicate correct @throws clauses for the IndexInput and IndexOutput. You'll get an IOException (translated from EINVAL) if the filesystem doesn't support it, not a UOE.

cc: @zacharymorn I think you helped on this Directory, if you have time to review, thank you. I know Uwe is currently busy. Mainly I want the lucene tests passing on my machine again :)

[rmuir]

I'm curious if there's a better way to probe this, I'm not happy that the check here could mask real bugs, but at the same time the test is failing always for me...

[zacharymorn]

Hi @rmuir, thanks for cc-ing me! For the exception that's being thrown when the file system does not support direct IO, per java doc of ExtendedOpenOption.DIRECT it should actually throw UnsupportedOperationException:

Attempting to open a file with this option set will result in an {@code UnsupportedOperationException} if the operating system or file system does not support Direct I/O or a sufficient equivalent.

so it's a bit unexpected to me that IOException is being thrown there. Could it be a jdk implementation bug?

For probing alternative in addition to checking IOException / UnsupportedOperationException, I searched around a bit and found this:

https://github.com/openjdk/jdk11u-dev/blob/58082bd009883c6dcb779ac02333ea225097e182/test/jdk/java/nio/channels/FileChannel/directio/DirectIOTest.java#L82-L94

Maybe we can do something similar if file system such as tmpfs has different file system type naming?

[dweiss]

I think this check should be built-in in the directory itself somehow - verify both the JDK's NIO support flag, check if the location is writeable and brute-force try to create a direct-access file in the pointed directory (in a static method checkFilesystemSupported(Path) perhaps)? Yes, it may mask some other issues but it'll ensure some consistency? checkFilesystemSupported can throw UnsupportedOperationException with a suppressed original IOException so that the cause is not lost.

If the JDK throws an inconsistent exception - add this hacky test to isFilesystemSupported... It'd be also great to have a small repro test case and submit it to core-libs-dev@openjdk.java.net perhaps? Or NIO-appropriate mailing list (I'm not subscribed to all of them...).

Btw. @zacharymorn you have to be careful about where you look and copy code from as legal issues may kick in (incompatible licenses, copyright ownership)... even if it's not the case this time, you know... ;)

[rmuir]

I looked more and I don't think its a JDK bug FWIW, you have to open a file to even get the EINVAL from the kernel. Actually I think it would be a bad idea for the JDK to translate EINVAL to something like UnsupportedOperationException across the board, forget about lucene, just think about the bugs that could hide :)

And by the way, when it fails, your probe file is left on the filesystem, it doesn't even bother to clean it up: https://github.com/torvalds/linux/blob/faf145d6f3f3d6f2c066f65602ba9d0a03106915/fs/open.c#L839

This is all no problem for our tests, but just letting you know there is some complexity to move this out of test code into a runtime check.

You can see this behavior easily, if you have a tmpfs mount somewhere, e.g.: dd if=/dev/zero bs=512 count=1 of=/tmp/testfile oflag=direct

I also don't think it helps to try to inspect filesystem type and be "helpful" because there are so many out there (think FUSE and network filesystems and so on) that the logic would never be really correct. The only way to know for sure is to test it.

Can we start with this tests-only PR just so that the test suite passes for me again? We can open followup if we want to do more...

[dweiss]

Ok, go ahead.

[mikemccand]

Thanks @rmuir! Looks great ... it is weird that O_DIRECT returns EINVAL on tmpfs mounts! Isn't all IO "direct" on ram-backed filesystem :)

[dweiss]

Maybe it's something that should be fixed in tmpfs? This is an ancient answer about a different tmpfs issue but made me laugh :)
http://lkml.iu.edu/hypermail/linux/kernel/0506.2/0123.html

[rmuir]

@mikemccand I think it isn't in the sense that usually the user wants to do their own caching and disable the kernel's caching, so if tmpfs just ignored the flag it would not be respecting the user's wishes

[rmuir]

@dweiss maybe, but it seems controversial. you can see attempted patch/thread here: https://lwn.net/Articles/216885/

at the same time it is annoying, if you research the issue enough, you will find numerous users complaining that they cannot use their favorite database on tmpfs :)

[rmuir]

I opened followup for any future (non-tests) improvements related to this: https://issues.apache.org/jira/browse/LUCENE-9788