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

Posix ACL Support #1809

Closed
wants to merge 3 commits into from
Closed

Posix ACL Support #1809

wants to merge 3 commits into from

Conversation

behlendorf
Copy link
Contributor

This change adds support for Posix ACLs by storing them as an xattr
which is common practice for many Linux file systems. Since the
Posix ACL is stored as an xattr it will not overwrite any existing
ZFS/NFSv4 ACLs which may have been set. The Posix ACL will also
be non-functional on other platforms although it may be visible
as an xattr if that platform understands SA based xattrs.

By default Posix ACLs are disabled but they may be enabled with
the new 'aclmode=noacl|posixacl' property. Set the property to
'posixacl' to enable them. If ZFS/NFSv4 ACL support is ever added
an appropriate acltype will be added.

This change passes the POSIX Test Suite cleanly with the exception
of xacl/00.t test 45 which is incorrect for Linux (Ext4 fails too).

http://www.tuxera.com/community/posix-test-suite/

This was referenced Oct 23, 2013
@ryao
Copy link
Contributor

ryao commented Oct 25, 2013

If xattr=sa is not set, will SA based xattrs be used anyway?

@behlendorf
Copy link
Contributor Author

Yes. If there's still space left in the bonus or spill blocks. Failing that it will use the legacy directory format. We thought always doing SA was preferable since there isn't any real need for other platforms to access them.

@ryao
Copy link
Contributor

ryao commented Oct 25, 2013

@behlendorf I think that could cause headaches for people in some corner cases. Specifically, people would be inclined expect to be able to take their pool to another Open ZFS system with the only loss in functionality that the xattrs storing the POSIX ACLs would be ignored. They would not realize that their pool has had a ZoL-specific extension used. They might even think that they could make sense of the ZoL POSIX ACLs manually by using their own system's tools to read the xattrs storing the POSIX ACLs.

I would feel much better about this change if the SA based xattrs are used only when xattr=sa is set until ZFS Version feature flags are implemented. That way the implementation would match people's natural expectations.

@maxximino
Copy link
Contributor

@ryao Doing what you suggest will force a performance impact (accessing a file becomes accessing the file itself, its xattr directory and its system.posix_acl_access xattr file) on every first access to an inode for every user in the default configuration.
Personally I think that this should mainly be fixed by documentation and maybe by changing the "acltype=posixacl" into "acltype=posixacl-linux-only" to give a strong hint to whoever does not read docs carefully.
An option to write posixacl into regular xattrs can be considered, but should not be the default.
IF OpenZFS is going to import SA-based xattrs soon, this might not be a problem. That change is listed within the "portable" section of ZoL-specifc changes (http://open-zfs.org/wiki/Platform_code_differences).
On ZoL it's also possible to use zdb to read them (zdb -vvv mypool/testfs [objectnumber]).

@ryao
Copy link
Contributor

ryao commented Oct 25, 2013

@maxximino Open ZFS is a forum for collaboration between the developers of the 4 major ZFS platforms. Having the code in ZoL implies that it is in Open ZFS, but we still need to make changes in a way that plays nicely with other implementations. Usually, this means just getting it into illumos-gate. That would require implementing ZFS Version feature flags. Someone will need to do that. Once that is done, then everything should be fine compatibility wise. People who want this functionality could just flip on the feature. Then other implementations would see the feature flag that says that the dataset can still be read and everything would be fine.

@behlendorf
Copy link
Contributor Author

@ryao Yes, I hear what you're saying and I went back and forth repeatedly on this. I could definitely be persuaded we should do something else since overriding the set xattr behavior wouldn't be expected. But as @maxximino says the performance penalty for storing them in the legacy directory format is steep! The initial access of a cold file will cost three IOs instead of just one.

We actually already have this exact problem for SELinux security access controls which are also stored as xattrs. In that case we just documented the fact that xattr=sa should be set when using SELinux for best performance.

How about this. We add a paragraph to the acltype section of the man page suggesting the users set xattr=sa if they intend to use ACLs and have no need for portability. We can add a similar section to the various FAQs. Then we drop the code to prefer the SA xattr from this patch.

We could go one step father to reduce the support burden and make xattr=sa the new default value for new filesystems in ZoL. This code is now well tested and will result in performance improvements for the vast majority of users. Plus enabling all new features by default is more consistent with ZFSs behavior on other platforms. If you prize compatibility over performance you must take steps to ensure it.

@ryao
Copy link
Contributor

ryao commented Oct 26, 2013

How about this. We add a paragraph to the acltype section of the man page suggesting the users set xattr=sa if they intend to use ACLs and have no need for portability. We can add a similar section to the various FAQs. Then we drop the code to prefer the SA xattr from this patch.

That sounds good to me.
We could go one step father to reduce the support burden and make xattr=sa the new default value for new filesystems in ZoL. This code is now well tested and will result in performance improvements for the vast majority of users. Plus enabling all new features by default is more consistent with ZFSs behavior on other platforms. If you prize compatibility over performance you must take steps to ensure it.

I would prefer to see ZFS version feature flags implemented before that is done. Once they are in place, I am all for it.

@behlendorf
Copy link
Contributor Author

@maxximino @ryao Refreshed against the latest master with the following changes. I'm personally happy with this version and it passes all automated testing, so if you're also OK with it I'll get it merged.

  • Core xattr remove changes broken in to a separate patch.
  • Extended zfs(8) xattr property section which discusses both style of xattrs.
  • Extended zfs(8) acltype property section recommending setting xattr=sa property.
  • Posix ACLs are now always stored based on the current xattr property setting.

@maxximino
Copy link
Contributor

It's OK for me.

Attempting to remove an xattr from a file which does not contain
any directory based xattrs would result in the xattr directory
being created.  This behavior is non-optimal because it results
in write operations to the pool in addition to the expected error
being returned.

To prevent this the CREATE_XATTR_DIR flag is only passed in
zpl_xattr_set_dir() when setting a non-NULL xattr value.  In
addition, zpl_xattr_set() is updated similarly such that it will
return immediately if passed an xattr name which doesn't exist
and a NULL value.

Signed-off-by: Massimo Maggi <me@massimo-maggi.eu>
Signed-off-by: Richard Yao <ryao@gentoo.org>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#170
Extend the xattr property section of zfs(8) such that it covers
both styles of supported xattr.  A short discussion of the benefits
and drawbacks of each type is presented to allow users to make an
informed choice.

Signed-off-by: Massimo Maggi <me@massimo-maggi.eu>
Signed-off-by: Richard Yao <ryao@gentoo.org>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#170
This change adds support for Posix ACLs by storing them as an xattr
which is common practice for many Linux file systems.  Since the
Posix ACL is stored as an xattr it will not overwrite any existing
ZFS/NFSv4 ACLs which may have been set.  The Posix ACL will also
be non-functional on other platforms although it may be visible
as an xattr if that platform understands SA based xattrs.

By default Posix ACLs are disabled but they may be enabled with
the new 'aclmode=noacl|posixacl' property.  Set the property to
'posixacl' to enable them.  If ZFS/NFSv4 ACL support is ever added
an appropriate acltype will be added.

This change passes the POSIX Test Suite cleanly with the exception
of xacl/00.t test 45 which is incorrect for Linux (Ext4 fails too).

  http://www.tuxera.com/community/posix-test-suite/

Signed-off-by: Massimo Maggi <me@massimo-maggi.eu>
Signed-off-by: Richard Yao <ryao@gentoo.org>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#170
@behlendorf
Copy link
Contributor Author

Merged.

023699c Posix ACL Support
7c2448a Improve xattr property documentation
fc9e053 Prevent xattr remove from creating xattr directory

@behlendorf behlendorf closed this Oct 29, 2013
@behlendorf behlendorf deleted the acl-xattr branch February 16, 2017 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants