Proposal for adding O_DIRECT support into JDK 9
huaming.li at oracle.com
huaming.li at oracle.com
Mon Sep 4 07:16:52 UTC 2017
Hi Lucy,
Thank you for the great work.
I tried to review based on
http://cr.openjdk.java.net/~kkharbas/8164900/webrev.08/, but can not
view file change diffs there, so following comments are based on
webrev.07, hope it's helpful.
====================== Comments about implementation ======================
http://cr.openjdk.java.net/~kkharbas/8164900/webrev.07/src/java.base/share/classes/sun/nio/ch/Util.java.frames.html
<http://cr.openjdk.java.net/%7Ekkharbas/8164900/webrev.07/src/java.base/share/classes/sun/nio/ch/Util.java.frames.html>
In line 250, should it use a different cache than non-direct buffer? (Can different files use direct & non-direct io at the same time?)
In line 254, what’s the possibility of to get here, if it’s very low, maybe 1) another cache is necessary? 2) or no cache at all
243 public static ByteBuffer getTemporaryAlignedDirectBuffer(int size,
244 int alignment) {
245 if (isBufferTooLarge(size)) {
246 return ByteBuffer.allocateDirect(size + alignment - 1)
247 .alignedSlice(alignment);
248 }
249
250 BufferCache cache = bufferCache.get();
251 ByteBuffer buf = cache.get(size);
252 if (buf != null) {
253 if (buf.alignmentOffset(0, alignment) == 0) {
254 return buf;
255 }
256 } else {
257 if (!cache.isEmpty()) {
258 buf = cache.removeFirst();
259 free(buf);
260 }
261 }
http://cr.openjdk.java.net/~kkharbas/8164900/webrev.07/src/java.base/windows/classes/sun/nio/fs/WindowsFileStore.java.frames.html
<http://cr.openjdk.java.net/%7Ekkharbas/8164900/webrev.07/src/java.base/windows/classes/sun/nio/fs/WindowsFileStore.java.frames.html>
137 private DiskFreeSpace readDiskFreeSpace() throws IOException {
138 try {
139 return GetDiskFreeSpace(root); // should this or below one be cached?
140 } catch (WindowsException x) {
141 x.rethrowAsIOException(root);
142 return null;
143 }
144 }
156 public long getBlockSize() throws IOException {
157 return readDiskFreeSpace().bytesPerSector(); // should this or above one be cached?
158 }
http://cr.openjdk.java.net/~kkharbas/8164900/webrev.07/src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java.frames.html
<http://cr.openjdk.java.net/%7Ekkharbas/8164900/webrev.07/src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java.frames.html>
Alignment is checked for both remained length and position in lots places (for example below 2 blocks of code), could they be extracted and made 2 functions calls?
179 if (dst.remaining() % alignment != 0) {
180 throw new IOException("Number of remaining bytes is not "
181 + "a multiple of the block size");
182 }
183 if (position() % alignment != 0) {
184 throw new IOException("Channel position is not a multiple "
185 + "of the block size");
186 }
http://cr.openjdk.java.net/~kkharbas/8164900/webrev.07/src/java.base/share/classes/sun/nio/ch/IOUtil.java.frames.html
<http://cr.openjdk.java.net/%7Ekkharbas/8164900/webrev.07/src/java.base/share/classes/sun/nio/ch/IOUtil.java.frames.html>
Alignment is checked for both remained length and position in lots places, could they be extracted and made 2 functions calls?
Are the checks in IOUtil.java and FileChannelImpl.java duplicated with each other, Should they be all moved and merged into IOUtil.java?
In java.nio.ByteBuffer
Should a new method be introduced in ByteBuffer to combine operations like ByteBuffer.allocateDirect(SIZE + alignment - 1).alignedSlice(alignment);
Besides of above comments, I have some questions about this new direct io feature:
What’s the expected FileChannel.read behavior when remaining bytes in
file is less than block size? For example, read from a file which is
only 10 bytes long through ExtendedOpenOption.DIRECT.
What’s the expected FileChannel.write behavior when remaining valid
bytes in buffer is less than block size? For example, write just 10
bytes to a file through ExtendedOpenOption.DIRECT.
What’s the expected FileChannel.position behavior after
FileChannel.read/write?
What’s the expected FileChannel.read behavior when remaining bytes in
file is more than block size? For example, read 4096 bytes from a file
which is 8192 long, is it guaranteed to read out 4096 bytes at once?
Similar question for write.
Is it necessary to add test case for above scenarios?
And, I think in some place, it should document the usage of
ExtendedOpenOption.DIRECT and cautions for example constraints on file
position, buffer position/remaning...
====================== Comments about tests ======================
http://cr.openjdk.java.net/~kkharbas/8164900/webrev.07/test/java/nio/channels/FileChannel/directio/DirectIOTest.java.html
<http://cr.openjdk.java.net/%7Ekkharbas/8164900/webrev.07/test/java/nio/channels/FileChannel/directio/DirectIOTest.java.html>
Should add “return” after print out message? else it will not skip the test
97 if (!fsType.equals("nfs") && !fsType.equals("ufs")) {
98 // print a message and return without failing
99 System.out.format("Skipping test: file system type %s of "
100 + "FileStore of %s is neither nfs nor ufs.%n", fsType, p);
101 }
http://cr.openjdk.java.net/~kkharbas/8164900/webrev.07/test/java/nio/channels/FileChannel/directio/libDirectIO.c.html
<http://cr.openjdk.java.net/%7Ekkharbas/8164900/webrev.07/test/java/nio/channels/FileChannel/directio/libDirectIO.c.html>
Inhttp://man7.org/linux/man-pages/man2/mincore.2.html, it says:
The/vec/ argument must point to an array containing at least
/(length+PAGE_SIZE-1) / PAGE_SIZE/ bytes. On return, the least
significant bit of each byte will be set if the corresponding page is
currently resident in memory, and be clear otherwise. (The settings
of the other bits in each byte are undefined; these bits are reserved
for possible later use.) Of course the information returned in/vec/
is only a snapshot: pages that are not locked in memory can come and
go at any moment, and the contents of/vec/ may already be stale by the
time this call returns.
So, is it possible that DirectIOTest.java may report false positive failure intermittently when Java_DirectIOTest_isFileInCache0 return true?
http://cr.openjdk.java.net/~kkharbas/8164900/webrev.07/test/java/nio/channels/FileChannel/directio/WriteDirect.java.html
<http://cr.openjdk.java.net/%7Ekkharbas/8164900/webrev.07/test/java/nio/channels/FileChannel/directio/WriteDirect.java.html>
Do you need to add negative test cases for below exception in IOUtil.java? Similar suggestion for ReadDirect.java
105 if (bb.alignmentOffset(pos, alignment) != 0) {
106 throw new IOException("Current location of the bytebuffer "
107 + "is not a multiple of the block size");
108 }
Should .alignedSlice(alignment); be appended at line 93?
92 try {
93 ByteBuffer bb = ByteBuffer.allocateDirect(8192);
94 fc.read(bb);
http://cr.openjdk.java.net/~kkharbas/8164900/webrev.07/test/java/nio/channels/FileChannel/directio/PreadDirect.java.html
<http://cr.openjdk.java.net/%7Ekkharbas/8164900/webrev.07/test/java/nio/channels/FileChannel/directio/PreadDirect.java.html>
The direct read/write in PreadDirect/PwriteDirect.java are put in a while loop, is it necessary? If yes, then is it necessary for similar read/write in ReadDirect/WriteDirect.java be put in a while loop too?
130 while (totalRead < 4096) {
131 int read = c.read(block, offset);
132 if (read < 0)
133 throw new Exception("Read failed");
134 totalRead += read;
135 }
And I don’t quite understand the logic of above code.
General comments for tests:
1. It would be better to replace magic number such as 4096 with
meaningfully named variables
2. It would be better to rename test methods with self-explained names,
for example, testNegativePosition is a positive case, genericTest2
is a negative case
Thank you
-Hamlin
On 01/09/2017 6:49 AM, Lu, Yingqi wrote:
>
> Hi Brian,
>
> Here is the webrev.07 version of the patch:
> http://cr.openjdk.java.net/~kkharbas/8164900/webrev.07/
> <http://cr.openjdk.java.net/%7Ekkharbas/8164900/webrev.07/>
>
> In this version, I have addressed the following issues:
>
> 1.Following the example of stringPlatformChars, made the DirectIOTest
> native library being compiled automatically. Removed
> DirectIOTest.java, README file and Makefile.
>
> 2.Added the filesystem type check on Solaris for DirectIOTest.
>
> 3.Checked if there is an equivalent system call like mincore in
> Windows to enable DirectIOTest for Windows. I could not find anything
> unfortunately.
>
> 4.Added the DirectIO flag FILE_FLAG_NO_BUFFERING and
> FILE_FLAG_WRITE_THROUGH back to Windows version of setDirect0() function.
>
> 5.Addressed the comments from you on DirectIO.c and DirectIO.java (now
> being renamed as DirectIOTest.java)
>
> 6.Added the tests for the read() and write() methods which take as
> parameter an array of buffers ByteBuffer[].
>
> 7.Changed the indentation for continuous lines.
>
> Issues that are still open:
>
> 1.How should we address the issue of AIX OS does not support setting
> O_DIRECT after the open()
>
> 2.Provide more detailed messages for IOExceptions with DirectIO
>
> 3.Are there any more tests we need to add?
>
> Please take a look at this version and let us know your feedback. I
> would also like to learn what will be a proper approach to solve the
> AIX issue and what kind of details we need to provide for the
> exception messages. I will try to incorporate all of those changes
> into the next version.
>
> Thanks,
>
> Lucy
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20170904/cc9f4395/attachment-0001.html>
More information about the nio-dev
mailing list