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 updating the zvol_htable when renaming a zvol #16128

Merged
merged 1 commit into from
Apr 25, 2024
Merged

Fix updating the zvol_htable when renaming a zvol #16128

merged 1 commit into from
Apr 25, 2024

Conversation

asomers
Copy link
Contributor

@asomers asomers commented Apr 24, 2024

When renaming a zvol, insert it into zvol_htable using the new name, not the old name. Otherwise some operations won't work. For example, "zfs set volsize" while the zvol is open.

Sponsored by: Axcient
Signed-off-by: Alan Somers asomers@FreeBSD.org
Closes #16127

Motivation and Context

Fixes #16127

Description

Must use the correct name when updating zvol_htable during zvol_os_rename_minor

How Has This Been Tested?

Manually tested on FreeBSD -CURRENT using the procedure described in the issue.

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:

@asomers
Fix updating the zvol_htable when renaming a zvol
969fe6c 
When renaming a zvol, insert it into zvol_htable using the new name, not
the old name.  Otherwise some operations won't work.  For example,
"zfs set volsize" while the zvol is open.

Sponsored by:	Axcient
Signed-off-by:	Alan Somers <asomers@FreeBSD.org>
Closes openzfs#16127
behlendorf
behlendorf approved these changes Apr 24, 2024
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find, thanks for sorting this out.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 24, 2024
@jumbi77
Copy link
Contributor

jumbi77 commented Apr 24, 2024

wonder if test case is useful here? ..at least you have a preproducer (linked issue/ticket)

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 25, 2024
@behlendorf
Copy link
Contributor

wonder if test case is useful here? ..at least you have a preproducer (linked issue/ticket)

Wouldn't hurt. But I don't think it needs to hold up merging this fix.

@behlendorf behlendorf merged commit 21bc066 into openzfs:master Apr 25, 2024
23 of 26 checks passed
@asomers asomers deleted the zvol-rename branch April 26, 2024 14:34
tonyhutter pushed a commit that referenced this pull request May 2, 2024
@asomers @tonyhutter
Fix updating the zvol_htable when renaming a zvol
3d4d619 
When renaming a zvol, insert it into zvol_htable using the new name, not
the old name.  Otherwise some operations won't work.  For example,
"zfs set volsize" while the zvol is open.

Sponsored by:	Axcient
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alek Pinchuk <apinchuk@axcient.com>
Signed-off-by:	Alan Somers <asomers@FreeBSD.org>
Closes #16127
Closes #16128
behlendorf pushed a commit that referenced this pull request May 29, 2024
@pjd
zpool import output is not formated properly.
5137c13 
The 'zpool status' output assumes that the longest prefix is six
character long plus colon plus space, eg. 'status: ', 'action: '
or 'config: ' (so eight in total). This works well even when we have
messages that requires more than one line, as '\t' is exactly eight
characters, just like the longest prefix.

The 'zpool import' output is a bit different, as it may display the
comment pool property, then the longest prefix is 'comment: ', which is
nine characters long, not eight.
All the prefixes were given an extra space in front, but:
- 'status: ' did not get an extra space.
- Messages that require more than one line should use nine spaces of
  indentation, not eight.
- The extra space in front looks redundant if there is no comment
  property set on the given pool.

Fix it by adding an extra space to all prefixes, but only if the comment
property is defined. Also, when we need to continue the message in a new
line, use '\t ' for indentation.

While here, apply small corrections to a couple messages.

Before:

   pool: tank
     id: 7412636063178848859
  state: ONLINE
status: Some supported features are not enabled on the pool.
	(Note that they may be intentionally disabled if the
	'compatibility' property is set.)
 action: The pool can be imported using its name or numeric identif[...]
	some features will not be available without an explicit 'zp[...]
comment: Example comment.
 config:

	bclone      ONLINE
	  ada0      ONLINE

After:

  pool: tank
    id: 10180960571062436759
 state: ONLINE
status: Some supported features are not enabled on the pool.
	(Note that they may be intentionally disabled if the
	'compatibility' property is set.)
action: The pool can be imported using its name or numeric identifi[...]
	some features will not be available without an explicit 'zp[...]
config:

	tank        ONLINE
	  ada3      ONLINE

   pool: dozer
     id: 11028319538368222579
  state: ONLINE
 status: Some supported features are not enabled on the pool.
	 (Note that they may be intentionally disabled if the
	 'compatibility' property is set.)
 action: The pool can be imported using its name or numeric identif[...]
	 some features will not be available without an explicit 'z[...]
comment: Example comment.
 config:

	dozer       ONLINE
	  ada1      ONLINE

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Pawel Dawidek <pawel@dawidek.net>
Closes #16128
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@asomers @lundman
Fix updating the zvol_htable when renaming a zvol
78dccd7 
When renaming a zvol, insert it into zvol_htable using the new name, not
the old name.  Otherwise some operations won't work.  For example,
"zfs set volsize" while the zvol is open.

Sponsored by:	Axcient
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alek Pinchuk <apinchuk@axcient.com>
Signed-off-by:	Alan Somers <asomers@FreeBSD.org>
Closes openzfs#16127
Closes openzfs#16128
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@pjd @lundman
zpool import output is not formated properly.
8c9b1f1 
The 'zpool status' output assumes that the longest prefix is six
character long plus colon plus space, eg. 'status: ', 'action: '
or 'config: ' (so eight in total). This works well even when we have
messages that requires more than one line, as '\t' is exactly eight
characters, just like the longest prefix.

The 'zpool import' output is a bit different, as it may display the
comment pool property, then the longest prefix is 'comment: ', which is
nine characters long, not eight.
All the prefixes were given an extra space in front, but:
- 'status: ' did not get an extra space.
- Messages that require more than one line should use nine spaces of
  indentation, not eight.
- The extra space in front looks redundant if there is no comment
  property set on the given pool.

Fix it by adding an extra space to all prefixes, but only if the comment
property is defined. Also, when we need to continue the message in a new
line, use '\t ' for indentation.

While here, apply small corrections to a couple messages.

Before:

   pool: tank
     id: 7412636063178848859
  state: ONLINE
status: Some supported features are not enabled on the pool.
	(Note that they may be intentionally disabled if the
	'compatibility' property is set.)
 action: The pool can be imported using its name or numeric identif[...]
	some features will not be available without an explicit 'zp[...]
comment: Example comment.
 config:

	bclone      ONLINE
	  ada0      ONLINE

After:

  pool: tank
    id: 10180960571062436759
 state: ONLINE
status: Some supported features are not enabled on the pool.
	(Note that they may be intentionally disabled if the
	'compatibility' property is set.)
action: The pool can be imported using its name or numeric identifi[...]
	some features will not be available without an explicit 'zp[...]
config:

	tank        ONLINE
	  ada3      ONLINE

   pool: dozer
     id: 11028319538368222579
  state: ONLINE
 status: Some supported features are not enabled on the pool.
	 (Note that they may be intentionally disabled if the
	 'compatibility' property is set.)
 action: The pool can be imported using its name or numeric identif[...]
	 some features will not be available without an explicit 'z[...]
comment: Example comment.
 config:

	dozer       ONLINE
	  ada1      ONLINE

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Pawel Dawidek <pawel@dawidek.net>
Closes openzfs#16128
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alek-p alek-p alek-p 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.

Can't change an open zvol's volsize after a rename
4 participants
@asomers @jumbi77 @behlendorf @alek-p