Fix 'zfs userspace' for received datasets in encrypted root #9596
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9596 +/- ##
==========================================
- Coverage 79.80% 79.46% -0.35%
==========================================
Files 398 385 -13
Lines 125765 121503 -4262
==========================================
- Hits 100372 96552 -3820
+ Misses 25393 24951 -442
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
The reasoning seems right to me, but I would prefer if someone more familiar with the object accounting code approved the PR. @loli10K Small nitpick on the commit message: In the code an encrypted receive is any receive that will create an encrypted dataset (even if thats just receiving into an encryption child). A RAW receive will disable the accounting and cause the issue youre fixing here. Just for consistency. |
But the issue seems to be affecting un-encrypted datasets received with "normal" |
|
Oh, Im sorry. You are right. we did that for regular encrypted sends as well to avoid some interference with raw sending the received datasets. My mistake. |
The code is a bit convoluted in places, but I believe I can shed some light on why that's the case. Hopefully it will help guide any refactoring we may have to do here. Originally ZFS only supported byte level space accounting. This functionality was added in Since the object and project quota accounting doesn't technically depend on the original user space accounting the logic was kept separate ( This code was then subsequently reworked slightly to handle some of the new complications presented by encrypted pools. Specifically the restriction that some accounting information could only be updated when the encryption key was loaded. In order to solve this efficiently I think we might want to do a little more refactoring. My concern with the proposed fix is that I don't believe it handles the case when neither user object or project quotas features are enabled on the pool. In this case, it looks like the upgrade won't run at all resulting in the same issue. One minimal solution might be to kick off a |
|
I will not be able to work on the needed refactoring for some time, adding the work in progress tag but feel free to close it. |
loli10K
added
Status: Work in Progress
|
No problem. To be a little more specific, I believe this untested additional change if included in this PR would be sufficient. Though we'd want to include a test case as well. diff --git a/module/zfs/dmu_objset.c b/module/zfs/dmu_objset.c
index 2a9464e2a2c..7ebbe533256 100644
--- a/module/zfs/dmu_objset.c
+++ b/module/zfs/dmu_objset.c
@@ -783,11 +783,15 @@ dmu_objset_own(const char *name, dmu_objset_type_t type,
* speed up pool import times and to keep this txg reserved
* completely for recovery work.
*/
- if ((dmu_objset_userobjspace_upgradable(*osp) ||
- dmu_objset_projectquota_upgradable(*osp)) &&
- !readonly && !dp->dp_spa->spa_claiming &&
- (ds->ds_dir->dd_crypto_obj == 0 || decrypt))
- dmu_objset_id_quota_upgrade(*osp);
+ if (!readonly && !dp->dp_spa->spa_claiming &&
+ (ds->ds_dir->dd_crypto_obj == 0 || decrypt)) {
+ if (dmu_objset_userobjspace_upgradable(*osp) ||
+ dmu_objset_projectquota_upgradable(*osp)) {
+ dmu_objset_id_quota_upgrade(*osp);
+ } else if (dmu_objset_userused_enabled(*osp)) {
+ dmu_objset_userspace_upgrade(*osp);
+ }
+ }
dsl_pool_rele(dp, FTAG);
return (0); |
This is indeed correct, pools created with Currently testing the additional Will debug later this weekend. EDIT: fixed by running the upgrade in a callback function via |
loli10K
force-pushed
the
issue-9501
branch
4 times, most recently
from
dc9acbe to
d45e0db
Compare
loli10K
added
Status: Code Review Needed
|
Hi, Is this likely to merged into a release soon? |
|
@scratchings there's still some needed work before this can be merged. @loli10K any chance you might have some time so we can try and get this one wrapped up? It sounds like running |
|
It may take me some time to refresh this, i haven't used my development workspace even since i stopped contributing code. I am going to try something in the upcoming weekend. |
| log_must_retry "unsupported" 3 \ | ||
| eval "zfs userspace $DATASET_ENCROOT/recvfs > /dev/null" | ||
| log_must_retry "unsupported" 3 \ | ||
| eval "zfs groupspace $DATASET_ENCROOT/recvfs > /dev/null" |
There was a problem hiding this comment.
Was the issue here that it might return unsupported until it completes? I wasn't able to reproduce this locally, and it's not what I'd expect. Unless you're able to reproduce this I'd suggest we simplify this like so:
@@ -65,13 +65,12 @@ for opts in "${POOL_OPTS[@]}"; do
log_must eval "zfs send $DATASET_SENDFS@snap | zfs recv " \
"$DATASET_ENCROOT/recvfs"
- # 3. Verify encrypted datasets support 'zfs userspace' and 'zfs groupsp
- log_must eval "zfs userspace $DATASET_ENCROOT/fs > /dev/null"
- log_must eval "zfs groupspace $DATASET_ENCROOT/fs > /dev/null"
- log_must_retry "unsupported" 3 \
- eval "zfs userspace $DATASET_ENCROOT/recvfs > /dev/null"
- log_must_retry "unsupported" 3 \
- eval "zfs groupspace $DATASET_ENCROOT/recvfs > /dev/null"
+ # 3. Verify encrypted datasets support 'zfs userspace' and
+ # 'zfs groupspace'
+ log_must zfs userspace $DATASET_ENCROOT/fs
+ log_must zfs groupspace $DATASET_ENCROOT/fs
+ log_must zfs userspace $DATASET_ENCROOT/recvfs
+ log_must zfs groupspace $DATASET_ENCROOT/recvfs
# Cleanup
cleanupThere was a problem hiding this comment.
My local builder seems to be a lot slower than those running on build.zfsonlinux.org; i can sometimes triggers failures if i remove log_must_retry()
--- Configuration ---
Runfiles: /usr/share/zfs/runfiles/zfs-tests.run
STF_TOOLS: /usr/share/zfs/test-runner
STF_SUITE: /usr/share/zfs/zfs-tests
STF_PATH: /var/tmp/constrained_path.QRLQ
FILEDIR: /var/tmp
FILES: /var/tmp/file-vdev0 /var/tmp/file-vdev1 /var/tmp/file-vdev2
LOOPBACKS: /dev/loop0 /dev/loop1 /dev/loop2
DISKS: loop0 loop1 loop2
NUM_DISKS: 3
FILESIZE: 4G
ITERATIONS: 1
TAGS: functional
STACK_TRACER: no
Keep pool(s): rpool
Missing util(s): mmap_libaio pamtester umask wait
/usr/share/zfs/test-runner/bin/test-runner.py -c "/usr/share/zfs/runfiles/zfs-tests.run" -T "functional" -i "/usr/share/zfs/zfs-tests" -I "1"
Test: /usr/share/zfs/zfs-tests/tests/functional/userquota/setup (run as root) [00:01] [PASS]
Test: /usr/share/zfs/zfs-tests/tests/functional/userquota/userspace_encrypted (run as root) [00:07] [PASS]
Test: /usr/share/zfs/zfs-tests/tests/functional/userquota/userspace_encrypted (run as root) [00:07] [PASS]
Test: /usr/share/zfs/zfs-tests/tests/functional/userquota/userspace_encrypted (run as root) [00:03] [FAIL]
Test: /usr/share/zfs/zfs-tests/tests/functional/userquota/userspace_encrypted (run as root) [00:07] [PASS]
Test: /usr/share/zfs/zfs-tests/tests/functional/userquota/userspace_encrypted (run as root) [00:06] [PASS]
Test: /usr/share/zfs/zfs-tests/tests/functional/userquota/userspace_encrypted (run as root) [00:03] [FAIL]
Test: /usr/share/zfs/zfs-tests/tests/functional/userquota/cleanup (run as root) [00:01] [PASS]
Results Summary
FAIL 2
PASS 6
Running Time: 00:00:38
Percent passed: 75.0%
Log directory: /var/tmp/test_results/20201113T161214
Tests with results other than PASS that are expected:
Tests with result of PASS that are unexpected:
Tests with results other than PASS that are unexpected:
FAIL userquota/userspace_encrypted (expected PASS)
Test: /usr/share/zfs/zfs-tests/tests/functional/userquota/userspace_encrypted (run as root) [00:03] [FAIL]
16:12:48.10 ASSERTION: 'zfs user/groupspace' should work on encrypted datasets
16:12:48.20 SUCCESS: zpool create -o feature@encryption=enabled testpool14821 /var/tmp/userspace_encrypted
16:12:48.23 SUCCESS: zfs create testpool14821/sendfs
16:12:49.35 SUCCESS: eval echo 'password' | zfs create -o encryption=on -o keyformat=passphrase -o keylocation=prompt testpool14821/encroot
16:12:49.45 SUCCESS: zfs create testpool14821/encroot/fs
16:12:49.52 SUCCESS: zfs snap testpool14821/sendfs@snap
16:12:49.65 SUCCESS: eval zfs send testpool14821/sendfs@snap | zfs recv testpool14821/encroot/recvfs
16:12:49.65 TYPE NAME USED QUOTA OBJUSED OBJQUOTA
16:12:49.65 POSIX User root 1.50K none 1 none
16:12:49.66 SUCCESS: zfs userspace testpool14821/encroot/fs
16:12:49.67 TYPE NAME USED QUOTA OBJUSED OBJQUOTA
16:12:49.67 POSIX Group root 1.50K none 1 none
16:12:49.68 SUCCESS: zfs groupspace testpool14821/encroot/fs
16:12:49.69 TYPE NAME USED QUOTA OBJUSED OBJQUOTA
16:12:49.69 POSIX User root 1.50K none 1 none
16:12:49.69 SUCCESS: zfs userspace testpool14821/encroot/recvfs
16:12:49.70 TYPE NAME USED QUOTA OBJUSED OBJQUOTA
16:12:49.70 POSIX Group root 1.50K none 1 none
16:12:49.71 SUCCESS: zfs groupspace testpool14821/encroot/recvfs
16:12:49.95 SUCCESS: zpool destroy -f testpool14821
16:12:50.10 SUCCESS: zpool create -d -o feature@encryption=enabled testpool14821 /var/tmp/userspace_encrypted
16:12:50.15 SUCCESS: zfs create testpool14821/sendfs
16:12:51.13 SUCCESS: eval echo 'password' | zfs create -o encryption=on -o keyformat=passphrase -o keylocation=prompt testpool14821/encroot
16:12:51.19 SUCCESS: zfs create testpool14821/encroot/fs
16:12:51.22 SUCCESS: zfs snap testpool14821/sendfs@snap
16:12:51.32 SUCCESS: eval zfs send testpool14821/sendfs@snap | zfs recv testpool14821/encroot/recvfs
16:12:51.34 TYPE NAME USED QUOTA OBJUSED OBJQUOTA
16:12:51.34 POSIX User root 1.50K none - -
16:12:51.34 SUCCESS: zfs userspace testpool14821/encroot/fs
16:12:51.35 TYPE NAME USED QUOTA OBJUSED OBJQUOTA
16:12:51.35 POSIX Group root 1.50K none - -
16:12:51.36 SUCCESS: zfs groupspace testpool14821/encroot/fs
16:12:51.38 cannot get used/quota for testpool14821/encroot/recvfs: unsupported version or feature
16:12:51.38 ERROR: zfs userspace testpool14821/encroot/recvfs exited 255
Keeping log_must_retry() should prevent spurious failures.
There was a problem hiding this comment.
OK. Well in that case we should still add something like (( $? != 0 )) && log_fail after each log_must_retry call. Otherwise it won't fail properly after 3 retries and will incorrectly pass, see the log_must_busy helper function as an example. How about something like this:
function log_must_unsupported
{
log_must_retry "unsupported" 3 "$@"
(( $? != 0 )) && log_fail
}
....
# 3. Verify encrypted datasets support 'zfs userspace' and
# 'zfs groupspace'
log_must zfs userspace $DATASET_ENCROOT/fs
log_must zfs groupspace $DATASET_ENCROOT/fs
log_must_unsupported zfs userspace $DATASET_ENCROOT/recvfs
log_must_unsupported zfs groupspace $DATASET_ENCROOT/recvfsThere was a problem hiding this comment.
With log_must_unsupported() it is indeed failing if i inject failures with
stap -g -e 'probe module("zfs").function("zfs_ioc_userspace_many").return { $return = 95; }'so it is safe to assume it will catch real issues: it wasn't doing that with just log_must_retry(), thank you for catching this.
For encrypted receives, where user accounting is initially disabled on creation, both 'zfs userspace' and 'zfs groupspace' fails with EOPNOTSUPP: this is because dmu_objset_id_quota_upgrade_cb() forgets to set OBJSET_FLAG_USERACCOUNTING_COMPLETE on the objset flags after a successful dmu_objset_space_upgrade(). Signed-off-by: loli10K <[email protected]> Co-authored-by: Brian Behlendorf <[email protected]>
behlendorf
added
Status: Accepted
For encrypted receives, where user accounting is initially disabled on creation, both 'zfs userspace' and 'zfs groupspace' fails with EOPNOTSUPP: this is because dmu_objset_id_quota_upgrade_cb() forgets to set OBJSET_FLAG_USERACCOUNTING_COMPLETE on the objset flags after a successful dmu_objset_space_upgrade(). Reviewed-by: Brian Behlendorf <[email protected]> Co-authored-by: Brian Behlendorf <[email protected]> Signed-off-by: loli10K <[email protected]> Closes #9501 Closes #9596
For encrypted receives, where user accounting is initially disabled on creation, both 'zfs userspace' and 'zfs groupspace' fails with EOPNOTSUPP: this is because dmu_objset_id_quota_upgrade_cb() forgets to set OBJSET_FLAG_USERACCOUNTING_COMPLETE on the objset flags after a successful dmu_objset_space_upgrade(). Reviewed-by: Brian Behlendorf <[email protected]> Co-authored-by: Brian Behlendorf <[email protected]> Signed-off-by: loli10K <[email protected]> Closes openzfs#9501 Closes openzfs#9596
For encrypted receives, where user accounting is initially disabled on creation, both 'zfs userspace' and 'zfs groupspace' fails with EOPNOTSUPP: this is because dmu_objset_id_quota_upgrade_cb() forgets to set OBJSET_FLAG_USERACCOUNTING_COMPLETE on the objset flags after a successful dmu_objset_space_upgrade(). Reviewed-by: Brian Behlendorf <[email protected]> Co-authored-by: Brian Behlendorf <[email protected]> Signed-off-by: loli10K <[email protected]> Closes openzfs#9501 Closes openzfs#9596
Motivation and Context
Fix #9501
Description
For encrypted receives, where user accounting is initially disabled on creation, both 'zfs userspace' and 'zfs groupspace' fails with
EOPNOTSUPP: this is becausedmu_objset_id_quota_upgrade_cb()forgets to setOBJSET_FLAG_USERACCOUNTING_COMPLETEon the objset flags after a successfuldmu_objset_space_upgrade().Analysis here: #9501 (comment)
I agree with @tcaputi comment here #9501 (comment): while i was debugging the original issue i found some user accounting code confusing, unfortunately i don't have the time nor the knowledge to refactor the existing code right now, i am just pushing the fix so i can cleanup my local workspace.
How Has This Been Tested?
ZFS Test Suite was updated with new test case.
Types of changes
Checklist:
Signed-off-by.