ZIL claiming should not start user accounting #7163
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
behlendorf
approved these changes
Feb 13, 2018
tcaputi
force-pushed
the
more_encryption_fixes
branch
from
0674dbd to
da6f40e
Compare
|
On the surface it looks like the |
|
The project quota commit broke this one :) |
tcaputi
force-pushed
the
more_encryption_fixes
branch
3 times, most recently
from
b0500b0 to
ed94075
Compare
|
The following issue seems to have something to do with user accounting, posting here in case it's relevant: We are failing here: 2074 /*
2075 * Useraccounting is not portable and must be done with the keys loaded.
2076 * Therefore, whenever we do any kind of receive the useraccounting
2077 * must not be present.
2078 */
2079 ASSERT0(os->os_flags & OBJSET_FLAG_USERACCOUNTING_COMPLETE);
2080 ASSERT0(os->os_flags & OBJSET_FLAG_USEROBJACCOUNTING_COMPLETE); |
|
@loli10K I see what the issue is and should have a fix soon. |
|
I just imported a dd copy from my broken pool (#6916) and it worked fine with this patch. |
Currently, ZIL claiming dirties objsets which causes dsl_pool_sync() to attempt to perform user accounting on them. This causes problems for encrypted datasets that were raw received before the system went offline since they cannot perform user accounting until they have their keys loaded. This triggers an ASSERT in zio_encrypt(). Since encryption was added, the code now depends on the fact that data should only be written when objsets are owned. This patch adds a check in dmu_objset_do_userquota_updates() to ensure that useraccounting is only done when the objsets are actually owned for write. As part of this work, the zfsvfs and zvol code was updated so that it no longer lies about owning objsets readonly. Fixes: openzfs#6916 Signed-off-by: Tom Caputi <tcaputi@datto.com>
behlendorf
requested changes
Feb 20, 2018
module/zfs/zfs_vfsops.c
Outdated
| if (error) | ||
| return (error); | ||
|
|
||
| error = zfsvfs_parse_options(zm->mnt_data, &zfsvfs->z_vfs); | ||
| error = zfsvfs_create(osname, vfs->vfs_readonly, &zfsvfs); | ||
| if (error) |
There was a problem hiding this comment.
This error path and the next one appear to leak vfs.
| */ | ||
| if (!readonly && | ||
| dmu_objset_incompatible_encryption_version(zfsvfs->z_os)) | ||
| return (SET_ERROR(EROFS)); |
There was a problem hiding this comment.
To be clear, this check is no longer needed since it's now handled properly in dmu_objset_own_impl() since we're now tell the truth about the dataset being r/w.
tcaputi
force-pushed
the
more_encryption_fixes
branch
from
ed94075 to
f26b531
Compare
behlendorf
approved these changes
Feb 20, 2018
Codecov Report
@@ Coverage Diff @@
## master #7163 +/- ##
==========================================
+ Coverage 76.35% 76.35% +<.01%
==========================================
Files 327 327
Lines 103775 103773 -2
==========================================
+ Hits 79235 79240 +5
+ Misses 24540 24533 -7
Continue to review full report at Codecov.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, ZIL claiming dirties objsets which causes
dsl_pool_sync() to attempt to perform user accounting on
them. This causes problems for encrypted datasets that were
raw received before the system went offline since they
cannot perform user accounting until they have their keys
loaded. This triggers an ASSERT in zio_encrypt(). Since
encryption was added, the code now depends on the fact that
data should only be written when objsets are owned. This
patch adds a check in dmu_objset_do_userquota_updates()
to ensure that useraccounting is only done when the objsets
are actually owned.
Fixes: #6916
Signed-off-by: Tom Caputi tcaputi@datto.com
Types of changes
Checklist:
Signed-off-by.