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.

Sign up for GitHub

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

Jump to bottom

Fix a race condition in dsl_dataset_sync() when activating features #13816

Merged
merged 1 commit into from
Feb 14, 2023
Merged

Fix a race condition in dsl_dataset_sync() when activating features #13816

merged 1 commit into from
Feb 14, 2023

Conversation

gamanakis
Copy link
Contributor

@gamanakis gamanakis commented Aug 30, 2022 •
edited
Loading

Motivation and Context

See commits 80a650b, 9f346ab

Description

The zio returned from arc_write() in dmu_objset_sync() uses
zio_nowait(). However we may reach the end of dsl_dataset_sync()
which checks if we need to activate features in the filesystem
without knowing if that zio has even run through the ZIO pipeline yet.
In that case we will flag features to be activated in
dsl_dataset_block_born() but dsl_dataset_sync() has already
completed its run and those features will not actually be activated.
Mitigate this by moving the feature activation code in
dsl_dataset_sync_done(). Also add new ASSERTs in
dsl_scan_visitbp() checking if a block contradicts any filesystem
flags.

How Has This Been Tested?

Added ASSERTs in dsl_scan_visitbp() and running zloop.sh.

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:

@gamanakis gamanakis marked this pull request as ready for review August 31, 2022 19:12
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Sep 2, 2022
bwatkinson
bwatkinson suggested changes Sep 6, 2022
View reviewed changes
module/zfs/dmu_objset.c Outdated Show resolved Hide resolved
@bwatkinson bwatkinson requested a review from ahrens September 6, 2022 23:24
@gamanakis gamanakis force-pushed the feature_race branch 2 times, most recently from a70e797 to 9a1c7e7 Compare November 19, 2022 15:21
@gamanakis
Copy link
Contributor Author

9a1c7e7 : Rebased to master.

@jumbi77
Copy link
Contributor

jumbi77 commented Jan 28, 2023

Politely ping @ahrens for a review?

@gamanakis gamanakis force-pushed the feature_race branch 3 times, most recently from 3b7fc66 to 54c068b Compare February 1, 2023 13:47
@gamanakis
Copy link
Contributor Author

54c068b: Rebased to master, implemented @ahrens suggestion.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 1, 2023
ghost
ghost reviewed Feb 2, 2023
View reviewed changes
module/zfs/dsl_dataset.c Outdated Show resolved Hide resolved
module/zfs/dsl_scan.c Outdated Show resolved Hide resolved
@gamanakis
Copy link
Contributor Author

gamanakis commented Feb 2, 2023 •
edited
Loading

a55b09f: Implemented suggestions by @freqlabs

@gamanakis
Fix a race condition in dsl_dataset_sync() when activating features
eecffc9 
The zio returned from arc_write() in dmu_objset_sync() uses
zio_nowait(). However we may reach the end of dsl_dataset_sync()
which checks if we need to activate features in the filesystem
without knowing if that zio has even run through the ZIO pipeline yet.
In that case we will flag features to be activated in
dsl_dataset_block_born() but dsl_dataset_sync() has already
completed its run and those features will not actually be activated.
Mitigate this by moving the feature activation code in
dsl_dataset_sync_done(). Also add new ASSERTs in
dsl_scan_visitbp() checking if a block contradicts any filesystem
flags.

Signed-off-by: George Amanakis <gamanakis@gmail.com>
@behlendorf behlendorf merged commit 34ce4c4 into openzfs:master Feb 14, 2023
@gamanakis
Copy link
Contributor Author

@tonyhutter I strongly think that this PR along with #14502 and #14523 should be included in 2.1.10. Thank you.

@behlendorf
Copy link
Contributor

@gamanakis would you mind opening a PR against the 2.1.10-staging branch with the needed changes so we can make sure they get included.

gamanakis added a commit to gamanakis/zfs that referenced this pull request Feb 27, 2023
@gamanakis
Fix a race condition in dsl_dataset_sync() when activating features
8c349cf 
The zio returned from arc_write() in dmu_objset_sync() uses
zio_nowait(). However we may reach the end of dsl_dataset_sync()
which checks if we need to activate features in the filesystem
without knowing if that zio has even run through the ZIO pipeline yet.
In that case we will flag features to be activated in
dsl_dataset_block_born() but dsl_dataset_sync() has already
completed its run and those features will not actually be activated.
Mitigate this by moving the feature activation code in
dsl_dataset_sync_done(). Also add new ASSERTs in
dsl_scan_visitbp() checking if a block contradicts any filesystem
flags.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Signed-off-by: George Amanakis <gamanakis@gmail.com>
Closes openzfs#13816
@gamanakis
Copy link
Contributor Author

@behlendorf, done! #14537

behlendorf pushed a commit that referenced this pull request Mar 1, 2023
@gamanakis @behlendorf
Fix a race condition in dsl_dataset_sync() when activating features
57159a5 
The zio returned from arc_write() in dmu_objset_sync() uses
zio_nowait(). However we may reach the end of dsl_dataset_sync()
which checks if we need to activate features in the filesystem
without knowing if that zio has even run through the ZIO pipeline yet.
In that case we will flag features to be activated in
dsl_dataset_block_born() but dsl_dataset_sync() has already
completed its run and those features will not actually be activated.
Mitigate this by moving the feature activation code in
dsl_dataset_sync_done(). Also add new ASSERTs in
dsl_scan_visitbp() checking if a block contradicts any filesystem
flags.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Signed-off-by: George Amanakis <gamanakis@gmail.com>
Closes #13816
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 3, 2023
@gamanakis @lundman
Fix a race condition in dsl_dataset_sync() when activating features
0dddba3 
The zio returned from arc_write() in dmu_objset_sync() uses
zio_nowait(). However we may reach the end of dsl_dataset_sync()
which checks if we need to activate features in the filesystem
without knowing if that zio has even run through the ZIO pipeline yet.
In that case we will flag features to be activated in
dsl_dataset_block_born() but dsl_dataset_sync() has already
completed its run and those features will not actually be activated.
Mitigate this by moving the feature activation code in
dsl_dataset_sync_done(). Also add new ASSERTs in
dsl_scan_visitbp() checking if a block contradicts any filesystem
flags.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Signed-off-by: George Amanakis <gamanakis@gmail.com>
Closes openzfs#13816
gamanakis added a commit to gamanakis/zfs that referenced this pull request Mar 5, 2023
@gamanakis
Fix a race condition in dsl_dataset_sync() when activating features
cc9ea5b 
The zio returned from arc_write() in dmu_objset_sync() uses
zio_nowait(). However we may reach the end of dsl_dataset_sync()
which checks if we need to activate features in the filesystem
without knowing if that zio has even run through the ZIO pipeline yet.
In that case we will flag features to be activated in
dsl_dataset_block_born() but dsl_dataset_sync() has already
completed its run and those features will not actually be activated.
Mitigate this by moving the feature activation code in
dsl_dataset_sync_done(). Also add new ASSERTs in
dsl_scan_visitbp() checking if a block contradicts any filesystem
flags.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Signed-off-by: George Amanakis <gamanakis@gmail.com>
Closes openzfs#13816
datacore-rm pushed a commit to DataCoreSoftware/openzfs that referenced this pull request Feb 9, 2024
@gamanakis @datacore-rm
Fix a race condition in dsl_dataset_sync() when activating features
c85f161 
The zio returned from arc_write() in dmu_objset_sync() uses
zio_nowait(). However we may reach the end of dsl_dataset_sync()
which checks if we need to activate features in the filesystem
without knowing if that zio has even run through the ZIO pipeline yet.
In that case we will flag features to be activated in
dsl_dataset_block_born() but dsl_dataset_sync() has already
completed its run and those features will not actually be activated.
Mitigate this by moving the feature activation code in
dsl_dataset_sync_done(). Also add new ASSERTs in
dsl_scan_visitbp() checking if a block contradicts any filesystem
flags.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Signed-off-by: George Amanakis <gamanakis@gmail.com>
Closes openzfs#13816
datacore-rm pushed a commit to DataCoreSoftware/openzfs that referenced this pull request Feb 28, 2024
@gamanakis @datacore-rm
Fix a race condition in dsl_dataset_sync() when activating features
e76a37f 
The zio returned from arc_write() in dmu_objset_sync() uses
zio_nowait(). However we may reach the end of dsl_dataset_sync()
which checks if we need to activate features in the filesystem
without knowing if that zio has even run through the ZIO pipeline yet.
In that case we will flag features to be activated in
dsl_dataset_block_born() but dsl_dataset_sync() has already
completed its run and those features will not actually be activated.
Mitigate this by moving the feature activation code in
dsl_dataset_sync_done(). Also add new ASSERTs in
dsl_scan_visitbp() checking if a block contradicts any filesystem
flags.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Signed-off-by: George Amanakis <gamanakis@gmail.com>
Closes openzfs#13816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwatkinson bwatkinson bwatkinson approved these changes

@ahrens ahrens ahrens approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gamanakis @jumbi77 @behlendorf @ahrens @bwatkinson