Simplify/fix dnode_move() for dn_zfetch. #11998
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
Previous code tried to keep prefetch streams while moving dnode. But it was at least not updating per-stream zs_fetchback pointers, causing use-after-free on next access. Instead of that I see much easier and cleaner to just drop old prefetch state and start new from scratch. Signed-off-by: Alexander Motin <[email protected]> Sponsored-By: iXsystems, Inc. Closes openzfs#11936
ahrens
reviewed
May 5, 2021
| @@ -822,19 +821,14 @@ dnode_move_impl(dnode_t *odn, dnode_t *ndn) | |||
| ndn->dn_newgid = odn->dn_newgid; | |||
| ndn->dn_newprojid = odn->dn_newprojid; | |||
| ndn->dn_id_flags = odn->dn_id_flags; | |||
| dmu_zfetch_init(&ndn->dn_zfetch, NULL); | |||
| list_move_tail(&ndn->dn_zfetch.zf_stream, &odn->dn_zfetch.zf_stream); | |||
| ndn->dn_zfetch.zf_dnode = odn->dn_zfetch.zf_dnode; | |||
There was a problem hiding this comment.
Does this leak the zstream_t's on the zf_stream?
The move callbacks are pretty tricky code to get right. IMO, if we aren't able to maintain the move callbacks, because we can't test them, because they are dead code, we should remove them from the codebase.
There was a problem hiding this comment.
dmu_zfetch_fini(&odn->dn_zfetch) below should free the old streams gracefully. I've preferred to drop streams exactly to not complicate this code and avoid new breakages later.
There was a problem hiding this comment.
OK. I'm fine with integrating this change, but I would welcome a future PR to remove dnode_move_impl() entirely.
ahrens
approved these changes
May 6, 2021
ikozhukhov
approved these changes
May 6, 2021
behlendorf
added
Status: Accepted
behlendorf
pushed a commit
to behlendorf/zfs
that referenced
this pull request
May 10, 2021
Previous code tried to keep prefetch streams while moving dnode. But it was at least not updating per-stream zs_fetchback pointers, causing use-after-free on next access. Instead of that I see much easier and cleaner to just drop old prefetch state and start new from scratch. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Igor Kozhukhov <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored-By: iXsystems, Inc. Closes openzfs#11936 Closes openzfs#11998
sempervictus
pushed a commit
to sempervictus/zfs
that referenced
this pull request
May 31, 2021
Previous code tried to keep prefetch streams while moving dnode. But it was at least not updating per-stream zs_fetchback pointers, causing use-after-free on next access. Instead of that I see much easier and cleaner to just drop old prefetch state and start new from scratch. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Igor Kozhukhov <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored-By: iXsystems, Inc. Closes openzfs#11936 Closes openzfs#11998
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.
Previous code tried to keep prefetch streams while moving dnode. But
it was at least not updating per-stream zs_fetchback pointers, causing
use-after-free on next access. Instead of that I see much easier and
cleaner to just drop old prefetch state and start new from scratch.
Closes #11936
How Has This Been Tested?
The code builds on FreeBSD. Unfortunately I can't run test it, since neither FreeBSD nor Linux dnode_move().
Types of changes
Checklist:
Signed-off-by.