Proposal for adding O_DIRECT support into JDK 9
Lu, Yingqi
yingqi.lu at intel.com
Mon Sep 4 17:50:10 UTC 2017
Hi Hamlin,
Thank you very much for reviewing the patch. It is very helpful!
webrev.08 has not been published yet as I am waiting on the feedback to resolve to the AIX issue. I sent a separate email couple days ago with the details.
Please see my answers below:
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
Yes, direct and non-direct IO can be used at the same time. I am not familiar on how often the code is accessed. I will let Alan/Brian and others comment on the proper approach
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?
Good point! As you suggested, I will create 2 static functions inside IOUtil.java, so that both FileChannelImpl and IOUtil class can use them.
In java.nio.ByteBuffer
Should a new method be introduced in ByteBuffer to combine operations like ByteBuffer.allocateDirect(SIZE + alignment - 1).alignedSlice(alignment);
I only see ByteBuffer.allocateDirect() being called twice. I am not totally convinced whether we need to introduce a new API. However, I am surely open to this suggestion if people here agree :)
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
This is already solved in my local webrev.08.
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>
In http://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?
Yes, pages are not locked in memory and they can move freely. However, I think this is the nature of the test. Given the temp file size is very small compare to the memory size and the page check happens right after the IO operation, I think the test should be good in most of the cases.
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);
The channel is opened as a non-Direct channel. It is just to read the content and confirm the write operation is performed correctly. I think there is no need to append .alignedSlice()
I think the negative test for non-aligned position are added in PreadDirect.java and PwriteDirect.java. Please let me know if I missed anything though.
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.
I followed the example of Read.java and Write.java on the while loop. I can modify it accordingly.
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.
It will read 10 bytes and return.
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.
It will throw exception. The remaining buffer size needs to a multiple of the alignment value.
What's the expected FileChannel.position behavior after FileChannel.read/write?
Similarly to the non-DirectIO case, it will move to the current position.
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.
It depends on the current position of the file. If the position is at the beginning of the file, yes, it will read 4096 bytes. If the position is > 0, but not aligned, then it will throw error. If the position is at 4096, it will read 4096 bytes. If the position is at the end of the file, it will read 0. Everything is similar to non-Direct channel, except the current position needs to be aligned.
Is it necessary to add test case for above scenarios?
I can add tests for case 1. I think other cases are covered with the existing tests. Please let me know if I missed anything though.
Thanks,
Lucy
From: huaming.li at oracle.com [mailto:huaming.li at oracle.com]
Sent: Monday, September 04, 2017 12:17 AM
To: nio-dev at openjdk.java.net; Lu, Yingqi <yingqi.lu at intel.com>
Subject: Re: Proposal for adding O_DIRECT support into JDK 9
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>
In http://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/2ff39eb7/attachment-0001.html>
More information about the nio-dev
mailing list