Proposal for adding O_DIRECT support into JDK 9

[Lu, Yingqi]

Hi Hamlin,

Thank you again for the quick feedback.


Regarding to add an API into ByteBuffer, I like your suggestion. It makes DirectIO coding much easier. If everyone agrees, I can add the API "ByteBuffer.allocateAlignedDirect(size, alignment)" into ByteBuffer. Please let me know if this is appropriate.


Regarding to DirectIOTest with mincore, if the test runs on an idle system with a reasonable amount of memory, I do not think it will be a problem. However, I agree with you that it might be a good idea to add some comments. Is something like this sufficient? "This test is to check whether a file or part of the file exists in the kernel file system cache. As memory pages are not locked in the file system cache, result of the test might be stale before the function returns. The test should not be run concurrently with other programs that clear file system cache"

I will add one more test for "not aligned buffer position" check. Thank you for pointing this out.


Base on my understanding, we cannot write to a file the length of which is not aligned without padding. I think this the expected behavior of DirectIO. In Java applications, user need to work on their own padding.

Thanks,
Lucy


From: huaming.li at oracle.com [mailto:huaming.li at oracle.com]
Sent: Monday, September 04, 2017 8:44 PM
To: Lu, Yingqi <yingqi.lu at intel.com>; nio-dev at openjdk.java.net
Subject: Re: Proposal for adding O_DIRECT support into JDK 9


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 :)
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/24939c1c/attachment-0001.html>