xattr dataset prop: change defaults to sa

[gmelikov]

I'll push this PR as proposal for better defaults, but will set it as draft until manual testing.

Motivation and Context

Let's have optimal default settings.

Description

It's the main recommendation to set xattr=sa
even in man pages, so let's set it by default.

xattr=sa don't use feature flag, so in the worst
case we'll have non-readable xattrs by other
non-openzfs platforms.

Non-overridden default xattr prop of existing pools
will automatically use sa after this commit too.

How Has This Been Tested?

CI's tests looks good, I'll check compatibility with existing pools later.

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:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[gmelikov]

Works well, but there is an interesting nit:

• new explicit xattr=on set will set ZFS_XATTR_SA (same as it was set ZFS_XATTR_DIR on pre-PR code)
• if xattr wasn't overridden, then it'll get anything which is under xattr_table['on'] in code

, SO if we change on in this PR (as I've done), then existing pools with default xattr, which weren't overridden manually will use sa on code upgrade automatically. While it looks not so obvious, it will help majority of people who didn't tune their pools. I propose to do so, xattr=sa is already 12 years with us 82a3718 .

There is only one side note in this case - someone have to set xattr=dir explicitly to use previous logic for compatibility with other ZFS implementations. BUT let's be brave, I really want ZFS to be on par with other filesystems by default, not after see that page for best settings.

tests:

Create pool on pre-PR code:

# zpool create testpool_pre /home/debian/pre.img
# zfs create testpool_pre/test -o xattr=dir
# zfs create testpool_pre/test2 -o xattr=sa
# zfs get xattr
NAME            PROPERTY  VALUE  SOURCE
testpool_pre        xattr     on     default
testpool_pre/test   xattr     on     local
testpool_pre/test2  xattr     sa     local

Import that existing pool via PR code:

# zfs get xattr
NAME            PROPERTY  VALUE  SOURCE
testpool_pre        xattr     on     default
testpool_pre/test   xattr     dir    local
testpool_pre/test2  xattr     on     local

Create pool on PR code:

$ zpool create testpool_sa /home/debian/PR.img -f
$ zfs create testpool_sa/test -o xattr=dir
$ zfs create testpool_sa/test2 -o xattr=sa
$ zfs create testpool_sa/test3 -o xattr=on
$ zfs get xattr

NAME               PROPERTY  VALUE  SOURCE
testpool_pre           xattr     on     default
testpool_pre/test      xattr     dir    local
testpool_pre/test2     xattr     on     local
testpool_sa        xattr     on     default
testpool_sa/test   xattr     dir    local
testpool_sa/test2  xattr     on     local
testpool_sa/test3  xattr     on     local

Import pool created by PR code via pre-PR code:

$ zpool import testpool_pre -d /home/debian/

$ zpool import testpool_sa -d /home/debian/

$ zfs get xattr

NAME               PROPERTY  VALUE  SOURCE
testpool_pre           xattr     on     default
testpool_pre/test      xattr     on     local
testpool_pre/test2     xattr     sa     local
testpool_sa        xattr     on     default
testpool_sa/test   xattr     on     local
testpool_sa/test2  xattr     sa     local
testpool_sa/test3  xattr     sa     local

[amotin]

I wonder what would be typical real-world scenarios of new xattr=sa in combination with default dnodesize=legacy. Default dnode has not so much space bonus space and partially it is already used, so sa's above hundred or two bytes will probably use a spill block, that may still be cheaper than xattr=dir if the xattr is accessed often, but still an additional block read. Man page recommends to use dnodesize=auto (means double dnode size now) in case of xattr=sa, but changing it by default is a waste is xattrs are not used.

[snajpa]

\o here :))

we're running dnodesize=legacy + xattr=sa b/c of bugs with running any other configuration, we were forced into this setup over time :D

btw this isn't too nice to get hit by either - #11353

[behlendorf]

We should strongly consider setting dnodesize=auto if we change the default to xattr=sa. For the best performance we want to make sure in the common case there's enough extra space in the dnode to avoid allocating a spill block. That would double the space used by dnodes but given that metadata should be a small percentage of the total pool capacity that seems reasonable. On systems where capacity needs to be prioritized dnodesize=legacy can be set or even redundant_metadata=some.

@snajpa do you know if #11353 is still an major issue with the 2.2 release candidates. https://github.com/openzfs/zfs/releases/tag/zfs-2.2.0-rc3. I thought we'd done significant work to avoid the performance penalties when possible. I'd also expect new pools which have always used dnodesize=auto not to suffer from this, only when mixing dnode sizes would it be an issue.

[amotin]

On systems where capacity needs to be prioritized dnodesize=legacy can be set

@behlendorf I personally worry not so much about on-disk capacity, as for in-memory. In case of many dnodes cached we get twice more unevictable buffers backing them. ARC should be able to limit that unevictedness, but would be good to not have to bother about it.

[shodanshok]

I set xattr=sa on all my pools, without changing dnodesize, and I get higher performance on large dirs with many files compared to default xattr=dir. So I like this commit.

It would like to use dnodesize=auto but #11353 kept me from changing the default dnodesize=legacy

I'd also expect new pools which have always used dnodesize=auto not to suffer from this, only when mixing dnode sizes would it be an issue.

Just to be sure to understand: sending a dnodesize=legacy pool to a dnodesize=auto should be ok now?

[gmelikov]

Freshly rebased.

FWIW, in 2015-2016 (wow!) I had a home server with HDDs (and still have it), where GUI was very sluggish with zfs 0.6 and xattr=on, but with xattr=sa (without changes on dnodesize) it became much-much more fancy, and I didn't have visible sub-second freezes after that. Yes, it's not so technical review, but FWIW. Since that time I recommend everybody to use xattr=sa and use it myself everywhere I can.

If there's no downsides to set dnodesize=auto today - I can change it's defaults too, but I didn't investigate it myself.

[behlendorf]

@gmelikov thanks for rebasing this. Now is definitely the best time to make this change so it's the default for the 2.3 release. I agree, let's move forward with this so so new datasets just do the best thing. As for the dnodesize=auto change lets hold off on that, unlike the xattr=sa change that's not always going to be optimal thing to do and there are some minor downsides.

[gmelikov]

@behlendorf I think it's important to highlight that this patch: will override xattr not only for new datasets, but for old datasets with not explicitly changed xattr=on, you may see my analysis in this comment #15147 (comment)

I still think it's good to go, but wanted to be explicit here.

[ixhamza]

Although this issue existed before this PR, in case anyone has any ideas:

buildroot@root:~# zpool create tank /tmp/f1 -O compression=lz4 -O xattr=sa
buildroot@root:~# zfs get compression tank
NAME  PROPERTY     VALUE           SOURCE
tank  compression  lz4             local
buildroot@root:~# zfs get xattr tank
NAME  PROPERTY  VALUE  SOURCE
tank  xattr     on     local

Both sa and lz4 are the defaults for xattr and compression, respectively, but they appear to be encoded differently. For example, explicitly setting xattr=sa shows the property as on unlike compression=lz4. This is because we use the same index for both on and sa, and on takes priority during property read since it appears earlier in the list. I think switching the order of on and sa in the xattr_table might resolve this, but a cleaner way could be to introduce a separate ZFS_XATTR_ON and handle it explicitly, though that would be a bit bigger change.

[gmelikov]

@ixhamza sorry, missed your comment, good point!

I think switching the order of on and sa in the xattr_table might resolve this

then all people start to see =sa by default, and it may break some business logic around ZFS

but a cleaner way could be to introduce a separate ZFS_XATTR_ON and handle it explicitly, though that would be a bit bigger change.

Maybe I'm wrong, but it'll need a feature flag (glad to be wrong here).

So unfortunately, I don't see a cleaner way for now, again - will be glad to be wrong here.