Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8338884: java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3 #20687

Closed
wants to merge 23 commits into from

Conversation

sendaoYan
Copy link
Member

@sendaoYan sendaoYan commented Aug 23, 2024

Hi all,
On alinux3(alibaba cloud linux version 3) system, the /tmp disk partition is mounted as tmpfs filesystem type, this filesystem type doesn't support create time(birth time).

Before this PR, this test check if there is statx system call present or not to determise the test environment support birth time or not. I think it's not enough when the tested filesystem type is tmpfs. When the tested filesystem type is tmpfs, then the tested file doesn't support birth time.

On RHEL 8 tmpfs doesn't seem to support birth time, but on F39 tmpfs does seem to support birth time. Looks like this might be related to the kernel version. It's difficult to enumerate all the combination of file system type and linux kernel version to determine the testd file support birth time or not. So in this PR, I get the result from statx linux syscall, to determine the testd file support birth time or not.

Test fix only, the change has been verified, risk is low.

Additional test:

  • Alinux3 glibc:2.32
  1. /tmp/file supportsCreationTimeRead == false
  2. ./file supportsCreationTimeRead == true
  • CentOS7 docker container glibc:2.17
  1. /tmp/file supportsCreationTimeRead == false
  2. ./file supportsCreationTimeRead == false

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8338884: java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3 (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20687/head:pull/20687
$ git checkout pull/20687

Update a local copy of the PR:
$ git checkout pull/20687
$ git pull https://git.openjdk.org/jdk.git pull/20687/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20687

View PR using the GUI difftool:
$ git pr show -t 20687

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20687.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 23, 2024

👋 Welcome back syan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Aug 23, 2024

@sendaoYan This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8338884: java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3

Reviewed-by: sgehwolf, bpb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 618 new commits pushed to the master branch:

  • 38c1d65: 8337980: Javac allows invocation of an inherited instance method from a static method
  • 950e3a7: 8341625: Improve ZipFile validation of the END header
  • e704c05: 8340547: Starting many threads can delay safepoints
  • c30ad01: 8325495: C2: implement optimization for series of Add of unique value
  • ff2f39f: 8340214: C2 compilation asserts with "no node with a side effect" in PhaseIdealLoop::try_sink_out_of_loop
  • ecc77a5: 8336702: C2 compilation fails with "all memory state should have been processed" assert
  • d936556: 8341633: StatSampler::assert_system_property: Print the keys and values of the assert
  • 3fba170: 8340786: Introduce Predicate classes with predicate iterators and visitors for simplified walking
  • 047c2d7: 8341141: Optimize DirectCodeBuilder
  • d636e0d: 8341688: Aarch64: Generate comments in -XX:+PrintInterpreter to link to source code
  • ... and 608 more: https://git.openjdk.org/jdk/compare/ea3370982bfd3da4b200b738dd3b8c16cebb3a34...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 23, 2024
@openjdk
Copy link

openjdk bot commented Aug 23, 2024

@sendaoYan The following label will be automatically applied to this pull request:

  • nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the nio nio-dev@openjdk.org label Aug 23, 2024
@mlbridge
Copy link

mlbridge bot commented Aug 23, 2024

@sendaoYan
Copy link
Member Author

/issue JDK-8338884

@openjdk openjdk bot changed the title 8338884: Test BasicFileAttributeView/CreationTime.java#tmp fails on alinux3 after JDK-8338696 8338884: java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3 Aug 23, 2024
@openjdk
Copy link

openjdk bot commented Aug 23, 2024

@sendaoYan This issue is referenced in the PR title - it will now be updated.

@bplb
Copy link
Member

bplb commented Aug 23, 2024

What happens in the test without this change if the fix for JDK-8338696 is applied?

@sendaoYan
Copy link
Member Author

sendaoYan commented Aug 23, 2024

As descript in JDK-8338884, this test will report fails java.lang.RuntimeException: Creation time should not have changed without this change.
On alinux3(alibaba cloud system v3) the /tmp disk partition is mounted as tmpfs filesystem, and linux doesn't support birth time with this type filesystem.
Without this change before the fix JDK-8338696 is applied, then the creation time is set to the epoch (1970-01-01T00:00:00Z) so this test will throw SkippedException.
Without this change after the fix JDK-8338696 is applied, the SkippedException do not throw any more, and the supportsCreationTimeRead set to true, actually on alinux3 the file in /tmp partition doesn't support create time(birth time), and create time always equals to modified time, then this test report fails.

@bplb
Copy link
Member

bplb commented Aug 23, 2024

Without this change after the fix JDK-8338696 is applied, the SkippedException do not throw any more, and the supportsCreationTimeRead set to true, actually on alinux3 the file in /tmp partition doesn't support create time(birth time), and create time always equals to modified time, then this test report fails.

So you are saying that with the fix for JDK-8338696 applied, this test fails with RuntimeException?

@sendaoYan
Copy link
Member Author

sendaoYan commented Aug 23, 2024

So you are saying that with the fix for JDK-8338696 applied, this test fails with RuntimeException?

Yes, without this change, with the fix for JDK-8338696 applied, this test fails with RuntimeException

@sendaoYan
Copy link
Member Author

To reproduce the fails, test command on most linux system:

rm -rf /dev/shm/tmp/ ; jtreg -v:fail,error -w /dev/shm/tmp -nr test/jdk/java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#cwd

… and !Files.getFileStore(file).type().contentEquals("tmpfs")
@sendaoYan sendaoYan requested a review from AlanBateman August 26, 2024 13:25
@sendaoYan
Copy link
Member Author

nfs on linux also doesn't support create time:

> mount | grep "/vmfarm "
pepsi.jvm.alibaba.net:/vmfarm on /vmfarm type nfs4 (rw,relatime,vers=4.1,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp,port=0,timeo=600,retrans=2,sec=sys,clientaddr=11.165.41.2,local_lock=none,addr=11.158.137.189)
> stat /vmfarm/mountVmFarm.sh 
  File: ‘/vmfarm/mountVmFarm.sh’
  Size: 71              Blocks: 8          IO Block: 1048576 regular file
Device: 3ch/60d Inode: 12          Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2024-02-28 17:03:54.713251462 +0800
Modify: 2020-03-03 11:56:41.548038922 +0800
Change: 2020-03-03 11:56:41.548038922 +0800
 Birth: -

@sendaoYan
Copy link
Member Author

On RHEL 8 tmpfs doesn't seem to support birth time, but on F39 tmpfs does seem to support birth time. Looks like this might be related to the kernel version. It's difficult to enumerate all the combination of file system type and linux kernel version to determine the testd file support birth time or not. So in this PR, I get the output from stat -c linux command, to determine the testd file support birth ot not.

@sendaoYan
Copy link
Member Author

I think this PR should be set as draft status, sorry for that.

@bplb
Copy link
Member

bplb commented Sep 12, 2024

For whatever it shows, the test passed on Linux, macOS, and Windows in our CI.

@jerboaa
Copy link
Contributor

jerboaa commented Sep 13, 2024

For whatever it shows, the test passed on Linux, macOS, and Windows in our CI.

@bplb Do you mean the updated one in this PR or the old one without this patch?

@bplb
Copy link
Member

bplb commented Sep 13, 2024

For whatever it shows, the test passed on Linux, macOS, and Windows in our CI.

@bplb Do you mean the updated one in this PR or the old one without this patch?

@jerboaa The latest version in this PR.

@jerboaa
Copy link
Contributor

jerboaa commented Sep 13, 2024

For whatever it shows, the test passed on Linux, macOS, and Windows in our CI.

@bplb Do you mean the updated one in this PR or the old one without this patch?

@jerboaa The latest version in this PR.

OK, thanks!

@sendaoYan
Copy link
Member Author

For whatever it shows, the test passed on Linux, macOS, and Windows in our CI.
@jerboaa The latest version in this PR.

Thanks for the test.

@sendaoYan
Copy link
Member Author

@AlanBateman Can you review this again, thanks.

@AlanBateman
Copy link
Contributor

@AlanBateman Can you review this again, thanks.

I don't have cycles right now but I'm sure @bplb can help.

@bplb
Copy link
Member

bplb commented Oct 7, 2024

I don't have cycles right now but I'm sure @bplb can help.

It's now in my queue.

@bplb
Copy link
Member

bplb commented Oct 8, 2024

The test passed on Linux, macOS, and Windows in our internal test platform. I think after the two most recent points mentioned above are addressed it is good to go.

…ttributeView/libCreationTimeHelper.c; 2. use System.err instead of System.out in test/jdk/java/nio/file/attribute/BasicFileAttributeView/CreationTime.java
@sendaoYan
Copy link
Member Author

The test passed on Linux, macOS, and Windows in our internal test platform. I think after the two most recent points mentioned above are addressed it is good to go.

Thanks for the verify and review. The two advices has been addressed.

…st/jdk/java/nio/file/attribute/BasicFileAttributeView/CreationTime.java
Copy link
Member

@bplb bplb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all right now.

@sendaoYan sendaoYan requested a review from jerboaa October 9, 2024 15:49
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 9, 2024
@sendaoYan
Copy link
Member Author

Thanks all for the review.
/integrate

@openjdk
Copy link

openjdk bot commented Oct 10, 2024

Going to push as commit 9d621d3.
Since your change was applied there have been 630 commits pushed to the master branch:

  • 475f8f9: 8341859: Optimize ClassFile Benchmark Write
  • c850ecb: 8341755: Optimize argNames in InnerClassLambdaMetafactory
  • 172f744: 8340985: Open source some Desktop related tests
  • e7045e9: 8341684: Typo in External Specifications link of java.util.Currency
  • 49c7148: 8341366: Suspicious check in Locale.getDisplayName(Locale inLocale)
  • 52eded4: 8341170: Open source several Choice related tests (part 2)
  • a45abf1: 8341860: ProblemList applications/ctw/modules/java_base_2.java on linux-x64
  • 593c27e: 8341535: sun/awt/font/TestDevTransform.java fails with RuntimeException: Different rendering
  • 3180aaa: 8341832: Incorrect continuation address of synthetic SIGSEGV for APX in product builds
  • 3ab519f: 8341424: GHA: Collect hs_errs from build time failures
  • ... and 620 more: https://git.openjdk.org/jdk/compare/ea3370982bfd3da4b200b738dd3b8c16cebb3a34...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 10, 2024
@openjdk openjdk bot closed this Oct 10, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 10, 2024
@openjdk
Copy link

openjdk bot commented Oct 10, 2024

@sendaoYan Pushed as commit 9d621d3.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated nio nio-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

4 participants