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

Honor xattr=sa dataset property #3788

Closed
wants to merge 1 commit into from
Closed

Conversation

nedbass
Copy link
Contributor

@nedbass nedbass commented Sep 16, 2015

ZFS incorrectly uses directory-based extended attributes even when
xattr=sa is specified as a dataset property or mount option. Support to
honor temporary mount options including "xattr" was added in commit
0282c41. There are two issues with the
mount option handling:

  • Libzfs has historically included "xattr" in its list of default mount
    options with no value specified. Omitting the value implies xattr=on or
    directory-based xattrs, so the dataset is configured to use
    directory-based xattrs regardless of the xattr dataset property value.
    Address this by removing "xattr" from the list of default mount
    options.
  • The option parser incorrectly treats the xattr mount option as a
    boolean value. Even if xattr=sa is specified, the parser translates
    that to xattr=on. Address this by treating the option as a scalar
    value.

Issue #3787

Signed-off-by: Ned Bass bass6@llnl.gov

@dweeezil
Copy link
Contributor

I really wish we could move to a reference-counted feature flag to manage SA xattrs and use the xattr property as a boolean as is the case with other OpenZFS implementations.

@dweeezil
Copy link
Contributor

One other salient point is that Solaris has MNTOPT_XATTR which is interpreted by their VFS whereas Linux has no such flag. An argument could be made for completely removing the handling of xattr/noxattr in libzfs.

@behlendorf
Copy link
Contributor

We should plan to do exactly that for the next tag. In addition I think we should take the opportunity to fix some of the other rough edges too. We'll want to carefully think through the behavior we want from a user perspective.

In the meanwhile we can address this in a point release with a minimal fix.

@nedbass
Copy link
Contributor Author

nedbass commented Sep 19, 2015

Updated patch to use saxattr and dirxattr mount options instead of xattr=sa and xattr=dir. This mirrors the valid dataset property values and adheres to mount option naming conventions. Also eliminates ugly string handling from the option parsing code.

ZFS incorrectly uses directory-based extended attributes even when
xattr=sa is specified as a dataset property or mount option. Support to
honor temporary mount options including "xattr" was added in commit
0282c41. There are two issues with the
mount option handling:

* Libzfs has historically included "xattr" in its list of default mount
  options. This overrides the dataset property, so the dataset is always
  configured to use directory-based xattrs even when the xattr dataset
  property is set to off or sa. Address this by removing "xattr" from
  the set of default mount options in libzfs.

* There was no way to enable system attribute-based extended attributes
  using temporary mount options. Add the mount options "saxattr" and
  "dirxattr" which enable the xattr behavior their names suggest.  This
  approach has the advantages of mirroring the valid xattr dataset
  property values and following existing conventions for mount option
  names.

Issue openzfs#3787

Signed-off-by: Ned Bass <bass6@llnl.gov>
@nedbass
Copy link
Contributor Author

nedbass commented Sep 19, 2015

Added new mount options to mount.zfs.

@behlendorf
Copy link
Contributor

Merged as:

3af56fd Honor xattr=sa dataset property

@behlendorf behlendorf closed this Sep 19, 2015
@behlendorf behlendorf added this to the 0.7.0 milestone Sep 19, 2015
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.

None yet

3 participants