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

8262844: (fs) FileStore.supportsFileAttributeView might return false negative in case of ext3 #2778

Closed
wants to merge 4 commits into from

Conversation

ArnoZeller
Copy link
Contributor

@ArnoZeller ArnoZeller commented Mar 1, 2021

FileStore.supportsFileAttributeView might return a wrong value for ext3 mounts. In case of newer Linux distributions the extended attributes are enabled by default for ext3 mounts and do not show up /proc/mounts.
Currently the mount table is parsed to determine whether the mount options contain user_xattr or nouser_xattr. In case of neither entry it is expected that extended attributes are not supported on ext3.

This change removes the special handling for ext3 and ext4 and will always probe the fs if none of the options above are found. This should ensure that we return the right value.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8262844: (fs) FileStore.supportsFileAttributeView might return false negative in case of ext3

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2778/head:pull/2778
$ git checkout pull/2778

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 1, 2021

👋 Welcome back azeller! 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 Mar 1, 2021

@ArnoZeller 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 Mar 1, 2021
@ArnoZeller ArnoZeller changed the title Fix ext3 detection in LinuxFileStore FileStore.supportsFileAttributeView might return false negative in case of ext3. Mar 2, 2021
@ArnoZeller ArnoZeller changed the title FileStore.supportsFileAttributeView might return false negative in case of ext3. 8262844: FileStore.supportsFileAttributeView might return false negative in case of ext3. Mar 2, 2021
@ArnoZeller ArnoZeller changed the title 8262844: FileStore.supportsFileAttributeView might return false negative in case of ext3. 8262844: FileStore.supportsFileAttributeView might return false negative in case of ext3 Mar 2, 2021
@ArnoZeller ArnoZeller marked this pull request as ready for review March 2, 2021 13:30
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 2, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 2, 2021

Webrevs

@ArnoZeller ArnoZeller marked this pull request as draft March 2, 2021 17:39
@openjdk openjdk bot removed the rfr Pull request is ready for review label Mar 2, 2021
@AlanBateman
Copy link
Contributor

@bplb Would you have cycles to look at this one? We've always tried to reduce the probing on the mount points but this one means we will probe ext3 systems. I can't quire tell if this is something specific to SuSE or whether other distributions enable it too.

@bplb
Copy link
Member

bplb commented Mar 2, 2021

Apparently as of version 11-SP3, the default file system of SLES was changed to Ext3 [1]. In [2] it is stated that

"The default inode size of ext3 file systems has been increased from 128 bytes to 256 bytes on SLES11, because of extended attributes / ACLs."

From this it may be inferred that extended attributes are enabled by default from SLES 11. One may therefore suppose that the kernel was built with configuration item CONFIG_EXT3_FS_XATTR set to Y [3].

[1] https://www.suse.com/releasenotes/x86_64/SUSE-SLES/11-SP3/, section 6.8
[2] https://www.suse.com/support/kb/doc/?id=000017683
[3] https://cateee.net/lkddb/web-lkddb/EXT3_FS_XATTR.html

@bplb
Copy link
Member

bplb commented Mar 3, 2021

Based on testing it looks like xattr is enabled by default for ext2 and ext3 file systems in Ubuntu 18.04 LTS (Linux kernel 4.15.0).

@bplb
Copy link
Member

bplb commented Mar 3, 2021

From [1]:

   Some filesystems, such as Reiserfs (and, historically, ext2 and
   ext3), require the filesystem to be mounted with the user_xattr
   mount option in order for user extended attributes to be used.

   In the current ext2, ext3, and ext4 filesystem implementations,
   the total bytes used by the names and values of all of a file's
   extended attributes must fit in a single filesystem block ....

which reads as if extended attributes are now always supported in ext2, ext3, and ext4.

[1] https://man7.org/linux/man-pages/man7/xattr.7.html

@ArnoZeller
Copy link
Contributor Author

ArnoZeller commented Mar 3, 2021

I can add that RHEL 8.3 also has extended attributes as default mount option for ext3 and will not show up in /proc/mounts.

To me it seems that the default was changed some time ago, but it might not be so easy to find a kernel version that enabled it by default as it was in case of ext4.

And even if we could find a kernel version that started to use it as default, it might still be possible to disable it during build time or by setting default mount options on the ext3 filesystem with tune2fs. These will also not show up in /proc/mounts (just checked on RHEL 8.3):

#tune2fs -l /dev/sdb1 | grep "Default mount"
Default mount options: user_xattr acl
#cat /proc/mounts | grep /dev/sdb1
/dev/sdb1 /mnt ext3 rw,relatime 0 0

And this is the same for ext4.

I think the only save way to determine the support is to check on the filesystem. What do you think?

@RealCLanger
Copy link
Contributor

I also think that if we were to do it right, we should go with always probing. @ArnoZeller, can you try that out in our test landscape? Alternatively, if there's a reason against always probing, maybe we could add an option to enable probing?

@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Mailing list message from Alan Bateman on nio-dev:

On 03/03/2021 11:10, Arno Zeller wrote:

:
I can add that RHEL 8.3 also has extended attributes as default mount option for ext3 and will not show up in /proc/mounts.

To me it seems that the default was changed some time ago, but it might not be so easy to find a kernel version that enabled it by default as it was in case of ext4.

Okay, it seems there is enough evidence now that extended attributes are
enabled on ext3 on several distributions so I think we'll have to
proceed with this.

-Alan.

@ArnoZeller ArnoZeller changed the title 8262844: FileStore.supportsFileAttributeView might return false negative in case of ext3 8262844: (fs) FileStore.supportsFileAttributeView might return false negative in case of ext3 Mar 3, 2021
@RealCLanger
Copy link
Contributor

The latest changes remove the ext3/ext4 special handling and we should end up in probing. Let's see what the outcome of testing will be. If we were to do it like that, getKernelVersion() can be removed then as well.

}

// not ext3/4 so probe mount point
// no other information available so probe mount point
Copy link
Contributor

Choose a reason for hiding this comment

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

You've removed the special handling for ext4 too, did you mean to do that? I think we'll need to do a lot of testing with this change as I suspect it going to bite back in some unusual configurations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the change was intentional. I thought that ext4 has the same issues as ext3 and it might be possible to change the mount options or default mount options for ext4 with tune2fs to contain nouser_xattr. In this case the JDK would not detect that extended attributes are not supported. But I must admit that I just falsified my theory. tune2fs does not allow nouser_xattr as mount option. So for ext4 it can only be that someone uses a kernel that was configured to not support extended attributes on ext4 - this might not occur in real life.
I will revert the change back to the original first change.

@ArnoZeller ArnoZeller marked this pull request as ready for review March 3, 2021 17:05
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 3, 2021
Copy link
Contributor

@AlanBateman AlanBateman left a comment

Choose a reason for hiding this comment

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

Thanks for the analysis. I think we've converged and we are in agreement that we are forced to remove the special handing of ext3.

@openjdk
Copy link

openjdk bot commented Mar 3, 2021

@ArnoZeller 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:

8262844: (fs) FileStore.supportsFileAttributeView might return false negative in case of ext3

Reviewed-by: alanb, clanger, 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 34 new commits pushed to the master branch:

  • 75fb7cc: 8259228: Zero: rewrite (put|get)field from if-else chains to switches
  • 9730266: 8262973: Verify ParCompactionManager instance in PCAdjustPointerClosure
  • d91550e: 8262998: Vector API intrinsincs should not modify IR when bailing out
  • 80182f9: 8260925: HttpsURLConnection does not work with other JSSE provider.
  • dbef0ec: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
  • ee09bad: 8262300: jpackage app-launcher fails on linux when using JDK11 based runtime
  • 351889f: 8262508: Vector API's ergonomics is incorrect
  • 718d4d4: 8262989: Vectorize VectorShuffle checkIndexes, wrapIndexes and laneIsValid methods
  • c8b23e2: 8262064: Make compiler/ciReplay tests ignore lambdas in compilation replay
  • 02fbcb5: 8261532: Archived superinterface class cannot be accessed
  • ... and 24 more: https://git.openjdk.java.net/jdk/compare/3d3eb5c8d31ecd80309fdcbb67cd2aeeb2037c52...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@AlanBateman, @RealCLanger, @bplb) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 3, 2021
@ArnoZeller
Copy link
Contributor Author

Thanks for the analysis. I think we've converged and we are in agreement that we are forced to remove the special handing of ext3.

@AlanBateman, @RealCLanger, @bplb : Thanks for the reviews!
I am no committer and therefore I will need a sponsor.

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 5, 2021
@openjdk
Copy link

openjdk bot commented Mar 5, 2021

@ArnoZeller
Your change (at version 833f888) is now ready to be sponsored by a Committer.

@RealCLanger
Copy link
Contributor

/sponsor

@openjdk openjdk bot closed this Mar 5, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 5, 2021
@openjdk
Copy link

openjdk bot commented Mar 5, 2021

@RealCLanger @ArnoZeller Since your change was applied there have been 34 commits pushed to the master branch:

  • 75fb7cc: 8259228: Zero: rewrite (put|get)field from if-else chains to switches
  • 9730266: 8262973: Verify ParCompactionManager instance in PCAdjustPointerClosure
  • d91550e: 8262998: Vector API intrinsincs should not modify IR when bailing out
  • 80182f9: 8260925: HttpsURLConnection does not work with other JSSE provider.
  • dbef0ec: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
  • ee09bad: 8262300: jpackage app-launcher fails on linux when using JDK11 based runtime
  • 351889f: 8262508: Vector API's ergonomics is incorrect
  • 718d4d4: 8262989: Vectorize VectorShuffle checkIndexes, wrapIndexes and laneIsValid methods
  • c8b23e2: 8262064: Make compiler/ciReplay tests ignore lambdas in compilation replay
  • 02fbcb5: 8261532: Archived superinterface class cannot be accessed
  • ... and 24 more: https://git.openjdk.java.net/jdk/compare/3d3eb5c8d31ecd80309fdcbb67cd2aeeb2037c52...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8d3de4b.

💡 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
4 participants