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.

Sign up for GitHub

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

Jump to bottom

FreeBSD: Implement xattr=sa #11997

Closed
wants to merge 3 commits into from
Closed

FreeBSD: Implement xattr=sa #11997

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 4, 2021

Motivation and Context

Feature parity.

Description

Implement xattr=sa similarly to how it has been done for Linux.

I have split the changes into several commits to produce more readable diffs.

How Has This Been Tested?

ZTS, some manual testing of xattrs on a pool transferred between Linux and FreeBSD.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ghost ghost added the Status: Code Review Needed Ready for review and testing label May 4, 2021
@ghost ghost requested a review from amotin May 4, 2021 17:25
@ghost
Copy link
Author

ghost commented May 4, 2021 •
edited by ghost
Loading

TODO:

  • Update documentation
  • Squash some of the commits
  • Write a decent commit message

@ghost ghost force-pushed the xattr-sa branch 4 times, most recently from 10f7ba3 to d0f84dc Compare May 5, 2021 19:54
amotin
amotin approved these changes May 5, 2021
View reviewed changes
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

The last iteration looks good to me.

@ghost
Copy link
Author

ghost commented May 6, 2021

  • Rebased
  • Added mention in zfsprops(8)
  • Squashed dependent commits
  • Added a commit message with some context

@ghost ghost marked this pull request as ready for review May 6, 2021 17:15
Ryan Moeller added 3 commits May 12, 2021 19:02
FreeBSD: Don't force xattr mount option
2bf781a 
The kernel will use the xattr property by default when not overridden
by a mount option.

Signed-off-by: Ryan Moeller <[email protected]>
FreeBSD: Use SET_ERROR to trace xattr name errors
f91b573 
Signed-off-by: Ryan Moeller <[email protected]>
FreeBSD: Implement xattr=sa
e143d60 
FreeBSD historically has not cared about the xattr property; it was
always treated as xattr=on.  With xattr=on, xattrs are stored as files
in a hidden xattr directory.  With xattr=sa, xattrs are stored as
system attributes and get cached in nvlists during xattr operations.
This makes SA xattrs simpler and more efficient to manipulate.  FreeBSD
needs to implement the SA xattr operations for feature parity with
Linux and to ensure that SA xattrs are accessible when migrated or
replicated from Linux.

Following the example set by Linux, refactor our existing extattr vnops
to split off the parts handling dir style xattrs, and add the
corresponding SA handling parts.

Signed-off-by: Ryan Moeller <[email protected]>
@ghost
Copy link
Author

ghost commented May 12, 2021

  • Rebased

behlendorf
behlendorf approved these changes May 12, 2021
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

It's great to see this implemented for FreeBSD! I'd just suggest going through the ZTS tests and making sure all of the tests which us xattr=sa are now enabled for FreeBSD. My recollection is we may have disabled some of these tests by moving them from the common.run file to the linux.run file or wrapping them with is_linux.

@ghost
Copy link
Author

ghost commented May 13, 2021 •
edited by ghost
Loading

Somewhat amusingly, we've been running the tests that set xattr=sa on FreeBSD this whole time, they just haven't been respecting the property. During development I did confirm with debug messages that we are using the new SA code when appropriate now.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 13, 2021
behlendorf pushed a commit that referenced this pull request May 13, 2021
@behlendorf
FreeBSD: Use SET_ERROR to trace xattr name errors
d86debf 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #11997
behlendorf pushed a commit that referenced this pull request May 13, 2021
@behlendorf
FreeBSD: Implement xattr=sa
210231e 
FreeBSD historically has not cared about the xattr property; it was
always treated as xattr=on.  With xattr=on, xattrs are stored as files
in a hidden xattr directory.  With xattr=sa, xattrs are stored as
system attributes and get cached in nvlists during xattr operations.
This makes SA xattrs simpler and more efficient to manipulate.  FreeBSD
needs to implement the SA xattr operations for feature parity with
Linux and to ensure that SA xattrs are accessible when migrated or
replicated from Linux.

Following the example set by Linux, refactor our existing extattr vnops
to split off the parts handling dir style xattrs, and add the
corresponding SA handling parts.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #11997
@ghost ghost deleted the xattr-sa branch May 13, 2021 23:10
ghost pushed a commit to truenas/zfs that referenced this pull request May 14, 2021
FreeBSD: Don't force xattr mount option
0b02e60 
The kernel will use the xattr property by default when not overridden
by a mount option.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
ghost pushed a commit to truenas/zfs that referenced this pull request May 14, 2021
FreeBSD: Use SET_ERROR to trace xattr name errors
7a5f6d0 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
ghost pushed a commit to truenas/zfs that referenced this pull request May 14, 2021
FreeBSD: Implement xattr=sa
8208734 
FreeBSD historically has not cared about the xattr property; it was
always treated as xattr=on.  With xattr=on, xattrs are stored as files
in a hidden xattr directory.  With xattr=sa, xattrs are stored as
system attributes and get cached in nvlists during xattr operations.
This makes SA xattrs simpler and more efficient to manipulate.  FreeBSD
needs to implement the SA xattr operations for feature parity with
Linux and to ensure that SA xattrs are accessible when migrated or
replicated from Linux.

Following the example set by Linux, refactor our existing extattr vnops
to split off the parts handling dir style xattrs, and add the
corresponding SA handling parts.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
rincebrain pushed a commit to rincebrain/zfs that referenced this pull request May 17, 2021
@rincebrain
FreeBSD: Don't force xattr mount option
40e3a6f 
The kernel will use the xattr property by default when not overridden
by a mount option.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
rincebrain pushed a commit to rincebrain/zfs that referenced this pull request May 17, 2021
@rincebrain
FreeBSD: Use SET_ERROR to trace xattr name errors
352b6d7 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
rincebrain pushed a commit to rincebrain/zfs that referenced this pull request May 17, 2021
@rincebrain
FreeBSD: Implement xattr=sa
867c5d1 
FreeBSD historically has not cared about the xattr property; it was
always treated as xattr=on.  With xattr=on, xattrs are stored as files
in a hidden xattr directory.  With xattr=sa, xattrs are stored as
system attributes and get cached in nvlists during xattr operations.
This makes SA xattrs simpler and more efficient to manipulate.  FreeBSD
needs to implement the SA xattr operations for feature parity with
Linux and to ensure that SA xattrs are accessible when migrated or
replicated from Linux.

Following the example set by Linux, refactor our existing extattr vnops
to split off the parts handling dir style xattrs, and add the
corresponding SA handling parts.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
rincebrain pushed a commit to rincebrain/zfs that referenced this pull request May 17, 2021
@rincebrain
FreeBSD: Don't force xattr mount option
eb019fe 
The kernel will use the xattr property by default when not overridden
by a mount option.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
rincebrain pushed a commit to rincebrain/zfs that referenced this pull request May 17, 2021
@rincebrain
FreeBSD: Use SET_ERROR to trace xattr name errors
92f2657 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
rincebrain pushed a commit to rincebrain/zfs that referenced this pull request May 17, 2021
@rincebrain
FreeBSD: Implement xattr=sa
2adc412 
FreeBSD historically has not cared about the xattr property; it was
always treated as xattr=on.  With xattr=on, xattrs are stored as files
in a hidden xattr directory.  With xattr=sa, xattrs are stored as
system attributes and get cached in nvlists during xattr operations.
This makes SA xattrs simpler and more efficient to manipulate.  FreeBSD
needs to implement the SA xattr operations for feature parity with
Linux and to ensure that SA xattrs are accessible when migrated or
replicated from Linux.

Following the example set by Linux, refactor our existing extattr vnops
to split off the parts handling dir style xattrs, and add the
corresponding SA handling parts.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
@ghost ghost mentioned this pull request May 24, 2021
Merged
19 tasks
@ghost ghost linked an issue May 27, 2021 that may be closed by this pull request
FreeBSD: xattr property is silently unsupported #11975
Closed
@ghost ghost mentioned this pull request May 27, 2021
Closed
ghost pushed a commit to truenas/zfs that referenced this pull request May 27, 2021
FreeBSD: Don't force xattr mount option
3bf8bcd 
The kernel will use the xattr property by default when not overridden
by a mount option.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
ghost pushed a commit to truenas/zfs that referenced this pull request May 27, 2021
FreeBSD: Use SET_ERROR to trace xattr name errors
c98764f 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
ghost pushed a commit to truenas/zfs that referenced this pull request May 27, 2021
FreeBSD: Implement xattr=sa
c674876 
FreeBSD historically has not cared about the xattr property; it was
always treated as xattr=on.  With xattr=on, xattrs are stored as files
in a hidden xattr directory.  With xattr=sa, xattrs are stored as
system attributes and get cached in nvlists during xattr operations.
This makes SA xattrs simpler and more efficient to manipulate.  FreeBSD
needs to implement the SA xattr operations for feature parity with
Linux and to ensure that SA xattrs are accessible when migrated or
replicated from Linux.

Following the example set by Linux, refactor our existing extattr vnops
to split off the parts handling dir style xattrs, and add the
corresponding SA handling parts.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 28, 2021
@behlendorf
FreeBSD: Don't force xattr mount option
793dffb 
The kernel will use the xattr property by default when not overridden
by a mount option.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 28, 2021
@behlendorf
FreeBSD: Use SET_ERROR to trace xattr name errors
c2c02e4 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
ghost pushed a commit to truenas/zfs that referenced this pull request May 28, 2021
FreeBSD: Implement xattr=sa
f9f19a9 
FreeBSD historically has not cared about the xattr property; it was
always treated as xattr=on.  With xattr=on, xattrs are stored as files
in a hidden xattr directory.  With xattr=sa, xattrs are stored as
system attributes and get cached in nvlists during xattr operations.
This makes SA xattrs simpler and more efficient to manipulate.  FreeBSD
needs to implement the SA xattr operations for feature parity with
Linux and to ensure that SA xattrs are accessible when migrated or
replicated from Linux.

Following the example set by Linux, refactor our existing extattr vnops
to split off the parts handling dir style xattrs, and add the
corresponding SA handling parts.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
@behlendorf behlendorf mentioned this pull request May 30, 2021
Closed
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
FreeBSD: Don't force xattr mount option
8b4eccc 
The kernel will use the xattr property by default when not overridden
by a mount option.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
FreeBSD: Use SET_ERROR to trace xattr name errors
13d9ca9 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 15, 2021
@tonyhutter
FreeBSD: Implement xattr=sa
6fe6192 
FreeBSD historically has not cared about the xattr property; it was
always treated as xattr=on.  With xattr=on, xattrs are stored as files
in a hidden xattr directory.  With xattr=sa, xattrs are stored as
system attributes and get cached in nvlists during xattr operations.
This makes SA xattrs simpler and more efficient to manipulate.  FreeBSD
needs to implement the SA xattr operations for feature parity with
Linux and to ensure that SA xattrs are accessible when migrated or
replicated from Linux.

Following the example set by Linux, refactor our existing extattr vnops
to split off the parts handling dir style xattrs, and add the
corresponding SA handling parts.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 13, 2021
@tonyhutter
FreeBSD: Implement xattr=sa
7100db9 
FreeBSD historically has not cared about the xattr property; it was
always treated as xattr=on.  With xattr=on, xattrs are stored as files
in a hidden xattr directory.  With xattr=sa, xattrs are stored as
system attributes and get cached in nvlists during xattr operations.
This makes SA xattrs simpler and more efficient to manipulate.  FreeBSD
needs to implement the SA xattr operations for feature parity with
Linux and to ensure that SA xattrs are accessible when migrated or
replicated from Linux.

Following the example set by Linux, refactor our existing extattr vnops
to split off the parts handling dir style xattrs, and add the
corresponding SA handling parts.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11997
@CyberCr33p
Copy link

I create a dataset with xattr=sa on FreeBSD 13.1 (ZFS 2.1.4), touch a file and setfacl it.

Using zdb I don't see any reference for xattr in this file inode.

Any idea why?

@ghost
Copy link
Author

ghost commented Jan 31, 2023

setfacl touches the ACL, not an xattr

@CyberCr33p
Copy link

setfacl touches the ACL, not an xattr

Thank you for your reply.

I asked here too https://forums.freebsd.org/threads/zfs-and-xattr-sa.87902/ and I realized my mistake.

Also I did some tests and it works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FreeBSD: xattr property is silently unsupported
3 participants
@CyberCr33p @behlendorf @amotin