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

zvol_os: Code cleanup #11117

Closed
wants to merge 6 commits into from
Closed

zvol_os: Code cleanup #11117

wants to merge 6 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 27, 2020

Motivation and Context

Various issues were found while investigating -o volmode=xxx being ignored on FreeBSD when creating a zvol.
These commits do not fix the volmode issue, but a separate FreeBSD issue is fixed where changing volmode after creation would not take effect until the next time the pool is imported or the zvol is renamed.

Description

  • Tidy up asserts (FreeBSD & Linux):

    • Using more specific assert variants gives better messages on failure.
  • Keep better track of open count in close (FreeBSD):

    • zvol_geom_close gets a count of the number of close operations to do. Make sure we're always using this count to check if this will be the last close operation performed on the zvol.
  • Code cleanup in zvol_create_minor_impl (FreeBSD):

    • Nonfunctional changes for readability and consistency.
  • Properly ignore error in volmode lookup (FreeBSD):

    • We fall back to a default volmode and continue when looking up a zvol's volmode property fails. After this we should set the error to 0 to ensure we take the success paths in the out section.
    • While here, make sure we only log that the zvol was created on success.
  • Don't leak doi in cdev error path (FreeBSD):

    • Make sure to free doi in zvol_create_minor impl when make_dev_s fails.
  • Fix handling of zvol private data (FreeBSD):

    • zvol private data is supposed to be nulled by zvol_clear_private before zvol_free is called as an indicator that the zvol is going away.
    • Implement zvol_clear_private for volmode=dev.
    • Assert that zvol_clear_private has been called before zvol_free.
    • Check that zvol_clear_private has not been called when updating volsize. If it has, fail with ENXIO.

How Has This Been Tested?

ZTS on FreeBSD; manual messing with the volmode property

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

Ryan Moeller added 6 commits October 27, 2020 08:58
Using more specific assert variants gives better messages on failure.

No functional change.

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
zvol_geom_close gets a count of the number of close operations to do.

Make sure we're always using this count to check if this will be the
last close operation performed on the zvol.

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Nonfunctional changes for readability and consistency.

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
We fall back to a default volmode and continue when looking up a zvol's
volmode property fails.  After this we should set the error to 0 to
ensure we take the success paths in the out section.

While here, make sure we only log that the zvol was created on success.

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Make sure to free doi in zvol_create_minor impl when make_dev_s fails.

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
zvol private data is supposed to be nulled by zvol_clear_private before
zvol_free is called as an indicator that the zvol is going away.

Implement zvol_clear_private for volmode=dev.

Assert that zvol_clear_private has been called before zvol_free.

Check that zvol_clear_private has not been called when updating
volsize.  If it has, fail with ENXIO.

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
@ghost ghost added Component: ZVOL ZFS Volumes Status: Code Review Needed Ready for review and testing labels Oct 27, 2020
@codecov
Copy link

codecov bot commented Oct 27, 2020

Codecov Report

Merging #11117 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11117   +/-   ##
=======================================
  Coverage   79.81%   79.82%           
=======================================
  Files         398      398           
  Lines      125754   125757    +3     
=======================================
+ Hits       100367   100381   +14     
+ Misses      25387    25376   -11     
Flag Coverage Δ
#kernel 80.44% <100.00%> (-0.04%) ⬇️
#user 66.30% <ø> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
module/os/linux/zfs/zvol_os.c 87.44% <100.00%> (-0.81%) ⬇️
cmd/zdb/zdb_il.c 30.86% <0.00%> (-24.08%) ⬇️
module/zfs/vdev_indirect.c 73.92% <0.00%> (-10.97%) ⬇️
module/zfs/spa_errlog.c 90.83% <0.00%> (-3.06%) ⬇️
lib/libzfs/libzfs_changelist.c 84.44% <0.00%> (-1.86%) ⬇️
module/zfs/zfs_fm.c 85.80% <0.00%> (-1.75%) ⬇️
module/zfs/vdev_raidz.c 93.15% <0.00%> (-0.77%) ⬇️
module/zfs/vdev_mirror.c 94.44% <0.00%> (-0.75%) ⬇️
module/zfs/btree.c 83.02% <0.00%> (-0.71%) ⬇️
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c810ac...d2afd22. Read the comment docs.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 30, 2020
behlendorf pushed a commit that referenced this pull request Oct 30, 2020
zvol_geom_close gets a count of the number of close operations to do.

Make sure we're always using this count to check if this will be the
last close operation performed on the zvol.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #11117
behlendorf pushed a commit that referenced this pull request Oct 30, 2020
Nonfunctional changes for readability and consistency.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #11117
behlendorf pushed a commit that referenced this pull request Oct 30, 2020
We fall back to a default volmode and continue when looking up a zvol's
volmode property fails.  After this we should set the error to 0 to
ensure we take the success paths in the out section.

While here, make sure we only log that the zvol was created on success.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #11117
behlendorf pushed a commit that referenced this pull request Oct 30, 2020
Make sure to free doi in zvol_create_minor impl when make_dev_s fails.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #11117
behlendorf pushed a commit that referenced this pull request Oct 30, 2020
zvol private data is supposed to be nulled by zvol_clear_private before
zvol_free is called as an indicator that the zvol is going away.

Implement zvol_clear_private for volmode=dev.

Assert that zvol_clear_private has been called before zvol_free.

Check that zvol_clear_private has not been called when updating
volsize.  If it has, fail with ENXIO.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #11117
behlendorf pushed a commit that referenced this pull request Oct 30, 2020
Using more specific assert variants gives better messages on failure.

No functional change.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #11117
behlendorf pushed a commit that referenced this pull request Oct 30, 2020
zvol_geom_close gets a count of the number of close operations to do.

Make sure we're always using this count to check if this will be the
last close operation performed on the zvol.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #11117
behlendorf pushed a commit that referenced this pull request Oct 30, 2020
Nonfunctional changes for readability and consistency.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #11117
behlendorf pushed a commit that referenced this pull request Oct 30, 2020
We fall back to a default volmode and continue when looking up a zvol's
volmode property fails.  After this we should set the error to 0 to
ensure we take the success paths in the out section.

While here, make sure we only log that the zvol was created on success.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #11117
behlendorf pushed a commit that referenced this pull request Oct 30, 2020
Make sure to free doi in zvol_create_minor impl when make_dev_s fails.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #11117
behlendorf pushed a commit that referenced this pull request Oct 30, 2020
zvol private data is supposed to be nulled by zvol_clear_private before
zvol_free is called as an indicator that the zvol is going away.

Implement zvol_clear_private for volmode=dev.

Assert that zvol_clear_private has been called before zvol_free.

Check that zvol_clear_private has not been called when updating
volsize.  If it has, fail with ENXIO.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #11117
@ghost ghost deleted the zvol-audit branch October 30, 2020 23:46
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Using more specific assert variants gives better messages on failure.

No functional change.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
zvol_geom_close gets a count of the number of close operations to do.

Make sure we're always using this count to check if this will be the
last close operation performed on the zvol.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Nonfunctional changes for readability and consistency.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
We fall back to a default volmode and continue when looking up a zvol's
volmode property fails.  After this we should set the error to 0 to
ensure we take the success paths in the out section.

While here, make sure we only log that the zvol was created on success.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Make sure to free doi in zvol_create_minor impl when make_dev_s fails.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
zvol private data is supposed to be nulled by zvol_clear_private before
zvol_free is called as an indicator that the zvol is going away.

Implement zvol_clear_private for volmode=dev.

Assert that zvol_clear_private has been called before zvol_free.

Check that zvol_clear_private has not been called when updating
volsize.  If it has, fail with ENXIO.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Using more specific assert variants gives better messages on failure.

No functional change.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
zvol_geom_close gets a count of the number of close operations to do.

Make sure we're always using this count to check if this will be the
last close operation performed on the zvol.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Nonfunctional changes for readability and consistency.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
We fall back to a default volmode and continue when looking up a zvol's
volmode property fails.  After this we should set the error to 0 to
ensure we take the success paths in the out section.

While here, make sure we only log that the zvol was created on success.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Make sure to free doi in zvol_create_minor impl when make_dev_s fails.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
zvol private data is supposed to be nulled by zvol_clear_private before
zvol_free is called as an indicator that the zvol is going away.

Implement zvol_clear_private for volmode=dev.

Assert that zvol_clear_private has been called before zvol_free.

Check that zvol_clear_private has not been called when updating
volsize.  If it has, fail with ENXIO.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Matt Macy <mmacy@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11117
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ZVOL ZFS Volumes Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants