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

issue #14223: fix generation of kernel uevents for snapshot rename on linux #16600

Merged

Conversation

JKDingwall
Copy link
Contributor

@JKDingwall JKDingwall commented Oct 3, 2024

zvol_rename_minors() needs to be given the full path not just the snapshot name. Use code removed in a0bd735 as a guide to providing the necessary values.

Closes #14223

Motivation and Context

Renaming a snapshot zfs rename zpool/volume@snap1 zpool/volume@snap2 should result in the udev generated link for /dev/zvol/zpool/volume@snap1 being removed and a new /dev/zvol/zpool/volume@snap2 being created. Without this then trying to access the snapshot content via the old name results in ENOENT and the new name is not present.

#14223

Description

The change here ensures that the full path to the snapshot is provided to zvol_rename_minors() and not just the plain snapshot name.

How Has This Been Tested?

The kernel module has been rebuilt on the test system with this change and the behaviour of the rename is now as expected, the old link is removed and a new link exists. Content of the snapshot is available via the new link.

make deb has also been executed and the full set of packages has been used to rebuild a virtual machine disk image. The image has been booted and no regressions have been noted.

# modinfo zfs | grep version
version:        2.2.6-7_ge638f31346
srcversion:     A2EB6FF13910945DA37E614
vermagic:       6.8.0-40-generic SMP preempt mod_unload modversions 

# lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 22.04.4 LTS
Release:        22.04
Codename:       jammy

# uname -a
Linux hostname 6.8.0-40-generic #40~22.04.1 SMP PREEMPT_DYNAMIC Tue Jul 30 17:51:21 UTC  x86_64 x86_64 x86_64 GNU/Linux

I am not a FreeBSD user so I'm unsure if there was an issue on that platform or how this code may affect it.

N/A.

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:

@JKDingwall JKDingwall force-pushed the JKDingwall/14223-zfs-snapshot-rename branch 3 times, most recently from 9958306 to 9f207e2 Compare October 3, 2024 12:22
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.

Looks good, thanks for running this down. Let's just make sure to also update at least one of the functional/cli_root/zfs_rename/ test cases to verify the link is update for renamed volume snapshots.

@JKDingwall
Copy link
Contributor Author

JKDingwall commented Oct 4, 2024

@behlendorf I've had a look around the tests and wondered if function/zvol/... might be a more appropriate area as there a lots of tests here using blockdev_missing and blockdev_exists which is the important thing to check, under cli_root there is no use at all. Since snapdev=visible is also required perhaps it's ok to test the rename here?

diff --git a/tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh b/tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh
index 1fc2d2780..3d229dccd 100755
--- a/tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh
+++ b/tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh
@@ -118,4 +118,15 @@ verify_inherited 'snapdev' 'hidden' $SUBZVOL $VOLFS
 blockdev_missing $SUBSNAPDEV
 blockdev_exists $SNAPDEV
 
+# 4. Verify "rename" is correctly reflected when "snapdev=visible"
+# 4.1 First create a snapshot and verify the device is present
+log_must zfs snapshot $SNAP
+log_must zfs set snapdev=visible $ZVOL
+blockdev_exists $SNAPDEV
+# 4.2 rename the snapshot and verify the devices are updated
+log_must zfs rename $SNAP $SNAP-new
+blockdev_missing $SNAPDEV
+blockdev_exists $SNAPDEV-new
+log_must zfs destroy $SNAP-new
+
 log_pass "ZFS volume property 'snapdev' works as expected"

@behlendorf
Copy link
Contributor

@JKDingwall sure that'd be fine. It does look like it would fit in better there.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Oct 4, 2024
@JKDingwall JKDingwall force-pushed the JKDingwall/14223-zfs-snapshot-rename branch from 9f207e2 to 3c2f69a Compare October 4, 2024 18:34
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 4, 2024
…ot rename

`zvol_rename_minors()` needs to be given the full path not just the
snapshot name.  Use code removed in
a0bd735 as a guide to providing the
necessary values.

Closes openzfs#14223

Signed-off-by: James Dingwall <james@dingwall.me.uk>
…ename

After renaming a snapshot with 'snapdev=visible' ensure that the /dev
entries are updated to reflect the rename.

Signed-off-by: James Dingwall <james@dingwall.me.uk>
@JKDingwall JKDingwall force-pushed the JKDingwall/14223-zfs-snapshot-rename branch from 3c2f69a to 156c952 Compare October 5, 2024 07:08
@JKDingwall
Copy link
Contributor Author

The test failures appear to be in a different area to this change so I think it is all in order now.

@JKDingwall JKDingwall marked this pull request as ready for review October 6, 2024 07:18
@behlendorf
Copy link
Contributor

Looks good. The couple of failures observed are all unrelated known test issues.

@behlendorf behlendorf merged commit 0b4dcbe into openzfs:master Oct 6, 2024
15 of 21 checks passed
@JKDingwall JKDingwall deleted the JKDingwall/14223-zfs-snapshot-rename branch October 7, 2024 06:17
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Oct 9, 2024
`zvol_rename_minors()` needs to be given the full path not just the
snapshot name.  Use code removed in a0bd735 as a guide
to providing the necessary values.

Add ZTS check for /dev changes after snapshot rename.  After
renaming a snapshot with 'snapdev=visible' ensure that the /dev
entries are updated to reflect the rename.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: James Dingwall <james@dingwall.me.uk>
Closes openzfs#14223 
Closes openzfs#16600
ixhamza pushed a commit to truenas/zfs that referenced this pull request Nov 11, 2024
`zvol_rename_minors()` needs to be given the full path not just the
snapshot name.  Use code removed in a0bd735 as a guide
to providing the necessary values.

Add ZTS check for /dev changes after snapshot rename.  After
renaming a snapshot with 'snapdev=visible' ensure that the /dev
entries are updated to reflect the rename.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: James Dingwall <james@dingwall.me.uk>
Closes openzfs#14223 
Closes openzfs#16600
ptr1337 pushed a commit to CachyOS/zfs that referenced this pull request Nov 14, 2024
`zvol_rename_minors()` needs to be given the full path not just the
snapshot name.  Use code removed in a0bd735 as a guide
to providing the necessary values.

Add ZTS check for /dev changes after snapshot rename.  After
renaming a snapshot with 'snapdev=visible' ensure that the /dev
entries are updated to reflect the rename.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: James Dingwall <james@dingwall.me.uk>
Closes openzfs#14223 
Closes openzfs#16600
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

linux: zfs volume snapshot rename does not generate KOBJ_CHANGE, /dev/zvol/.... link is not updated
2 participants