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

Remove feature@xattr_compat and xattr_fallback #24

Merged
3 commits merged into from
Oct 8, 2021

Conversation

ghost
Copy link

@ghost ghost commented Oct 7, 2021

A quick bandage so we don't activate this feature going forward, since it won't be in the upstreamed version.

I've kept knowledge of the feature in for now so upgraders won't lose access to their pools, but removed the activation so the feature can be disabled in the future as long as it's not already active.

Jira: NAS-112725

@ghost ghost requested a review from amotin October 7, 2021 20:00
@ghost
Copy link
Author

ghost commented Oct 7, 2021

Maybe I should remove the xattr_fallback property while I'm at it.

@ghost ghost requested review from kmoore134 and rick-mesta October 7, 2021 21:01
@ghost
Copy link
Author

ghost commented Oct 7, 2021

Fully removing the feature flag from ZFS feels like the better long term solution to me, but it may be painful for a handful of beta users.

@ghost
Copy link
Author

ghost commented Oct 7, 2021

Just sorting out the DEB build failure...

lib/libzfs/libzfs.abi Outdated Show resolved Hide resolved
man/man7/zfsprops.7 Outdated Show resolved Hide resolved
flags &= ~XATTR_REPLACE;
error = zpl_xattr_set(ip, name, value, size, flags);

dsl_dataset_t *ds = dmu_objset_ds(ITOZSB(ip)->z_os);
ds->ds_feature_activation[SPA_FEATURE_XATTR_COMPAT] = (void *)B_TRUE;

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am thinking, who of users could already have this feature activated (like that one who had complained)? Only those who hit the original version of the patch before we made Scale default to Linux format? We should figure out what to write on release notes.

Copy link
Author

@ghost ghost Oct 8, 2021

Choose a reason for hiding this comment

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

Users would have had to upgrade their existing pool from CORE in SCALE or created a new pool in SCALE to enable the feature, but then the default is xattr_compat=linux, so they would have had to change that property manually as well as far as I'm aware. I don't see anything in middleware that changes xattr_compat from the default for you.

xattr_compat was merged in #8 on June 29 and the default was changed to xattr_compat=linux in #16 on July 15, so only nightly users who created or upgraded a pool using a build from those two weeks would have xattr_compat=all on Linux unwittingly.

Ryan Moeller added 3 commits October 8, 2021 10:00
For now we'll keep the feature available, but never activate it.

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
@ghost ghost changed the title Remove feature@xattr_compat activation Remove feature@xattr_compat and xattr_fallback Oct 8, 2021
@ghost
Copy link
Author

ghost commented Oct 8, 2021

The CI failures seem to be on Github's end, perhaps we were on a particularly busy host yesterday. I ran the test suite locally and built a deb successfully as well yesterday. I'm building and running sanity tests again for this latest push right now just in case the CI is still acting up.

@ghost
Copy link
Author

ghost commented Oct 8, 2021

Tests pass locally, I'll work out the CI issues separately.

@ghost ghost merged commit 1e31b14 into truenas/zfs-2.1-release Oct 8, 2021
@ghost ghost deleted the NAS-112725 branch October 8, 2021 15:39
This pull request was closed.
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.

2 participants