Proposal for adding O_DIRECT support into JDK 9

Tue Sep 5 03:44:20 UTC 2017

Hi Lucy,

Thank you for response, please check my comments inline.

On 05/09/2017 1:50 AM, Lu, Yingqi wrote:
>
> 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
>
OK.
> 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.
>
OK.
> 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 J
>
Maybe there is misunderstanding here, I don't mean we should add such 
method in test, I means to add a new API in java.nio.ByteBuffer class, 
the reason is that "ByteBuffer.allocateDirect(SIZE + alignment - 
1).alignedSlice(alignment);" is necessary for a direct read/write, a new 
API for example ByteBuffer.allocateAlignedDirect(size, alignment) or 
allocateAlignedDirect(size) might be helpful for direct io end user.
>
> 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.
>
OK.
>
> 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.
>
Maybe we need to add some comments in the test, I'm not sure if 
@intermittent is needed to be added into test. So when test fails in the 
future, it can be analyzed quickly.
>
> 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()
>
Thank you for clarifying, I miss the line 91 " fc = FileChannel.open(p);".
>
> 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.
>
The negative tests in P[read|write]Direct.java are checking channel 
position, but in IOUtil.java line 106, it's throwing exception when 
checking buffer position.
>
> 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.
>
OK.
>
> 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.
Does this mean that there is no way for direct io to write a file the 
length of which is not multiple of block size?
> What’s the expected FileChannel.position behavior after FileChannel.read/write?
>          Similarly to the non-DirectIO case, it will move to the current position.
Maybe I missed something. But in 
http://cr.openjdk.java.net/~kkharbas/8164900/webrev.07/test/java/nio/channels/FileChannel/directio/PreadDirect.java.html, 
there is following check,

  150                 // Ensure that file pointer position has not changed
  151                 if (originalPosition != newPosition)
  152                     throw new Exception("File position modified");

> 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.
>
The major reason I asked above several questions is that direct io works 
a little different from non-direct io, but currently we don't have a 
java doc to state this behavior difference, I think it would be helpful 
for direct io end users if we do so, but I'm not sure where to add this 
java doc, and how detailed it should be.

Thank you
-Hamlin
>
> Thanks,
>
> Lucy
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20170905/0569351f/attachment-0001.html>